Make unused Scalafix suppression actionnable

[bjaglin]

Scalafix automatically warns of unused scalafix suppressions (see https://scalacenter.github.io/scalafix/docs/users/suppression.html#unused-suppression-warnings), but it easily get lost with other warnings.

To keep a good hygiene, it could be possible to (and/or)

1. configure the level of these diagnostics to fail the build if/when needed
2. add a CLI flag or a separate rule to automatically remove them

[mlachkar]

Unused suppressions make the CI fails I guess if there is a scalafixAll --check test?

[bjaglin]

@mlachkar, no since https://scalacenter.github.io/scalafix/docs/users/suppression.html#unused-suppression-warnings are just warnings, and only errors trigger failure, thus my first proposition.

[olafurpg]

I think it's OK to make these errors by default. I'm not if Scalafix should report any diagnostics as warnings by default, there should only be errors. We could give the unused diagnostic a category ID so that users can optionally override the severity

[mlachkar]

For me, the main issue is why do we have many unused Scalafix suppressions.
To have suppression taken into account, the rule needs to specify that each patch is Atomic, which is correctly done in scalafix rules (RemoveUnused and ExplicitResultTypes).
Maybe we can add by default .atomic in those methods :

  /** Add this string to the right of this tree. */
  def addRight(tree: Tree, toAdd: String): Patch =
    tree.tokens.lastOption.fold(Patch.empty)(addRight(_, toAdd)).atomic

  /** Add this string to the left of this tree. */
  def addLeft(tree: Tree, toAdd: String): Patch =
    tree.tokens.headOption.fold(Patch.empty)(addLeft(_, toAdd)).atomic

This will at least reduce the number of warnings and will respect the intent of user who add // scalafix:ok

[bjaglin]

For me, the main issue is why do we have many unused Scalafix suppressions.

Do you mean in the scalafix codebase itself?

[mlachkar]

no, not especially in our code base. When adding tests to make sure that the new api take into account suppressions, I realized that the rule should add .atomic to patches, otherwise suppressions are ignored.
I think that by default addRight and addLeft should create atomic Patch.

There are 3 cases where scalafix produce unused Scalafix suppression warnings:

• val test: Seq[Int] = Seq(1) // scalafix:ok There is nothing to fix, the excplicit result type is added already. User should remove this comment.
• empty line // scalafix:ok: no patch on this line, and nothing to disable. User should remove this comment.
• Thrid case: Let imagine a rule defined like this

  val commentFileRule: Rule = Rule.syntactic("commentEntireFile") { ctx =>
    addLeft(ctx.tree, "/*") + addRight(ctx.tree, "*/")
  }

Running this rule on the file below:

import scala.util.Failure // scalafix:ok

we would expect that the file is not fixed, but actually scalafix:ok is ignored, and we will get a warning

/*import a.b // scalafix:ok*/

To make this suppression taken into account, the rule should be defined like this

 val commentFileRule: Rule = Rule.syntactic("commentEntireFile") { ctx =>
   (addLeft(ctx.tree, "/*") + addRight(ctx.tree, "*/")).atomic
 }

I propose to make addLeft and addRight atomic by default. The rule authors can still make their rule output one unique patch atomic, and adding a suppression will deactivate the entire change.

[mlachkar]

I'm more worried about suppressions that are not taken into account because the rule has not been defined correctly.