golangci / golangci-lint

Fast linters runner for Go
https://golangci-lint.run
GNU General Public License v3.0
15.71k stars 1.39k forks source link

Add support for SuggestedFixes #1779

Open ldez opened 3 years ago

ldez commented 3 years ago

Is your feature request related to a problem? Please describe.

Add support for SuggestedFixes.

https://github.com/golang/tools/blob/b4639ccb830b1c9602f1c96eba624bbbe20650ea/go/analysis/doc/suggested_fixes.md

Describe the solution you'd like

Be able to apply automatically the fixes via --fix.

Describe alternatives you've considered

Display the suggestion as extra information.

Additional context

type Diagnostic struct {
    // ...

    // SuggestedFixes contains suggested fixes for a diagnostic which can be used to perform
    // edits to a file that address the diagnostic.
    // TODO(matloob): Should multiple SuggestedFixes be allowed for a diagnostic?
    // Diagnostics should not contain SuggestedFixes that overlap.
    // Experimental: This API is experimental and may change in the future.
    SuggestedFixes []SuggestedFix // optional

    // ...
}

https://github.com/golang/tools/blob/b4639ccb830b1c9602f1c96eba624bbbe20650ea/go/analysis/diagnostic.go#L23-L28

Current linters that support SuggestedFixes:

Related to #1728

ldez commented 3 years ago

I'm working on and for now, I'm able to apply suggestions from goerr113 because the suggestions concern only one line.

For the other, the problem is a bit more complex because the fix must be applied on multiple lines.

SVilgelm commented 3 years ago

I think we need to print the suggested fixes if they are not applied (without --fix)

ldez commented 3 years ago

In some cases, it will be very weird to display the suggestions, because a suggestion can be a \n or the removal of a line, and TextEdits is a slice.

SVilgelm commented 3 years ago

agree

SVilgelm commented 3 years ago

we can print a diff, anyway you are going to create a diff, so we can print replacement, maybe in verbose output

SVilgelm commented 3 years ago

@ldez I think we need to replace the Replacement with SuggestedFixes in golangci-lint, to avoid a transformation.

ldez commented 3 years ago

I think we can support both as the first step, but I'm thinking about it, I need more time.

SVilgelm commented 3 years ago

Here is the code of how to apply SuggestedFixes: https://github.com/golang/tools/blob/54dc8c5edb561003330ec7e649333181bbd0a9bd/go/analysis/internal/checker/checker.go#L316

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

nightlyone commented 2 years ago

@ldez I guess this will be the next major feature after the generics issues are fixed? The developer productivity and happiness enabled by just clicking Apply Suggested fixes is quite significant on a Go ecosystem scale.

ldez commented 2 years ago

This issue is not related to GitHub review suggestions. It's related to the CLI flag --fix of the command line.

pohly commented 1 year ago

Is there anything that can be done about this on a per-linter basis? In https://github.com/nunnatsa/ginkgolinter/issues/63#issuecomment-1439726442 it was pointed out that for some linters, fixing already works.

ldez commented 1 year ago

I plan to handle the topic by a breaking change, I don't want to introduce several ways to handle fixes.

bombsimon commented 1 year ago

Would you discourage and/or reject a PR for a linter specific implementation converting suggested fixes to replacements? I've added support for suggested fixers in a linter that only adds or removes a newline so converting it would be fairly simple. However maybe it's better to spend the time on a generic solution instead?

ldez commented 1 year ago

I think that it's the right thing for a linter to support SuggestedFixes (even if SuggestedFixes is still an experimental API), I also believe that we have to remove our custom implementation.

For now, it's better to not introduce partial or linter-specific support/converter, this is why the current PRs on the topic are still blocked. But everything depends on the context and the scope of changes.

Antonboom commented 1 year ago

SuggestedFix is not experimental more. https://github.com/golang/tools/blob/fa12f34b4218307705bf0365ab7df7c119b3653a/go/analysis/diagnostic.go#L54

ldez commented 1 year ago

the commit of the change: https://github.com/golang/tools/commit/dbd600188431770bd182cfa975efa023bea79af1

Antonboom commented 1 year ago

Related discussions:

  1. https://github.com/golangci/golangci-lint/discussions/2346
  2. https://github.com/golangci/golangci-lint/discussions/2298#discussioncomment-1633335
StevenACoffman commented 5 months ago

Additional linters that support SuggestedFixes:

It's not clear to me if gci and gofumpt run as linters by golangci-lint would fully support the SuggestedFixes mechanism even if golangci-lint did, but those programs can correct their own identified issues.

FYI, until SuggestedFixes for all linters is supported in golangci-lint, you can individually install linters that support SuggestedFixes and run:

#!/bin/bash

# this will run all the lint fixes for all the module locally

function is_bin_in_path {
  builtin type -P "$1" &> /dev/null
}

function lintall {
    export GOBIN="$HOME/go/bin"
    ! is_bin_in_path gocritic && go install -v github.com/go-critic/go-critic/cmd/gocritic@latest
    ! is_bin_in_path whitespace && go install github.com/ultraware/whitespace/cmd/whitespace@latest
    ! is_bin_in_path wsl && go install github.com/bombsimon/wsl/v4/cmd/wsl@latest
    ! is_bin_in_path mirror && go install github.com/butuzov/mirror/cmd/mirror@latest
    ! is_bin_in_path misspell && go install github.com/client9/misspell/cmd/misspell@latest
    ! is_bin_in_path perfsprint && go install -v github.com/catenacyber/perfsprint@latest
    ! is_bin_in_path testifylint && go install github.com/Antonboom/testifylint@latest
    ! is_bin_in_path usestdlibvars && go install github.com/sashamelentyev/usestdlibvars@latest
    ! is_bin_in_path gci && go install github.com/daixiang0/gci@latest
    ! is_bin_in_path golines && go install github.com/segmentio/golines@latest
    ! is_bin_in_path gofumpt && go install mvdan.cc/gofumpt@latest
    ! is_bin_in_path golangci-lint && GOBIN=$HOME/go/bin curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $GOBIN latest
    for D in */; do
        if [ -f "${D}go.mod" ]; then
            echo -e "\033[32m\xE2\x9c\x93 Running linting for ${D}\n\n\033[0m"
            cd "${D}"
            whitespace --fix ./... 2>&1 > /dev/null
            mirror --fix ./... 2>&1 > /dev/null
            perfsprint --fix ./... 2>&1 > /dev/null
            testifylint --suite-extra-assert-call.mode=require --fix ./... 2>&1 > /dev/null
            # usestdlibvars --fix ./... 2>&1 > /dev/null # noisy on generated files
            gci write -s standard -s default -s "prefix(github.com/Khan)" --skip-generated . 2>&1 > /dev/null
            gofumpt -w . 2>&1 > /dev/null
            wsl --fix . 
            "${HOME}/go/bin/golangci-lint" run --fix --config=~/Documents/git/districts-jobs/.golangci.yml ./...
            cd ..
        fi
    done
}

lintall

While some govet linters supports SuggestedFixes, go vet does not have a --fix flag to apply them. One of it's linters provides an independent entrypoint that does:

go install golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment
fieldalignment -fix ./...

The other govet linters that support SuggestedFixes without an obvious corresponding installable entrypoint that can apply them: