Include ONLY nonpublic vars in API hash of traits

[Duhemm]

The previous logic was wrong and included all members of traits in their
API hash, which degraded the performance of the incremental compiler.

This commit changes this logic so that only the non-public members of
traits are included into their API hash.

Fixes #2436

[typesafe-tools]

Can one of the admins verify this patch?

[eed3si9n]

@gkossakowski Could you review this?

[gkossakowski]

What happens in situation like this:

// A.scala
trait A {
  private type Foo = Int // (1) change Int to String
  private def foo: Int = 12 // (2) change Int to String and constant to a string constant
}
// B.scala
class B extends A

Will (1) or (2) trigger recompilation of B.scala? What should be the right behavior?

[eed3si9n]

See also #2155 and c6b0043

[Duhemm]

@gkossakowski In both cases, A.scala and B.scala will be recompiled, which is not required. If we revert #2160 then B.scala is not recompiled. Should we only include private vars in traits?

[Duhemm]

@gkossakowski I've added a new commit that fixes the problems you exposed and behaves correctly on the following scenario:

// A.scala
trait A {
  private var a: Int = 0 // change to String, B gets recompiled and runs fine
}

// B.scala
object B extends A {
  def main(args: Array[String]): Unit = println("Hello")
}

[gkossakowski]

I think you need to cover both private vars and vals. For the val, you have to generate a backing field. Try out changing a var to val.

[Duhemm]

You're right, it still crashes if we use vals.

[smarter]

It would be interesting to know how much of this is still valid in Scala 2.12 with the new trait encoding.

[gkossakowski]

Let's cover vals by the existing test.

[Duhemm]

Let's cover vals by the existing test.

What do you mean?

[eed3si9n]

@Duhemm I think he's asking you to test this in a scripted test by adding more changes to https://github.com/sbt/sbt/tree/0.13/sbt/src/sbt-test/source-dependencies/trait-private-var

[Duhemm]

I updated the scripted test

[gkossakowski]

Ok, so let's systematically go over the list of different private members we can have:

• private var
• private val
• private lazy val
• private type
• private class
• private trait
• private object
• private def

I checked the ones we discussed so far. What about the rest? Should the hash code of a trait include private hashes of any remaining members from the list?

[eed3si9n]

source-dependencies / trait-super failed.

[Duhemm]

Private objects must be included as well in the API hash:

// A.scala
trait A {
  val foo = 0 // + X.a  Run once and then uncomment these two lines
//  private object X { val a = 1 }    
}

// B.scala
object B extends A {
  def main(args: Array[String]): Unit = {
    println("Hello " + foo)
  }
}

> run
[info] Updating {file:/Users/martin/Desktop/trait/}trait...
[info] Resolving jline#jline;2.12.1 ...
[info] Done updating.
[info] Compiling 2 Scala sources to /Users/martin/Desktop/trait/target/scala-2.11/classes...
[info] Running B 
Hello 0
[success] Total time: 1 s, completed Feb 10, 2016 12:02:39 PM
> run
[info] Compiling 1 Scala source to /Users/martin/Desktop/trait/target/scala-2.11/classes...
[info] Running B 
[error] (run-main-9) java.lang.AbstractMethodError: B$.A$$X()LA$X$;
java.lang.AbstractMethodError: B$.A$$X()LA$X$;
    at A$class.$init$(A.scala:2)
    at B$.<init>(B.scala:1)
    at B$.<clinit>(B.scala)
    at B.main(B.scala)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
[trace] Stack trace suppressed: run last compile:run for the full output.
java.lang.RuntimeException: Nonzero exit code: 1
    at scala.sys.package$.error(package.scala:27)
[trace] Stack trace suppressed: run last compile:run for the full output.
[error] (compile:run) Nonzero exit code: 1
[error] Total time: 1 s, completed Feb 10, 2016 12:02:44 PM

We get exactly the same result by replacing the private object by a private lazy val

[gkossakowski]

Yes, objects are resembling closely lazy vals at their encoding. The type checking rules are different (objects introduce singleton types) but here we are mostly worried about what's going on at the byte code level.
I checked private objects for you, what do you think should happen to the rest? Also, it's good idea to try out closing something from the outer scope (e.g. constructor parameter) and see what happens. I suspect you don't need to include hashes of private defs defined in the source but you may need to include the hash of a synthetic member like $init$. Or, even better, just treat $init$ as non private.

[eed3si9n]

Assuming this is a 0.13.10-RC show stopper, and we need to 0.13.10-RC3, how much more time do you guys need to settle this? Could we fix the most common cases for now, and accept some perf regression?

[Duhemm]

@eed3si9n To fix source-dependencies/trait-super, we need to include some private defs, like B$$super$x in the case of this test. Maybe we should include all private methods whose name contains $?

@gkossakowski If I try with something coming from outside of the trait, then this something will be part of the API of the trait and once modified the clients of the trait will be recompiled. I guess I misunderstood what you meant?

I have been trying to add private inner traits and classes and looking at the resulting byte code in the implementing classes, but I do not see any mention of these classes in them.

What I have now is:

def isTraitBreaker(d: Definition): Boolean = {
  d match {
    case fl: FieldLike => true
    case cl: ClassLike => cl.definitionType == DefinitionType.Module
    case d: Def        => d.name contains "$"
    case _             => false
  }
}

[eed3si9n]

Maybe we should include all private methods whose name contains $?

That feels hacky, but I don't have any good alternative.

[gkossakowski]

That feels hacky, but I don't have any good alternative.

I think you want to check if a method symbol has the SUPERACCESSOR flag set and treat it as non-private in that case. Checking for dollars is fragile, e.g. dollars are used to encode names of identifiers that have spaces or unicode characters in them.

[eed3si9n]

LGTM

[gkossakowski]

LGTM

Good work!

[gkossakowski]

And thanks for the recap.

[gkossakowski]

Shall commits be squashed a little bit?

[eed3si9n]

I'm not too fussy about the git history. I'll just merge this as is

[gkossakowski]

@Duhemm why is !topLevel here?

[Duhemm]

@gkossakowski Because if topLevel is true, then we have defs == ds.

[gkossakowski]

I think I don't understand the original logic. Why do we care about whether definitions are enclosed by a top level class or not? Or is it about top level classes themselves?

[Duhemm]

My understanding is that topLevel = true for definitions that are only enclosed in a package.