Add error code to diagnostics about unused code #19780

ghostbuster91 · 2024-02-25T21:54:56Z

No description provided.

rochala

The CI failed because you skipped source position in imports report method.

Also, please rebase the PR as the org has changed to scala/scala3

I think that next step could be defining the rest of the quick actions.

compiler/src/dotty/tools/dotc/reporting/messages.scala

rochala · 2024-03-12T10:21:18Z

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

-      case class UnusedSymbol(pos: SrcPos, name: Name, warnType: WarnTypes)
+      enum UnusedSymbol:
+          case Symbol(tree: tpd.NamedDefTree, warnType: WarnTypes)
+          case ImportSelector(pos: SrcPos, name: Name)


Do we really need to distinguish them ?
Maybe we could model it more like

case class UnusedSymbol(defnTree: SrcPos, namePos: SrcPos, warnType: WarnTypes)

I agree that it is simpler that way but here is why we might want to pass the whole tree rather than the pair of SrcPos (at least for imports). The thing is that with imports we have bunch of different cases:

single, regular import import java.io.File

multiple import import java.io.{File, FileDescriptor}

import with rename import java.io.{File as JFile}

multiple imports with renames

In case of a multiple import there might be a case when only one of the selectors should be removed. This means going from: import java.io.{File, FileDescriptor} to import java.io.{File}.

With the tree passed there one could do some better analysis in the method that will create a removal action (by e.g. pattern matching).

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

compiler/src/dotty/tools/dotc/reporting/messages.scala

rochala · 2024-03-12T10:44:11Z

compiler/src/dotty/tools/dotc/reporting/messages.scala

+    def imports(pos: SourcePosition)(using Context): UnusedSymbol= new UnusedSymbol(pos, i"unused import", List.empty)
+    def localDefs(tree: tpd.NamedDefTree)(using Context): UnusedSymbol = new UnusedSymbol(tree.namePos, i"unused local definition", createRemoveLocalDefAction(tree))
+    def explicitParams(tree: tpd.NamedDefTree)(using Context): UnusedSymbol = new UnusedSymbol(tree.namePos, i"unused explicit parameter", List.empty)
+    def implicitParams(tree: tpd.NamedDefTree)(using Context): UnusedSymbol = new UnusedSymbol(tree.namePos, i"unused implicit parameter", List.empty)
+    def privateMembers(tree: tpd.NamedDefTree)(using Context): UnusedSymbol = new UnusedSymbol(tree.namePos, i"unused private member", List.empty)
+    def patVars(tree: tpd.NamedDefTree)(using Context): UnusedSymbol = new UnusedSymbol(tree.namePos, i"unused pattern variable", List.empty)


This is note to myself. It leads to at least 4 different scenarios.

patVars should rename the variable to _,

privateMembers, localDefs, explicitParams can just remove whole definition site,

implicitParams may require removal of parentheses used to define them if we remove the last case,

imports should also remove whole import line when we remove last import selector

ghostbuster91 · 2024-03-24T18:01:05Z

compiler/test/dotty/tools/dotc/reporting/CodeActionTest.scala

+         |    123
+         |  }
+         |""".stripMargin , 
+         afterPhase = "checkUnusedPostInlining"


thought: In theory there are also other cases with variable definitions e.g.:

val List(a, b) = List(1, 2)

but they are not being detected at the moment

ghostbuster91 · 2024-03-24T18:22:43Z

compiler/src/dotty/tools/dotc/reporting/messages.scala

+    def explicitParams(treePos: SourcePosition)(using Context): UnusedSymbol = new UnusedSymbol(treePos, i"unused explicit parameter", List.empty)
+    def implicitParams(treePos: SourcePosition)(using Context): UnusedSymbol = new UnusedSymbol(treePos, i"unused implicit parameter", List.empty)


thought: those two will also be problematic when it comes to create automatic actions because one will also have to update call-sites.

Also, they are only reported for classes not methods.

Right, we'd need to know all call sites, and it is an expensive operation or even not possible if it is called in different modules.

What seems like a way of handling this is finding the references on the tooling site, and then compute the diagnostic for the call sites. That would require us to compute additional replacements on tooling site.

Maybe we should consider adding something like we have in lsp: additionalEdits which could be populated later on ?

This won't solve the issue with recomputing the edit, but would provide a nice API to handle this case. Another problem that arises it that there is a specific scenario that will fail if we choose this approach:

API module in lets say 3.5.0 where we want to remove implicit param in definition.

Different module in 3.6.0 which is using api with removed implicit params laeding to an error.
If we let tooling do the refactor, we have to take changes in internal AST structure into consideration, unless we rely on some other parser like Scalameta.

This leads to another idea, which in my opinion can work:
Lets add actionable diagnostic to remove extra param in call site, and then after removing implicit param in definition, we can run extra actionable diagnostics on all call sites. This would solve an issue with different versions.

rochala · 2024-04-20T08:45:09Z

Sorry for the review to take that long, but it seems like we've aligned the solution.
I think we can move into the next stage where we implement the remaining diagnostics and a dozen of tests.

ghostbuster91 · 2024-05-05T20:43:15Z

As discussed in DMs I stripped the PR from any implementation of actionable diagnostics for unused symbols so it can be merged faster. Actionable diagnostics will be implemented as a follow-up.

In the meantime merging this PR will unblock other work in metals as we need to have the errorCode set in stone before we start using it.

rochala · 2024-05-06T12:39:50Z

compiler/src/dotty/tools/dotc/reporting/MessageKind.scala

@@ -37,5 +38,6 @@ enum MessageKind:
      case PatternMatchExhaustivity => "Pattern Match Exhaustivity"
      case MatchCaseUnreachable => "Match case Unreachable"
      case PotentialIssue => "Potential Issue"
+      case UnusedSymbol => "Unused symbol"


If you could change this to "Unused Symbol" as it seems we define them in Pascal case, you can also take the chance to change Match Case Unreachable. Tests will fail after this change.

rochala

For the context. This change is required to filter unused diagnostic in non-hacky manner (without using regex).

With this change, it will allow us e.g. to add special highlighting for unused names.

Co-authored-by: ghostbuster91 <ghostbuster91@users.noreply.github.com> [Cherry-picked 3fdb292][modified]

…1127) Backports #19780 to the LTS branch. PR submitted by the release tooling. [skip ci]

rochala self-assigned this Mar 1, 2024

rochala reviewed Mar 12, 2024

View reviewed changes

ghostbuster91 force-pushed the unused-diag branch from ef56f16 to b11265a Compare March 24, 2024 16:20

ghostbuster91 commented Mar 24, 2024

View reviewed changes

ghostbuster91 force-pushed the unused-diag branch 2 times, most recently from b29de63 to bde33f0 Compare May 5, 2024 20:40

ghostbuster91 mentioned this pull request May 5, 2024

feat: Set unused tag for unused diagnostics scalameta/metals#6378

Merged

ghostbuster91 marked this pull request as ready for review May 6, 2024 08:33

rochala reviewed May 6, 2024

View reviewed changes

Add error code to unused symbol diagnostics

00d6eae

ghostbuster91 force-pushed the unused-diag branch from 54a4cc1 to 00d6eae Compare May 26, 2024 16:52

ghostbuster91 requested a review from rochala May 26, 2024 21:16

rochala approved these changes May 28, 2024

View reviewed changes

rochala merged commit 3fdb292 into scala:main May 28, 2024
19 checks passed

Kordyjan added this to the 3.5.1 milestone Jul 3, 2024

WojciechMazur added a commit that referenced this pull request Jul 8, 2024

Add error code to diagnostics about unused code (#19780)

8e3f0c9

Co-authored-by: ghostbuster91 <ghostbuster91@users.noreply.github.com> [Cherry-picked 3fdb292][modified]

WojciechMazur mentioned this pull request Jul 8, 2024

Backport "Add error code to diagnostics about unused code" to LTS #21127

Merged

WojciechMazur added a commit that referenced this pull request Jul 9, 2024

Backport "Add error code to diagnostics about unused code" to LTS (#2…

9d44884

…1127) Backports #19780 to the LTS branch. PR submitted by the release tooling. [skip ci]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add error code to diagnostics about unused code #19780

Add error code to diagnostics about unused code #19780

ghostbuster91 commented Feb 25, 2024

rochala left a comment

rochala Mar 12, 2024

ghostbuster91 Mar 24, 2024 •

edited

Loading

rochala Mar 12, 2024

ghostbuster91 Mar 24, 2024

ghostbuster91 Mar 24, 2024

rochala Apr 20, 2024 •

edited

Loading

rochala commented Apr 20, 2024

ghostbuster91 commented May 5, 2024

rochala May 6, 2024

rochala left a comment

		def explicitParams(treePos: SourcePosition)(using Context): UnusedSymbol = new UnusedSymbol(treePos, i"unused explicit parameter", List.empty)
		def implicitParams(treePos: SourcePosition)(using Context): UnusedSymbol = new UnusedSymbol(treePos, i"unused implicit parameter", List.empty)

Add error code to diagnostics about unused code #19780

Add error code to diagnostics about unused code #19780

Conversation

ghostbuster91 commented Feb 25, 2024

rochala left a comment

Choose a reason for hiding this comment

rochala Mar 12, 2024

Choose a reason for hiding this comment

ghostbuster91 Mar 24, 2024 • edited Loading

Choose a reason for hiding this comment

rochala Mar 12, 2024

Choose a reason for hiding this comment

ghostbuster91 Mar 24, 2024

Choose a reason for hiding this comment

ghostbuster91 Mar 24, 2024

Choose a reason for hiding this comment

rochala Apr 20, 2024 • edited Loading

Choose a reason for hiding this comment

rochala commented Apr 20, 2024

ghostbuster91 commented May 5, 2024

rochala May 6, 2024

Choose a reason for hiding this comment

rochala left a comment

Choose a reason for hiding this comment

ghostbuster91 Mar 24, 2024 •

edited

Loading

rochala Apr 20, 2024 •

edited

Loading