Skip to content

Commit 88ef93e

Browse files
ldezldez
authored
fix: no export data must always be handled as a typecheck error (#6061)
1 parent 96e3184 commit 88ef93e

File tree

3 files changed

+157
-1
lines changed

3 files changed

+157
-1
lines changed

‎pkg/goanalysis/pkgerrors/extract.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package pkgerrors
22

33
import (
44
"fmt"
5+
"maps"
56
"regexp"
67
"strings"
78

@@ -18,23 +19,45 @@ func extractErrors(pkg *packages.Package) []packages.Error {
1819
return errors
1920
}
2021

22+
skippedErrors := map[string]packages.Error{}
2123
seenErrors := map[string]bool{}
24+
2225
var uniqErrors []packages.Error
2326
for _, err := range errors {
2427
msg := stackCrusher(err.Error())
2528
if seenErrors[msg] {
2629
continue
2730
}
2831

32+
// This `if` is important to avoid duplicate errors.
33+
// The goal is to keep the most relevant error.
2934
if msg != err.Error() {
35+
prev, alreadySkip := skippedErrors[msg]
36+
if !alreadySkip {
37+
skippedErrors[msg] = err
38+
continue
39+
}
40+
41+
if len(err.Error()) < len(prev.Error()) {
42+
skippedErrors[msg] = err
43+
}
44+
3045
continue
3146
}
3247

48+
delete(skippedErrors, msg)
49+
3350
seenErrors[msg] = true
3451

3552
uniqErrors = append(uniqErrors, err)
3653
}
3754

55+
// In some cases, the error stack doesn't contain the tip error.
56+
// We must keep at least one of the original errors that contain the specific message.
57+
for skippedError := range maps.Values(skippedErrors) {
58+
uniqErrors = append(uniqErrors, skippedError)
59+
}
60+
3861
if len(pkg.GoFiles) != 0 {
3962
// errors were extracted from deps and have at least one file in package
4063
for i := range uniqErrors {

‎pkg/goanalysis/pkgerrors/extract_test.go‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,130 @@ import (
44
"testing"
55

66
"github.com/stretchr/testify/assert"
7+
"golang.org/x/tools/go/packages"
78
)
89

10+
func Test_extractErrors(t *testing.T) {
11+
testCases := []struct {
12+
desc string
13+
pkg *packages.Package
14+
15+
expected []packages.Error
16+
}{
17+
{
18+
desc: "package with errors",
19+
pkg: &packages.Package{
20+
IllTyped: true,
21+
Errors: []packages.Error{
22+
{Pos: "/home/ldez/sources/golangci/sandbox/main.go:6:11", Msg: "test"},
23+
},
24+
},
25+
expected: []packages.Error{
26+
{Pos: "/home/ldez/sources/golangci/sandbox/main.go:6:11", Msg: "test"},
27+
},
28+
},
29+
{
30+
desc: "full error stack deduplication",
31+
pkg: &packages.Package{
32+
IllTyped: true,
33+
Imports: map[string]*packages.Package{
34+
"test": {
35+
IllTyped: true,
36+
Errors: []packages.Error{
37+
{
38+
Pos: "/home/ldez/sources/golangci/sandbox/main.go:6:11",
39+
Msg: `/home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/result/processors/nolint.go:13:2: /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/result/processors/nolint.go:13: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))`,
40+
Kind: 3,
41+
},
42+
{
43+
Pos: "/home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/result/processors/nolint.go:13:2",
44+
Msg: `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))`,
45+
Kind: 3,
46+
},
47+
{
48+
Pos: "/home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/lint/lintersdb/manager.go:13:2",
49+
Msg: `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)`,
50+
Kind: 3,
51+
},
52+
{
53+
Pos: "/home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/deadcode.go:21:9",
54+
Msg: `undeclared name: linterName`,
55+
Kind: 3,
56+
},
57+
},
58+
},
59+
},
60+
},
61+
expected: []packages.Error{{
62+
Pos: "/home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/deadcode.go:21:9",
63+
Msg: "undeclared name: linterName",
64+
Kind: 3,
65+
}},
66+
},
67+
{
68+
desc: "package with import errors but with only one error and without tip error",
69+
pkg: &packages.Package{
70+
IllTyped: true,
71+
Imports: map[string]*packages.Package{
72+
"test": {
73+
IllTyped: true,
74+
Errors: []packages.Error{
75+
{
76+
Pos: "/home/ldez/sources/golangci/sandbox/main.go:6:11",
77+
Msg: "could not import github.com/example/foo (main.go:6:2: missing go.sum entry for module providing package github.com/example/foo (imported by github.com/golangci/sandbox); to add:\n\tgo get github.com/golangci/sandbox)",
78+
Kind: 3,
79+
},
80+
},
81+
},
82+
},
83+
},
84+
expected: []packages.Error{{
85+
Pos: "/home/ldez/sources/golangci/sandbox/main.go:6:11",
86+
Msg: "could not import github.com/example/foo (main.go:6:2: missing go.sum entry for module providing package github.com/example/foo (imported by github.com/golangci/sandbox); to add:\n\tgo get github.com/golangci/sandbox)",
87+
Kind: 3,
88+
}},
89+
},
90+
{
91+
desc: "package with import errors but without tip error",
92+
pkg: &packages.Package{
93+
IllTyped: true,
94+
Imports: map[string]*packages.Package{
95+
"test": {
96+
IllTyped: true,
97+
Errors: []packages.Error{
98+
{
99+
Pos: "/home/ldez/sources/golangci/sandbox/main.go:6:1",
100+
Msg: "foo (/home/ldez/sources/golangci/sandbox/main.go:6:11: could not import github.com/example/foo (main.go:6:2: missing go.sum entry for module providing package github.com/example/foo (imported by github.com/golangci/sandbox); to add:\n\tgo get github.com/golangci/sandbox))",
101+
Kind: 3,
102+
},
103+
{
104+
Pos: "/home/ldez/sources/golangci/sandbox/main.go:6:11",
105+
Msg: "could not import github.com/example/foo (main.go:6:2: missing go.sum entry for module providing package github.com/example/foo (imported by github.com/golangci/sandbox); to add:\n\tgo get github.com/golangci/sandbox)",
106+
Kind: 3,
107+
},
108+
},
109+
},
110+
},
111+
},
112+
expected: []packages.Error{{
113+
Pos: "/home/ldez/sources/golangci/sandbox/main.go:6:11",
114+
Msg: "could not import github.com/example/foo (main.go:6:2: missing go.sum entry for module providing package github.com/example/foo (imported by github.com/golangci/sandbox); to add:\n\tgo get github.com/golangci/sandbox)",
115+
Kind: 3,
116+
}},
117+
},
118+
}
119+
120+
for _, test := range testCases {
121+
t.Run(test.desc, func(t *testing.T) {
122+
t.Parallel()
123+
124+
errors := extractErrors(test.pkg)
125+
126+
assert.Equal(t, test.expected, errors)
127+
})
128+
}
129+
}
130+
9131
func Test_stackCrusher(t *testing.T) {
10132
testCases := []struct {
11133
desc string

‎pkg/goanalysis/runner_loadingpackage.go‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,9 @@ func (lp *loadingPackage) analyze(ctx context.Context, cancel context.CancelFunc
7878
defer lp.decUse(loadMode < LoadModeWholeProgram)
7979

8080
if err := lp.loadWithFacts(loadMode); err != nil {
81+
// Note: this error is ignored when there is no facts loading (e.g. with 98% of linters).
82+
// But this is not a problem because the errors are added to the package.Errors.
83+
// You through an error, try to add it to actions, but there is no action annnddd it's gone!
8184
werr := fmt.Errorf("failed to load package %s: %w", lp.pkg.Name, err)
8285

8386
// Don't need to write error to errCh, it will be extracted and reported on another layer.
@@ -88,6 +91,10 @@ func (lp *loadingPackage) analyze(ctx context.Context, cancel context.CancelFunc
8891
act.Err = werr
8992
}
9093

94+
if len(lp.actions) == 0 {
95+
lp.log.Warnf("no action but there is an error: %v", err)
96+
}
97+
9198
return
9299
}
93100

@@ -239,9 +246,11 @@ func (lp *loadingPackage) loadFromExportData() error {
239246
return fmt.Errorf("dependency %q hasn't been loaded yet", path)
240247
}
241248
}
249+
242250
if pkg.ExportFile == "" {
243251
return fmt.Errorf("no export data for %q", pkg.ID)
244252
}
253+
245254
f, err := os.Open(pkg.ExportFile)
246255
if err != nil {
247256
return err
@@ -332,13 +341,15 @@ func (lp *loadingPackage) loadImportedPackageWithFacts(loadMode LoadMode) error
332341
if srcErr := lp.loadFromSource(loadMode); srcErr != nil {
333342
return srcErr
334343
}
344+
335345
// Make sure this package can't be imported successfully
336346
pkg.Errors = append(pkg.Errors, packages.Error{
337347
Pos: "-",
338348
Msg: fmt.Sprintf("could not load export data: %s", err),
339349
Kind: packages.ParseError,
340350
})
341-
return fmt.Errorf("could not load export data: %w", err)
351+
352+
return nil
342353
}
343354
}
344355

0 commit comments

Comments
 (0)