Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

typecheck: display compilation errors as report instead of error #1861

Merged
merged 6 commits into from
Mar 25, 2021

Conversation

ldez
Copy link
Member

@ldez ldez commented Mar 20, 2021

This PR will improve the report when the analyzed code contains compilation errors.

It's a bit complex to explain but I will try to explain it.

typecheck is like the front-end of a Go compiler: parses and type-checks Go code, this "linter" can manage compilation errors, that's its role.

typecheck is not a real linter it's just a way to parse some errors produced by the types.Checker.
The code in pkg/golinters/typecheck.go is just a kind of placeholder.

Run: func(pass *analysis.Pass) (interface{}, error) {
return nil, nil
},

The real code is here:

func parseError(srcErr packages.Error) (*result.Issue, error) {
pos, err := libpackages.ParseErrorPosition(srcErr.Pos)
if err != nil {
return nil, err
}
return &result.Issue{
Pos: *pos,
Text: srcErr.Msg,
FromLinter: "typecheck",
}, nil
}
func buildIssuesFromErrorsForTypecheckMode(errs []error, lintCtx *linter.Context) ([]result.Issue, error) {
var issues []result.Issue
uniqReportedIssues := map[string]bool{}
for _, err := range errs {
itErr, ok := errors.Cause(err).(*IllTypedError)
if !ok {
return nil, err
}
for _, err := range libpackages.ExtractErrors(itErr.Pkg) {
i, perr := parseError(err)
if perr != nil { // failed to parse
if uniqReportedIssues[err.Msg] {
continue
}
uniqReportedIssues[err.Msg] = true
lintCtx.Log.Errorf("typechecking error: %s", err.Msg)
} else {
i.Pkg = itErr.Pkg // to save to cache later
issues = append(issues, *i)
}
}
}
return issues, nil
}

The core of the fix is to always handle errors as issues if the errors are IllTypedError.
And to be able to detect this kind of error, I dropped the FailedPrerequisitesError because it grouped errors as string instead of error.

This applies to all linters 🎉

I improved the number of issues reported by typecheck by removing the duplicate elements with a "stack crusher", so now the hardcoded limit can be removed safely.

I have split the pkg/golinters/goanalysis/runner.go file into multiple files because it's a pain to debug a 1500 lines file with recursive calls, multiple structures, and multiple embedded and anonymous functions.

Fixes #1820
Fixes #1826
Fixes #1168
Fixes #1043
Fixes #1002
Fixes #886

I think this also partially fixes #1792 (#1792 (comment))

Before my PR
$ golangci-lint run
WARN [runner] Can't run linter goanalysis_metalinter: bodyclose: failed prerequisites: [buildssa@github.com/golangci/golangci-lint/pkg/lint: analysis skipped: errors in package: [/home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/lint/runner.go:18:2: could not import github.com/golangci/golangci-lint/pkg/lint/lintersdb (/home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/lint/lintersdb/manager.go:13:2: could not import github.com/golangci/golangci-lint/pkg/golinters (/home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/deadcode.go:21:9: undeclared name: linterName)) /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/lint/runner.go:22:2: could not import github.com/golangci/golangci-lint/pkg/result/processors (/home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/result/processors/nolint.go:11:2: could not import github.com/golangci/golangci-lint/pkg/golinters (/home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/deadcode.go:21:9: undeclared name: linterName))]] 
WARN [runner] Can't run linter unused: buildir: failed to load package golinters: could not load export data: no export data for "github.com/golangci/golangci-lint/pkg/golinters" 
ERRO Running error: buildir: failed to load package golinters: could not load export data: no export data for "github.com/golangci/golangci-lint/pkg/golinters" 
With my PR
$ ./golangci-lint run
pkg/golinters/deadcode.go:21:9: undeclared name: `linterName` (typecheck)
                Name: linterName,
                      ^
pkg/golinters/deadcode.go:34:18: undeclared name: `linterName` (typecheck)
                                        FromLinter: linterName,
                                                    ^
pkg/golinters/deadcode.go:45:3: undeclared name: `linterName` (typecheck)
                linterName,
                ^
pkg/lint/lintersdb/manager.go:500:2: isLocalRun declared but not used (typecheck)
        isLocalRun := os.Getenv("GOLANGCI_COM_RUN") == ""
        ^

@ldez ldez added the bug Something isn't working label Mar 20, 2021
@ldez ldez force-pushed the fix/output-compilation-errors branch 2 times, most recently from c33f4dd to 9e10dcc Compare March 20, 2021 18:49
@ldez ldez changed the title Display compilation errors as issues instead of errors Display compilation errors as report instead of error Mar 20, 2021
@ldez ldez force-pushed the fix/output-compilation-errors branch 2 times, most recently from 018445b to a52604b Compare March 21, 2021 22:08
@ldez ldez force-pushed the fix/output-compilation-errors branch from 567a7f8 to 7b3ed82 Compare March 24, 2021 00:08
@ldez ldez changed the title Display compilation errors as report instead of error typecheck: display compilation errors as report instead of error Mar 24, 2021
@ldez ldez force-pushed the fix/output-compilation-errors branch from 7b3ed82 to 4285e92 Compare March 24, 2021 00:28
Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job, thanks for fixing this! Also thanks for nicely splitting this up across several commits so I could easy follow them and not mix restructuring of the code with your changes to fix this. This will be much appreciated!

🌟 🌟 🌟

pkg/packages/util.go Show resolved Hide resolved
@ldez ldez merged commit 9aea4ae into golangci:master Mar 25, 2021
@ldez ldez deleted the fix/output-compilation-errors branch March 25, 2021 22:52
@thebeline

This comment has been minimized.

@ldez
Copy link
Member Author

ldez commented Mar 27, 2021

@thebeline could you open a new issue?

@thebeline

This comment has been minimized.

@thebeline
Copy link

@ldez - I would love to, but I am not exactly sure what I am reporting on, how to phrase it, etc.

@ldez
Copy link
Member Author

ldez commented Mar 27, 2021

@thebeline I will open a new issue BUT please provide a way to reproduce it.

@golangci golangci locked as off-topic and limited conversation to collaborators Mar 27, 2021
@ldez ldez mentioned this pull request Mar 27, 2021
4 tasks
@golangci golangci unlocked this conversation Apr 24, 2021
@ldez ldez added this to the v1.39 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment