Include used types in the set of used names

[smarter]

This test demonstrate that name hashing as implemented in sbt is
incorrect: When AB2.scala replaces AB.scala the hash of x does not
change, it only contains the syntactic signature of B.x which is
this#A1, the supertypes of the type of B.x are not recorded.
Therefore, Test is not recompiled and compilation succeeds when it
shouldn't.

[smarter]

The only way I can think of fixing this would be to change ExtractAPI#makeType to return a list of supertypes and not just the actual type passed as an argument, e.g. if you have the following class hierarchy:

class A
class B[T] extends A
class X
class C extends B[Int] with X

object Test {
  val x: C = new C
}

then when you process the type C in the signature of x, you return List(C, B[Int], X, A) instead of C.

@gkossakowski : What do you think?

[smarter]

I've just pushed a commit that attempts to fix this by adding type names to the set of used names, let's see how many tests that break.

[smarter]

Looks like it works! I'll try to add more tricky test cases tomorrow. Note that when rebasing this on top of #86, I also need to add class dependencies on used types, this might severely affect the benefits of #86, I'll have to run some tests, see https://github.com/smarter/zinc/commits/class-dependencies

[gkossakowski]

Hi @smarter! Thanks for the test case and a PR. It's a good find. I've stumbled across similar problem before but I thought the problem was limited to structural types: sbt/sbt#1545.

Regarding the fix, what you pushed looks different than what you described in #87 (comment) right?

[gkossakowski]

Is the problem is that we don't capture all of used names?

[gkossakowski]

Oh, I see you answered my question in the commit message. I think your observation about us not extracting all necessary used names is spot on.
We only track used names that appear in tress, you want to extend this to types. Is it a good way to think about the issue is that we should think of Select and Apply nodes to be fully described by both their name and their signature similarly how methods and fields are identified in Java bytecode? If you imagine that after typechecking Scala source is rewritten to include full information of signatures and we extract used names from that, then I think we'll have everything covered.

Your example from the test after typechecking becomes

// AB.scala
class A0
class A1 extends A0
object B {
  val x: A1 = { (new A).();A::<init>() }
}

// Test.scala
object Test {
  val a: A0 = B.;A1::x
}

I used the (param list);returnType:: syntax to describe signatures of Selects and Applys.

[smarter]

Yes, that seems like a good way to think about it, my patch probably misses some of these names like formal parameter types which may be important.

[gkossakowski]

Would you like to refine your patch to cover the missing cases? I think you
may need to perform a full type traversal. Check out how TypeTrees are
handled in Dependency.scala.

Also, it would be good to add more cases to tests.
The ExtractUsedNamesSpecification makes it easy to test different scenarios
in isolation.

On 22 March 2016 at 14:19, Guillaume Martres [email protected]
wrote:

Yes, that seems like a good way to think about it, my patch probably
misses some of these names like formal parameter types which may be
important.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#87 (comment)

Grzegorz Kossakowski
twitter: @gkossakowski http://twitter.com/gkossakowski
github: @gkossakowski http://github.com/gkossakowski

[smarter]

Yes, I'll try to do that this evening or tomorrow.

[smarter]

Code fixed, tests added!

[smarter]

I can rebase this on top of #86 if you prefer, assuming it will be merged soon

[smarter]

0.13 backport: sbt/sbt#2523

[smarter]

If you compile your example with scalac -Xprint:typer -Xprint-types you'll notice that the type of c.f.t is actually (x: Test.c.T)Test.c.T and Test.c.T is an alias for X, so you should be able to collect both T and X easily.

[gkossakowski]

I see:

private[this] val x2: asf.X = Test.this{asf.Test.type}.c.f.t{(x: asf.X)asf.X}(Test.this{asf.Test.type}.x{asf.X}){asf.X};

With Scala version 2.11.8. Do you use the same Scala version?

[smarter]

Yes, and I don't see any asf in the code example you gave above.

[gkossakowski]

That's the package asf I stripped from my example. Let me check this with a fresh sbt project.

[gkossakowski]

I tried outside of sbt and without package asf and I get:

private[this] val x2: X = Test.this{Test.type}.c.f.t{(x: X)X}(Test.this{Test.type}.x{X}){X};

[smarter]

Weird, here's what I'm doing:

% scalac -version
Scala compiler version 2.11.8 -- Copyright 2002-2016, LAMP/EPFL
% cat sbt87.scala
class X
class A {
  type T
  class Foo { def t(x: T): T = x }
  val f: Foo = ???
}
class B extends A { type T = X }
class C extends B

object Test {
  val c = new C
  val x = new X
  val x2 = c.f.t(x)
}
% scalac -Xprint:typer -Xprint-types sbt87.scala
[[syntax trees at end of                     typer]] // sbt87.scala
package <empty>{<empty>.type} {
  class X extends scala.AnyRef {
    def <init>(): X = {
      X.super{X.super.type}.<init>{()Object}(){Object};
      (){Unit}
    }{Unit}
  };
  class A extends scala.AnyRef {
    def <init>(): A = {
      A.super{A.super.type}.<init>{()Object}(){Object};
      (){Unit}
    }{Unit};
    type T;
    class Foo extends scala.AnyRef {
      def <init>(): A.this.Foo = {
        Foo.super{Foo.super.type}.<init>{()Object}(){Object};
        (){Unit}
      }{Unit};
      def t(x: A.this.T): A.this.T = x{A.this.T}
    };
    private[this] val f: A.this.Foo = scala.this{scala.type}.Predef.???{Nothing};
    <stable> <accessor> def f: A.this.Foo = A.this{A.this.type}.f{A.this.Foo}
  };
  class B extends A {
    def <init>(): B = {
      B.super{B.super.type}.<init>{()A}(){A};
      (){Unit}
    }{Unit};
    type T = X
  };
  class C extends B {
    def <init>(): C = {
      C.super{C.super.type}.<init>{()B}(){B};
      (){Unit}
    }{Unit}
  };
  object Test extends scala.AnyRef {
    def <init>(): Test.type = {
      Test.super{Test.type}.<init>{()Object}(){Object};
      (){Unit}
    }{Unit};
    private[this] val c: C = new C{C}{()C}(){C};
    <stable> <accessor> def c: C = Test.this{Test.type}.c{C};
    private[this] val x: X = new X{X}{()X}(){X};
    <stable> <accessor> def x: X = Test.this{Test.type}.x{X};
    private[this] val x2: Test.c.T = Test.this{Test.type}.c.f.t{(x: Test.c.T)Test.c.T}(Test.this{Test.type}.x{X}){Test.c.T};
    <stable> <accessor> def x2: Test.c.T = Test.this{Test.type}.x2{Test.c.T}
  }
}

[gkossakowski]

Ah, I was using the variant with a type parameter:

class X
class A[T] {
  class Foo { def t(x: T): T = x }
  val f: Foo = ???
}
class B extends A[X]
class C extends B

object Test {
  val c = new C
  val x = new X
  val x2 = c.f.t(x)
}

Sorry for the confusion. However, that leads me to another question: how do you detect dependency on B that passes the type argument to A. Here's what I get:

private[this] val x2: X = Test.this{Test.type}.c.f.t{(x: X)X}(Test.this{Test.type}.x{X}){X};

[smarter]

Good catch, I thought that this would work in dotty since type parameters are encoded as type members but it behaves similarly, this might be something we can fix. Meanwhile, I think we can workaround this by also explicitly looking at the info of the symbols of methods.

[smarter]

This is definitely something we should have a pending test for.

[gkossakowski]

Which symbol points at dependency on B?

[smarter]

None, on the other hand we have val c = new C above, and C depends on B by inheritance, can you think of a case where that wouldn't be enough?

[smarter]

Ah, I spoke too soon on the dotty case, I was fooled by the pretty-printer which dealiases type parameters, the type is actually ((x: Test$.this.c.A$$T)Test$.this.c.A$$T) which is what I want (I just have to decode A$$T to T).

Anyway in both dotty and scalac, prefixes have singleton types, and one of the underlying type of those is C which depends on B by inheritance, so I think we're good.

[gkossakowski]

Do you propose to depend on all super classes of all classes mentioned anywhere in a type?

[smarter]

Do we need to? If C is a used name then the name hash of C will change if B changes, isn't that enough?

[gkossakowski]

Aren't we referring to B as a parent of C with a TypeRef? If so, then the hash of type ref for B doesn't change so hash of parents of C do not change and the hash of C doesn't change either.

[smarter]

Structure#parents has type xsbti.api.Type[] indeed, bu it includes all base classes with type arguments, not just direct parent, if you pretty print the API of C you get:

class C extends this#B with this#A[this#X] with java.lang.this#Object with scala.this#Any {
   ...
}

So if anything changes in the extends clause of B, it changes the name hash of C.

[gkossakowski]

Ok, good point on base classes. But what if we change class B extends A[X] to

object Bla {
  type T = X
}
class B extends A[Bla.T]

We'll refer to Bla.T in base types with a TypeRef and we lose the property that hash changes.

[smarter]

Could you make a complete example that illustrates the issue? If we just do:

class X
class A[T] {
  class Foo { def t(x: T): T = x }
  val f: Foo = ???
}
object Bla {
  type T = X
}
class B extends A[Bla.T]
class C extends B

object Test {
  val c = new C
  val x = new X
  val x2 = c.f.t(x)
}

Then we're fine because c.f.t has type (x: Bla.T)Bla.T and we just have to collect the underlying types in the set of used names.

[gkossakowski]

Ok, I'll think of a complete example. The worry I'm trying to express is that hash sums do not include all necessary information so with enough of indirection you can hide changes that should trigger a recompilation.