Implement Scala2 traits #622

odersky · 2015-05-29T13:15:09Z

Added new phases that let Dotty classes implement Scala 2.x compiled traits.

The treatment of lazy vals is probably not yet correct. @DarkDimius can you take a look.

If @retronym or @lrytz could take a look to review this would be helpful as well.

odersky · 2015-05-29T14:32:22Z

Rebased to #623 and changed comments.

odersky · 2015-05-29T16:45:41Z

Rebased to latest make-tests-pass to be able to pass junit tests.

odersky · 2015-05-31T11:16:33Z

Added one more commit to make tests pass and rebased to master.

odersky · 2015-05-31T11:29:15Z

partest seems to hang for all PRs. The log says it's succesful but the task is not terminated.

smarter · 2015-05-31T13:03:50Z

/synch

smarter · 2015-05-31T13:05:54Z

@adriaanm: any idea why /synch is necessary in all PRs, but only for partest?

odersky · 2015-06-01T04:44:04Z

/synch

lrytz · 2015-06-01T06:44:45Z

i had a quick pass over the change, couldn't see any obvious issues. how will this be tested?

adriaanm · 2015-06-01T11:55:58Z

I checked scabot's log but didn't see any irregularities. I'm on vacation this week, but will have a look next week. I would not recommend blindly trusting the result of /synch, because it seems it's marking things green that shouldn't be. I've never seen it do that before... I'll disable that behavior at least.

retronym · 2015-06-01T12:50:31Z

Is this expected to work?

% tail sandbox/{scala-trait,dotty-subclass}.scala
==> sandbox/scala-trait.scala <==
trait T {
  def d = 42
  val v = ""
  object O
}

==> sandbox/dotty-subclass.scala <==
class Sub extends T

object Main {
  def main(args: Array[String]): Unit = {
    val sub = new Sub
    println(sub.d)
    println(sub.v)
    println(sub.O)
  }
}

% scalac sandbox/scala-trait.scala && ./bin/dotc sandbox/dotty-subclass.scala && scala Main
java.lang.NoSuchMethodError: T.$init$()V
    at Sub.<init>(dotty-subclass.scala)

The last tree from -Xprint:all

  class Sub extends Object with T {
    def <init>(): Sub = {
      super()
      super[T]()
      ()
    }
    private val v$$local: String
    <accessor> def v(): String = this.v$$local
    private val O$$local: T.this.T$O$
    <accessor> module def O(): T.this.T$O$ = this.O$$local
    <accessor> def T$_setter_$v_=(x$0: String): Unit = ()
    <accessor> def T$_setter_$O_=(x$0: T.this.T$O$): Unit = ()
  }

If memoize and constructors are run in different groups, memoize's previous postcondition "all concerete methods are implemented" is wrong, because constructors are not implemengted yet. Solved by moving the postcondition to phase Constructors.

It definitely does appear in trees, so should be included in the set. Affects how things are printed. Before, typed var's would still show up as vals.

Also: generalize expandedName so that it can cater for trait setters.

We'd like to make it reusable for a phase that treats Scala2 traits.

This brings it in line with Scala2 conventions.

Previously it didn't, because it created an ExprType, which is illegal after erasure.

A typed `_'. This is needed in a few places.

They don't exist for Scala2 traits. Instead we let the initializer be `_' and rely on trait setters (to be implemented) to initialize the field.

Erasure uncurries arguments, need to track that in memberSignature.

This phase rewrites supercalls to calls to static implementation class methods.

Fixes junit failure in dotty where a lazy val was initialized with a "...$lazy = _" assignment. Moved ElimWiildcard to one group before. It does not really matter where it goes so it might as well go someshere in the middle of the pack.

Instead of cleaning up, generate sensical code in the first place. This is shorter and (I would argue) clearer, and also has the advantage that some initializing assignments are not generated at all.

retronym · 2015-06-01T13:05:01Z

Looks like LinkScala2ImplClasses is called for Sub before Mixin::superCallOpt. I'm not up to speed with dotty's mini-phase / SymTransformer architecture to understand why this happens.

Moving it to a new phase just before LambdaLift advances the error to:

java.lang.AbstractMethodError: Sub.d()I
    at Main$.main(dotty-subclass.scala:5)
    at Main.main(dotty-subclass.scala)

A transformFollowingDeep was missing, so LinkScala2ImplClasses never got to see the call.

Scala 2 doe snot generate default methods, so we always need forwarders.

We are still lacking the setup to do this right for mixed Scala 2/Dotty runtime tests. So I checked into `pos` for now.

smarter · 2015-06-01T14:53:03Z

I would not recommend blindly trusting the result of /synch, because it seems it's marking things green that shouldn't be

Where are you seeing that?

For implemented getters and forwarded methods we need a notion of "isCurrent", which means: would the getter or method before the implementation is added be a member of the implementing class? Only in this case do we need to do anything. The method formulation was previously weaker than the getter formulation, which led to an error when compiling core (duplicate methods: andThen and size).

odersky · 2015-06-01T19:36:52Z

@retronym Oops that was not quite ready for prime time yet. I got distracted with the build problems. It's in somewhat better shape with the recent commits which address the problems you have encountered. There's a more comprehensive test in scala2traits (or rather a pre-test, it would be good if someone turned this into a real test).

@DarkDimius Lazy vals do not work yet under this scheme. I have left some code in the test files regarding lazy vals, but they are currently commented out. Uncomment once they are implemented.

odersky · 2015-06-01T20:03:26Z

Still something wrong with isCurrent. I'm investigating.

adriaanm · 2015-06-01T21:40:03Z

I would not recommend blindly trusting the result of /synch, because it seems it's marking things green that shouldn't be

Where are you seeing that?

Those WARNING statuses are green by default: https://github.com/scala/scabot/blob/master/server/src/main/scala/Actors.scala#L361 (they are stupidly added when the connection fails.. my bad)

adriaanm · 2015-06-01T21:47:41Z

It looks like it's an issue with Jenkins. At the end of the partest job, it says:

Notifying endpoint 'HTTP:http://scala-ci.typesafe.com:8888/jenkins'
ERROR: Failed to notify endpoint 'HTTP:http://scala-ci.typesafe.com:8888/jenkins'

This means Scabot likely did not see the status update (it's listening at that URL for Jenkins to inform us about the job ending). I suspect it could be because the ansicolor plugin does some fancy formatting with the buildlog, which is part of the notification. Maybe it can't be serialized.

To test the hypothesis, I've disabled the plugin and hit rebuild on the job.

UPDATE: the rebuild has a stacktrace of the jenkins notification plugin failing to notify Scabot. No idea why this broke all of a sudden:

ERROR: Failed to notify endpoint 'HTTP:http://scala-ci.typesafe.com:8888/jenkins'
java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
    at java.util.LinkedList.checkElementIndex(LinkedList.java:553)
    at java.util.LinkedList.set(LinkedList.java:488)
    at hudson.model.Run.getLog(Run.java:1956)
    at com.tikal.hudson.plugins.notification.Phase.getLog(Phase.java:141)
    at com.tikal.hudson.plugins.notification.Phase.buildJobState(Phase.java:79)
    at com.tikal.hudson.plugins.notification.Phase.handle(Phase.java:42)
    at com.tikal.hudson.plugins.notification.JobListener.onStarted(JobListener.java:31)
    at hudson.model.listeners.RunListener.fireStarted(RunListener.java:215)
    at hudson.model.Run.execute(Run.java:1740)
    at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
    at hudson.model.ResourceController.execute(ResourceController.java:98)
    at hudson.model.Executor.run(Executor.java:374)
Failed to notify endpoint 'HTTP:http://scala-ci.typesafe.com:8888/jenkins' - java.lang.IndexOutOfBoundsException: Index: 0, Size: 0

UPDATE2: known bug: https://issues.jenkins-ci.org/browse/JENKINS-27441, fixed in jenkins 1.613 (we're on 1.611 :-() -- I'll see about upgrading

UPDATE3: ah, great, there's another bug involved that's not fixed: https://issues.jenkins-ci.org/browse/JENKINS-26162 -- oh shitty jenkins and your shitty plugins... We'll have to (privately) release our own version of that plugin with this line commented out: https://github.com/jenkinsci/notification-plugin/blob/master/src/main/java/com/tikal/hudson/plugins/notification/Phase.java#L99.. The project does not seem very active.

retronym · 2015-06-01T23:28:14Z

@adriaanm I presume that dotty could turn off direct archiving of partest logs as a workaround?

adriaanm · 2015-06-02T10:33:39Z

Yep, that would work around it, but I've deployed a patched notification plugin that doesn't include logs/artifacts (not needed anyway). Plugin installed manually for now. Tracking automation: scala/scala-jenkins-infra#69

The policy is now made clear in a doc comment. The new part is that we will prefer a symbol defined in a subclass over a symbol defined in a superclass. With the previous commit 0a0d3dd on "More precise and uniform modelling of isCurrent" we got runtime test failures for Course-2002-03.scala because the new definition isCurrent assumed a behavior of `member` which was not assured: Namely that the merged denotation would prefer symbols in subclasses over symbols in superclasses.

The previius commit on "Refine & for Denotations" caused NotDefinedHere errors in the backend when compiling dotc/typer. These were tracked down to three occurrences in LazyVals where `enter` instead of `enteredAfter` was used. `enter` will enter a symbol in an unknown set of previous phases. Transformations that traverse scope (like erasedMembers in TypeErasure will then see the denotations of these symbols outside the scope in which they are defined.

odersky · 2015-06-02T11:08:57Z

This PR has strirred up quite a lot of trouble. Let's see whether the dust settles down now.

Until the previous fix to Denotation-& the test here spuriously passed. The Dotty spec is that value classes synthesize equals and hashCode only if no concrete definitions are inherited except the ones from Any. That was previously miscalculated. The test has been updated to reflect the specified behavior.

odersky · 2015-06-02T12:01:43Z

Yay! And we did not even need a /synch! Thanks @adriaanm for this.

odersky · 2015-06-02T13:19:29Z

@smarter There's plenty of precendent regarding equals != ==:

scala> 1L.equals(1)
res5: Boolean = false

scala> 1L == 1
res6: Boolean = true

smarter · 2015-06-02T13:24:33Z

Yeah, but if I have a val x which I know is not null, then having x == x and x.equals(x) behave differently is pretty surprising. In any case breaking the semantics of value classes here does not buy us anything. We should explore allowing the user to override equals and hashCode in value classes but this will only work correctly if we have extension methods in universal traits (which is something I'm looking at).

odersky · 2015-06-02T13:25:18Z

@smarter The advantage of Dotty's scheme is that it treats value classes and case classes in the same way. All the bad scenarios you describe hold equally for case classes.

smarter · 2015-06-02T13:32:12Z

For value classes we rewrite new Foo(3).==(new Foo(4)) to 3.==(4) in VCInline because we assume that equals cannot be overriden. For case classes we don't make this assumption and don't do this rewriting, so equals and == stay consistent.

odersky · 2015-06-02T13:40:27Z

And for hashcode? That's the Scala bug we were talking about. But I think
the resolution should still be that universal traits cannot override equals
and hashCode.

On Tue, Jun 2, 2015 at 3:32 PM, Guillaume Martres [email protected]
wrote:

For value classes we rewrite new Foo(3).==(new Foo(3)) to 3.==(4) in
VCInline because we assume that equals cannot be overriden. For case
classes we don't make this assumption and don't do this rewriting, so
equals and == stay consistent.

—
Reply to this email directly or view it on GitHub
#622 (comment).

Martin Odersky
EPFL

smarter · 2015-06-02T14:15:45Z

Yes, disallowing equals/hashCode in universal traits might be the only sane solution, it breaks the scala standard library but we've already given up on that anyway.

odersky · 2015-06-02T19:37:25Z

@smarter Agreed. Can you open an issue? Thanks! -

DarkDimius · 2015-06-02T19:39:34Z

But I think the resolution should still be that universal traits cannot override equals and hashCode.

Why? Those methods could be handled by the same rewriting scheme. Additionally, that will supersede current special rule for hashcode and equals, making whole scheme more regular. And what about toString?

smarter · 2015-06-02T19:52:06Z

See #629.

adriaanm · 2015-06-05T14:45:40Z

Yay!

🎉 thanks -- happy it's working!

odersky · 2015-06-05T20:59:37Z

Any more review remarks? Would be good to get this in, as it fixes quite a lot of problems and unblocks the bootstrapping project.

DarkDimius · 2015-06-05T21:01:59Z

I propose we merge this one, and continue with run tests.

Implement Scala2 traits

odersky force-pushed the add/implement-scala2-traits branch from bd86088 to 0a86409 Compare May 29, 2015 14:30

odersky force-pushed the add/implement-scala2-traits branch from 0a86409 to d93b964 Compare May 29, 2015 16:44

odersky force-pushed the add/implement-scala2-traits branch from d93b964 to 55c6d08 Compare May 31, 2015 11:15

odersky added 14 commits June 1, 2015 15:04

Make Mutable a ModifierFlag.

069010b

It definitely does appear in trees, so should be included in the set. Affects how things are printed. Before, typed var's would still show up as vals.

New NameOps methods for Scala2 traits

11848a8

Also: generalize expandedName so that it can cater for trait setters.

Move needsForwarder logic from ResolveSuper to MixinOps.

9a167de

We'd like to make it reusable for a phase that treats Scala2 traits.

Use $init$ as the name of trait constructors

4f0af8e

This brings it in line with Scala2 conventions.

Make ensureMethodic work after erasure.

6cdb3d4

Previously it didn't, because it created an ExprType, which is illegal after erasure.

New utility method tpd.Underscore

31a7f64

A typed `_'. This is needed in a few places.

Avoid referring to initial$x methods when implementing Scala2 traits

4f50e8f

They don't exist for Scala2 traits. Instead we let the initializer be `_' and rely on trait setters (to be implemented) to initialize the field.

New phase: AugmentScala2Traits

71067e6

Make memberSignature work after erasure.

e4dc4fa

Erasure uncurries arguments, need to track that in memberSignature.

New phase: LinkScala2ImplClasses

8263d33

This phase rewrites supercalls to calls to static implementation class methods.

Improve comments

0b34395

Make elimWildcardIdents apply to Assignments

9b85e30

Fixes junit failure in dotty where a lazy val was initialized with a "...$lazy = _" assignment. Moved ElimWiildcard to one group before. It does not really matter where it goes so it might as well go someshere in the middle of the pack.

Elim ElimWildcardIdents

9220844

Instead of cleaning up, generate sensical code in the first place. This is shorter and (I would argue) clearer, and also has the advantage that some initializing assignments are not generated at all.

odersky added 2 commits June 1, 2015 15:32

Fix missing implementation class forward

86f954c

A transformFollowingDeep was missing, so LinkScala2ImplClasses never got to see the call.

Uncomment important test for Scala trait forwarding

2ab65fd

Scala 2 doe snot generate default methods, so we always need forwarders.

Add test case.

5aaa406

We are still lacking the setup to do this right for mixed Scala 2/Dotty runtime tests. So I checked into `pos` for now.

odersky force-pushed the add/implement-scala2-traits branch from c00974c to 0a0d3dd Compare June 1, 2015 19:32

odersky added 2 commits June 2, 2015 12:56

odersky added a commit that referenced this pull request Jun 6, 2015

Merge pull request #622 from dotty-staging/add/implement-scala2-traits

6ca52b0

Implement Scala2 traits

odersky merged commit 6ca52b0 into scala:master Jun 6, 2015

allanrenucci deleted the add/implement-scala2-traits branch December 14, 2017 16:57

Implement Scala2 traits #622

Implement Scala2 traits #622

Uh oh!

Conversation

odersky commented May 29, 2015

Uh oh!

odersky commented May 29, 2015

Uh oh!

odersky commented May 29, 2015

Uh oh!

odersky commented May 31, 2015

Uh oh!

odersky commented May 31, 2015

Uh oh!

smarter commented May 31, 2015

Uh oh!

smarter commented May 31, 2015

Uh oh!

odersky commented Jun 1, 2015

Uh oh!

lrytz commented Jun 1, 2015

Uh oh!

adriaanm commented Jun 1, 2015

Uh oh!

retronym commented Jun 1, 2015

Uh oh!

retronym commented Jun 1, 2015

Uh oh!

smarter commented Jun 1, 2015

Uh oh!

odersky commented Jun 1, 2015

Uh oh!

odersky commented Jun 1, 2015

Uh oh!

adriaanm commented Jun 1, 2015

Uh oh!

adriaanm commented Jun 1, 2015

Uh oh!

retronym commented Jun 1, 2015

Uh oh!

adriaanm commented Jun 2, 2015

Uh oh!

odersky commented Jun 2, 2015

Uh oh!

odersky commented Jun 2, 2015

Uh oh!

odersky commented Jun 2, 2015

Uh oh!

smarter commented Jun 2, 2015

Uh oh!

odersky commented Jun 2, 2015

Uh oh!

smarter commented Jun 2, 2015

Uh oh!

odersky commented Jun 2, 2015

Uh oh!

smarter commented Jun 2, 2015

Uh oh!

odersky commented Jun 2, 2015

Uh oh!

DarkDimius commented Jun 2, 2015

Uh oh!

smarter commented Jun 2, 2015

Uh oh!

adriaanm commented Jun 5, 2015

Uh oh!

odersky commented Jun 5, 2015

Uh oh!

DarkDimius commented Jun 5, 2015

Uh oh!

Uh oh!