Proper laziness for by-name args of right-associative operators #5969

szeiger · 2017-06-30T17:21:52Z

This fixes scala/bug#1980 by changing the
desugaring of right-associative operator syntax in the spec such that
by-name operands now get the same desugaring as left-associative
operators (except for the reversed operands, of course). Only by-value
operands are pulled out into intermediate vals to preserve their
left-to-right evaluation order.

The (revised) implementation is as follows:

Parsers still performs the val desugaring for all calls, except
that the generated synthetic names use the new RIGHT_ASSOC_OP_PREFIX
to identify them later.
Everything else happens in Typers: After typechecking a ValDef
resulting from desugaring of a right-associative operator its Symbol
and RHS are stored in a Map (without knowing at that point if they are
by-name or by-value).
After typechecking a method application with an Ident for one of these
Symbols, check if the parameter is by-name in which case it is
replaced with the RHS and the Symbol added to a Set.
After typechecking a Block, check for a leading ValDef with the
Symbol that was inlined and remove the ValDef.

Fixes scala/bug#1980

lrytz · 2017-07-03T08:55:37Z

Additional test case that could be added (works correctly):

scala> class C { def f_:(x: => Int)(implicit y: Int) = 0 }
scala> val c = new C
scala> implicit val i = 1
scala> def k = { println("hi"); 1 }

scala> c.f_:(k)
res0: Int = 0

scala> k f_: c
res1: Int = 0

lrytz

I like the idea, it's nice to see this being fixed! Could we do the transformation in typers? Why is there the intermediate step of marking the symbol lazy?

lrytz · 2017-07-03T09:15:56Z

src/compiler/scala/tools/nsc/typechecker/RefChecks.scala

@@ -1703,7 +1707,7 @@ abstract class RefChecks extends Transform {
              assert(sym != NoSymbol, "transformCaseApply: name = " + name.debugString + " tree = " + tree + " / " + tree.getClass) //debug
              enterReference(tree.pos, sym)
            }
-            tree
+            eliminatedRightAssocValDefs.getOrElse(tree.symbol, tree)


In general we cannot just move trees around like this, because the owner chain of symbols defined in the tree might mess up. This is maybe what's causing the following crash:

scala> class C { def f_:(x: => Int) = 0 } defined class C scala> val c = new C c: C = C@4dd94931 scala> { val x = 1; x } f_: c java.util.NoSuchElementException: key not found: value x at scala.collection.MapLike.default(MapLike.scala:230) at scala.collection.MapLike.default$(MapLike.scala:229) at scala.collection.AbstractMap.default(Map.scala:59) at scala.collection.mutable.HashMap.apply(HashMap.scala:61) at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder$locals$.load(BCodeSkelBuilder.scala:391)

Hm, I assumed it would be safe here because I'm only moving it within the same block. But maybe that's no longer true in all cases after uncurry?

Wait, I'm not moving it within the same block. I'm eliminating the block.

OK, I see. It's owned by the ValDef that gets eliminated, so I have to change it anyway, even within the same block.

lrytz · 2017-07-03T09:16:17Z

src/compiler/scala/tools/nsc/typechecker/RefChecks.scala

@@ -1714,6 +1718,9 @@ abstract class RefChecks extends Transform {
                           // probably not, until we allow parameterised extractors
            tree

+          case Block((vd: ValDef) :: Nil, expr) if vd.symbol.isLazy && vd.name.toString.startsWith(nme.RIGHT_ASSOC_OP_PREFIX) =>
+            eliminatedRightAssocValDefs += ((vd.symbol, vd.rhs))


Missing call to transform on vd.rhs?

Yes, I think there should be one

lrytz · 2017-07-03T09:18:33Z

Yet another test case (that works correctly):

scala> class C { def f_:[T](x: => T) = 0 }
scala> val c = new C
scala> def k = { println("hi"); 1 }

scala> k f_:[Any] c
res9: Int = 0

szeiger · 2017-07-03T11:02:00Z

There are two steps because we first have to typecheck the application before we know that the definition can be removed. AFAICT typers only does a single full transformation of the tree. Could be done in typedBlock on the way up?

lrytz · 2017-07-03T11:20:12Z

Could be done in typedBlock on the way up

That's what I was thinking, it might work that way. Also, instead of "abusing" the LAZY flag, we could use a symbol attachment.

szeiger · 2017-07-03T18:32:03Z

Looks like the typedBlock approach works. I also added your additional tests and fixed the owner chain problem.

lrytz · 2017-07-04T09:37:46Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+          case _ => statsTyped
+        }
+
+        treeCopy.Block(block, statsTyped2, expr1)


This leaves a single-expression block in place, but I guess that's fine. We'd have to handle it outside typedBlock, as this method returns a Block.

scala> def foo = { 1 f_: c; 2 } [[syntax trees at end of typer]] // <console> def foo: Int = { { $line5.$read.$iw.$iw.c.f_:(1) }; 2 }

typedBlock could be changed to return a Tree but I didn't want to add unnecessary complications. The empty blocks don't seem to cause any problem.

lrytz · 2017-07-04T09:53:36Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                val args2 = (args1, mt.params) match {
+                  case ((ident: Ident) :: Nil, param :: Nil) if param.isByNameParam && rightAssocValDefs.contains(ident.symbol) =>
+                    inlinedRightAssocValDefs += ident.symbol
+                    val rhs = rightAssocValDefs(ident.symbol)


I'd prefer a symbol attachment over the two collections. Something like class RightAssocValDefAttachment(var rhsInlined), added in typedValDef, the var could be set to true here. Then you can use getAndRemoveAttachment in typedBlock.

We could even just add an empty marker attachment RightAssocValDefInlined here, and skip the test in typedValDef - what value does it add?

I'd prefer a symbol attachment over the two collections.

Are you sure? I checked for other uses of attachments and they all seem to be for communication between phases. I didn't find any precedent for data of a single phase being stored in attachments.

skip the test in typedValDef - what value does it add?

It stores the RHS which is needed in doTypedApply. Is there a better way to get it?

Are you sure?

To me it feels more like keeping state local, but it's fine either way in the end.

It stores the RHS which is needed in doTypedApply

Right, of course, I missed that.

lrytz · 2017-07-04T09:53:53Z

test/files/run/t1980.scala

+    implicit val i = 1
+    def k = { println("hi"); 1 }
+    c.f_:(k)
+    k f_: c


maybe also test lazy evaluation here (and in all tests below), just to make sure.

odersky · 2017-07-04T17:37:38Z

This is very promising, but I think the desugaring needs to go to defs instead of lazy vals. If

xs: { def &: (x: => T): U }

then, logically

e &: xs

should be the same as

xs.&:(e)

But it is only if it is expanded to

def x$ = xs; e &: (x$)

Or, the spec could simply demand that by-name arguments are not lifted out. That would be even clearer.

szeiger · 2017-07-04T17:59:24Z

Or, the spec could simply demand that by-name arguments are not lifted out. That would be even clearer.

But that's exactly what it does in this PR.

lrytz · 2017-07-05T11:51:07Z

Alternative pattern, suggested in peer-reviewing with Jason

add an attachment to the outer block (containing the rassoc$ val-def) during parsing
catch that in typedBlock, type-check the function part of the invocation, if it's by-name, inline the ValDef rhs before even typing it

That way we could live without the hash map / set altogether.

szeiger · 2017-07-05T12:57:38Z

catch that in typedBlock, type-check the function part of the invocation, if it's by-name, inline the ValDef rhs before even typing it

You can't check if it's by-name before typing the RHS because the method could have mixed by-name and by-value overloads. You need to type the RHS before doing overload resolution to find the right method.

lrytz · 2017-07-05T20:52:28Z

Nice one! Could you add a test for that case?

retronym · 2017-07-06T02:02:06Z

We should (separately) pursue an analogous change for the desugaring of default arguments:

scala> def foo(a: => Any, b: => Any) = a
foo: (a: => Any, b: => Any)Any

scala> foo(b = toString, a = toString) //print

{
  val x$1: () => String @scala.reflect.internal.annotations.uncheckedBounds = (() => $iw.this.toString());
  val x$2: () => String @scala.reflect.internal.annotations.uncheckedBounds = (() => $iw.this.toString());
  $line9.$read.$iw.$iw.foo(x$2.apply(), x$1.apply())
} // : Any

retronym · 2017-07-06T02:02:27Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+  // All typechecked RHS of ValDefs for right-associative operator desugaring
+  val rightAssocValDefs = new mutable.AnyRefMap[Symbol, Tree]
+  // Symbols of ValDefs for right-associative operator desugaring which are passed by name and have been inlined
+  val inlinedRightAssocValDefs = new mutable.HashSet[Symbol]


retronym · 2017-07-06T02:04:16Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

@@ -2063,7 +2070,10 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
          } else tpt1.tpe
          transformedOrTyped(vdef.rhs, EXPRmode | BYVALmode, tpt2)
        }
-      treeCopy.ValDef(vdef, typedMods, sym.name, tpt1, checkDead(rhs1)) setType NoType
+      val vdef1 = treeCopy.ValDef(vdef, typedMods, sym.name, tpt1, checkDead(rhs1)) setType NoType
+      if (sym.isSynthetic && sym.name.toString.startsWith(nme.RIGHT_ASSOC_OP_PREFIX))


Should need the to toString here.

retronym · 2017-07-06T02:06:31Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

-        treeCopy.Block(block, statsTyped, expr1)
+        // Remove ValDef for right-associative by-value operator desugaring which has been inlined into expr1
+        val statsTyped2 = statsTyped match {
+          case (vd: ValDef) :: Nil if inlinedRightAssocValDefs contains vd.symbol => Nil


Could eagerly clear the entry from the Map here if (inlinedRightAssocValDefs.remove(vd.symbol).isDefined) =>

retronym · 2017-07-06T02:07:41Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                val args2 = (args1, mt.params) match {
+                  case ((ident: Ident) :: Nil, param :: Nil) if param.isByNameParam && rightAssocValDefs.contains(ident.symbol) =>
+                    inlinedRightAssocValDefs += ident.symbol
+                    val rhs = rightAssocValDefs(ident.symbol)


Again, I'd like to use Map#remove here to clean up as we go.

retronym · 2017-07-06T02:32:23Z

Another approach here would be to modify the parser to emit a regular application, but mark it with a RightAssociative tree attachment. This would be typechecked as is, and afterwards we could lift out the val for strict arguments.

Pros:

Better type inference (the formal parameter type of the method (assuming non-overloaded would be used expected type when typechecking the argument.
Implementation is closer to the treatment of named/default argument desugaring

Cons:

Change in tree shape might have downstream effects on tools that look at the pre-typer tree (maybe quasiquotes, IDEs, ???). While these effects might actually simplify things, they are hard to predict
"better" type inference can break existing code in corner cases.

(I don't want to filibuster this PR with the alternative proposal, it can come as a follow up if we think it is worth pursuing.)

We also should consider how easy/hard alternatives are to spec. Currently it quite prescriptive:

If op is right associative, the same operation is interpreted as { val x=e1; e2.op(x ) }, where x is a fresh name.

Perhaps we could abstract this to specify the evaluation without specifying the desugaring. The spec for named/default applications is also currently prescriptive but doesn't discuss the treatment of by-name params.

szeiger · 2017-07-06T11:02:50Z

Another approach here would be to modify the parser to emit a regular application, but mark it with a RightAssociative tree attachment. This would be typechecked as is, and afterwards we could lift out the val for strict arguments

Yes, I was also thinking about this option since I read your previous proposal yesterday. It would be nice to get the same level of type inference for right-associative operators that you get for other operators and method calls.

lrytz

LGTM! I think you can squash all in one (and eliminate the no-op changes in RefChecks.scala)

odersky · 2017-07-11T14:29:50Z

I am still very positive on this, but believe it definitely needs a SIP.

jvican · 2017-07-14T23:37:11Z

Can you prepare a quick SIP @szeiger and we discuss it in the next meeting? I'm scheduling one for this month.

lrytz · 2017-07-15T05:01:07Z

He did already scala/docs.scala-lang#805

szeiger · 2017-07-17T13:54:35Z

Moving to M3, pending SIP

adriaanm · 2017-08-25T18:10:49Z

@szeiger, could you update the PR description to reflect the latest implementation strategy?

This fixes scala/bug#1980 as specified in SIP-34 (http://docs.scala-lang.org/sips/right-associative-by-name-operators.html) by changing the desugaring of right-associative operator syntax such that by-name operands now get the same desugaring as left-associative operators (except for the reversed operands, of course). Only by-value operands are pulled out into intermediate vals to preserve their left-to-right evaluation order. The implementation is as follows: - `Parsers` still performs the `val` desugaring for all calls, except that the generated synthetic names use the new `RIGHT_ASSOC_OP_PREFIX` to identify them later. - Everything else happens in `Typers`: After typechecking a ValDef resulting from desugaring of a right-associative operator its Symbol and RHS are stored in a Map (without knowing at that point if they are by-name or by-value). - After typechecking a method application with an Ident for one of these Symbols, check if the parameter is by-name in which case it is replaced with the RHS and the Symbol added to a Set. - After typechecking a Block, check for a leading ValDef with the Symbol that was inlined and remove the ValDef. Fixes scala/bug#1980

szeiger · 2017-11-16T16:14:29Z

Rebased and updated commit comment. Now that SIP-34 was accepted this should be ready to merge for M3.

Implemented in Scala 2.13 in scala/scala#5969 Implemented in Scala 3 in scala/scala3#3841

scala-jenkins added this to the 2.13.0-M2 milestone Jun 30, 2017

szeiger mentioned this pull request Jun 30, 2017

By-name arguments do not behave as expected with right-associative operators scala/scala3#2808

Closed

lrytz requested changes Jul 3, 2017

View reviewed changes

lrytz reviewed Jul 4, 2017

View reviewed changes

szeiger force-pushed the issue/1980 branch from 408fe7d to 01b7ce5 Compare July 4, 2017 13:20

szeiger mentioned this pull request Jul 4, 2017

LazyList.#:: not lazy due to parser rewriting of right-associative infix operators scala/collection-strawman#127

Closed

xuwei-k mentioned this pull request Jul 4, 2017

SI-1980 by-name argument incorrectly evaluated on :-ending operator scalaz/scalaz#528

Closed

retronym reviewed Jul 6, 2017

View reviewed changes

lrytz reviewed Jul 6, 2017

View reviewed changes

retronym added the release-notes worth highlighting in next release notes label Jul 7, 2017

retronym mentioned this pull request Jul 7, 2017

Release 2.12.3 scala/scala-dev#404

Closed

37 tasks

szeiger force-pushed the issue/1980 branch from a604fb5 to ffec78f Compare July 10, 2017 13:14

szeiger mentioned this pull request Jul 12, 2017

SIP-NN: Right-Associative By-Name Operators scala/docs.scala-lang#805

Merged

szeiger modified the milestones: 2.13.0-M3, 2.13.0-M2 Jul 17, 2017

szeiger force-pushed the issue/1980 branch from ffec78f to c76b245 Compare November 16, 2017 16:14

adriaanm approved these changes Nov 29, 2017

View reviewed changes

adriaanm merged commit 8084591 into scala:2.13.x Nov 29, 2017

julienrf mentioned this pull request Nov 30, 2017

StrawmanTest.mainTest failing in Scala 2.13.0-M3 scala/collection-strawman#308

Closed

lrytz mentioned this pull request Jan 17, 2018

Stabilize receiver of extension method application so that implicits accessible via the prefix can be candidate implicit arguments #5999

Merged

som-snytt mentioned this pull request Jun 15, 2018

Remove obsolete lint of by-name-right-assoc #6805

Merged

som-snytt mentioned this pull request Feb 12, 2019

Rewrite right-associative rewrite #7741

Draft

ijuma mentioned this pull request May 5, 2019

KAFKA-7197 expand gradle build: include Scala 2.13 apache/kafka#5454

Merged

Jasper-M mentioned this pull request Jul 23, 2019

Determine how lazy LazyList should be scala/bug#11307

Closed

S11001001 mentioned this pull request Jul 20, 2020

add more scalac 2.12 warnings digital-asset/daml#6798

Merged

6 tasks

julienrf added a commit to scalacenter/docs.scala-lang that referenced this pull request May 3, 2022

Mark SIP 34 as completed

a6f5a9e

Implemented in Scala 2.13 in scala/scala#5969 Implemented in Scala 3 in scala/scala3#3841

julienrf added a commit to scala/improvement-proposals that referenced this pull request Jun 9, 2022

Mark SIP 34 as completed

e23a834

Implemented in Scala 2.13 in scala/scala#5969 Implemented in Scala 3 in scala/scala3#3841

Proper laziness for by-name args of right-associative operators #5969

Proper laziness for by-name args of right-associative operators #5969

Conversation

szeiger commented Jun 30, 2017 • edited Loading

lrytz commented Jul 3, 2017

lrytz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrytz commented Jul 3, 2017

szeiger commented Jul 3, 2017

lrytz commented Jul 3, 2017

szeiger commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented Jul 4, 2017

szeiger commented Jul 4, 2017

lrytz commented Jul 5, 2017

szeiger commented Jul 5, 2017

lrytz commented Jul 5, 2017

retronym commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retronym commented Jul 6, 2017

szeiger commented Jul 6, 2017

lrytz left a comment

Choose a reason for hiding this comment

odersky commented Jul 11, 2017

jvican commented Jul 14, 2017

lrytz commented Jul 15, 2017

szeiger commented Jul 17, 2017

adriaanm commented Aug 25, 2017

szeiger commented Nov 16, 2017

szeiger commented Jun 30, 2017 •

edited

Loading