Fix #2869: Have constructors run after group of Memoize #2870
Conversation
odersky
force-pushed
the
fix-param-captures
branch
from
8ade44c to
16d0421
Compare
tests/pos/capturing.scala
Outdated
| @@ -0,0 +1,4 @@ | |||
| class MT(sf: MT => String) { | |||
| // `s` is retained as a field, but `sf` should not be. | |||
There was a problem hiding this comment.
To actually test this, I suggest making a run test and using Java reflection like https://github.com/lampepfl/dotty/blob/master/tests/run/paramForwarding.scala#L46
As explained in the code comment, the two get too entangled otherwise which leads to parameters being stored as class fields when they should not.
odersky
force-pushed
the
fix-param-captures
branch
from
16d0421 to
c637535
Compare
|
@smarter Ready for review once tests have passed. |
odersky
changed the title
| // `s` is retained as a field, but `sf` should not be. | ||
| val s = sf(this) | ||
| } | ||
| object Test extends App { |
There was a problem hiding this comment.
I think we've deprecated/discouraged the use of App
There was a problem hiding this comment.
Yes but I am not sure why? I hate to write all these main methods.
There was a problem hiding this comment.
I am still not sure why we use LegacyApp instead of App now. They seem to be equivalent.
There was a problem hiding this comment.
/cc @DarkDimius do you remember why LegacyApp had to be introduced?
It turns out with the fix to Constructors we don't generate the a value for the intermediate field anymore. This seems great! (scalac does generate the field, but it looks redundant)
| // This test is done in the right-hand side of a value definition. If Memoize | ||
| // was in the same group as Constructors, the test on the rhs ident would be | ||
| // performed before the rhs undergoes the owner change. This would lead | ||
| // to more symbls being retained as parameters. Test case is |
| // performed before the rhs undergoes the owner change. This would lead | ||
| // to more symbls being retained as parameters. Test case is | ||
| // | ||
| // dotc tests/pos/captuing.scala -Xprint:constr |
| @@ -79,8 +93,7 @@ class Constructors extends MiniPhaseTransform with IdentityDenotTransformer { th | |||
| } | |||
|
|
|||
| override def transformValDef(tree: tpd.ValDef)(implicit ctx: Context, info: TransformerInfo): tpd.Tree = { | |||
| if (mightBeDropped(tree.symbol)) | |||
| (if (isWildcardStarArg(tree.rhs)) retainedPrivateVals else seenPrivateVals) += tree.symbol | |||
There was a problem hiding this comment.
Why was this condition dropped?
There was a problem hiding this comment.
It is (probably) dead code. The test looks non-sensical to me. I fail to see how a definition of a class member can end up as something like this:
val xs = y: _*
Even if it did, why would it make a difference? I put in an assert to flag the condition, but it never triggered in all our tests.
|
Do we really need to split the group of phases in two? Perhaps we could move Constructors to the next group of phase instead? /cc @DarkDimius |
I verified experimentally that LamdaLift and Constructors cannot run in the same group.
Unfortunately no. See last commit. |
|
OK. I assume that this will have a non-trivial impact on performance but it should be worth it anyway. |
|
I think the win of not having to store all these parameters might well be stronger than the loss of having an extra phase. If we only had benchmarks that worked... |
As explained in the code comment, the two get too entangled otherwise
which leads to parameters being stored as class fields when they should not.
This was the most obvious source of memory leaks in the IDE. Once this is fixed
we can check whether there are others.