Fix overriding problems #1243
Merged
Fix overriding problems #1243
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
| !sym1.is(Bridge) && !sym2.is(Bridge)) | ||
| // double definition of two methods with same signature in one class; | ||
| // don't merge them. | ||
| return NoDenotation |
There was a problem hiding this comment.
Can we get here with private sym1 or sym2? That's another case that prevents overriding.
There was a problem hiding this comment.
Also, can we get here with constructors? These also cannot be invoved in overriding.
There was a problem hiding this comment.
In both cases, the existing resolution will pick the right symbol. So these should not be a problem.
|
Going to merge by tomorrow unless there are further comments |
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.
|
Added one important commit and rebased to master. |
Any idea why? |
|
@odersky I didn't yet have a chance to see if this fix is sufficient. I'm fine with meging it now if it's needed in the mainline. |
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.
Merged
|
/rebuild |
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 fixes the missing "ambiguous overload" error of #1240. More work is needed to fix the multiple overriddenSymbols scenario.
Review by @retronym or @DarkDimius