Skip to content

Warn on bad extensions of aliases #22362

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 29, 2025
Merged

Warn on bad extensions of aliases #22362

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a94c8e1
Warn on bad extensions of effectively final aliases
RoccoMathijn Jan 13, 2025
527ef94
Signed CLA
RoccoMathijn Jan 13, 2025
2685ba9
pr remarks
RoccoMathijn Jan 28, 2025
8047784
pr remarks
RoccoMathijn Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/RefChecks.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1190,7 +1190,8 @@ object RefChecks {
}
}
.exists
if !target.typeSymbol.denot.isAliasType && !target.typeSymbol.denot.isOpaqueAlias && hidden
if !target.typeSymbol.denot.isOpaqueAlias // Opaque types hide original members so extension is not nullified by the original member
&& hidden
then report.warning(ExtensionNullifiedByMember(sym, target.typeSymbol), sym.srcPos)
end checkExtensionMethods

Expand Down
7 changes: 7 additions & 0 deletions tests/warn/i16743.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
-- [E194] Potential Issue Warning: tests/warn/i16743.scala:90:8 --------------------------------------------------------
90 | def length() = 42 // warn This extension method will be shadowed by .length() on String.
| ^
| Extension method length will never be selected from type String
| because String already has a member with the same name and compatible parameter types.
|
| longer explanation available when compiling with `-explain`
-- [E194] Potential Issue Warning: tests/warn/i16743.scala:30:6 --------------------------------------------------------
30 | def t = 27 // warn
| ^
Expand Down
2 changes: 1 addition & 1 deletion tests/warn/i16743.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class Depends:
object Depending:
extension (using depends: Depends)(x: depends.Thing)
def y = 42
def length() = 42 // nowarn see Quote above
def length() = 42 // warn This extension method will be shadowed by .length() on String.
def f(using d: Depends) = d.thing.y
def g(using d: Depends) = d.thing.length()

Expand Down
7 changes: 7 additions & 0 deletions tests/warn/i22233.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
-- [E194] Potential Issue Warning: tests/warn/i22233.scala:1:26 --------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably a check file is not needed, as the text is vetted by the original test.

what I would do is move tests/pos/ext-override.scala to warns.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the check file 👍

Also I moved tests/pos/ext-override.scala to the warn but actually I don't understand the reasoning. The tests in warn all give a warning right? In this test case there are no warnings. Could you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RoccoMathijn they allow warn tests to have zero warnings, which is useful. I think (IMHO) tests about lack of warning should go in warn, while testing "does this compile" remain in pos. A warn test can still have -Werror to announce that no warns are expected (so you don't have to scan the test code).

1 |extension (s: String) def length = 42 // warn
| ^
| Extension method length will never be selected from type String
| because String already has a member with the same name and compatible parameter types.
|
| longer explanation available when compiling with `-explain`
1 change: 1 addition & 0 deletions tests/warn/i22233.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
extension (s: String) def length = 42 // warn
Loading