From dd44ba399786a0b5827780bfc2be5412ddc7d2e6 Mon Sep 17 00:00:00 2001 From: Talon Bowler Date: Tue, 16 Apr 2024 12:00:16 -0700 Subject: [PATCH 1/3] sort lint warning output by file and line number, sourcemap format now consistent Signed-off-by: Talon Bowler --- frontend/subrequests/lint/lint.go | 81 +++++++++++++------------------ 1 file changed, 33 insertions(+), 48 deletions(-) diff --git a/frontend/subrequests/lint/lint.go b/frontend/subrequests/lint/lint.go index a4a05837324b..dcd205d68d7a 100644 --- a/frontend/subrequests/lint/lint.go +++ b/frontend/subrequests/lint/lint.go @@ -6,12 +6,12 @@ import ( "fmt" "io" "sort" - "text/tabwriter" "github.com/moby/buildkit/client/llb" "github.com/moby/buildkit/frontend/dockerfile/parser" "github.com/moby/buildkit/frontend/gateway/client" "github.com/moby/buildkit/frontend/subrequests" + "github.com/moby/buildkit/solver/errdefs" "github.com/moby/buildkit/solver/pb" ) @@ -30,13 +30,6 @@ var SubrequestLintDefinition = subrequests.Request{ }, } -type Source struct { - Filename string `json:"fileName"` - Language string `json:"language"` - Definition *pb.Definition `json:"definition"` - Data []byte `json:"data"` -} - type Warning struct { RuleName string `json:"ruleName"` Description string `json:"description,omitempty"` @@ -46,19 +39,19 @@ type Warning struct { } type LintResults struct { - Warnings []Warning `json:"warnings"` - Sources []Source `json:"sources"` + Warnings []Warning `json:"warnings"` + Sources []*pb.SourceInfo `json:"sources"` } func (results *LintResults) AddSource(sourceMap *llb.SourceMap) int { - newSource := Source{ + newSource := &pb.SourceInfo{ Filename: sourceMap.Filename, Language: sourceMap.Language, Definition: sourceMap.Definition.ToPB(), Data: sourceMap.Data, } for i, source := range results.Sources { - if sourceEqual(source, newSource) { + if sourceInfoEqual(source, newSource) { return i } } @@ -93,7 +86,7 @@ func (results *LintResults) AddWarning(rulename, description, url, fmtmsg string }) } -func sourceEqual(a, b Source) bool { +func sourceInfoEqual(a, b *pb.SourceInfo) bool { if a.Filename != b.Filename || a.Language != b.Language { return false } @@ -125,46 +118,38 @@ func (results *LintResults) ToResult() (*client.Result, error) { } func PrintLintViolations(dt []byte, w io.Writer) error { - var warnings LintResults + var results LintResults - if err := json.Unmarshal(dt, &warnings); err != nil { + if err := json.Unmarshal(dt, &results); err != nil { return err } - // Here, we're grouping the warnings by rule name - lintWarnings := make(map[string][]Warning) - lintWarningRules := []string{} - for _, warning := range warnings.Warnings { - if _, ok := lintWarnings[warning.RuleName]; !ok { - lintWarningRules = append(lintWarningRules, warning.RuleName) - lintWarnings[warning.RuleName] = []Warning{} + sort.Slice(results.Warnings, func(i, j int) bool { + sourceInfoI := results.Sources[results.Warnings[i].Location.SourceIndex] + sourceInfoJ := results.Sources[results.Warnings[j].Location.SourceIndex] + if sourceInfoI.Filename != sourceInfoJ.Filename { + return sourceInfoI.Filename < sourceInfoJ.Filename } - lintWarnings[warning.RuleName] = append(lintWarnings[warning.RuleName], warning) - } - sort.Strings(lintWarningRules) - - tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) - for _, rule := range lintWarningRules { - fmt.Fprintf(tw, "Lint Rule %s\n", rule) - for _, warning := range lintWarnings[rule] { - source := warnings.Sources[warning.Location.SourceIndex] - sourceData := bytes.Split(source.Data, []byte("\n")) - firstRange := warning.Location.Ranges[0] - if firstRange.Start.Line != firstRange.End.Line { - fmt.Fprintf(tw, "\t%s:%d-%d\n", source.Filename, firstRange.Start.Line, firstRange.End.Line) - } else { - fmt.Fprintf(tw, "\t%s:%d\n", source.Filename, firstRange.Start.Line) - } - fmt.Fprintf(tw, "\t%s\n", warning.Detail) - for _, r := range warning.Location.Ranges { - for i := r.Start.Line; i <= r.End.Line; i++ { - fmt.Fprintf(tw, "\t%d\t|\t%s\n", i, sourceData[i-1]) - } - } - fmt.Fprintln(tw) + if len(results.Warnings[i].Location.Ranges) == 0 || len(results.Warnings[j].Location.Ranges) == 0 { + return false } - fmt.Fprintln(tw) - } + return results.Warnings[i].Location.Ranges[0].Start.Line < results.Warnings[j].Location.Ranges[0].Start.Line + }) - return tw.Flush() + for _, warning := range results.Warnings { + fmt.Fprintf(w, "\n- %s\n%s\n", warning.Detail, warning.Description) + if warning.URL != "" { + fmt.Fprintf(w, "URL: %s\n", warning.URL) + } + sourceInfo := results.Sources[warning.Location.SourceIndex] + source := errdefs.Source{ + Info: sourceInfo, + Ranges: warning.Location.Ranges, + } + err := source.Print(w) + if err != nil { + return err + } + } + return nil } From 1999baeaee96d6d6296ede427de68d0c0accf116 Mon Sep 17 00:00:00 2001 From: Talon Bowler Date: Tue, 16 Apr 2024 19:25:41 -0700 Subject: [PATCH 2/3] add basic warning validations so we can safely sort and print the warnings we receive from the lint subrequest Signed-off-by: Talon Bowler --- frontend/subrequests/lint/lint.go | 43 ++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/frontend/subrequests/lint/lint.go b/frontend/subrequests/lint/lint.go index dcd205d68d7a..993b90fad5fc 100644 --- a/frontend/subrequests/lint/lint.go +++ b/frontend/subrequests/lint/lint.go @@ -86,13 +86,6 @@ func (results *LintResults) AddWarning(rulename, description, url, fmtmsg string }) } -func sourceInfoEqual(a, b *pb.SourceInfo) bool { - if a.Filename != b.Filename || a.Language != b.Language { - return false - } - return bytes.Equal(a.Data, b.Data) -} - func (results *LintResults) ToResult() (*client.Result, error) { res := client.NewResult() dt, err := json.MarshalIndent(results, "", " ") @@ -117,6 +110,28 @@ func (results *LintResults) ToResult() (*client.Result, error) { return res, nil } +func (results *LintResults) validateWarnings() error { + for _, warning := range results.Warnings { + if warning.Location.SourceIndex < 0 { + return fmt.Errorf("sourceIndex is required") + } + if int(warning.Location.SourceIndex) >= len(results.Sources) { + return fmt.Errorf("sourceIndex is out of range") + } + warningSource := results.Sources[warning.Location.SourceIndex] + if warningSource == nil { + return fmt.Errorf("sourceIndex points to nil source") + } + if warningSource.Definition == nil { + return fmt.Errorf("sourceIndex points to source with nil definition") + } + if len(warning.Location.Ranges) == 0 { + return fmt.Errorf("ranges is required") + } + } + return nil +} + func PrintLintViolations(dt []byte, w io.Writer) error { var results LintResults @@ -124,15 +139,16 @@ func PrintLintViolations(dt []byte, w io.Writer) error { return err } + if err := results.validateWarnings(); err != nil { + return err + } + sort.Slice(results.Warnings, func(i, j int) bool { sourceInfoI := results.Sources[results.Warnings[i].Location.SourceIndex] sourceInfoJ := results.Sources[results.Warnings[j].Location.SourceIndex] if sourceInfoI.Filename != sourceInfoJ.Filename { return sourceInfoI.Filename < sourceInfoJ.Filename } - if len(results.Warnings[i].Location.Ranges) == 0 || len(results.Warnings[j].Location.Ranges) == 0 { - return false - } return results.Warnings[i].Location.Ranges[0].Start.Line < results.Warnings[j].Location.Ranges[0].Start.Line }) @@ -153,3 +169,10 @@ func PrintLintViolations(dt []byte, w io.Writer) error { } return nil } + +func sourceInfoEqual(a, b *pb.SourceInfo) bool { + if a.Filename != b.Filename || a.Language != b.Language { + return false + } + return bytes.Equal(a.Data, b.Data) +} From 04d85f3ea57a5c5982a4e59c74d1bfbe64e89532 Mon Sep 17 00:00:00 2001 From: Talon Bowler Date: Tue, 16 Apr 2024 20:28:03 -0700 Subject: [PATCH 3/3] Update lint warning sorting to handle mssing ranges and < 1 sourceIndex. Signed-off-by: Talon Bowler --- frontend/subrequests/lint/lint.go | 45 ++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 16 deletions(-) diff --git a/frontend/subrequests/lint/lint.go b/frontend/subrequests/lint/lint.go index 993b90fad5fc..6237eb3c1c21 100644 --- a/frontend/subrequests/lint/lint.go +++ b/frontend/subrequests/lint/lint.go @@ -13,6 +13,7 @@ import ( "github.com/moby/buildkit/frontend/subrequests" "github.com/moby/buildkit/solver/errdefs" "github.com/moby/buildkit/solver/pb" + "github.com/pkg/errors" ) const RequestLint = "frontend.lint" @@ -112,21 +113,14 @@ func (results *LintResults) ToResult() (*client.Result, error) { func (results *LintResults) validateWarnings() error { for _, warning := range results.Warnings { - if warning.Location.SourceIndex < 0 { - return fmt.Errorf("sourceIndex is required") - } if int(warning.Location.SourceIndex) >= len(results.Sources) { - return fmt.Errorf("sourceIndex is out of range") - } - warningSource := results.Sources[warning.Location.SourceIndex] - if warningSource == nil { - return fmt.Errorf("sourceIndex points to nil source") + return errors.Errorf("sourceIndex is out of range") } - if warningSource.Definition == nil { - return fmt.Errorf("sourceIndex points to source with nil definition") - } - if len(warning.Location.Ranges) == 0 { - return fmt.Errorf("ranges is required") + if warning.Location.SourceIndex > 0 { + warningSource := results.Sources[warning.Location.SourceIndex] + if warningSource == nil { + return errors.Errorf("sourceIndex points to nil source") + } } } return nil @@ -144,12 +138,28 @@ func PrintLintViolations(dt []byte, w io.Writer) error { } sort.Slice(results.Warnings, func(i, j int) bool { - sourceInfoI := results.Sources[results.Warnings[i].Location.SourceIndex] - sourceInfoJ := results.Sources[results.Warnings[j].Location.SourceIndex] + warningI := results.Warnings[i] + warningJ := results.Warnings[j] + sourceIndexI := warningI.Location.SourceIndex + sourceIndexJ := warningJ.Location.SourceIndex + if sourceIndexI < 0 && sourceIndexJ < 0 { + return warningI.RuleName < warningJ.RuleName + } else if sourceIndexI < 0 || sourceIndexJ < 0 { + return sourceIndexI < 0 + } + + sourceInfoI := results.Sources[warningI.Location.SourceIndex] + sourceInfoJ := results.Sources[warningJ.Location.SourceIndex] if sourceInfoI.Filename != sourceInfoJ.Filename { return sourceInfoI.Filename < sourceInfoJ.Filename } - return results.Warnings[i].Location.Ranges[0].Start.Line < results.Warnings[j].Location.Ranges[0].Start.Line + if len(warningI.Location.Ranges) == 0 && len(warningJ.Location.Ranges) == 0 { + return warningI.RuleName < warningJ.RuleName + } else if len(warningI.Location.Ranges) == 0 || len(warningJ.Location.Ranges) == 0 { + return len(warningI.Location.Ranges) == 0 + } + + return warningI.Location.Ranges[0].Start.Line < warningJ.Location.Ranges[0].Start.Line }) for _, warning := range results.Warnings { @@ -157,6 +167,9 @@ func PrintLintViolations(dt []byte, w io.Writer) error { if warning.URL != "" { fmt.Fprintf(w, "URL: %s\n", warning.URL) } + if warning.Location.SourceIndex < 0 { + continue + } sourceInfo := results.Sources[warning.Location.SourceIndex] source := errdefs.Source{ Info: sourceInfo,