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

Introducing: the fields phase [ci: last-only] #5141

Merged
merged 17 commits into from Aug 11, 2016

Conversation

Projects
None yet
8 participants
@adriaanm
Member

adriaanm commented Apr 29, 2016

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:

  • correct bridges & signatures for trait setters (fields runs before erasure)
  • implement final vals of constant type as getters in trait (no fields/overridden accessors in subclasses)
  • other (non-constant) accessors for vals are abstract in traits
  • no spurious outer class attributes

Fix scala/scala-dev#118

TODO

  • Needs a benign update for test/files/run/t8549.scala, which was lost in the rebase. All green otherwise!
  • SI-9840 method cannot override immutable value:
abstract class Node(name: String) {
  var value: Option[Boolean] = None
}

case class Group(name: String, nodes: Node*) extends Node(name) {
  override def value = ???
  override def value_=(b: Option[Boolean]) = ???
}
  • revert accidental deletion of "// Phase travel necessary. For example, nullary methods (getter of an abstract val) get an"

community build

latest: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/448/consoleFull:

  • failing tests in scala-js
  • scala-js has sprouted a new dependency (mixin plugin) -- its test suite passed for me locally, though
  • twitter-util exploits a weirdness with private[this] val/vars:
trait T {
  private[this] var threads = Set[String]()
  protected def threads(): Array[String] = ???
}
[twitter-util] [error] /home/jenkins/workspace/scala-2.12.x-integrate-community-build/target-0.9.5/project-builds/twitter-util-3ecf6d03ea63cea0a4fd70085fc0a7dee036c0ca/util-core/src/main/scala/com/twitter/concurrent/Scheduler.scala:229: values cannot be volatile
[twitter-util] [error]   @volatile private[this] var _threads = Set[Thread]() // XXX: unused?
[twitter-util] [error]                               ^
[twitter-util] [error] two errors found

@adriaanm adriaanm added the on-hold label Apr 29, 2016

@scala-jenkins scala-jenkins added this to the 2.12.0-M5 milestone Apr 29, 2016

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Apr 29, 2016

Member

Wow, it only just now hit me that the fields phase should go after uncurry:

  • sam expansion synthesizes classes, which may need trait fields mixed in
  • the fields phase adds synthetic abstract methods to traits that should not disqualify them from being a SAM type

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...

Member

adriaanm commented Apr 29, 2016

Wow, it only just now hit me that the fields phase should go after uncurry:

  • sam expansion synthesizes classes, which may need trait fields mixed in
  • the fields phase adds synthetic abstract methods to traits that should not disqualify them from being a SAM type

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...

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Apr 29, 2016

Member

There's also the added advantage of moving after refchecks, which had to make exceptions for synthetic accessors.

Member

adriaanm commented Apr 29, 2016

There's also the added advantage of moving after refchecks, which had to make exceptions for synthetic accessors.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Apr 30, 2016

Member

test fail diag:

  • run/t5914.scala, run/virtpatmat_typetag.scala: val sig infer
  • run/SymbolsTest, run/t5269.scala, run/t5270.scala: name expansion gets out of whack in the toolbox compiler (trait owner receives a new $1-suffixed name between fields and constructors??)
  • run/constant-type.scala, run/repl-power.scala: was using substInfo, which calls setInfo, which discards type history. Wasn't a problem before since there was no history when we ran right after typer, but now we're after uncurry and history matters... UGH
  • run/t8549: serialization (test needs update)
Member

adriaanm commented Apr 30, 2016

test fail diag:

  • run/t5914.scala, run/virtpatmat_typetag.scala: val sig infer
  • run/SymbolsTest, run/t5269.scala, run/t5270.scala: name expansion gets out of whack in the toolbox compiler (trait owner receives a new $1-suffixed name between fields and constructors??)
  • run/constant-type.scala, run/repl-power.scala: was using substInfo, which calls setInfo, which discards type history. Wasn't a problem before since there was no history when we ran right after typer, but now we're after uncurry and history matters... UGH
  • run/t8549: serialization (test needs update)
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 3, 2016

Member

with 2050e we're down to:

Test suite finished with 3 cases failing:
fail - neg/t7622-cyclic-dependency  [output differs]% scalac t7622-cyclic-dependency/ThePlugin.scala
% scalac t7622-cyclic-dependency/sample_2.scala
error: Cycle in phase dependencies detected at cyclicdependency2, created phase-cycle.dot

% diff /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/neg/t7622-cyclic-dependency-neg.log /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/neg/t7622-cyclic-dependency.check
@@ -1 +1 @@
-error: Cycle in phase dependencies detected at cyclicdependency2, created phase-cycle.dot
+error: Cycle in phase dependencies detected at cyclicdependency1, created phase-cycle.dot

fail - run/t5914.scala  [compilation failed]% scalac t5914.scala
error: null

fail - run/virtpatmat_typetag.scala  [compilation failed]% scalac virtpatmat_typetag.scala
error: null

Member

adriaanm commented May 3, 2016

with 2050e we're down to:

Test suite finished with 3 cases failing:
fail - neg/t7622-cyclic-dependency  [output differs]% scalac t7622-cyclic-dependency/ThePlugin.scala
% scalac t7622-cyclic-dependency/sample_2.scala
error: Cycle in phase dependencies detected at cyclicdependency2, created phase-cycle.dot

% diff /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/neg/t7622-cyclic-dependency-neg.log /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/neg/t7622-cyclic-dependency.check
@@ -1 +1 @@
-error: Cycle in phase dependencies detected at cyclicdependency2, created phase-cycle.dot
+error: Cycle in phase dependencies detected at cyclicdependency1, created phase-cycle.dot

fail - run/t5914.scala  [compilation failed]% scalac t5914.scala
error: null

fail - run/virtpatmat_typetag.scala  [compilation failed]% scalac virtpatmat_typetag.scala
error: null

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 3, 2016

Member

🎆

Member

adriaanm commented May 3, 2016

🎆

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 3, 2016

Member

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. 🎸

Member

adriaanm commented May 3, 2016

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. 🎸

@adriaanm adriaanm assigned szeiger and retronym and unassigned szeiger May 3, 2016

@adriaanm adriaanm added WIP and removed on-hold labels May 3, 2016

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 5, 2016

Member

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):

object TraitValCapturing {
  def bar(local: Any) = {
    trait LT { def capture = local }

    (new LT{}).capture
  }
}

Member

adriaanm commented May 5, 2016

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):

object TraitValCapturing {
  def bar(local: Any) = {
    trait LT { def capture = local }

    (new LT{}).capture
  }
}

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 17, 2016

Member

Seeing if I can't roll module def elimination into the fields phase: adriaanm@8b41f19

Member

adriaanm commented May 17, 2016

Seeing if I can't roll module def elimination into the fields phase: adriaanm@8b41f19

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd May 17, 2016

Member

Seeing if I can't roll module def elimination into the fields phase: adriaanm/scala@8b41f19

Er ... if this is what I think it is, please don't? Module accessors as MODULE$ fields are a platform-specific implementation strategy. Scala.js has an entirely different strategy.

Member

sjrd commented May 17, 2016

Seeing if I can't roll module def elimination into the fields phase: adriaanm/scala@8b41f19

Er ... if this is what I think it is, please don't? Module accessors as MODULE$ fields are a platform-specific implementation strategy. Scala.js has an entirely different strategy.

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd May 17, 2016

Member

Or is it only about "non-static" modules, i.e., those nested in classes and traits?

Member

sjrd commented May 17, 2016

Or is it only about "non-static" modules, i.e., those nested in classes and traits?

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 17, 2016

Member

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).

Member

adriaanm commented May 17, 2016

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).

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd May 17, 2016

Member

Oh OK, that's fine. Thanks for the clarification.

Member

sjrd commented May 17, 2016

Oh OK, that's fine. Thanks for the clarification.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm
Member

adriaanm commented May 19, 2016

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 27, 2016

Member

Messed something up with the encoding -- causing deadlocks in run/t0091.scala, run/t1537.scala and run/t3932.scala

Member

adriaanm commented May 27, 2016

Messed something up with the encoding -- causing deadlocks in run/t0091.scala, run/t1537.scala and run/t3932.scala

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 27, 2016

Member

Pardon me -- not a deadlock, but this seems to explain it:

diff --git i/C$.class.java w/C$.class.java
index fa5169b98c..9b1af62f66 100644
--- i/C$.class.java
+++ w/C$.class.java
@@ -14,7 +14,9 @@ implements B {

     @Override
     public C.m. m() {
-        return C.m..MODULE$;
+        do {
+            // Infinite loop
+        } while (true);
     }
Member

adriaanm commented May 27, 2016

Pardon me -- not a deadlock, but this seems to explain it:

diff --git i/C$.class.java w/C$.class.java
index fa5169b98c..9b1af62f66 100644
--- i/C$.class.java
+++ w/C$.class.java
@@ -14,7 +14,9 @@ implements B {

     @Override
     public C.m. m() {
-        return C.m..MODULE$;
+        do {
+            // Infinite loop
+        } while (true);
     }
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 27, 2016

Member
  • neg/override-object-no.scala
  • run/t6240-universe-code-gen.scala
  • run/t7747-repl.scala
  • run/t8549.scala
Member

adriaanm commented May 27, 2016

  • neg/override-object-no.scala
  • run/t6240-universe-code-gen.scala
  • run/t7747-repl.scala
  • run/t8549.scala
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 28, 2016

Member

I checked the bytecode diff again on library/reflect/compiler: https://gist.github.com/adriaanm/3f2de4349209755532c5d127f3322306

Looks good except for the OUTERCLASS attribute being messed up in a couple of spots. @lrytz could you point me in the right direction?

Member

adriaanm commented May 28, 2016

I checked the bytecode diff again on library/reflect/compiler: https://gist.github.com/adriaanm/3f2de4349209755532c5d127f3322306

Looks good except for the OUTERCLASS attribute being messed up in a couple of spots. @lrytz could you point me in the right direction?

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz May 28, 2016

Member

i'll take a look on monday, most likely i won't have time this weekend

Member

lrytz commented May 28, 2016

i'll take a look on monday, most likely i won't have time this weekend

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 28, 2016

Member

No worries!

Member

adriaanm commented May 28, 2016

No worries!

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz May 30, 2016

Member

i can reproduce the OUTERCLASS change, and it's probably even a bugfix :) the case is not covered by existing tests. need to spend some more time, i'll prepare a patch to extend the tests.

Member

lrytz commented May 30, 2016

i can reproduce the OUTERCLASS change, and it's probably even a bugfix :) the case is not covered by existing tests. need to spend some more time, i'll prepare a patch to extend the tests.

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz May 30, 2016

Member

@adriaanm see https://github.com/adriaanm/scala/compare/fields...lrytz:pr5141?expand=1 for that test.

As far as I can tell, all the the OUTERCLASS diffs you list in your gist (https://gist.github.com/adriaanm/3f2de4349209755532c5d127f3322306#outerclass) fall into this category.

Member

lrytz commented May 30, 2016

@adriaanm see https://github.com/adriaanm/scala/compare/fields...lrytz:pr5141?expand=1 for that test.

As far as I can tell, all the the OUTERCLASS diffs you list in your gist (https://gist.github.com/adriaanm/3f2de4349209755532c5d127f3322306#outerclass) fall into this category.

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz May 30, 2016

Member

(i checked that the test fails when applied to current 2.12.x)

Member

lrytz commented May 30, 2016

(i checked that the test fails when applied to current 2.12.x)

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 30, 2016

Member

Ausgezeichnet! Thanks :-)

Member

adriaanm commented May 30, 2016

Ausgezeichnet! Thanks :-)

adriaanm added some commits Jul 6, 2016

Allow 'overriding' deferred var
Discovered by scala-js's test suite.
Align double definition check with spec
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.)
Drive accessor synthesis from info transformer
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.
Admit @volatile on accessor in trait
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.
Mixed in getter needs NullaryMethodType too
Clone at uncurry to preserve it in its info history.
Discovered by the scala-js test suite.
Make fewer trait methods not-{private, protected}
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).

@scala-jenkins scala-jenkins removed the reviewed label Aug 11, 2016

@adriaanm adriaanm added the reviewed label Aug 11, 2016

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 11, 2016

Member

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.

Member

adriaanm commented Aug 11, 2016

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.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm
Member

adriaanm commented Aug 11, 2016

poppy the corn

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 11, 2016

Member

I hope I won't cause too many merge conflicts, but I'm planning to go ahead and merge this before happy hour :-)

Member

adriaanm commented Aug 11, 2016

I hope I won't cause too many merge conflicts, but I'm planning to go ahead and merge this before happy hour :-)

@adriaanm adriaanm merged commit fdb3105 into scala:2.12.x Aug 11, 2016

5 checks passed

cla @adriaanm signed the Scala CLA. Thanks!
Details
integrate-ide [2830] SUCCESS. Took 12 s.
Details
validate-main [3188] SUCCESS. Took 104 min.
Details
validate-publish-core [3129] SUCCESS. Took 5 min.
Details
validate-test [2758] SUCCESS. Took 98 min.
Details

@adriaanm adriaanm referenced this pull request Aug 13, 2016

Closed

2.12 community build omnibus ticket #203

13 of 16 tasks complete

adriaanm added a commit to adriaanm/paradise that referenced this pull request Aug 13, 2016

@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Sep 9, 2016

Member

a97297d has usable text for the release notes

Member

SethTisue commented Sep 9, 2016

a97297d has usable text for the release notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment