SI-7046 partial fix to knownDirectSubclasses for reflection users and macro authors #5284

Merged
merged 1 commit into from Dec 1, 2016

Projects

None yet
@milessabin
Contributor
milessabin commented Jul 15, 2016 edited

This is a 95% fix of SI-7046 for comment and feedback.

This appears to do the right thing in the most typical scenarios in which knownDirectSubclasses would be used. The missing 5% is that subclasses defined in local scopes might not be seen by knownDirectSubclasses (see Local and Riddle in the test below). In mitigation, though, it is almost certain that a local subclass would represent an error in any scenario where knownDirectSubclasses might be used.

Errors for such situations are reported by recording (via a symbol attachment) that knownDirectSubclasses has been called and reporting an error if any additional children are added subsequently.

Despite these limitations and caveats, I believe that this represents a huge improvement over the status quo, and would eliminate 100% of the failures that I've seen in practice with people using shapeless for type class derivation.

Aside from that, all (par)tests pass.

@xeno-by xeno-by commented on the diff Jul 15, 2016
...piler/scala/tools/nsc/typechecker/ContextErrors.scala
@@ -1175,6 +1172,9 @@ trait ContextErrors {
def MissingParameterOrValTypeError(vparam: Tree) =
issueNormalTypeError(vparam, "missing parameter type")
+ def ParentSealedInheritanceError(parent: Tree, psym: Symbol) =
+ NormalTypeError(parent, "illegal inheritance from sealed " + psym )
+
@xeno-by
xeno-by Jul 15, 2016 Member

Why do we need to reorder this error message?

@milessabin
milessabin Jul 15, 2016 Contributor

It's moved from ContextErrors to NamerContextErrors so that it's accessible in Namers.

@xeno-by xeno-by commented on the diff Jul 15, 2016
src/reflect/scala/reflect/internal/Symbols.scala
@@ -113,6 +113,13 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
def knownDirectSubclasses = {
// See `getFlag` to learn more about the `isThreadsafe` call in the body of this method.
if (!isCompilerUniverse && !isThreadsafe(purpose = AllOps)) initialize
+
+ enclosingPackage.info.decls.foreach { sym =>
+ if(sourceFile == sym.sourceFile) {
@xeno-by
xeno-by Jul 15, 2016 Member

What does this check amount to?

@milessabin
milessabin Jul 15, 2016 Contributor

The sealedness constraint means that we only need to search within the same compilation unit.

@xeno-by
Member
xeno-by commented Jul 15, 2016

I think that it may still be possible to address an important case of subclasses nested in a top-level object. Do you think you'll need an involved context for that?

@milessabin
Contributor

@xeno-by the scenario you described is handled ... see the test.

@milessabin
Contributor

Specifically Wibble and Wobble nested in object Foo.

@dwijnand
Member

Maybe I'm just fixating too much on the name, but it's knownDirectSubclasses, not allDirectSubclasses - ie. known at the level in which you ask them, anything declared in some local scope isn't known.

So I would expect that if you called Test.subs in those local scopes it would see those subclass too. Perhaps it would be good to add those cases to the test.

@milessabin
Contributor
milessabin commented Jul 15, 2016 edited

@dwijnand yes, that's true, but I would prefer to see an error reported in that case ... it would be reintroducing a compilation order dependency which this PR is intended to eliminate.

@Blaisorblade
Contributor

IIUC, the biggest potential danger is the added call to the new method forceDirectSuperclasses in knowDirectSubclasses, because it forces more types than before and that's a well-known source of bugs. The danger is a forcing too much too early; to me, earlier discussions on SI-7046 assumed (implicitly) this couldn't be done. And while tests pass, that might of course be insufficient.

I'd guess the patch should spell out (in code) an informal argument why that's safe, to allow inspecting it for corner cases and writing appropriate testcases.

In particular, this should be fine if classes are typechecked in inheritance order, but is that really the case in Scalac? Can we loop Scalac with this patch by calling knowDirectSubclasses on an inheritance cycle?

@milessabin
Contributor

@Blaisorblade the use of maybeInitialize means that cycles are harmless. The Unrelated class explicitly tests the case where there is a cycle (via the reference to the macro-defined val which is triggering the force) ... this example was suggested by @retronym.

@xeno-by
Member
xeno-by commented Jul 15, 2016

@milessabin Very interesting! Why?

@milessabin
Contributor

@xeno-by we only care about initialising things sufficiently to ensure that children add themselves to their sealed parents. These types won't participate in a cycle (if they did it would be an error which we would expect to be reported), so if we initialise as far as we can we'll get the parent-child relationships that we need. If there are no objectionable cycles we'll get the correct (modulo local classes) result; if there are objectionable cycles they will be an error and be reported as such.

@cb372 cb372 referenced this pull request in julien-truffaut/Monocle Jul 16, 2016
Open

add macro annotation to generate Prism for sealed trait #281

@notxcain

Any chance of getting this in 2.12.0?

@retronym retronym and 1 other commented on an outdated diff Jul 20, 2016
src/compiler/scala/tools/nsc/typechecker/Namers.scala
@@ -1675,6 +1696,9 @@ trait Namers extends MethodSynthesis {
abstract class TypeCompleter extends LazyType {
val tree: Tree
+ override def forceDirectSuperclasses: Unit = {
+ tree.foreach(t => Option(t.symbol).map(_.maybeInitialize))
@retronym
retronym Jul 20, 2016 edited Member

I believe this should be restricted to DefTrees:

tree.foreach {
  case dt: DefTree if dt.symbol != null => dt.symbol.maybeInitialize
  case _ =>
}
@retronym
retronym Jul 20, 2016 Member

I think that maybeInitialize is a weak link in this chain. It is defined as:

    def maybeInitialize = {
      try   { initialize ; true }
      catch { case _: CyclicReference => debuglog("Hit cycle in maybeInitialize of $this") ; false }
    }

The problem is that this doesn't undo any side effects performed by the type completer before it ran into the cycle. The type completer will be run again, and we might observe problems if those side effects aren't idempotent.

maybeInitialize is currently only used in -Ybreak-cycles, and was described as "sketchy" when it landed: 432f936

@milessabin
milessabin Jul 20, 2016 Contributor

Agreed on the DefTree restriction.

On the sketchiness of maybeInitialize ... yes, side-effects won't be undone. However any side effects that are triggered before a cycle is observed should be stable, ie. we're merely moving them to an earlier point rather than changing the outcome.

Also, this path will only be followed if knownDirectSubclasses is called, and the result can't be any sketchier than the status quo.

Could we run a community build with this change and see how it plays out?

@milessabin
milessabin Jul 20, 2016 Contributor

@retronym any thoughts on the idea of using a flag to record that knownDirectSubclasses has been observed so that we can error if any additional subclasses add themselves later?

@milessabin
Contributor

As an additional data point wrt maybeInitialize I can confirm that shapeless builds and tests perfectly with this patch applied. It has several uses of knownDirectSubclasses and these are exercised heavily in its tests and examples.

@retronym
Member
retronym commented Jul 21, 2016 edited

Here's a very contrived example to that shows a leakage in maybeInitialize.

package p1

import scala.reflect.macros.blackbox._
import language.experimental._

object Macro {
  def impl(c: Context): c.Tree = {
    import c.universe._
    println(rootMirror.staticClass("p1.Base").knownDirectSubclasses)
    q"()"
  }
  def p1_Base_knownDirectSubclasses: Unit = macro impl
}
package p1

sealed trait Base

object Test {
  val x = {
    Macro.p1_Base_knownDirectSubclasses
    ""
  } 
}

case class B(val b: Test.x.type)

This issues a cyclic error under this PR. I think this is because the CyclicError is being caught in typedInternal:

      try runTyper() catch {
        case ex: TypeError =>
          tree.clearType()
          // The only problematic case are (recoverable) cyclic reference errors which can pop up almost anywhere.
          typingStack.printTyping(tree, "caught %s: while typing %s".format(ex, tree)) //DEBUG
          reportTypeError(context, tree.pos, ex)
          setError(tree)
        case ex: Exception =>
          // @M causes cyclic reference error
          devWarning(s"exception when typing $tree, pt=$ptPlugins")
          if (context != null && context.unit.exists && tree != null)
            logError("AT: " + tree.pos, ex)
          throw ex
      }

rather than bubbling up to the catch-and-discard in maybeInitialize.

This isn't quite what I had in mind above, but it does qualify as a side-effect (issuing the type error).

The example itself needs a bit more minimization before its worthy of detailed discussion. But it shows basis for my unease about maybeInitialize is its current state.

I notice that you've put the forcing in directKnownSubclasses in the public reflection API which is good in the sense that this won't be triggered when the typer calls children in CheckabilityChecker; we know that the problems will only affect files that use shapeless-like macros.

I'm going to keep thinking about this one.

@milessabin
Contributor

@retronym would be keen to get your take on the CyclicReference propagation fix in the most recent commit.

@milessabin
Contributor
milessabin commented Jul 21, 2016 edited

Failures appear to be transient ...

@milessabin
Contributor

/rebuild

@retronym retronym commented on an outdated diff Jul 22, 2016
src/compiler/scala/tools/nsc/typechecker/Contexts.scala
@@ -265,6 +265,9 @@ trait Contexts { self: Analyzer =>
def inSecondTry_=(value: Boolean) = this(SecondTry) = value
def inReturnExpr = this(ReturnExpr)
def inTypeConstructorAllowed = this(TypeConstructorAllowed)
+ def propagateCyclicReferences_=(value: Boolean)
+ = this(PropagateCyclicReferences) = value
+ def propagateCyclicReferences = this(PropagateCyclicReferences)
@retronym
retronym Jul 22, 2016 Member

I think this is more appropriate as a single boolean for the entire Global, rather than one per context. Typechecking in one context can call an type competer in another context which wouldn't have this flag set.

I say that we have another thing called RecoverableCyclicReference. Unfortunately, I'm not familar with it. But it seems like it might be something that should be incorporated into this this design. Maybe when the compiler is in propagate mode, cyclic errors should be thrown as the recoverable version?

@milessabin
Contributor
milessabin commented Jul 22, 2016 edited

@retronym apropos RecoverableCyclicReference vs. CyclicReference my understanding is that a RecoverableCyclicReference indicates that a cycle has been encountered but that the typer should continue to follow alternative paths if any ... only once the alternatives are exhausted is a non-recoverable CyclicReference thrown. I think that RecoverableCyclicReference is recoverable precisely because it's just an ordinary subtype of TypeError with no special handling.

If this is the case then I think what I'm doing here is correct: the only things which result in a CyclicReference being thrown are definitely not-recoverable and should be reported in the normal case. In the maybeInitialize case I believe that it's correct to suppress the report and propagate to the try/catch at the top.

Apropos the flag being context-based vs. global: I take your point about the flag not being propagated to the context associated with a different completer. What would you recommend as a mechanism for representing the flag globally? A counter var on Global?

@adriaanm adriaanm modified the milestone: 2.12.1 Jul 26, 2016
@adriaanm
Member

For now, I've assigned this to 2.12.1. We can discuss whether we can make an exception to the RC cycle rules for this one (if it can be fixed post 2.12.0, it should be), but we first need to focus on clearing the PRs blocking RC1.

@milessabin
Contributor

I've done as @retronym suggested and replaced the context-based flag with a global counted one. It seems that the existing tests already exercise typechecking in one context calling a type completer in another context. I'd welcome suggestions for more tests which bear on that point.

@milessabin
Contributor
milessabin commented Aug 9, 2016 edited

FTR, the use of global mutable state makes me ... uncomfortable. But it's in good company in this area and there doesn't seem to be any straightforward way of avoiding it.

@milessabin milessabin changed the title from Proof of concept fix for SI-7046 to Fix for SI-7046 Aug 9, 2016
@milessabin
Contributor

Title and description updated to reflect where we've got to.

@milessabin milessabin changed the title from Fix for SI-7046 to Partial fix for SI-7046 Aug 9, 2016
@milessabin
Contributor

Squashed and rebased.

@soc
Member
soc commented Aug 26, 2016 edited

Does anyone know what happened to CI? It seems to be stuck here.

@milessabin
Contributor

Backport to 2.11.8 available here.

@SethTisue SethTisue referenced this pull request in scala/scala-jenkins-infra Aug 26, 2016
Closed

Jenkins isn't validating PR 5284, hey Scabot wtf? #185

@ikhoon
ikhoon commented Aug 28, 2016

👍

@philwills philwills referenced this pull request in guardian/marley Sep 6, 2016
Merged

Unions #3

@philwills philwills added a commit to guardian/marley that referenced this pull request Sep 6, 2016
@philwills philwills Move test thrift into separate project
By forcing it into a separate compilation unit we can get scalac to be happy. This is a workaround for  https://issues.scala-lang.org/browse/SI-7046 for which there's an open PR scala/scala#5284.
9dcf04a
@philwills philwills added a commit to guardian/marley that referenced this pull request Sep 6, 2016
@philwills philwills Move test thrift into separate project
By forcing it into a separate compilation unit we can get scalac to be happy. This is a workaround for  https://issues.scala-lang.org/browse/SI-7046 for which there's an open PR scala/scala#5284.
c804aef
@philwills philwills added a commit to guardian/marley that referenced this pull request Sep 6, 2016
@philwills philwills Move test thrift into separate project
By forcing it into a separate compilation unit we can get scalac to be happy. This is a workaround for  https://issues.scala-lang.org/browse/SI-7046 for which there's an open PR scala/scala#5284.
d86276e
@philwills philwills referenced this pull request in guardian/marley Sep 6, 2016
Merged

Move test thrift into separate project #4

@SethTisue
Member

Jenkins says:

# Failed test paths (this command will update checkfiles)
partest --update-check \
  /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/run/t5418.scala \
  /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/run/t5418b.scala \
  /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/run/t5463.scala \
  /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/run/t5488-fn.scala \
  /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/run/t5488.scala \
  /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/run/t5500.scala \
  /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/run/t5500b.scala \
  /home/jenkins/workspace/scala-2.12.x-validate-test/test/files/run/t5535.scala
@thedmitriyk

👍 for this fix.
My project uses automatic typeclass derivation and was affected by SI-7046. The scenario was pretty trivial and not far off from known examples. This fix, as implemented in the Typelevel fork, resolved my issue brilliantly. Would love to see this merged into Lightbend Scala.

@milessabin
Contributor

Rebased ...

@milessabin
Contributor

Rebased.

This has been well received in TLS and merged for 2.11.9 ... is there anything more that needs to be done before merging for 2.12.1?

@adriaanm
Member

I expect this will be fine, but have not had time for a final review pass. Should get to it this week.

@milessabin milessabin Partial fix for SI-7046
dde13b5
@milessabin
Contributor

Rebased.

@adriaanm adriaanm self-assigned this Nov 30, 2016
@adriaanm
Member
adriaanm commented Dec 1, 2016

LGTM.

I'm a little concerned about going into the type checker from a method on Symbol to force all symbols in the enclosing package, but since this is a specialized enough use case, I guess we can see how this plays out in practice.

In future, please include design motivation in the commit message. Since this has been discussed in the PR, I guess we can make an exception.

@adriaanm adriaanm merged commit c2eb299 into scala:2.12.x Dec 1, 2016

5 checks passed

cla @milessabin signed the Scala CLA. Thanks!
Details
integrate-ide [3807] SUCCESS. Took 14 s.
Details
validate-main [4311] SUCCESS. Took 73 min.
Details
validate-publish-core [4181] SUCCESS. Took 4 min.
Details
validate-test [3682] SUCCESS. Took 65 min.
Details
@SethTisue SethTisue changed the title from Partial fix for SI-7046 to SI-7046 partial fix to knownDirectSubclasses for reflection users and macro authors Dec 5, 2016
@julienrf julienrf referenced this pull request in julienrf/play-json-derived-codecs Jan 10, 2017
Open

Formatters for an ADT needs to be placed after all instances #42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment