Merge pull request #2441 from Duhemm/wip/fix-2436

Include ONLY nonpublic vars in API hash of traits

compile/api/src/main/scala/sbt/ClassToAPI.scala

@@ -261,7 +261,7 @@ object ClassToAPI {
   def modifiers(i: Int): api.Modifiers =
     {
       import Modifier.{ isAbstract, isFinal }
-      new api.Modifiers(isAbstract(i), false, isFinal(i), false, false, false, false)
+      new api.Modifiers(isAbstract(i), false, isFinal(i), false, false, false, false, false)
     }
   def access(i: Int, pkg: Option[String]): api.Access =
     {

compile/api/src/main/scala/xsbt/api/APIUtil.scala

@@ -12,7 +12,7 @@ object APIUtil {
   }
   val byteToModifiers = (b: Byte) => {
     def x(bit: Int) = (b & (1 << bit)) != 0
-    new Modifiers(x(0), x(1), x(2), x(3), x(4), x(5), x(6))
+    new Modifiers(x(0), x(1), x(2), x(3), x(4), x(5), x(6), x(7))
   }
 
   def isScalaSourceName(name: String): Boolean = name.endsWith(".scala")

compile/api/src/main/scala/xsbt/api/HashAPI.scala

@@ -139,16 +139,37 @@ final class HashAPI(includePrivate: Boolean, includeParamNames: Boolean, include
     {
       hash = startHash(0)
       hashSymmetric(s.packages, hashPackage)
-      hashDefinitions(s.definitions, true)
+      hashDefinitions(s.definitions, topLevel = true,  isTrait = false)
       finalizeHash
     }
 
   def hashPackage(p: Package) = hashString(p.name)
 
+  @deprecated("Use the overload that indicates if the enclosing definition is a trait.", "0.14")
   def hashDefinitions(ds: Seq[Definition], topLevel: Boolean): Unit =
+    hashDefinitions(ds, topLevel, isTrait = false)
+
+  def hashDefinitions(ds: Seq[Definition], topLevel: Boolean, isTrait: Boolean): Unit =
     {
+      def isPublic(d: Definition): Boolean = d.access match { case _: xsbti.api.Public => true; case _ => false }
+      def isTraitBreaker(d: Definition): Boolean = d match {
+        // Vars and vals in traits introduce getters, setters and fields in the implementing classes.
+        // See test `source-dependencies/trait-private-var
+        case _: FieldLike  => true
+        // Objects in traits introduce fields in the implementing classes.
+        // See test `source-dependencies/trait-private-object`
+        case cl: ClassLike => cl.definitionType == DefinitionType.Module
+        // super calls introduce accessors that are not part of the public API
+        case d: Def        => d.modifiers.isSuperAccessor
+        case _             => false
+      }
+      val includedPrivateDefinitions =
+        if (!includePrivate && !topLevel && isTrait) {
+          ds filter (x => isTraitBreaker(x) && !isPublic(x))
+        } else Seq.empty
+
       val defs = SameAPI.filterDefinitions(ds, topLevel, includePrivate)
-      hashSymmetric(defs, hashDefinition)
+      hashSymmetric(includedPrivateDefinitions ++ defs, hashDefinition)
     }
 
   /**
@@ -356,9 +377,9 @@ final class HashAPI(includePrivate: Boolean, includeParamNames: Boolean, include
   def hashStructure0(structure: Structure, includeDefinitions: Boolean, isTrait: Boolean = false): Unit = {
     extend(StructureHash)
     hashTypes(structure.parents, includeDefinitions)
-    if (includeDefinitions || isTrait) {
-      hashDefinitions(structure.declared, isTrait)
-      hashDefinitions(structure.inherited, isTrait)
+    if (includeDefinitions) {
+      hashDefinitions(structure.declared, topLevel = false, isTrait)
+      hashDefinitions(structure.inherited, topLevel = false, isTrait)
     }
   }
   def hashParameters(parameters: Seq[TypeParameter], base: Type): Unit =

compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala

@@ -165,20 +165,39 @@ class NameHashingSpecification extends Specification {
   }
 
   /**
-   * Checks that private members are included in the hash of the public API of traits.
-   * Including the private members of traits is required because classes that implement a trait
-   * have to define the private members of the trait. Therefore, if a private member of a trait is added,
-   * modified or removed we need to recompile the classes that implement this trait.
+   * Checks that private vars are included in the hash of the public API of traits.
+   * Including the private vars of traits is required because classes that implement a trait
+   * have to define getters and setters for these vars.
    * For instance, if trait Foo is initially defined as:
-   *     trait Foo { private val x = new A }
+   *     trait Foo { private var x = new A }
    * changing it to
-   *     trait Foo { private val x = new B }
+   *     trait Foo { private var x = new B }
    * requires us to recompile all implementors of trait Foo, because scalac generates setters and getters
-   * for the private fields of trait Foo in its implementor. If the clients of trait Foo are not recompiled,
+   * for the private vars of trait Foo in its implementor. If the clients of trait Foo are not recompiled,
    * we get abstract method errors at runtime, because the types expected by the setter (for instance) does not
    * match.
    */
-  "private members in traits" in {
+  "private var in traits are included in API hash" in {
+    /* trait Foo { private var x } */
+    val fooTrait1 =
+      simpleTrait("Foo",
+        simpleStructure(new Var(emptyType, "x", privateAccess, defaultModifiers, Array.empty)),
+        publicAccess)
+
+    /* trait Foo */
+    val fooTrait2 =
+      simpleTrait("Foo",
+        simpleStructure(),
+        publicAccess)
+
+    val api1 = new SourceAPI(Array.empty, Array(fooTrait1))
+    val api2 = new SourceAPI(Array.empty, Array(fooTrait2))
+
+    HashAPI(api1) !== HashAPI(api2)
+
+  }
+
+  "private vals in traits are included in API hash" in {
     /* trait Foo { private val x } */
     val fooTrait1 =
       simpleTrait("Foo",
@@ -198,15 +217,97 @@ class NameHashingSpecification extends Specification {
 
   }
 
+  "private objects in traits are included in API hash" in {
+    /* trait Foo { private object x } */
+    val fooTrait1 =
+      simpleTrait("Foo",
+        simpleStructure(
+          new ClassLike(DefinitionType.Module, lzy(emptyType), lzy(simpleStructure()), Array.empty, Array.empty, "x", privateAccess, defaultModifiers, Array.empty)),
+        publicAccess)
+
+    /* trait Foo */
+    val fooTrait2 =
+      simpleTrait("Foo",
+        simpleStructure(),
+        publicAccess)
+
+    val api1 = new SourceAPI(Array.empty, Array(fooTrait1))
+    val api2 = new SourceAPI(Array.empty, Array(fooTrait2))
+
+    HashAPI(api1) !== HashAPI(api2)
+
+  }
+
+  "private non-synthetic def in traits are not included in API hash" in {
+    /* trait Foo { private def x } */
+    val fooTrait1 =
+      simpleTrait("Foo",
+        simpleStructure(new Def(Array.empty, emptyType, Array.empty, "x", privateAccess, defaultModifiers, Array.empty)),
+        publicAccess)
+
+    /* trait Foo */
+    val fooTrait2 =
+      simpleTrait("Foo",
+        simpleStructure(),
+        publicAccess)
+
+    val api1 = new SourceAPI(Array.empty, Array(fooTrait1))
+    val api2 = new SourceAPI(Array.empty, Array(fooTrait2))
+
+    HashAPI(api1) === HashAPI(api2)
+
+  }
+
+  "private synthetic def in traits are included in API hash" in {
+    /* trait Foo { private <superaccessor> def x } */
+    val modifiers = new xsbti.api.Modifiers(false, false, false, false, false, false, false, true)
+    val fooTrait1 =
+      simpleTrait("Foo",
+        simpleStructure(new Def(Array.empty, emptyType, Array.empty, "x", privateAccess, modifiers, Array.empty)),
+        publicAccess)
+
+    /* trait Foo */
+    val fooTrait2 =
+      simpleTrait("Foo",
+        simpleStructure(),
+        publicAccess)
+
+    val api1 = new SourceAPI(Array.empty, Array(fooTrait1))
+    val api2 = new SourceAPI(Array.empty, Array(fooTrait2))
+
+    HashAPI(api1) !== HashAPI(api2)
+
+  }
+
+  "private types in traits are included not in API hash" in {
+    /* trait Foo { private type x } */
+    val fooTrait1 =
+      simpleTrait("Foo",
+        simpleStructure(new TypeAlias(emptyType, Array.empty, "x", privateAccess, defaultModifiers, Array.empty)),
+        publicAccess)
+
+    /* trait Foo */
+    val fooTrait2 =
+      simpleTrait("Foo",
+        simpleStructure(),
+        publicAccess)
+
+    val api1 = new SourceAPI(Array.empty, Array(fooTrait1))
+    val api2 = new SourceAPI(Array.empty, Array(fooTrait2))
+
+    HashAPI(api1) === HashAPI(api2)
+
+  }
+
   /**
-   * Checks that private members in non-top-level traits are included as well.
+   * Checks that private vars in non-top-level traits are included as well.
    */
-  "private members in nested traits" in {
-    /* class A { trait Foo { private val x } } */
+  "private variables in nested traits are include in the API hash" in {
+    /* class A { trait Foo { private var x } } */
     val classA1 =
       simpleClass("A",
         simpleTrait("Foo",
-          simpleStructure(new Val(emptyType, "x", privateAccess, defaultModifiers, Array.empty)),
+          simpleStructure(new Var(emptyType, "x", privateAccess, defaultModifiers, Array.empty)),
           publicAccess))
 
     /* class A { trait Foo } */
@@ -226,12 +327,12 @@ class NameHashingSpecification extends Specification {
   /**
    * Checks that private traits are NOT included in the hash.
    */
-  "private traits" in {
-    /* class Foo { private trait T { private val x } } */
+  "private inner traits are not included in the API hash" in {
+    /* class Foo { private trait T { private var x } } */
     val classFoo1 =
       simpleClass("Foo",
         simpleTrait("T",
-          simpleStructure(new Val(emptyType, "x", privateAccess, defaultModifiers, Array.empty)),
+          simpleStructure(new Var(emptyType, "x", privateAccess, defaultModifiers, Array.empty)),
           privateAccess))
 
     /** class Foo { private trait T } */
@@ -256,10 +357,10 @@ class NameHashingSpecification extends Specification {
    * Checks that private members are NOT included in the hash of the public API of classes.
    */
   "private members in classes are not included in the api hash" in {
-    /* class Foo { private val x } */
+    /* class Foo { private var x } */
     val classFoo1 =
       simpleClass("Foo",
-        simpleStructure(new Val(emptyType, "x", privateAccess, defaultModifiers, Array.empty)))
+        simpleStructure(new Var(emptyType, "x", privateAccess, defaultModifiers, Array.empty)))
 
     /* class Foo */
     val classFoo2 =
@@ -274,9 +375,9 @@ class NameHashingSpecification extends Specification {
   }
 
   /**
-    * Checks that private members do NOT contribute to name hashes.
-    * Test for https://github.com/sbt/sbt/issues/2324
-    */
+   * Checks that private members do NOT contribute to name hashes.
+   * Test for https://github.com/sbt/sbt/issues/2324
+   */
   "private members in classes do not contribute to name hashes" in {
     /* class Foo { private val x } */
     val classFoo =
@@ -336,6 +437,6 @@ class NameHashingSpecification extends Specification {
   private val strTpe = new Projection(emptyType, "String")
   private val publicAccess = new Public
   private val privateAccess = new Private(new Unqualified)
-  private val defaultModifiers = new Modifiers(false, false, false, false, false, false, false)
+  private val defaultModifiers = new Modifiers(false, false, false, false, false, false, false, false)
 
 }

compile/inc/src/test/scala/sbt/inc/TestCaseGenerators.scala

@@ -76,7 +76,7 @@ object TestCaseGenerators {
   private[this] def makeDefinition(name: String): Definition =
     new ClassLike(DefinitionType.ClassDef, lzy(new EmptyType()),
       lzy(new Structure(lzy(Array()), lzy(Array()), lzy(Array()))), Array(), Array(),
-      name, new Public(), new Modifiers(false, false, false, false, false, false, false), Array())
+      name, new Public(), new Modifiers(false, false, false, false, false, false, false, false), Array())
 
   private[this] def lzy[T <: AnyRef](x: T) = SafeLazy.strict(x)
 

compile/interface/src/main/scala/xsbt/ExtractAPI.scala

@@ -438,7 +438,7 @@ class ExtractAPI[GlobalType <: CallbackGlobal](val global: GlobalType,
       val absOver = s.hasFlag(ABSOVERRIDE)
       val abs = s.hasFlag(ABSTRACT) || s.hasFlag(DEFERRED) || absOver
       val over = s.hasFlag(OVERRIDE) || absOver
-      new xsbti.api.Modifiers(abs, over, s.isFinal, s.hasFlag(SEALED), isImplicit(s), s.hasFlag(LAZY), hasMacro(s))
+      new xsbti.api.Modifiers(abs, over, s.isFinal, s.hasFlag(SEALED), isImplicit(s), s.hasFlag(LAZY), hasMacro(s), s.hasFlag(SUPERACCESSOR))
     }
 
   private def isImplicit(s: Symbol) = s.hasFlag(Flags.IMPLICIT)

interface/src/main/java/xsbti/api/Modifiers.java

@@ -9,13 +9,14 @@ public final class Modifiers implements java.io.Serializable
 	private static final int ImplicitBit = 4;
 	private static final int LazyBit = 5;
 	private static final int MacroBit = 6;
+	private static final int SuperAccessorBit = 7;
 
 	private static int flag(boolean set, int bit)
 	{
 		return set ? (1 << bit) : 0;
 	}
 
-	public Modifiers(boolean isAbstract, boolean isOverride, boolean isFinal, boolean isSealed, boolean isImplicit, boolean isLazy, boolean isMacro)
+	public Modifiers(boolean isAbstract, boolean isOverride, boolean isFinal, boolean isSealed, boolean isImplicit, boolean isLazy, boolean isMacro, boolean isSuperAccessor)
 	{
 		this.flags = (byte)(
 			flag(isAbstract, AbstractBit) |
@@ -24,7 +25,8 @@ public final class Modifiers implements java.io.Serializable
 			flag(isSealed, SealedBit) |
 			flag(isImplicit, ImplicitBit) |
 			flag(isLazy, LazyBit) |
-			flag(isMacro, MacroBit)
+			flag(isMacro, MacroBit) |
+			flag(isSuperAccessor, SuperAccessorBit)
 		);
 	}
 
@@ -68,6 +70,10 @@ public final class Modifiers implements java.io.Serializable
 	{
 		return flag(MacroBit);
 	}
+	public final boolean isSuperAccessor()
+	{
+		return flag(SuperAccessorBit);
+	}
 	public boolean equals(Object o)
 	{
 		return (o instanceof Modifiers) && flags == ((Modifiers)o).flags;
@@ -78,6 +84,6 @@ public final class Modifiers implements java.io.Serializable
 	}
 	public String toString()
 	{
-		return "Modifiers(" + "isAbstract: " + isAbstract() + ", " + "isOverride: " + isOverride() + ", " + "isFinal: " + isFinal() + ", " + "isSealed: " + isSealed() + ", " + "isImplicit: " + isImplicit() + ", " + "isLazy: " + isLazy() + ", " + "isMacro: " + isMacro()+ ")";
+		return "Modifiers(" + "isAbstract: " + isAbstract() + ", " + "isOverride: " + isOverride() + ", " + "isFinal: " + isFinal() + ", " + "isSealed: " + isSealed() + ", " + "isImplicit: " + isImplicit() + ", " + "isLazy: " + isLazy() + ", " + "isMacro: " + isMacro()+ ", isSuperAccessor:" + isSuperAccessor() + ")";
 	}
 }

sbt/src/sbt-test/source-dependencies/trait-member-modified/build.sbt

@@ -0,0 +1,27 @@
+/* Performs checks related to compilations:
+ *  a) checks in which compilation given set of files was recompiled
+ *  b) checks overall number of compilations performed
+ */
+TaskKey[Unit]("check-compilations") := {
+  val analysis = (compile in Compile).value
+  val srcDir = (scalaSource in Compile).value
+  def relative(f: java.io.File): java.io.File =  f.relativeTo(srcDir) getOrElse f
+  val allCompilations = analysis.compilations.allCompilations
+  val recompiledFiles: Seq[Set[java.io.File]] = allCompilations map { c =>
+    val recompiledFiles = analysis.apis.internal.collect {
+      case (file, api) if api.compilation.startTime == c.startTime => relative(file)
+    }
+    recompiledFiles.toSet
+  }
+  def recompiledFilesInIteration(iteration: Int, fileNames: Set[String]) = {
+    val files = fileNames.map(new java.io.File(_))
+    assert(recompiledFiles(iteration) == files, "%s != %s".format(recompiledFiles(iteration), files))
+  }
+  assert(allCompilations.size == 2)
+  // B.scala is just compiled at the beginning
+  recompiledFilesInIteration(0, Set("B.scala"))
+  // A.scala is changed and recompiled
+  recompiledFilesInIteration(1, Set("A.scala"))
+}
+
+logLevel := Level.Debug

sbt/src/sbt-test/source-dependencies/trait-member-modified/changes/A1.scala

@@ -0,0 +1,3 @@
+trait A {
+  def foo: Int = 12
+}

sbt/src/sbt-test/source-dependencies/trait-member-modified/src/main/scala/A.scala

@@ -0,0 +1 @@
+trait A

sbt/src/sbt-test/source-dependencies/trait-member-modified/src/main/scala/B.scala

@@ -0,0 +1 @@
+class B(a: A)

sbt/src/sbt-test/source-dependencies/trait-member-modified/test

@@ -0,0 +1,9 @@
+# Test if adding a member to a trait affects classes that refer to that trait
+# by a member reference
+> compile
+# add `foo` method to `A`
+$ copy-file changes/A1.scala src/main/scala/A.scala
+# only A.scala should be recompiled
+> compile
+# check if there are only two compile iterations performed
+> check-compilations

sbt/src/sbt-test/source-dependencies/trait-private-object/A.scala

@@ -0,0 +1,3 @@
+trait A {
+  val foo = 0
+}

sbt/src/sbt-test/source-dependencies/trait-private-object/B.scala

@@ -0,0 +1,5 @@
+object B extends A {
+  def main(args: Array[String]): Unit = {
+    println(foo)
+  }
+}

sbt/src/sbt-test/source-dependencies/trait-private-object/changes/A.scala

@@ -0,0 +1,4 @@
+trait A {
+  val foo = 0 + X.a
+  private object X { val a = 1 }
+}

sbt/src/sbt-test/source-dependencies/trait-private-object/test

@@ -0,0 +1,5 @@
+> run
+
+$ copy-file changes/A.scala A.scala
+
+> run

sbt/src/sbt-test/source-dependencies/trait-private-var/changes/A2.scala

@@ -0,0 +1,5 @@
+trait A {
+	private val foo = 12
+	// we need to access foo to trigger AbstractMethodError
+	def bar: Int = foo
+}

sbt/src/sbt-test/source-dependencies/trait-private-var/test

@@ -1,14 +1,28 @@
+$ copy-file changes/A0.scala A.scala
+
 # compile and run for the first time to verify that everything works
 > run
 
 # introduce private var and refer to it in a trait that we inherit from
 # there'll be pair of getters and setters generated for private var that
 # has to be implemented by a class (where you can declare corresponding field)
-$ copy-file changes/A.scala A.scala
+$ copy-file changes/A1.scala A.scala
 
-# this fails with AbstractMethodError because getters and setters for
-# a private var are not generated because introduction of a private var
-# does not trigger recompilation of B
-# B is not recompiled because incremental compiler tracks only public
-# interace (members visible from outside of given trait/class)
+# If the introduction of a private var did not trigger the recompilation of B,
+# then this will fail with AbstractMethodError because the getters and setters
+# for the private var have not been generated.
+> run
+
+# Try again with a private val
+> clean
+
+$ copy-file changes/A0.scala A.scala
+
+# compile and run a clean project to verify that everything works
+> run
+
+# introduce a private val in the trait
+$ copy-file changes/A2.scala A.scala
+
+# Verify that B has been recompiled and that everything runs fine.
 > run