Implement -Xlint:private-shadow, type-parameter-shadow #17622

schuetzcarl · 2023-05-30T15:35:37Z

Implemented two additional lint warnings for my bachelor project supervised by @anatoliykmetyuk.

Most of the work has been done in a new MiniPhase class called CheckShadowing and running in the same mega phase as the
CheckUnused.PostTyper mini phase.

They respectively warn about :

A private field or a class parameter that shadows a super-class field.
A type parameter that shadows a type already defined in the scope.

Some examples can be seen in #17612 and #17613.

Respectively warn about : - a private field or a class parameter that shadows a superclass field - a local type parameter that shadows a type already in the scope Fixes : scala#17612 and scala#17613

szymon-rd

Here is my review; sorry it took a bit long. I just came back from vacation.

tests/neg-custom-args/fatal-warnings/i17612a.scala

compiler/src/dotty/tools/dotc/transform/CheckShadowing.scala

tests/neg-custom-args/fatal-warnings/i17613b/i17613b.scala

…& Add Tests

schuetzcarl · 2023-09-26T13:55:54Z

Here is my review; sorry it took a bit long. I just came back from vacation.

Hi @szymon-rd, thanks for the review !
No problem, it took also a bit long for me, I had not so much time in the second part of the summer.

But there is the requested changes, let me know if it is convenient for you now. :)

szymon-rd

Looks good! I have some small comments, then we can merge it.

compiler/src/dotty/tools/dotc/Compiler.scala

compiler/src/dotty/tools/dotc/transform/CheckShadowing.scala

szymon-rd · 2023-09-26T16:57:00Z

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala

@@ -717,8 +730,7 @@ object CheckUnused:

      /** A function is overriden. Either has `override flags` or parent has a matching member (type and name) */
      private def isOverriden(using Context): Boolean =
-        sym.is(Flags.Override) ||
-          (sym.exists && sym.owner.thisType.parents.exists(p => sym.matchingMember(p).exists))
+        sym.is(Flags.Override) || lookForInheritedDecl(sym).isDefined


Is there a test covering a case for which this check had to be modified?

That modification has been necessary to overcome a new non handled exception.

That exception appeared when putting the CheckUnused and CheckShadowing phases in the same MegaPhase.
In facts when doing the same traversing for both phases, it seems that some Trees becomes empty values along with their symbols.
That work around solved all the test covering cases that appears in the CommunityBuildTestA and CommunityBuildTestC. One of them that I remembered exactly is the cask project.

That's so strange... I'd rather we knew about the cause, but we should be all right if tests and community builds pass.

Yes, I never knew the exact cause honestly. During debugging, I simply saw that my way to look after inherited symbols with the same name (in CheckShadowing's lookForShadowedField() method) was more robust for handling the possible exceptions when reaching an empty Trees.
Then for the global functionality, I indeed relied on the existent test cases.

schuetzcarl · 2023-09-27T11:11:02Z

Looks good! I have some small comments, then we can merge it.

Great, thank you. :)
I applied the last changes and I think there is only one remaining point about the compiler's pipeline, let me know which option do you prefer.

szymon-rd

Ok, so let's roll back to two separate megaphases and we should be good to go. (sorry for going back and forth with these phases)

szymon-rd · 2023-09-27T13:26:11Z

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala

@@ -717,8 +730,7 @@ object CheckUnused:

      /** A function is overriden. Either has `override flags` or parent has a matching member (type and name) */
      private def isOverriden(using Context): Boolean =
-        sym.is(Flags.Override) ||
-          (sym.exists && sym.owner.thisType.parents.exists(p => sym.matchingMember(p).exists))
+        sym.is(Flags.Override) || lookForInheritedDecl(sym).isDefined


That's so strange... I'd rather we knew about the cause, but we should be all right if tests and community builds pass.

compiler/src/dotty/tools/dotc/Compiler.scala

schuetzcarl · 2023-09-27T20:37:12Z

OK, it's done.

Would you like that I open a new issue for that or you will manage it yourself ?

szymon-rd · 2023-09-29T10:57:48Z

It's okay as it is, as more linting phases will come we will try to put them together.

dwijnand · 2023-10-12T20:33:58Z

compiler/test/dotty/tools/dotc/CompilationTests.scala

@@ -33,6 +33,7 @@ class CompilationTests {
      compileFilesInDir("tests/pos", defaultOptions.and("-Ysafe-init")),
      compileFilesInDir("tests/pos-deep-subtype", allowDeepSubtypes),
      compileFilesInDir("tests/pos-special/sourcepath/outer", defaultOptions.and("-sourcepath", "tests/pos-special/sourcepath")),
+      compileFilesInDir("tests/pos", defaultOptions.and("-Xlint:private-shadow", "-Xlint:type-parameter-shadow")),


Careful adding lines like this (#18683).

We did it to avoid suffering from the problems we encountered with WUnused. We need to run a big code base with the linting flags enabled, and linting is a piece of the compiler that relies heavily on Typer. We've already seen that some minor details may cause crashes. However, I think it can be done e.g. with community build.

dwijnand · 2023-10-12T20:34:18Z

compiler/src/dotty/tools/dotc/Compiler.scala

@@ -35,7 +35,8 @@ class Compiler {
  protected def frontendPhases: List[List[Phase]] =
    List(new Parser) ::             // Compiler frontend: scanner, parser
    List(new TyperPhase) ::         // Compiler frontend: namer, typer
-    List(new CheckUnused.PostTyper) ::  // Check for unused elements
+    List(new CheckUnused.PostTyper) :: // Check for unused elements
+    List(new CheckShadowing) :: // Check shadowing elements


Surely we can merge this miniphase with other mini phases?

Yes, that's the plan for linting. But a weird bug appeared when we did that, and we could not diagnose and fix it in a short time frame. I've planned to investigate that, but this version was tested, and we decided to go iteratively while not blocking this feature from releasing.

Especially given that a megaphase seems to be a no-op when it is not enabled (isRunnable == false) - it does not introduce much cost.
(https://github.com/lampepfl/dotty/blob/main/compiler/src/dotty/tools/dotc/Run.scala#L242)

odersky · 2023-10-13T09:10:05Z

We should not have merged this PR. Making the tests take twice as long is a productivity drain for everybody else working on the project. Also I think simply creating a new mega phase should not be done without careful deliberation and benchmarking. Saying we will merge them later is no justification; main should always be the best we can make. If there is a temporary sub-optimal, one should collaborate on a branch, not on main.

szymon-rd · 2023-10-13T12:26:23Z

@odersky

Making the tests take twice as long is a productivity drain for everybody else working on the project

We need these tests to be run in the CI - Linting is a piece of the compiler that relies heavily on Typer. We've already seen that some minor details may cause crashes. It was the case with WUnused, and even in this issue - adding these tests helped prevent a bug. However, we probably should run them only on CI, and I agree that it would be better if they were not run locally on testCompilation.

Also I think simply creating a new mega phase should not be done without careful deliberation and benchmarking.

We decided to merge this as a separate MegaPhase due to a bug that we had some trouble diagnosing in a short time frame. That way we were able to deliver this feature timely and, as far I know - without introducing a performance overhead (this phase is enabled by a flag and is seems to be no-op when the flag is false). We did a similar thing for WUnused, introducing even two MegaPhases to avoid false positives.

If there is a temporary sub-optimal, one should collaborate on a branch, not on main.

I see the value in implementing some features iteratively, especially when it does not introduce performance overhead. It also helps to deliver the updates fast. But I agree that it is something we can discuss in terms of our process.

I am also somewhat shocked by the size of this commit. The compiler is already large, adding loads of code for stuff like this will make harder for everyone else to deal with it.

We have planned this feature as part of the linting effort. It was present in Scala 2. Implementing it required adding some logic. However, it is not coupled with other parts, so I would argue that it will not significantly affect the experience of maintainers.

…LTS (#20628) Backports #17622 to the LTS branch. PR submitted by the release tooling. [skip ci]

…LTS (#20685) Backports #17622 to the LTS branch. PR submitted by the release tooling. [skip ci]

schuetzcarl marked this pull request as draft May 30, 2023 23:59

Implement -Xlint:private-shadow, type-parameter-shadow

e5fd477

Respectively warn about : - a private field or a class parameter that shadows a superclass field - a local type parameter that shadows a type already in the scope Fixes : scala#17612 and scala#17613

schuetzcarl force-pushed the implement/Xlint/shadow_warnings branch from 2c408fd to e5fd477 Compare June 1, 2023 12:34

schuetzcarl added 5 commits June 7, 2023 04:03

Move out miniphase

1f15b29

Fix Exception in CheckUnused isOverriden() helper

175d4f3

Warn instead of fail for invalid -Xlint args

07f6581

Merge branch 'main' into implement/Xlint/shadow_warnings

10c9af0

Lint MegaPhase after ExtractSemanticDB

297183c

schuetzcarl marked this pull request as ready for review June 20, 2023 20:04

nicolasstucki assigned szymon-rd Jul 13, 2023

szymon-rd suggested changes Jul 26, 2023

View reviewed changes

schuetzcarl added 5 commits September 26, 2023 01:02

Adjust export and synth. case class constructor false-positive cases …

46f2db6

…& Add Tests

Move tests from neg-custom-args/fatal-warnings to neg

fb8426a

Delete tests in old folder

2edd869

Merge branch 'main' into implement/Xlint/shadow_warnings

da5d244

Change options line for tests/neg folder

1db2afb

schuetzcarl requested a review from szymon-rd September 26, 2023 13:56

szymon-rd reviewed Sep 26, 2023

View reviewed changes

Apply review changes

e3c984e

schuetzcarl requested a review from szymon-rd September 27, 2023 11:11

szymon-rd approved these changes Sep 27, 2023

View reviewed changes

Separate CheckUnused and CheckShadowing into two MegaPhases

0f4f7bf

szymon-rd merged commit 6c37729 into scala:main Sep 29, 2023
18 checks passed

dwijnand reviewed Oct 12, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

szymon-rd mentioned this pull request Oct 17, 2023

Merge linting phases into one megaphase #18708

Open

Kordyjan added this to the 3.4.0 milestone Dec 20, 2023

WojciechMazur mentioned this pull request Jun 19, 2024

Backport "Implement -Xlint:private-shadow, type-parameter-shadow" to LTS #20628

Merged

WojciechMazur added a commit that referenced this pull request Jun 20, 2024

Backport "Implement -Xlint:private-shadow, type-parameter-shadow" to …

fd85735

…LTS (#20628) Backports #17622 to the LTS branch. PR submitted by the release tooling. [skip ci]

WojciechMazur mentioned this pull request Jun 20, 2024

Backport "Implement -Xlint:private-shadow, type-parameter-shadow" to LTS #20685

Merged

WojciechMazur added a commit that referenced this pull request Jun 20, 2024

Backport "Implement -Xlint:private-shadow, type-parameter-shadow" to …

b22e8e5

…LTS (#20685) Backports #17622 to the LTS branch. PR submitted by the release tooling. [skip ci]

mbovel mentioned this pull request Oct 22, 2024

Merge linting phases #18735

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement -Xlint:private-shadow, type-parameter-shadow #17622

Implement -Xlint:private-shadow, type-parameter-shadow #17622

schuetzcarl commented May 30, 2023

szymon-rd left a comment

schuetzcarl commented Sep 26, 2023

szymon-rd left a comment •

edited

Loading

szymon-rd Sep 26, 2023

schuetzcarl Sep 27, 2023

szymon-rd Sep 27, 2023

schuetzcarl Sep 27, 2023

schuetzcarl commented Sep 27, 2023 •

edited

Loading

szymon-rd left a comment

szymon-rd Sep 27, 2023

schuetzcarl commented Sep 27, 2023

szymon-rd commented Sep 29, 2023

dwijnand Oct 12, 2023

szymon-rd Oct 13, 2023 •

edited

Loading

dwijnand Oct 12, 2023

szymon-rd Oct 13, 2023

szymon-rd Oct 13, 2023 •

edited

Loading

odersky commented Oct 13, 2023

This comment was marked as resolved.

szymon-rd commented Oct 13, 2023 •

edited

Loading

Implement -Xlint:private-shadow, type-parameter-shadow #17622

Implement -Xlint:private-shadow, type-parameter-shadow #17622

Conversation

schuetzcarl commented May 30, 2023

szymon-rd left a comment

Choose a reason for hiding this comment

schuetzcarl commented Sep 26, 2023

szymon-rd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schuetzcarl commented Sep 27, 2023 • edited Loading

szymon-rd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schuetzcarl commented Sep 27, 2023

szymon-rd commented Sep 29, 2023

Choose a reason for hiding this comment

szymon-rd Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szymon-rd Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

odersky commented Oct 13, 2023

This comment was marked as resolved.

szymon-rd commented Oct 13, 2023 • edited Loading

szymon-rd left a comment •

edited

Loading

schuetzcarl commented Sep 27, 2023 •

edited

Loading

szymon-rd Oct 13, 2023 •

edited

Loading

szymon-rd Oct 13, 2023 •

edited

Loading

szymon-rd commented Oct 13, 2023 •

edited

Loading