Fix instability of self variable API representation#2507
Conversation
This commit enables control of whether a compiler instance should be reused between compiling groups of Scala source files. Check comments in the code for why this can be useful to control.
Add a pending test that shows a problem with instability of representing self variables. This test covers the bug described in sbt#2504. In order to test API representation of a class declared either in source file or unpickled from a class file, ScalaCompilerForUnitTesting has been extended to extract APIs from multiple compiler instances sharing a classpath.
The reason for instability is a bit tricky so let's unpack what the previous code checking if there's self type declared was doing. It would check if `thisSym` of a class is equal to a symbol representing the class. If that's true, we know that there's no self type. If it's false, then `thisSym` represents either a self type or a self variable. The second (type test) was supposed to check whether the type of `thisSym` is different from a type of the class. However, it would always yield false because TypeRef of `thisSym` was compared to ClassInfoType of a class. So if you had a self variable the logic would see a self type (and that's what API representation would give you). Now the tricky bit: `thisSym` is not pickled when it's representing just a self variable because self variable doesn't affect other classes referring to a class. If you looked at a type after unpickling, the symbol equality test would yield true and we would not see self type when just a self variable was declared. The fix is to check equality of type refs on both side of the type equality check. This makes the pending test passing. Also, I added another test that checks if self types are represented in various combinations of declaring a self variable or/and self type. Fixes sbt#2504.
b37bd31 to
a6ae339
Compare
|
Review by @adriaanm |
|
I think this fix (once reviewed and merged) might warrant a new release. The bug is rather bad because it degrades performance of incremental compiler in a way that's hard to both detect and debug. The counterargument against expedited release would be that it only affects code bases that declare inner classes with self variables. |
|
We can do a milestone right away so ppl can test it if they run into this. |
| |} | ||
| |""".stripMargin | ||
| val src2 = | ||
| """|class Global { |
There was a problem hiding this comment.
Did you end up figuring out why nesting was needed to repro?
There was a problem hiding this comment.
Yes, because you need to inherit a class as a member to have its full API representation. If you just inherit from a class directly, then you're going to have API representation of its members.
|
Good stuff, @gkossakowski! Thanks for picking this up. |
Per review request, add two more test cases for self variable (no self type).
|
I added more tests. |
|
Ping |
|
@adriaanm Can I merge it? |
|
LGTM, sorry for delay |
Fix instability of self variable API representation
|
@gkossakowski Please forward port this to sbt/zinc. |
|
It's part of class-based branch that I'm preparing for merge. On 18 March 2016 at 17:50, eugene yokota notifications@github.com wrote:
Grzegorz Kossakowski |
Forward port of sbt/sbt#2507
|
Forward port in sbt/zinc#90 |
The reason for instability is a bit tricky so let's unpack what the previous code checking if there's self type declared was doing. It would check if
thisSymof a class is equal to a symbol representing the class. If that's true, we know that there's no self type. If it's false, thenthisSymrepresents either a self type or a self variable. The second (type test) was supposed to check whether the type ofthisSymis different from a type of the class. However, it would always yield false because TypeRef ofthisSymwas compared to ClassInfoType of a class. So if you had a self variable the logic would see a self type (and that's what API representation would give you).Now the tricky bit:
thisSymis not pickled when it's representing just a self variable because self variable doesn't affect other classes referring to a class. If you looked at a type after unpickling, thesymbol equality test would yield true and we would not see self type when just a self variable was declared.
The fix is to check equality of type refs on both side of the type equality check. This makes the pending test passing.
Also, I added another test that checks if self types are represented in various combinations of declaring a self variable or/and self type.
Fixes #2504.