Rework NamedTypes #3494
Merged
Rework NamedTypes #3494
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
odersky
force-pushed
the
fix-#3488
branch
3 times, most recently
from
07d38c2 to
070b889
Compare
|
Something is not right with the CI. It seems to pick up an outdated version of SymDenotations.scala. The line that leads to the compile error (420) has been deleted and now contains something unrelated to the error message. |
|
It turned out to be a rebase problem. |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3494/ to see the changes. Benchmarks is based on merging with master (8558b4d) |
Make them update themselves across runs
All symbols sharing the same toplevel class are now considered localy defined. Previously, this was order-dependent.
Will be needed to represent overloaded TermRefs
Test types arguments for equality with =:= rather than ==. Also, better diagnostics for requiredSymbol
For now it's controlled under a config option.
They wold transform the pre-existing type of the denotation instead of the symbol's type. This is clearly the wrong behavior for erasure. It was not detected before because we changed all types from name-based to symbolic during erasure, which meant we computed the denotation after erasure by a loadDenot. So we never exercised the `d.current` code on a derived denotation.
It caused errors even in the old scheme, because we now had another symbol that "existed" yet did not have an owner. Also, change creation methods for NamedTypes to accommodate NoSymbol as a symbol of an overloaded denotation.
Simplifying an overloaded reference risks changing the prefix, which will lead into trouble using the new overloading scheme (because the only info is in the denotation, which will be thrown away when the prefix is changed). Also, both interpolating undetermined variables and simplifying are likely to be inefficient on overloaded types because they tend to be large. Delaying these operations means we only have to perform them on alternatives that might plausibly match the expected type.
Make PreDenotation a superclass of Denotation. The immediate need for this is that we would like to add an `asSeenFrom` to Denotations, but if we do that it clashes with the `asSeenFrom` in Predenotations.
JoinRefDenotations before erasure become UniqueRefDenotations after. The former are no longer legal after erasure.
- need to adapt denotation if prefix changes in `withPrefix` - need to recover name of reference from the denotation.
- don't suppress them when retying after a contextual search - give a more helpful error message
Gave sometimes nonsensical messages like
x is not a member of Object { ... }
It's called in a lot of maps and forcing presents a risk of stale symbol errors. So we should force the minimum possible.
TypeArgRefs with equal prefix, classes and indices should compare as equal. Previously this probably was not detected because equal prefixes happened to be identical and then the `eq` case in isSubType would kick in. The problematic case happened when comparing `ParMapLike[<lots of arguments>]#<typearg Sequential>` with itself except that one side had a Lazy(CC) where the other had a CC. It was in compileStdLib, in case you were wondering.
Update stale references only if -Yupdate-state is set, fail with a StaleSymbol error otherwise. This required some fixes to the way ScalaShadowing was treated. If we compile a ScalaShadowing (as is done in tasty-bootstrap) we need to also overwrite the mirrored symbol in the scala package. Otherwise we would get a stale reference when looking up the latter.
- don't compute members when unpickling termrefs, it caused cyclic reference errors on tasty-bootstrap. - to make this robust, have currentSignature in TermRef return a NotAMethof if not symbol or denotation is known.
- make private where possible - make public where the operation is immutable and has a simple to understand meaning.
There was a scenario in the IDE where this was not the case: If the owner of a denotation was deleted in the new run, the old symbol gets an updated validity. Then, a call to bringForward of a member of that invalid owner will lookup the symbol in its owner, yielding itself. But the validity still was the previous period.
This makes scala#2405 work again.
Also: reinstantiate an accidentally deleted test
In interactive mode, allow uncompleted symbols with completers from previous runs. This is possible due to the way we bring forwward stale symbols. Liek in manual cleanup in InteractiveDriver, we set the info of such symbols to UnspecifiedError.
smarter
reviewed
Nov 30, 2017
| @@ -106,6 +106,7 @@ class ScalaSettings extends Settings.SettingGroup { | |||
| val YkeepComments = BooleanSetting("-Ykeep-comments", "Keep comments when scanning source files.") | |||
| val YforceSbtPhases = BooleanSetting("-Yforce-sbt-phases", "Run the phases used by sbt for incremental compilation (ExtractDependencies and ExtractAPI) even if the compiler is ran outside of sbt, for debugging.") | |||
| val YdumpSbtInc = BooleanSetting("-Ydump-sbt-inc", "For every compiled foo.scala, output the API representation and dependencies used for sbt incremental compilation in foo.inc, implies -Yforce-sbt-phases.") | |||
| val YupdateStale = BooleanSetting("-Yupdate-stale", "Automically update stale top-level classes and objects to latest version") | |||
There was a problem hiding this comment.
Do we still need this? It's currently never enabled.
| final override def isTerm(implicit ctx: Context): Boolean = | ||
| (lastDenot.validFor.runId == ctx.runId || ctx.stillValid(lastDenot)) && | ||
| (lastDenot.symbol eq this) | ||
| // the last condition is needed because under option -Yupdate-stale overwritten |
There was a problem hiding this comment.
Add ctx.settings.YupdateStale.value && to that last condition?
There was a problem hiding this comment.
No that would not work. The condition is redundant unless stale symbols are accepted.
| d | ||
| } | ||
|
|
||
| def fromDesignator = designator match { |
There was a problem hiding this comment.
The finish call could be moved here to wrap the entire expression
There was a problem hiding this comment.
True. I left it like this for better debuggability. If checkDenot (called from setDenot) fails with an AssertionError, we know exactly how we got there.
|
|
||
| private def memberDenot(name: Name, allowPrivate: Boolean)(implicit ctx: Context): Denotation = { | ||
| var d = memberDenot(prefix, name, allowPrivate) | ||
| if (!d.exists && ctx.mode.is(Mode.Interactive)) |
There was a problem hiding this comment.
I would add && !allowPrivate, also maybe add a comment explaining why this may arise in the IDE.
| if (d.exists) d | ||
| else throw new Error(s"failure to reload $this of class $getClass") | ||
| } | ||
| /** Is this a reference to a class or object member? */ |
There was a problem hiding this comment.
This comment does not seem to match the implementation: infoDependsOnPrefix calls membersNeedAsSeenFrom(prefix) which will return false if prefix is a statically accessible object.
| val adapted = withSym(denot.symbol) | ||
| if (adapted ne this) adapted.withDenot(denot).asInstanceOf[ThisType] | ||
| else { | ||
| setDenot(denot) |
There was a problem hiding this comment.
Given the name of this method and its documentation, it's surprising that it can mutate this.
There was a problem hiding this comment.
Good point. I'll update the docs.
| } | ||
|
|
||
| abstract case class TermRef(override val prefix: Type, designator: TermDesignator) extends NamedType with SingletonType { | ||
| abstract case class TermRef(override val prefix: Type, var designator: Designator) |
There was a problem hiding this comment.
I see var designator here, doesn't that make the setter of TermRef#designator public?
Major changes - Drop -Yupdate-stale and simplify bringForward accordingly - Make designator a private field of TypeRef and TermRef
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.
When analyzing #3488, the root cause seems to be that constructing TermRefs with signatures is fragile. Signatures might not be fully known at the point a TermRef is constructed, which leads to complicated compensation actions.
In this PR I try to make NamedTypes by default symbolic (i.e. all designators should ideally be symbols, not names). We need to compensate for cross compilation unit references in Tasty trees.
These need names and signatures as before. But hopefully we can be symbol-only in the internal types.
Postscript: Some NamedTypes are still created with a name only, in order not to force too much. But the scheme should now be much more robust than before.