Rule.apply: List[Patch]

[olafurpg]

Following a conversation with @xeno-by from his experience with the scalafix api. This is a proposal to replace Rule.fix/check with a single method Rule.apply(ctx: RuleCtx): List[Fix] where (roughly)

trait Fix {
  def message: LintMessage // LintMessage.empty by default
  def patch: Patch // Patch.empty by default
}

This change addresses several problems with the current design

• escape hatches Escape hatch / disable linter messages over regions #241, a single Patch can contain a sequence of ctx.addLeft/addRight/remove edits that need atomically trigger or not. See example below, if Baz is inserted inside foo(...) along with an import at the top of a file, then that import should not be inserted since scalafix is disabled around foo(...).

// import a.Baz to be inserted by scalafix
object a {
  // scalafix: off
  foo(...) // Baz to be inserted here by scalafix
  // scalafix: on
}

• one signature makes it easier to convert a linter into a rewrite (autofix) or vice-versa turn a rewrite into a linter
• avoids the need for sprinkling .asPatch in most cases.

I think it's possible to introduce this change while keeping compatibility with the current design, and we deprecate Rule.fix/check.

[gabro]

+1 to the overall design, having a single "action" that returns a rich data structure feels more natural.

Now, for the bikeshedding:

• why not List[LintMessage] (default: Nil)?

• avoids the need for sprinkling .asPatch in most cases.

  how?

[MasseGuillaume]

I'm on this.

[gabro]

I was thinking about this yesterday. That's great @MasseGuillaume!

I think will make it easier to start treating the current fixes as lint rules too. For instance: UnusedImports can report an unused import (in the editor 😇 ) and then the user can decide to fix it (in bulk with scalafix or manually).

[MasseGuillaume]

Yup, we discovered something call CodeLens in lsp where you can attach an action such as remove import. This way you could just do it on one item.

[gabro]

Absolutely, CodeLens is exactly what I was implying we'd use in the future :)

[olafurpg]

Pending PR to fix this in #503

We went with a design that does not require any changes to the existing API. There is a new method .atomic: Patch to notify scalafix that the patch must be atomically applied. If there is a // scalafix: off Rule comment that affects a position that is touched by the Patch then the whole patch is ignored.

To simplify the Rule API, I think we should consider deprecating check since it's possible to implement it with fix by simply wrapping all ctx.lint(LintMessage) and .asPatch. This is what scalafix does behind the scenes anyways. We can discuss this in a separate PR.

I think will make it easier to start treating the current fixes as lint rules too. For instance: UnusedImports can report an unused import (in the editor 😇 ) and then the user can decide to fix it (in bulk with scalafix or manually).

IntelliJ has this option to run all fixes of a given category in the open file. I use this quite frequently to fix things like repeated missing () for effectful unary methods.