-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Implement multiple assignments (SIP-59) #19597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
888ec39
to
6b3ea8e
Compare
e476be7
to
9d96531
Compare
Co-authored-by: Matt Bovel <matthieu@bovel.net>
1af9159
to
5dab0f6
Compare
Does this leverage the |
No, it doesn't. It doesn't matter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. The strategy seems fine. I only have fairly localized comment.
@@ -33,6 +33,6 @@ trait UniqueMessagePositions extends Reporter { | |||
for offset <- dia.pos.start to dia.pos.end do | |||
positions.get((ctx.source, offset)) match | |||
case Some(dia1) if dia1.hides(dia) => | |||
case _ => positions((ctx.source, offset)) = dia | |||
case _ => positions((ctx.source, Integer.valueOf(offset).nn)) = dia |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change required? If yes, why?
11 | (x, y) = (1, 2) // error | ||
| ^^^^^^ | ||
| Found: (ev$1._2 : Int) | ||
| Required: Boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we improve the range of the error, here?
11 | (x, y) = (1, 2) // error | |
| ^^^^^^ | |
| Found: (ev$1._2 : Int) | |
| Required: Boolean | |
11 | (x, y) = (1, 2) // error | |
| ^ | |
| Found: (ev$1._2 : Int) | |
| Required: Boolean |
* Use this type to represent a part of a lvalue that must be evaluated before the lvalue gets | ||
* used for updating a value. | ||
*/ | ||
private[typer] final class PossiblyHoistedValue private (representation: tpd.Tree): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider putting all those helper classes inside an object PartialAssignment
. It's unusual to have top-level classes and objects that don't match the enclosing file name.
/** Returns a tree representing the value of `self`. */ | ||
def value(using Context): tpd.Tree = | ||
definition match | ||
case Some(d) => tpd.Ident(d.namedType).withSpan(representation.span) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this equivalent to
case Some(d) => tpd.Ident(d.namedType).withSpan(representation.span) | |
case Some(d) => ref(d).withSpan(representation.span) |
?
def valueAndDefinition(using Context): (tpd.Tree, Option[tpd.ValDef]) = | ||
definition match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to implement that method as
def valueAndDefinition(using Context): (tpd.Tree, Option[tpd.ValDef]) = | |
definition match | |
def valueAndDefinition(using Context): (tpd.Tree, Option[tpd.ValDef]) = | |
(value, definition) |
?
var i = 0 | ||
for l <- targets do | ||
val r = untpd.Select( | ||
untpd.TypedSplice(tpd.Ident(d.namedType)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
untpd.TypedSplice(tpd.Ident(d.namedType)), | |
untpd.TypedSplice(tpd.ref(d)), |
?
lhs: untpd.Tree, isSingleAssignment: Boolean | ||
)(using Context): PartialAssignment[LValue] = | ||
lhs match | ||
case lhs @ Apply(f, as) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider preserving the existing names, notably to help git
figure out that these lines were not changed:
case lhs @ Apply(f, as) => | |
case lhs @ Apply(fn, args) => |
val lvalue = ApplyLValue(callee, arguments) | ||
PartialAssignment(lvalue) { (l, r) => l.formAssignment(r) } | ||
|
||
case untpd.TypedSplice(Apply(MaybePoly(Select(fn, app), tas), as)) if app == nme.apply => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here:
case untpd.TypedSplice(Apply(MaybePoly(Select(fn, app), tas), as)) if app == nme.apply => | |
case untpd.TypedSplice(Apply(MaybePoly(Select(fn, app), targs), args)) if app == nme.apply => |
} | ||
|
||
/** Returns `true` if `s` is assignable. */ | ||
def canAssign(s: Symbol) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same:
def canAssign(s: Symbol) = | |
def canAssign(sym: Symbol) = |
* the variance check for these variables, which is done at PostTyper. It will be removed | ||
* after the variance check. | ||
*/ | ||
def rememberNonLocalAssignToPrivate(s: Symbol) = adapted match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More in this method. It seems like it could be unchanged modulo indentation compared to the code before.
This PR is a proof of concept for the implementation of multiple assignments (see SIP-59).
It is still a work in progress. In particular, it doesn't satisfy Scala's left-to-right evaluation order.Design overview
The main challenge is to respect Scala's left-to-right evaluation order, which implies that all effectual operations on the left-hand side of the assignment be evaluated before the right-hand side.
An assignment generally desugars to one of two forms:
lhs = rhs
remains anAssign
tree.lhs = rhs
is rewritten as anApply
tree (e.g.,a(0) = 1
becomesa.update(0, 1)
).In the case of a single assignment, the current implementation can "simply" perform the translation in one go as it has access to the expressions of both the left-hand and the right-hand sides. In the case of a multiple-assignment, however, the right-hand side is not available during the desugaring of the left-hand side because it may be the result of an expression that we have not yet evaluated. For example:
One solution to address this issue is to assign the result of all effectful sub-expressions to some temporary definitions an construct functions
untpd.Tree => tpd.Tree
that return the assignment of some left-hand side to the value represented by their arguments. These functions will be called after the tree representing their corresponding right-hand side is evaluated. They are called "assignment builders" in this PR.Consider this example to illustrate:
x0
andx1
represent the "hoisting" of the effectful operations performed in the left-hand side of the assignment. Thenx2
stores the result of the right-hand side. Finally the two updates perform the assignment. They are the result of calling instances of the aforementioned assignment builders.Note: the evaluation order is not "strictly" left-to-right in the sense that the assignments are notionally applications of functions that occur in sources before the right-hand side has been evaluated. These semantics are nonetheless inevitable and match the way an assignment "work" in Scala.
An assignment composed of a so-called "lvalue" (borrowing from C/C++ terminology) together with a function that constructs a typed tree given an untyped right-hand side.
A lvalue notionally represents the target of an assignment. It may be a simple value (e.g., a local variable) or the partial application of some update function or setter. It also contains the definitions of the sub-expressions whose values must be hoisted, which are called "locals" in the implementation:
Partial assignments are constructed in the typer by the method
formPartialAssignmentTo
. This method is essentially a refactoring oftypedAssign
that constructs lvalues rather than desugaring left-hand sides directly while visiting the tree.Once partial assignment has been constructed, the final result is built by creating a block containing, in this order:
No value is hoisted in the case of a single assignment so that the result is equivalent to the current behavior.