SI-7046 partial fix to knownDirectSubclasses for reflection users and macro authors #5284
Conversation
| @@ -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 ) | |||
|
|
|||
There was a problem hiding this comment.
Why do we need to reorder this error message?
There was a problem hiding this comment.
It's moved from ContextErrors to NamerContextErrors so that it's accessible in Namers.
|
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? |
|
@xeno-by the scenario you described is handled ... see the test. |
|
Specifically |
|
Maybe I'm just fixating too much on the name, but it's So I would expect that if you called |
|
@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. |
|
IIUC, the biggest potential danger is the added call to the new method 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 |
|
@Blaisorblade the use of |
|
@milessabin Very interesting! Why? |
|
@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. |
|
Any chance of getting this in 2.12.0? |
| @@ -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)) | |||
There was a problem hiding this comment.
I believe this should be restricted to DefTrees:
tree.foreach {
case dt: DefTree if dt.symbol != null => dt.symbol.maybeInitialize
case _ =>
}There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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?
|
As an additional data point wrt |
facf243 to
35c2ddd
Compare
|
Here's a very contrived example to that shows a leakage in 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 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 This isn't quite what I had in mind above, but it does qualify as a side-effect (issuing the type error). < 8000 p dir="auto">The example itself needs a bit more minimization before its worthy of detailed discussion. But it shows basis for my unease aboutmaybeInitialize is its current state.
I notice that you've put the forcing in I'm going to keep thinking about this one. |
35c2ddd to
e5ced65
Compare
|
@retronym would be keen to get your take on the |
e5ced65 to
02d8929
Compare
|
Failures appear to be transient ... |
|
/rebuild |
| @@ -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) | |||
There was a problem hiding this comment.
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?
|
@retronym apropos If this is the case then I think what I'm doing here is correct: the only things which result in a 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 |
|
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. |
|
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. |
|
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. |
d373130 to
3c9e1ff
Compare
|
Jenkins says: |
|
👍 for this fix. |
3c9e1ff to
d31cd06
Compare
|
Rebased ... |
d31cd06 to
f9daa77
Compare
|
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? |
|
I expect this will be fine, but have not had time for a final review pass. Should get to it this week. |
f9daa77 to
dde13b5
Compare
|
Rebased. |
|
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. |
The ZoneValidator protocol is now: 1. Independent of the WsProtocol. 2. Still represented by the same types that they always were, as far as speakers of the protocol are concerned. 3. Serialized to Protobuf messages. (3) was the goal of this change, while (2) was a constraint on the solution that I've imposed in order to retain meaningful types throughout most of the codebase without resorting to using ScalaPB's TypeMappers (which can only do so much). The introduction of several new `case object Empty ...` types on the application side is in order to enable inductive derivation of ProtoConverter instances for the wrapper coproducts that ScalaPB generates for `oneof` message types. The movement of ZoneValidatorMessageSerializer fromm the actor-protocol module to the server module was done in order ensure that its compilation happens _after_ the compilation of both the generated Protobuf protocol types and the original protocol types (both of which are compiled in the actor-protocol module). This was necessary because ScalaPB generates the _instances_ of the wrapper coproducts that it creats for `oneof` message types _within_ the companion object that it creates for the trait of which they are children. This property of the generated code interacted badly with the way that scalac's knownDirectSubclasses macro method works, causing errors of the form `error: knownDirectSubclasses of Foo observed before subclass Bar registered`. I found that commenting out the three lines in ZoneValidatorMessageSerializer that trigger the derivation of ProtoConverter instances that depend on coproduct ProtoConverter instances being derived was sufficient for compilation to succeed. I then found that a second compilation attempt with them _un_commented would somewhat suprisingly also succeed. This finding coupled with the notes on scala/scala#5284 confirmed to me that the issue was only happening because of the above property of the generated ScalaPB code (ProtoConverterSpec includes test cases for coproducts which work fine, so I knew that other derivations were working - although somewhat surprisingly it also works if the test coproduct instances are moved to be children of companions of their parent traits...). Hence the workaround of simply moving the serializer instances into the server module (which is the only place they're used anyway*). This ensures that derivation happens _after_ the necessary compiler phases have completed on the coproducts in question. *ZoneEventSerializer was in a separate module because analytics used to run as a separate deployable which built against the persistence module (but not the server module). Other notes: 1. There are two TODOs re. unused import warnings in the ZoneEvent and ZoneValidatorMessage serializers. I've left these for later resolution because: (a) it's taken a fair amount of time already to get this change to where it is, (b) the size of the diff is getting large, and (c) they aren't issues with the code anyway - they really seem to be an IntelliJ issue, so the TODOs are really about finding a fix or workaround for that. 2. ClientConnectionActor (and its Spec) are using ProtoConverter instances to convert WsProtocol ZoneCommands to ZoneValidatorMessage ZoneCommands. As the numerous comments where this occurs explain, the terms asScala and asProto are not appropriate here. Indeed, that's not the only way this is not ideal. The more preferable solution would have been to _move_ the ZoneCommands from WsProtocol, rather than _copy_ them. That's actually what I first attempted to do. However, the issue I ran into was in adapting the scala-json-rpc message format constructors to support wrapping an external message in a way that would preserve the original JSON-RPC wire format. There would have been a solution, but using the recently developed ProtoConverter functionality ended up being a much faster solution to implement (and is also less verbose). Since the JSON-RPC protocol is now living on borrowed time and should be deprecated and then removed in the near future now that all the prerequisite steps are complete (this being the last of them), this solution is reasonable enough (it will disappear when the JSON-RPC protocol is removed). This leads nicely on to the final point: 3. play-json removal: as part of the deprecation and later removal of the JSON-RPC protocol, the number of modules which include play-json as a dependency should decrease and eventually reach zero. For now it remains as a dependency of most things because I've deliberately left the application side ADTs with JsObject as the metadata type in order to minimise the volume of individual Protobuf-conversion-related commits.
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
knownDirectSubclasseswould be used. The missing 5% is that subclasses defined in local scopes might not be seen byknownDirectSubclasses(seeLocalandRiddlein the test below). In mitigation, though, it is almost certain that a local subclass would represent an error in any scenario whereknownDirectSubclassesmight be used.Errors for such situations are reported by recording (via a symbol attachment) that
knownDirectSubclasseshas 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.