Fix #6603: various changes to avoid cyclic references in separate compilation #6842
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It can't be a FromStartFlags since we don't know whether a top-level class is Java-defined before completing it once.
LazyType inherits the toString of Function2 which is just "<function2>" and therefore not very helpful. By contrast, the exact class of a completer can be very useful when debugging.
Doing the check in Typer leads to cyclic errors, and I can't figure out a nice way to do this in any other phase since it requires knowledge about the expected type.
It's only ever used on SymbolLoaders, and more importantly it means that a proxy to a SymbolLoader is now also a SymbolLoader which we'll use to enforce restrictions on which completers are allowed to set `privateWithin` in a later commit.
|
test performance please |
|
performance test scheduled: 5 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/6842/ to see the changes. Benchmarks is based on merging with master (e2130b9) |
odersky
approved these changes
Jul 15, 2019
When a SymbolLoader gets completed, we sometimes use NoCompleter as an intermediate info, we now use NoLoader instead. This will be useful in the next commit where we restrict which completers are allowed to set `isAbsent`. We must allow SymbolLoader to set `isAbsent` , but we don't want to allow any NoCompleter to set it, so instead we use NoLoader which is a subtype of both SymbolLoader and NoCompleter. It also turns out that even without the changes to `isAbsent`, using a `SymbolLoader` here is a good idea since `SymDenotation#isCurrent` handles SymbolLoader specially and did not take into account that they were sometimes replaced by NoCompleter. This means that checking for the presence of a flag can now lead to more completion, which required adding an early exit in `Types#isArgPrefixOf` to avoid a cyclic reference.
After the previous commits, only completers for ModuleCompleter and SymbolLoader actually need to be completed to avoid isAbsent returning the wrong result. A version if `isAbsent` that doesn't force at all is still necessary, but instead of having a separate `unforcedIsAbsent` method we roll its functionality into `isAbsent` which gains a `canForce` parameter
Calling `this.flags` requires completing the denotation, but directly caling `is(...)` doesn't if the flags are FromStartFlags/AfterLoadFlags. Most calls to `accessBoundary` will still require forcing because of the call to `privateWithin`, this is fixed by the next few commits by making `privateWithin` require less forcing.
First step towards making `privateWithin` force less.
Second step towards making `privateWithin` force less.
Third step towards making `privateWithin` force less.
At this point, ModuleCompleters and SymbolLoaders are the only two kinds of completers which need to set privateWithin, this is now enforced by the setter (which is therefore renamed to `setPrivateWithin` to make it clearer that it doesn't just set a field) and taken advantage of by the getter to avoid forcing unless necessary. This is the last piece needed to finally fix scala#6603. Unfortunately, the cumulative changes to completions done so far have caused cyclic reference errors to pop up in new situations, including bootstrapping Dotty itself. This is fixed by the remaining commits in this PR.
This reverts the remains of 393c620 which are no longer needed now that Eql instances are generated using typeclass derivation.
It turns out that in practice we always use the same selfInfo that was used to create the TempClassInfo, so no need to pass it again as a parameter.
Parameter accessors have a derived rhs based on the corresponding class constructor parameter. Before this commit, typing such a derived tree would involve completing the enclosing class, which requires typing the class parents, which might end up requiring the info of the original parameter accessor. This doesn't always cause cyclic reference since `Namer#typeDefSig` sets temporary empty bounds when completing a type definition, but it can lead to puzzling compilation errors. In particular, after the changes related to completions in this PR, it broke the bootstrap. We fix this by making `DerivedFromParamTree#ensureCompletions` force less: we only really need to force the constructors, this requires some refactoring in `ClassCompleter` to allow such partial completions.
Following the completions-related changes in this PR, we no longer encounter cyclic reference errors when annotating a class with itself (i1212.scala) as well as in some situation where an annotation class indirectly refers to itself (i3506.scala). While the latter is OK, the former is problematic because when unpickling from tasty, the modifiers and annotations of a symbol are read before the symbol itself is created, so we can't support self-annotation classes, we therefore need to disallow them explicitly.
`checkInfo` is just the identity on ClassInfo so no need to force the info in this case. This avoids some cyclic reference errors which started appearing after other completions-related changes in this PR.
According to its documentation, it should only be necessary when there's more than one parent, we can use this knowledge to exit early from the method. This avoids a cyclic reference involving a refinement type in t1957.scala which started happening after the completions-related changes in this PR.
Scala 2 preserves annotations both on the param accessors and on compiler-generated methods such as copy, so we should do the same. But there doesn't seem to be much point in preserving them on the type parameters of the constructors. Removing them avoids a cyclic reference error involving `@specialized` which happened in the past and resurfaced with the completions-related changes in this PR.
smarter
force-pushed
the
wip/avoid-cycle-6603-12
branch
from
5e752c3 to
b602d83
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Compiling
Desugar.scala,Trees.scalaandTypes.scalawith Dotty on the classpath lead to:This PR fixes that by judiciously reducing the amount of info which need to be completed. I've done this sort of PR many times before, but this one was by far the hardest. To solve the original problem, the following changes were necessary:
checkValuecheck after TyperJavaDefinedan AfterLoadFlagsisAbsentforce less, this required some refactoring to be done safelyprivateWithinforce less (likewise required some refactoring)These changes were enough to fix the original problem, but paradoxically they caused new cyclic references to appear (because we're forcing less things in some situations, we end up with more unforced things at the same time and thus more chances of a cycle between all these things). Fixing these new issues required the following changes:
DerivedFromParamTree#ensureCompletionsforce less, this required refactoring ClassCompleter to be able to complete the constructor without completing the whole classcheckNonCyclicandcheckNonCyclicInheritedforce less, those were simple changesReviewing this PR requires going commit-by-commit, I've taken great care to make each commit reviewable and sensible on its own.