Deprecate nested class shadowing in "override" position #8705
Conversation
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
eed3si9n
force-pushed
the
wip/deprecate-class-shadowing
branch
from
f21a26d
to
0fd836c
Compare
This comment has been minimized.
This comment has been minimized.
|
RefChecks result looks better |
eed3si9n
force-pushed
the
wip/deprecate-class-shadowing
branch
from
1e23705
to
4d89123
Compare
|
/rebuild |
lrytz
requested changes
Feb 11, 2020
|
is there some other kind of class shadowing that isn't "nested class shadowing"? |
|
The following is OK in Dotty scala> class C0 {
| class Status
| class C1 {
| class Status
| }
| }
// defined class C0The following is NOT ok in Dotty: scala> trait Base {
| class Status
| }
|
| class C0 extends Base {
| class Status
| }
|
6 | class Status
| ^
|class Status cannot have the same name as class Status in trait Base -- class definitions cannot be overriddenHow should we distinguish the two cases? |
eed3si9n
commented
Feb 11, 2020
The difference is this The current patch that you pushed only deprecates shadowing of an inherited class, which is what we want. Shadowing from an outer scope remains allowed, for classes and for any other definitions.
Maybe your question was not technical, but what the best deprecation message would be? I like your second version:
|
That's right, since Seth noticed that I changed the warning message in the last commit.
ok. I'll go with that then. |
eed3si9n
force-pushed
the
wip/deprecate-class-shadowing
branch
from
0fc4bcc
to
5c40039
Compare
eed3si9n
changed the title
|
Squashed and updated the description. |
SethTisue
approved these changes
Feb 12, 2020
Fixes scala/bug#8353 This deprecates nested class shadowing in "override" position for Dotty compatibility. In general Scala does not implement override of classes where "new Status" might take on different meaning based on what the subtypes are doing. To avoid the confusion, we are deprecating the nested class shadow another nested class if it was introduced by a parent class.
eed3si9n
force-pushed
the
wip/deprecate-class-shadowing
branch
from
5c40039
to
c25dfdc
Compare
|
This PR causes a false class shadowing warning. |
|
And also in this regard, has anyone compared the community-build's log before and after this PR to see how many deprecation warnings were added (if at all)? |
carefully worded to avoid actually volunteering ;-) I don't think we need a before-and-after, just an after, since the error message text is new and thus greppable. (or is it? not every repo has also It's difficult to get a truly complete log because it's typical in the community build for any full rebuild to suffer from at least one intermittent test failure. but an example of a recent, nearly complete (all but 5 repos) log is https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/3231/consoleText so let's have a look: HOWEVER, some of these may go away after @som-snytt's #8751 , which has not been run through the community build yet. I have set a reminder to come back to this ticket and try the same thing again after the next time the CB moves to a new Scala 2.13 SHA (which should be in a week or less) |
|
(haven't forgotten. waiting for #8763 to land before doing fresh runs) |
|
@eed3si9n here's a nearly-complete community build log on this, mind doing the log-groveling this time? https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/3267/ (ignore the actual failures, they are either intermittent or unrelated) |
From what I've seen, now all warnings are justified. Affected libraries are trait Parent {
trait Foo
}
trait Child extends Parent {
trait Foo extends super.Foo
}
|
|
Is it possible to fix such issue and still maintain binary/source compatibility?
Should we CC those libraries' maintainers and give them a heads-up? |
I cannot think of any way to fix it while maintaining binary compatibility. |
|
For the record, here is the result from running
[shapeless] [warn] core/jvm/target/scala-2.13/src_managed/main/shapeless/polyntraits.scala:31:9: shadowing a nested class of a parent is deprecated but class CaseBuilder shadows trait CaseBuilder defined in trait Poly; rename the class to something else
I think it would be better to open GitHub issues in those repos. The libraries can deal with this at their own timing when they support Scala 3.x. Shapless or example has already started on that so likely Miles is aware of this. |
This was referenced Mar 6, 2020
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.
Fixes scala/bug#8353
This deprecates nested class shadowing in "override" position for Dotty compatibility. In general Scala does not implement override of classes where "new Status" might take on different meaning based on what the subtypes are doing. To avoid the confusion, we are deprecating the nested class shadowing of another nested class if it was introduced by a parent class.
Under
-Xsource:3.0it becomes a compiler error: