Fix/double bindings #115
Merged
Fix/double bindings #115
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
|
Review by @DarkDimius @sjrd please. (@sjrd: need to look only at last 6 commits). |
|
|
||
| protected def sig: Signature = Signature.NotAMethod | ||
|
|
||
| private[dotc] def withDenot(denot: Denotation)(implicit ctx: Context): ThisType = |
There was a problem hiding this comment.
Let us not introduce more withX methods that mutate this, shall we?
There was a problem hiding this comment.
Note that there's setDenot and withDenot. setDenot is the mutator. withDenot returns potentially a new tree and, where it does not, verifies that it does not overwrite any existing denotations. So I think the name is appropriate.
|
The last commit message says
But I don't see any code related to that. |
|
The last two commits should probably be squashed. I don't see how the introduction of |
|
That is all from me. |
|
@sjrd The code related to passing the test suite is in Config.scala. checkNoDoubleBindings is now on. |
|
I just pushed an addiitional commit which removes the method with the confusing comment and also contains a fix for overriddenSymbol prompted by Jason's example. It does not fix a test case, however. |
Cannot discard a symbol simply because the other side's type is weaker. If in (A | B)#m A and B resolve to different symbols `m`, the resulting denotation cannot have either `m` as symbol; it must be NoSymbol instead.
Implemented splitting operations As a side effect, this contains a test ruling out structural term member dispatch. Tests 0586 and 0625 which used structural dispatch got moved to neg.
Used to print <none> for denotations with a symbol, now prints "some I" where I is the denotation's info.Reworked phases.
... to check whether tree types make sense. Still produces errors when enabled.
Retyping should not create new symbols and that includes local dummys.
The main problem with TermRef handling was that signatures were not always tracked correctly. New invariant: A TermRef that points to a symbol is always a TermRefWithSig, and the signature is the one of the corresponding member. We achieve this by sometimes generating a new TermRefWithSig if a TermRef gets a denotation. One possible simplification would be to always store a signature in a TermRef. There's still a problem in TermRefWithSig#newLikeThis, which currently works only if the previously stored denotation references a symbol. We will need to generalize JointRefDenotation to contain multiple symbols for a complete fix.
A self name may no longer have the same name as a parameterless class member (or param accessor). The restriction makes sense because otherwise scoping is confusing. It's needed because otherwise we get TermRefs that have the same name and prefix but denote different things. Moved some code which exercises this from pos/typers to neg/typers
A double binding is if a named type gets assigned two denotations in the same period. This is
a side-effect and also a race condition, so it's very bad. I am trying to eliminate all causes of this. But one cause which will likely remain are double defitions in a prgram, if a user writes
class C {
val x: Int
val x: Int
}
Then it's really hard to avoid setting two meanings of C.this.x! That's why the testing
against double bindings is enabled by a -YnoDoubleBindings option. The understanding is that
-YnoDoubleBindings should be set only if there are no double def errors anticipated. Otherwise
the program might fail with an assertion error before the double def error is reported.
In TypeAssigner#ensureAccible we sometimes pick an inherited public
member as the denotation of a NamedType instead of an inaccessible
private one. The problem is that both are denotations for the same type,
which caused a noDoubleBindings assert failure. We now solve this problem
by creating a "shadowed" named type to hold the inherited member.
The shadowed named type is distinguished by its name, which reads
(inherited)originalName
In the future, we should make this more robust by using a general
tagging scheme to create shadowed names.
Another fix is about import symbols. They are now referenced with
NonMemberTermRefs. With this fix, the test suite passes with no
double def violations.
WojciechMazur
pushed a commit
to WojciechMazur/dotty
that referenced
this pull request
Mar 19, 2025
Backport "improvement: Update mtags to 1.4.1 and backport remaining changes" to 3.3 LTS
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.
Builds on #113 and #114.
Main addition is that we now test for double bindings. The PR contains the necessary fixes so that the tests pass.