Skip to content

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

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

kyouko-taiga
Copy link
Contributor

@kyouko-taiga kyouko-taiga commented Feb 2, 2024

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:

  • If the left-hand side is a simple local variable, then lhs = rhs remains an Assign tree.
  • If the left-hand side is anything more sophisticated, then lhs = rhs is rewritten as an Apply tree (e.g., a(0) = 1 becomes a.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:

def makePair(): (Int, Int) = ???
val a = Array(0, 0)
(a(0), a(1)) = makePair()

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:

def f(x: Int): Int = ??? // possibly effectful expression
def makePair(): (Int, Int) = ???
val a = Array(0, 0)

// before
(a(f(0)), a(f(1))) = makePair()

// after
val x0 = f(0)
val x1 = f(1)
val x2 = makePair()
a.update(x0, x2._1)
a.update(x1, x2._2)

x0 and x1 represent the "hoisting" of the effectful operations performed in the left-hand side of the assignment. Then x2 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.

final class PartialAssignment[+T <: LValue](val lhs: T)(
    perform: (T, untpd.Tree) => untpd.Tree
)

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:

sealed abstract class LValue:
  def locals: List[tpd.ValDef]
  def formAssignment(rhs: untpd.Tree)(using Context): untpd.Tree
end LValue

Partial assignments are constructed in the typer by the method formPartialAssignmentTo. This method is essentially a refactoring of typedAssign 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:

  • The definitions of all hoisted values preserving the order in which sub-expressions must be evaluated
  • The evaluation of the right-hand side
  • The assignments of all lvalues, from left to right

No value is hoisted in the case of a single assignment so that the result is equivalent to the current behavior.

@kyouko-taiga kyouko-taiga force-pushed the multiple-assignment branch from e476be7 to 9d96531 Compare June 18, 2024 06:48
@kyouko-taiga kyouko-taiga force-pushed the multiple-assignment branch from 1af9159 to 5dab0f6 Compare July 26, 2024 12:22
@kyouko-taiga kyouko-taiga marked this pull request as ready for review July 27, 2024 06:36
@He-Pin
Copy link
Contributor

He-Pin commented Jan 14, 2025

Does this leverage the swap bytecode for two values.
https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-6.html#jvms-6.5.swap

@sjrd
Copy link
Member

sjrd commented Jan 14, 2025

No, it doesn't. It doesn't matter. swap does not magically provide a performance superpower. Once the (even baseline) JIT compiler is through with your bytecode, a swap or two stores are strictly equivalent. They're moving registers around.

Copy link
Member

@sjrd sjrd left a 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
Copy link
Member

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?

Comment on lines +13 to +16
11 | (x, y) = (1, 2) // error
| ^^^^^^
| Found: (ev$1._2 : Int)
| Required: Boolean
Copy link
Member

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?

Suggested change
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):
Copy link
Member

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this equivalent to

Suggested change
case Some(d) => tpd.Ident(d.namedType).withSpan(representation.span)
case Some(d) => ref(d).withSpan(representation.span)

?

Comment on lines +52 to +53
def valueAndDefinition(using Context): (tpd.Tree, Option[tpd.ValDef]) =
definition match
Copy link
Member

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

Suggested change
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)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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) =>
Copy link
Member

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:

Suggested change
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 =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here:

Suggested change
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) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same:

Suggested change
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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants