Introducing: the fields phase [ci: last-only] #5141
Conversation
|
Wow, it only just now hit me that the
This looks compelling enough to me. The final TODO after this that I'm aware of is dealing with fields in local traits in lambdalift... |
|
There's also the added advantage of moving after refchecks, which had to make exceptions for synthetic accessors. |
|
test fail diag:
|
|
with 2050e we're down to: |
adriaanm
force-pushed
the
fields
branch
4 times, most recently
from
f781bd7 to
0506f0e
Compare
|
🎆 |
|
Still need to think a bit about the valdef sig inference changes. I experimented with moving fields after specialization, but I think that's going to be too much work for M5. Still need to think a bit more about the impact of moving the phase after picklers, and clean up usage of flags. Otherwise, I think I've settled on this design. 🎸 |
|
Here's an example of lambdalift introducing a ValDef in a trait after fields, which will be handled by newGetter/newSetter in mixins (31868d3 introduced a println to see if I'm missing any other cases, at least those triggered by bootstrapping/test suite): |
|
Seeing if I can't roll module def elimination into the fields phase: adriaanm@8b41f19 |
Er ... if this is what I think it is, please don't? Module accessors as |
|
Or is it only about "non-static" modules, i.e., those nested in classes and traits? |
|
It's about moving the desugaring from refchecks to fields, so that refchecks becomes more of a "checking" phase instead of having a weird info transform and mixing in stuff to traits. The desugaring itself stays the same, but happens during a phase that already does a lot of the same tree transforms (fields). |
|
Oh OK, that's fine. Thanks for the clarification. |
|
note to self about modules: https://gist.github.com/adriaanm/1992163e1708045617e409037d1bd223 |
adriaanm
force-pushed
the
fields
branch
2 times, most recently
from
be91525 to
51aa7a4
Compare
|
Messed something up with the encoding -- causing deadlocks in run/t0091.scala, run/t1537.scala and run/t3932.scala |
|
Pardon me -- not a deadlock, but this seems to explain it: |
|
Remove weird special cases for private-local fields
and parameter accessor (fields).
One change with the new trait val encoding:
```
scala> trait T { private[this] var x: String = "1" ; def x(): Int = 1 }
<console>:11: error: method x is defined twice;
the conflicting variable x was defined at line 11:37
trait T { private[this] var x: String = "1" ; def x(): Int = 1 }
^
```
Whereas:
```
scala> class T { private[this] var x: String = "1" ; def x(): Int = 1 }
defined class T
```
Before, both the `class` and `trait` definition were accepted.
(Because there is no accessor for a private[this] val/var,
and a MethodType does not match the type of a value.)
(Dotty accepts neither the class or the trait definition.)
Discovered by scala-js's test suite.
Clone at uncurry to preserve it in its info history. Discovered by the scala-js test suite.
There's no other place to squirrel away the annotation until we create a field in a subclass. The test documents the idea, but does not capture the regression seen in the wild, as explained in a comment.
Derive/filter/propagate annotations in info transformer, don't rely on having type checked the derived trees in order to see the annotations. Use synthetics mechanism for bean accessors -- the others will soon follow. Propagate inferred tpt from valdef to accessors by setting type in right spot of synthetic tree during the info completer. No need to add trees in derivedTrees, and get rid of some overfactoring in method synthesis, now that we have joined symbol and tree creation. Preserve symbol order because tests are sensitive to it. Drop warning on potentially discarded annotations, I don't think this warrants a warning. Motivated by breaking the scala-js compiler, which relied on annotations appearing when trees are type checked. Now that ordering constraint is gone in the new encoding, we may as well finally fix annotation assignment.
No longer making trait methods not-protected. (The backend only does public/private because of the poor mapping between visibility from Scala to the JVM). Note that protected trait members will not receive static forwarders in module classes (when mixed into objects). Historic note: we used to `makeNotPrivate` during explicitouter, now we do it later, which means more private methods must be excluded (e.g., lambdaLIFTED ones).
|
Thanks for the reviews! I've implemented most of it, with one carry-over to my next PR. I fixed the accidental deletion in the commit that introduced it, which meant rebasing. |
|
|
|
I hope I won't cause too many merge conflicts, but I'm planning to go ahead and merge this before happy hour :-) |
probably needs more work see scala/scala-dev#203
|
a97297d has usable text for the release notes |
Part of the plan to simplify the mixin phase by making each phase that introduces new trait members responsible for mixing in the corresponding members into subclasses of these traits. (uncurry runs before fields, so it can expand SAM traits with vals without further work. Lambdalift, on the other hand introduces new fields when local traits capture -- mixins still has to deal with this, as well as lazy vals and early init vals)
The bytecode diff is progress:
Fix scala/scala-dev#118
TODO
community build
latest: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/448/consoleFull: