discrepancy in owner of refinement class causes spurious recompilation in sbt

[scabug]

the pickler "localizes" a refinement class's owner,
so when a refinement type is loaded from pickled information the owner structure will be different than when it's type checked from source

the following will demonstrate this, assuming you log the creation of RefinedTypes:

  final class RefinedType0(parents: List[Type], decls: Scope, clazz: Symbol) extends RefinedType(parents, decls) {
    println("RefinedType"+(clazz.ownerChain, parents, decls)) // logging

// A.scala
package test

trait A
trait C

trait B { self: A with C => 
  class Inner {
    def b = B.this
  }
}

// Impl.scala
package test

class Impl extends B with A with C

scalac A.scala Impl.scala > all
scalac Impl.scala > single
diff -u all single

yields (snipped)

-RefinedType(List(test, package test, package <root>),List(test.B, test.A, test.C),Scope{
+RefinedType(List(B, trait B, package test, package <root>),List(test.B, test.A with test.C),Scope{

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-6596?orig=1
Reporter: @adriaanm

[scabug]

@adriaanm said:
I think the type checker should use the same owner as the pickler. It makes no sense to nest the anonymous class that represents the refinement under the first parent of the refinement.

[scabug]

@adriaanm said:
... and the main point of the issue: in principle, the owner chain of the refinement class should be insensitive to whether we're compiling from source or against class files

i'm not sure if this affects anything aside from sbt, where we can simply work around it by ignoring the symbol for the refinement type when extracting the api, as the symbol doesn't convey any information that's not already captured by the RefinedType itself (as far as I know, the symbol for the refinement class is only needed in referencing the type in itself, to avoid cycles)

[scabug]

@adriaanm said:
I believe this bug was caused by the fix for #2323.

We need owners for refinement types since we must be able to check that outer-this references in its decls refer to an enclosing class (I tried removing the owner to see what breaks). We shouldn't, however, be rewriting owners during pickling. See my comments at #2323

[scabug]

@adriaanm said:
The underlying issue (rewriting owners during pickling) cannot be fixed in 2.10, but I think it should be adressed in 2.11.0.

[scabug]

@odersky said:
I believe owners for refinement types are a compiler-internal artefact only. What do you think of leaving them as they are, but disregarding them when computing any API hashes?

[scabug]

@adriaanm said:
I agree this would fix the symptoms we're seeing, but I suspect discrepancies between pickled symbols and information derived from typing source directly will keep causing trouble. In fact, in addition to this ticket it has caused the following regressions: #2741, #4079.

I believe we should fix this properly in 2.11. For older releases SBT will indeed have to deal with the status quo. If we judge the pickler by (non-existent) specification complexity, this rewrite adds complexity. If we keep it, it must be properly documented.

Having two owner structures seems unnecessary[*] and risky: the "correct" one when we were typing from source, and the "localized" one for unpickled symbols. It seems to me SBT/IDE/Reflection/... will all need to consider both possibilities.

[*] Is there another reason besides #2323 for this localizedOwner rewrite? (There's also this hotfix for building lift on 2.7.6: http://github.com/scala/scala/commit/44e60b3ae6)

Poking around showed that anonymous function symbols were not considered "local" and references to them were thus pickled as external. Is this intended? I think pickling anonymous functions symbols as local/internal fixes #2323.

Finally, I think StubSymbol was a mistake. When you remove the localizedOwner rewrite in master, everything still works, but only because now we're silently creating these stub symbols left and right to deal with anonymous owners that cannot be found. Of course this will still blow up, albeit later and more opaquely: #6640.

[scabug]

@gkossakowski said (edited on Nov 12, 2012 10:40:40 PM UTC):
I have a REPL transcript that shows you can access RefinedType through reflection. Also, it shows that you can detect the difference in the owner chain for symbol corresponding to RefinedType.

At the moment if you obtain owner for RefinedType coming from tree type-checked by toolbox you'll get NoSymbol. I believe this is a bug in toolboxes (because with regular type-checker you get real owner assigned). However, the point is that even if toolbox get fixed to return the correct symbol that the type-checker assigns it'll be still different from the one unpickler creates. The reason for that is owner rewriting done in pickler.

I think pickling/unpickling must be an identity function.

$ cat Foo.scala 
object test3 {
  trait A; trait C
  trait B { self: A with C => 
    class Inner {
      def b = B.this
    }
  }
}


$ ./bin/scalac Foo.scala -d sandbox/

$ ./bin/scala -cp sandbox/
Welcome to Scala version 2.10.0-RC2 (Java HotSpot(TM) 64-Bit Server VM, Java 1.7.0_10-ea).
Type in expressions to have them evaluated.
Type :help for more information.

scala> :silent
Switched off result printing.

scala> :paste
// Entering paste mode (ctrl-D to finish)

import scala.reflect.runtime.universe._
import scala.reflect.runtime.{currentMirror => cm}
import scala.tools.reflect.ToolBox
val toolbox = cm.mkToolBox()
val src = """object test3 {
  trait A; trait C
  trait B { self: A with C => 
    class Inner {
      def b = B.this
    }
  }
}
"""
val tree = toolbox.parse(src)
val typedTree = toolbox.typeCheck(tree)
val Btpe = typedTree.symbol.asModule.moduleClass.asType.toType.member(newTypeName("B")).asType.toType
val bMethod = Btpe.member(newTypeName("Inner")).asType.toType.member(newTermName("b")).asMethod
val returnTpe = bMethod.returnType
val returnTpeUnpickled = typeOf[test3.B].member(newTypeName("Inner")).asType.toType.member(newTermName("b")).asMethod.returnType

// Exiting paste mode, now interpreting.


scala> :silent
Switched on result printing.

scala> returnTpe
res0: toolbox.u.Type = <expression-owner>.test3.B with <expression-owner>.test3.A with <expression-owner>.test3.C

scala> returnTpe.typeSymbol.owner
res1: toolbox.u.Symbol = <none>

scala> returnTpe.typeSymbol.owner == NoSymbol
res2: Boolean = true

scala> returnTpeUnpickled
res3: reflect.runtime.universe.Type = test3.B with test3.A with test3.C

scala> returnTpeUnpickled.typeSymbol.owner
res4: reflect.runtime.universe.Symbol = object test3

scala> returnTpeUnpickled.typeSymbol.owner == NoSymbol
res5: Boolean = false

[scabug]

@retronym said:

Finally, I think StubSymbol was a mistake. When you remove the localizedOwner rewrite in master, everything still works, but only because now we're silently creating these stub symbols left and right to deal with anonymous owners that cannot be found. Of course this will still blow up, albeit later and more opaquely: SI-6640.

Opacity reduction filter applied:

https://github.com/scala/scala/pull/1607/files#L7R1

In the same PR, I've added tests to show what StubSymbol achieves. The ends are very worthwhile, are there other means?

[scabug]

@odersky said:
There's something deeply wrong with owners and refined types. I mean, it is fundamentally a problematic design because the owner of a refined type does not really make sense. But we need it to satisfy a lot of other invariants.

My strong advice would be to do as little as you can. I am convinced there is no satisfactory fix given the current compiler architecture. So when something is messy, better stirr up as little as possible! Inconsistencies in toolboxes are no reason to lose time over this. Just ignore the owner when computing the SBT hashes and you will be fine.

[scabug]

@retronym said:
But at the same time, please commit a test case that demonstrates all the wrinkles you've found. Even if that test is verifying wrong behaviour, it will at least keep things wrong in the same way.

[scabug]

@adriaanm said:
We're not proposing making radical changes like this right now, but long-term I do think we should fix this at the root(owner).

Last-minute rewrites during pickler are just going to keep surprising people, and as we move to open up our platform through reflection. Lack of surpris becomes more and more important.

(The owner of a refinement type does matter for at least one thing: during refchecks we check that this-references refer to an enclosing class, which is decided based on the owner -- the owner of the refinement in case a this in a refinement refers to an outer class.)

[scabug]

@gkossakowski said:
My main concern is that we see different symbol/type structure depending if you create it from source or unpickle it and there's no indication in compiler internals that this should happen.

I agree that owner for refinement type is a questionable concept but since we have it we should support it.

[scabug]

@gkossakowski said:
@jason: I'm happy to do that once we agree on resolution for this bug across different branches.

[scabug]

@adriaanm said:
Unassigning and rescheduling to M6 as previous deadline was missed.

[scabug]

@adriaanm said:
Greg worked around this in SBT.

[scabug]

@retronym said (edited by @xeno-by on Jan 27, 2014 9:12:01 AM UTC):
See related discussion about refinement owners in #8177.

;tl;dr: CompoundTypeTrees in the tpts of constructor parameters give rise to refinement class symbols owned by the classes owner. These are copied verbatim into the type of the associated accessor method. Subsequent attempts to asSeenFrom from this refinement class fail to substitute type parameters of the enclosing class, as the owner chain of the refinement suggests that is is not in scope. I suspect that similar problems could arise in joint/separate compilation as a result of the behaviour described above.

Reopening so we can re-assess for 2.12

[scabug]

@xeno-by said:
I guess #7736 is somewhat related as well.