diff --git a/internal/linters/go/golangci_lint/golangci_lint.go b/internal/linters/go/golangci_lint/golangci_lint.go index 19658d93..00e98853 100644 --- a/internal/linters/go/golangci_lint/golangci_lint.go +++ b/internal/linters/go/golangci_lint/golangci_lint.go @@ -40,24 +40,34 @@ func golangciLintHandler(log *xlog.Logger, a linters.Agent) error { } func parser(log *xlog.Logger, output []byte) (map[string][]linters.LinterOutput, []string) { - var lineParser = func(line string) (*linters.LinterOutput, error) { - if strings.HasSuffix(line, "(typecheck)") { - // refer: https://github.com/qiniu/reviewbot/issues/82#issuecomment-2002340788 - return nil, fmt.Errorf("skip golangci-lint typecheck error: %s", line) + log.Infof("golangci-lint raw output: %v", string(output)) + + var trainer = func(o linters.LinterOutput) (*linters.LinterOutput, []string) { + // Perhaps it may not be precise enough？ + // refer: https://golangci-lint.run/usage/linters/ + if strings.Contains(o.Message, "(typecheck)") { + unexpected := fmt.Sprintf("%s:%d:%d: %s", o.File, o.Line, o.Column, o.Message) + return nil, []string{unexpected} } + return &o, nil + } + + var unexpected []string + rawResults, rawUnexpected := linters.ParseV2(log, output, trainer) + for _, unexpected := range rawUnexpected { // skip the warning level log // example: level=warning msg="[linters_context] copyloopvar: this linter is disabled because the Go version (1.18) of your project is lower than Go 1.22" // the warning level log is not a real lint error, so we need to skip it - if strings.Contains(line, "level=warning") { - log.Warnf("skip golangci-lint warning: %s", line) - return nil, nil + if strings.Contains(unexpected, "level=warning") { + log.Warnf("skip golangci-lint warning: %s", unexpected) + continue } - - return linters.GeneralLineParser(line) + unexpected = strings.TrimSpace(unexpected) } - return linters.Parse(log, output, lineParser) + log.Infof("golangci-lint results: %v", rawResults) + return rawResults, unexpected } // argsApply is used to set the default parameters for golangci-lint @@ -214,6 +224,7 @@ func workDirApply(a linters.Agent) linters.Agent { } a.LinterConfig.WorkDir = path.Dir(gomodPath) + log.Infof("go mod tidy workdir: %v", a.LinterConfig.WorkDir) // Execute go mod tidy when go.mod exists. cmd := exec.Command("go", "mod", "tidy") diff --git a/internal/linters/go/golangci_lint/golangci_lint_test.go b/internal/linters/go/golangci_lint/golangci_lint_test.go index 01a65ef3..fefaeee8 100644 --- a/internal/linters/go/golangci_lint/golangci_lint_test.go +++ b/internal/linters/go/golangci_lint/golangci_lint_test.go @@ -394,4 +394,5 @@ func TestWorkDirApply(t *testing.T) { } }) } + } diff --git a/internal/linters/linters.go b/internal/linters/linters.go index 0150d589..07014c7f 100644 --- a/internal/linters/linters.go +++ b/internal/linters/linters.go @@ -175,7 +175,7 @@ func ExecRun(a Agent) ([]byte, error) { c.Env = append(os.Environ(), fmt.Sprintf("ARTIFACT=%s", artifact)) c.Env = append(c.Env, a.LinterConfig.Env...) - log.Infof("run command: %v", c) + log.Infof("run command: %v, workdir: %v, env: %v", c, c.Dir, c.Env) output, execErr := c.CombinedOutput() // read all files under the artifact dir @@ -190,7 +190,7 @@ func ExecRun(a Agent) ([]byte, error) { if file.IsDir() { continue } - + log.Infof("artifact file: %v", file.Name()) content, err := os.ReadFile(fmt.Sprintf("%s/%s", artifact, file.Name())) if err != nil { return nil, err @@ -342,6 +342,90 @@ func Parse(log *xlog.Logger, output []byte, lineParser LineParser) (results map[ return } +// trainer is a function that trains the linter output. +// It returns the trained linter output and unexpected lines. +type trainer func(LinterOutput) (LinterOutput, []string) + +func ParseV2(log *xlog.Logger, output []byte, trainer func(LinterOutput) (*LinterOutput, []string)) (map[string][]LinterOutput, []string) { + pattern := `(.*?):(\d+):(\d+)?:?` + regex := regexp.MustCompile(pattern) + indices := regex.FindAllStringSubmatchIndex(string(output), -1) + if len(indices) == 0 { + return nil, strings.Split(string(output), "\n") + } + + var unexpected []string + var prefix string + if len(indices) > 0 && indices[0][0] > 0 { + prefix = strings.TrimSpace(string(output[:indices[0][0]])) + unexpected = append(unexpected, strings.Split(prefix, "\n")...) + } + + var results = make(map[string][]LinterOutput, len(indices)) + for i := 0; i < len(indices); i++ { + issue := LinterOutput{ + File: string(output[indices[i][2]:indices[i][3]]), + } + + line, err := strconv.ParseInt(string(output[indices[i][4]:indices[i][5]]), 10, 64) + if err != nil { + log.Errorf("unexpected line number: %s, err: %v", string(output[indices[i][4]:indices[i][5]]), err) + continue + } + issue.Line = int(line) + + var msgStart = indices[i][5] + 1 + if indices[i][6] != -1 && indices[i][7] != -1 { + column, err := strconv.ParseInt(string(output[indices[i][6]:indices[i][7]]), 10, 64) + if err != nil { + log.Errorf("unexpected column number: %s, err: %v", string(output[indices[i][6]:indices[i][7]]), err) + continue + } + issue.Column = int(column) + msgStart = indices[i][7] + 1 + } + + if msgStart < len(output) { + if i+1 < len(indices) { + issue.Message = strings.TrimSpace(string(output[msgStart:indices[i+1][0]])) + } else { + issue.Message = strings.TrimSpace(string(output[msgStart:])) + } + } + + if trainer != nil { + t, u := trainer(issue) + if len(u) > 0 { + unexpected = append(unexpected, u...) + } + + if t == nil { + // skip this issue + continue + } + issue = *t + } + + if outputs, ok := results[issue.File]; !ok { + results[issue.File] = []LinterOutput{issue} + } else { + // remove duplicate + var existed bool + for _, v := range outputs { + if v.File == issue.File && v.Line == issue.Line && v.Column == issue.Column && v.Message == issue.Message { + existed = true + break + } + } + if !existed { + results[issue.File] = append(outputs, issue) + } + } + } + + return results, unexpected +} + // common format LinterLine func GeneralLineParser(line string) (*LinterOutput, error) { pattern := `^(.*?):(\d+):(\d+)?:? (.*)$` diff --git a/internal/linters/linters_test.go b/internal/linters/linters_test.go index 9cfe6173..7a3977fa 100644 --- a/internal/linters/linters_test.go +++ b/internal/linters/linters_test.go @@ -23,6 +23,7 @@ import ( "testing" "github.com/qiniu/reviewbot/config" + "github.com/qiniu/x/xlog" ) func TestFormatStaticcheckLine(t *testing.T) { @@ -236,3 +237,169 @@ func TestExecRun(t *testing.T) { }) } } + +func TestParseV2(t *testing.T) { + tcs := []struct { + id string + input []byte + trainer func(LinterOutput) (*LinterOutput, []string) + expected map[string][]LinterOutput + unexpected []string + }{ + { + id: "case1 - general case", + input: []byte("file.txt:6:7: message\nfile2.txt:6:7: message\nfile2.txt:7:9: message\n"), + trainer: nil, + expected: map[string][]LinterOutput{ + "file.txt": []LinterOutput{ + { + File: "file.txt", + Line: 6, + Column: 7, + Message: "message", + }, + }, + "file2.txt": []LinterOutput{ + { + File: "file2.txt", + Line: 6, + Column: 7, + Message: "message", + }, + { + File: "file2.txt", + Line: 7, + Column: 9, + Message: "message", + }, + }, + }, + unexpected: nil, + }, + { + id: "case2 - complex case", + input: []byte(` +level=warning msg="[linters_context] copyloopvar" +level=error msg="[linters_context]" +golangci_lint.go:16:1: warning: (gochecknoglobals) +xxx.go:18:1: warning: xxxx +xxx.gox (gochecknoglobals) +golangci_lint.go:17:1: warning: (gochecknoglobals) +io/upv2/upform/internal/mime/multipart/multipart_test.go:744:7: wrapperFunc: use strings.ReplaceAll method in ` + "`strings.Replace(`--08b84578eabc563dcba96\n7a945cd<...>: application/octet-stream\n\n`, \"\\n\", \"\\r\\n\", -1)` (gocritic)"), + trainer: nil, + expected: map[string][]LinterOutput{ + "golangci_lint.go": []LinterOutput{ + { + File: "golangci_lint.go", + Line: 16, + Column: 1, + Message: "warning: (gochecknoglobals)", + }, + { + File: "golangci_lint.go", + Line: 17, + Column: 1, + Message: "warning: (gochecknoglobals)", + }, + }, + "xxx.go": []LinterOutput{ + { + File: "xxx.go", + Line: 18, + Column: 1, + Message: "warning: xxxx\nxxx.gox (gochecknoglobals)", + }, + }, + "io/upv2/upform/internal/mime/multipart/multipart_test.go": []LinterOutput{ + { + File: "io/upv2/upform/internal/mime/multipart/multipart_test.go", + Line: 744, + Column: 7, + Message: "wrapperFunc: use strings.ReplaceAll method in `strings.Replace(`--08b84578eabc563dcba96\n7a945cd<...>: application/octet-stream\n\n`, \"\\n\", \"\\r\\n\", -1)` (gocritic)", + }, + }, + }, + unexpected: []string{"level=warning msg=\"[linters_context] copyloopvar\"", "level=error msg=\"[linters_context]\""}, + }, + { + id: "case3 - invalid input", + input: []byte("case3 - invalid input"), + trainer: nil, + expected: nil, + unexpected: []string{"case3 - invalid input"}, + }, + { + id: "case4 - msg reached max length", + input: []byte("file.txt:6:7: message\nfile2.txt:6:7: "), + trainer: nil, + expected: map[string][]LinterOutput{ + "file.txt": []LinterOutput{ + { + File: "file.txt", + Line: 6, + Column: 7, + Message: "message", + }, + }, + "file2.txt": []LinterOutput{ + { + File: "file2.txt", + Line: 6, + Column: 7, + Message: "", + }, + }, + }, + unexpected: nil, + }, + { + id: "case5 - duplicate", + input: []byte("file.txt:6:7: message\nfile.txt:6:7: message\n"), + trainer: nil, + expected: map[string][]LinterOutput{ + "file.txt": []LinterOutput{ + { + File: "file.txt", + Line: 6, + Column: 7, + Message: "message", + }, + }, + }, + unexpected: nil, + }, + { + id: "case6 - with trainer", + input: []byte("file.txt:6:7: message\nfile2.txt:7:9: message\n"), + trainer: func(o LinterOutput) (*LinterOutput, []string) { + if o.Line == 6 { + return nil, []string{fmt.Sprintf("%s:%d:%d: %s", o.File, o.Line, o.Column, o.Message)} + } + return &o, nil + }, + expected: map[string][]LinterOutput{ + "file2.txt": []LinterOutput{ + { + File: "file2.txt", + Line: 7, + Column: 9, + Message: "message", + }, + }, + }, + unexpected: []string{"file.txt:6:7: message"}, + }, + } + for _, tc := range tcs { + t.Run(tc.id, func(t *testing.T) { + got, unexpected := ParseV2(xlog.New("ut"), tc.input, tc.trainer) + if !reflect.DeepEqual(tc.expected, got) { + t.Errorf("expected: %v, got: %v", tc.expected, got) + } + + if !reflect.DeepEqual(tc.unexpected, unexpected) { + t.Errorf("expected: %v, got: %v", tc.unexpected, unexpected) + } + }) + } +}