feat: apply formatters after the suggested fixes

[ldez]

Sorry for the long description, but I want to be sure to provide all the information to understand the context.

---

Inside golangci-lint there are 4 "linters" that are not real linters/analyzers but "formatters": gofmt, goimports, gofumpt, and gci.

Their standard outputs are []byte and not Diagnostic, internally golangci-lint produces diff patches based on []byte and converts them into Diagnostic.

Also goimports and gci are not "import formatters": they format all the code and not only imports.

Problems

All those tools provide the same base functionality (format) with some variants but their changes could be incompatible (mainly on imports) unless they are run in a specific order and without incompatible settings.

Incompatibilities

• if gofmt + gci -> potential problem on imports
• if goimports + gci -> potential problem on imports
• if gofumpt + gci -> potential problem on imports
• if gofmt + goimports -> potential problem on imports
• if gofumpt + goimports -> potential problem on imports
• if gofmt + gofumpt -> potential problem on imports, and on function signatures
• etc.

I solved the main problem on imports with the support of suggested fixes: if a conflict is detected, only the suggested fixes of one linter are applied and a warning is displayed for the others.
But this has some side effects, and it can be handled differently.

Identical but Different

The real problem is that each formatter has specificities, there is no "winner".

tool	format	imports order	fix imports	rewrite-rules	simplify	extra
gofmt	x	x (alphabetically)		x	x
goimports	x	x (multiple imports)	x
gofumpt	x	x (one import)			x	x
gci	x	x (customizable)

• goimports is a "variant" of gofmt without simplify and rewrite-rules but with local-prefixes to organize imports.
• gofumpt is a "variant" of goimports with extra-rules, and module-path is more limited than local-prefixes.
• gci is a "variant" of goimports with options to customize import orders.

Note: gci has a terrible name because this is an acronym of golangci(-lint) instead of something related to "imports". I think we should evaluate asking for a renaming.

The PR

I don't want to recommend a formatter over another because each formatter has specificities.

With this PR, they will behave like a linter (reports) but suggested fixes will be skipped, be run after the fixing process, and be used in a specific order.

The specific order is:

1. gofmt
2. gofumpt
3. goimports
4. gci

This will allow applying all the changes, in one run, and without any conflicts.

Note, this doesn't mean I recommend using all those linters at the same time, it's the opposite, in the majority of cases, only one or two of those formatters is needed.
But golangci-lint should be able to provide a consistent output even in those unwanted situations.

Example

I will illustrate the topic with a terribly wrong configuration, nobody should do that: I will use, at the same time, all the options of all formatters even the conflicting options.

.golangci.yml

linters:
  disable-all: true
  enable:
    - gofumpt
    - gofmt
    - goimports
    - gci

linters-settings:
    gofmt:
      simplify: true
      rewrite-rules:
        - pattern: 'interface{}'
          replacement: 'any'
    goimports:
      local-prefixes: github.com/golangci/sandbox
    gofumpt:
      extra-rules: true
      module-path: github.com/golangci/sandbox
    gci:
      sections:
        - standard
        - default
        - localmodule

issues:
  max-issues-per-linter: 0
  max-same-issues: 0

main.go

package main

import (
	"github.com/golangci/sandbox/foo"
	"io"
	"golang.org/x/tools/go/analysis"
	"fmt"
)

func main() {
	foo.Foo()
	fmt.Println("Hello World")
	io.ReadAll(nil)
	analysis.Validate(nil)
}

func Bar(a interface{}, b interface{}) {
	fmt.Println(a, b)
}

In this example, there are several rules that should be applied:

• format (all)
• sort and group the imports (goimports, gofumpt, gci)
• interface{} should be replaced by any (gofmt)
• the signature of Bar should be changed to remove the "duplicate" type. (gofumpt)

Currently (v1.62), you must run golangci-lint 2 times and hope to be lucky:

1. The gofmt replacements are applied but the imports are duplicated because of a "fight" between all linters. Also, this is a "lucky case" because in some cases some imports can disappear.
2. The gofumpt applies extra-rules and the imports are fixed (by one of the linters).

$ golangci-lint run --fix
$ golangci-lint run --fix
$

With the PR about suggested fixes, you must run golangci-lint 3 times but there are no unexpected changes (no import duplication or removal):

1. only goimports will be applied (no import duplication)
2. gci and gofumpt are applied
3. The gofmt replacements are applied.

$ ./golangci-lint run --fix
WARN [runner] Changes related to "gci" are skipped for the file "main.go" due to conflicts with "gofmt". 
WARN [runner] Changes related to "gofmt" are skipped for the file "main.go" due to conflicts with "gofumpt". 
WARN [runner] Changes related to "gofumpt" are skipped for the file "main.go" due to conflicts with "goimports". 
$ ./golangci-lint run --fix
WARN [runner] Changes related to "gofmt" are skipped for the file "main.go" due to conflicts with "gofumpt". 
$ ./golangci-lint run --fix
$ 

With this PR, you only need one run:

1. everything is applied: no unexpected changes.

$ ./golangci-lint run --fix
$

[ccoVeille]

Thanks for the clear status. It was indeed a problem to explain these behaviors

[bombsimon]

Sorry for the long description

Nothing to be sorry about, thank you for the details description and examples making it easy to understand 😃

[bombsimon]

LGTM!