New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Under -Xsource:3
, warn that inherited members no longer take precedence over outer definitions in Scala 3
#10220
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
c212347
Inherited members can induce ambiguity
som-snytt fb49592
don't report type / term aliases as ambiguous
lrytz 70130da
unthis
lrytz 12c758e
change check for singletons
lrytz 189fc85
heuristic for forwarded class parameters
lrytz 9f0133f
remove heuristic, turn into a warning
lrytz db51e0a
latest diff from PR 10177
lrytz File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
t11921-alias.scala:18: warning: reference to TT is ambiguous; | ||
it is both defined in the enclosing object O and available in the enclosing class D as type TT (defined in class C) | ||
Since 3, symbols inherited from a superclass no longer shadow symbols defined in an outer scope. | ||
To continue using the symbol from the superclass, write `this.TT`. | ||
Or use `-Wconf:msg=legacy-binding:s` to silence this warning. | ||
def n(x: TT) = x // ambiguous | ||
^ | ||
t11921-alias.scala:38: warning: reference to c is ambiguous; | ||
it is both defined in the enclosing class B and available in the enclosing anonymous class as value c (defined in class A) | ||
Since 3, symbols inherited from a superclass no longer shadow symbols defined in an outer scope. | ||
To continue using the symbol from the superclass, write `this.c`. | ||
Or use `-Wconf:msg=legacy-binding:s` to silence this warning. | ||
def n = c // ambiguous | ||
^ | ||
t11921-alias.scala:57: warning: reference to name is ambiguous; | ||
it is both defined in the enclosing method m and available in the enclosing anonymous class as value name (defined in class C) | ||
Since 3, symbols inherited from a superclass no longer shadow symbols defined in an outer scope. | ||
To continue using the symbol from the superclass, write `this.name`. | ||
Or use `-Wconf:msg=legacy-binding:s` to silence this warning. | ||
println(name) | ||
^ | ||
t11921-alias.scala:67: warning: reference to name is ambiguous; | ||
it is both defined in the enclosing method m and available in the enclosing anonymous class as value name (defined in class A, inherited through parent class C) | ||
Since 3, symbols inherited from a superclass no longer shadow symbols defined in an outer scope. | ||
To continue using the symbol from the superclass, write `this.name`. | ||
Or use `-Wconf:msg=legacy-binding:s` to silence this warning. | ||
println(name) | ||
^ | ||
error: No warnings can be incurred under -Werror. | ||
4 warnings | ||
1 error |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@som-snytt I assume
top-level
is excluded here because that's what's implemented in Scala 3 (scala/scala3#8622). In Scala 3, the following compiles andnew X
resolves toC.this.X
Changing
package p
toobject p
makesnew X
ambiguous. Do you see a good reason for that? cc @oderskyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
T
has a memberdef name: String = "John Doe"
, then just where it can be inherited, introducing a different definition ofname
might be in conflict.Packagings are not classes and don't suffer from extension, so I should be able to write top-level
def name = "Gloria Mundi"
without concern about confusing anyone.But this is ambiguous:
I expected it to prefer the inherited
name
and not the enclosing definition which is a top-level def.I don't think they actually got rid of package objects...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you expect an inherited symbol takes precedence over an outer definition in the same source file, if that outer definition is package-owned? Can you say why? I don't yet see what difference it makes if the outer symbol is top-level or not.
What is the difference in that regard between
package p { def name = "..." }
andobject p { def name = "..."}
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, I'm on this topic because I'm testing #10297, and I see warnings that didn't show up in 2.13. These new warnings seem helpful to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the proof is in the pudding, or in golf it's the putting.
I have only a rationalization: in
package p
, I know nothing is inherited in this scope (because we do notpackage object p extends T
). Once someone defines a class, the folks working in that mine must manage that namespace, where stuff is inherited.I agree there is regularity in seeing
object p
as just a way to "associate" some definitions, and I think they speak of "package membership", but if the regularity were borne out, then we would lovepackage object p extends T
. You would be asking, Why is that different fromobject p extends T
?But all that is just a rationalization.
What are some examples of pernicious shadowing of top-level defs? I can't wait to see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example was:
A second file:
It's maybe clear here that we're not intending to refer to the synthetic case class companion, but this could still be a head scratcher for someone looking at the first file. One could for example do a search in the editor for
CommonName
and land on the case class.In this case I moved
CommonName
to a separate file instead of changing the reference.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, that does seem lintable, at least with
CommonName
in the same file. Conversely, it loses precedence when moved to a different file, but the reference inParent
does not.I just realized this feature is not released until 2.13.11. Let me take a moment to clear my fever-addled brain.
I notice
toString
is not exempt.Also I was about to wonder if the use case is really anon classes, esp SAM types. The user expectation is more obviously, "I am focused on implementing that one method, and probably unaware of inheriting anything."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I mean to warn only if the outer definition is in the same compilation unit.
Those are very good points. I certainly saw it pop up a lot with anonymous classes, and one would expect
toString
to resolve in the current object. Hmm. Since this is a spec change and not an impl detail (a lint rule), we need to have general and hopefully simple rules. An exception fortoString
wouldn't work...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can wait to see if
toString
really is problematic. So far (Scala 3, my tests of the new Scala 2 warning) it didn't seem so.