Fix bootstrap 2 #1271
Closed
Fix bootstrap 2 #1271
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
scala#1240 shows that we need to detect ambiguous overloads of methods coming from the same base class (with different signatures there) that have the same signature in some deriving class. This was undetected before because the two methods were simply merged into one overloaded alternative.
Once we do not merge methods with same signature anymore we got an ambiguous overload between the constructors of Any and Object after erasure (when all Any methods are moved to Object). To avoid this, we map the Any constructor to the Object constructor after erasure.
This happens once we do not merge methods with the same signature coming from the same class.
When finding two symbols in the same class that have the same signature as seen from some prefix, issue a merge error. This is simpler and more robust than the alternative of producing an overloaded denotation and dealing with it afterwards.
This showcases a tricky interaction between overloading and overriding. See discussion of scala#1240 for context.
…e TermRef This happens once we do not merge methods with the same signature coming from the same class. (reverted from commit 83262d0) This case no longer applies as such a situation will now give a MergeError instead.
We cannot throw a merge error if atSignature does not give a unique single denotation. Counter example is compiling dotty itself, where we get a false negative during bridge generation. Instead, atSigature needs to return a normal denotation, and we need to check separately where required that a denotation is in fact a SingleDenotation.
Used to throw an uncaught merge error in checkAllOverrides when compiling i1240c.scala.
Real test is in neg/customargs
During an attempted dotty bootstrap it was noted that Types.scala did not compile
anymore, because `checkUnique` threw a `TypeError` during erasure. The issue was an
overloaded member `name` in TermrefWithSig. In NamedType:
def name: Name
In TermRef:
def name: TermName
Before erasure, there's one member `name`, after erasure there are two (because after
erasure result type counts). The error arose when trying to recompute a member
of a `TermRefWithSig` where the name is `name` and the expected signature is `(Nil, ?)`.
Since there are two members that match the name and the signature, `checkUnique`
triggered a `TypeError`. Before adding `checkUnique`, the previous `atSignature`
call would just have returned an arbitrary choice among the two alternative definitions
of `name`.
The fix is not to use `checkUnique` but to fall back to `d.current` in the case where
several alternatives appear.
Interestingly, the failure only triggers when -Ycheck options are *disabled*. I added a new
test that compiles Types.scala without checks, so we catch this and possibly similar bugs
in the future.
The previous additional test messed up partest in that file Types.scala was copied twice into the partest-generated directory and then the pos/core tests would compile both copies. This gave a double definition which manifested itself under -Yno-double-bindings as an assertion error. Ideally, partest generation would guard against this situation. For now I avoid the problem by compiling the whole of core without -Ycheck, not jst Types.scala.
Unrelated to other commits but useful to get in.
When reading Tasty we need to pre-set the info of a class to some ClassInfoType with (as yet) unknown parents and self type. But for module classes, we need to know the source module at all time, and this gets determined by the self type. So we now produce a TermRef for the assumed self type of a module class.
It caused an assertion error when separately compiling parts of dotty against TASTY information. Not sure the test achieves anything or whether it produces a false negative.
Instrument Denotations#current to find CyclicReference errors arising during transforms.
Forcing it led to CyclicReferences involving RefChecks.OptLevelInfo when compiling dotc/*.scala against Tasty files. The problem was that when transforming OptLevelInfo the backend forced a transformInfo of RefChecks in TypeErasure which filtered RefCheck's scope to eliminate non-class type definitions. Without the tweak in this commit this tried to make all symbols current, and so came back to OptLevelInfo.
We had a problem where unpickling an annotation containing a class constant had the wrong type. Unpickling was done after erasure. The type given to the constant was an alias but aliases got eliminated during erasure, so the constant was malformed. Unpickling annotation contents at the same phase as unpickling the annotation carrier solves the problem. It seems similar problems can arise when data is unpickled using a LocalUnpickler. So we now make sure local unpickling runs at the latest at phase Pickler.
Do this in the inferred (result-)type of ValDefs and DefDefs. Without this fix, TreeTraverser#traverseChildren in Trees.scala gets a result type of BoxedUnit (but only when co-compiled from source, not when unpickled).
It's possible that the given phase argument does not exist, in which case we do not want to set the current phase to NoPhase.
Trying the dotc bootstrap revealed that Frozen logic can lead to assertion errors. So we want to remove it, but then taking fingerprints would no longer be cost-effective. Note: In local checking removing fingerprinting did cost some performance: junit test time went from 590s to 604s. We should watch the checkin benchmarks to see whether this gets confirmed.
Is it not possible to fix |
|
@smarter I don't see a way to fix |
would you elaborate about under which circumstances the Frozen logic is not working? Is it specific to Dotty bootstrap? |
|
It turned out that errors connected with Frozen were just early warnings of an unrelated problem with Tasty unpickling. So we do not need to drop it, after all. |
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.
This differs from #1270 only in the last commit, where finger printing was removed. The plan is to merge this PR with master one day after #1270 to verify what the effect on compiler performance is.