From 76a76d4aa6d5bd4f9b91ff569d2297c17d142677 Mon Sep 17 00:00:00 2001 From: SoMuchForSubtlety Date: Thu, 9 Apr 2026 19:53:48 +0200 Subject: [PATCH 1/2] fix: interpret span correctly for github format Reuse location logic from the lint package The old location logic had two bugs - keeping locations zero-indexed - interpreting end column as end line in case of span size 3 --- cmd/api-linter/github_actions.go | 23 ++++----------- cmd/api-linter/github_actions_test.go | 40 ++++++++----------------- lint/problem.go | 42 +++++++++++++-------------- 3 files changed, 38 insertions(+), 67 deletions(-) diff --git a/cmd/api-linter/github_actions.go b/cmd/api-linter/github_actions.go index 712f1b109..7e2e97bf4 100644 --- a/cmd/api-linter/github_actions.go +++ b/cmd/api-linter/github_actions.go @@ -32,23 +32,10 @@ func formatGitHubActionOutput(responses []lint.Response) []byte { // https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-an-error-message fmt.Fprintf(&buf, "::error file=%s", response.FilePath) + if problem.Location != nil { - // Some findings are *line level* and only have start positions but no - // starting column. Construct a switch fallthrough to emit as many of - // the location indicators are included. - switch len(problem.Location.Span) { - case 4: - fmt.Fprintf(&buf, ",endColumn=%d", problem.Location.Span[3]) - fallthrough - case 3: - fmt.Fprintf(&buf, ",endLine=%d", problem.Location.Span[2]) - fallthrough - case 2: - fmt.Fprintf(&buf, ",col=%d", problem.Location.Span[1]) - fallthrough - case 1: - fmt.Fprintf(&buf, ",line=%d", problem.Location.Span[0]) - } + loc := lint.FileLocationFromPBLocation(problem.Location, nil) + fmt.Fprintf(&buf, ",line=%d,endLine=%d,col=%d,endColumn=%d", loc.Start.Line, loc.End.Line, loc.Start.Column, loc.End.Column) } // GitHub uses :: as control characters (which are also used to delimit @@ -56,10 +43,10 @@ func formatGitHubActionOutput(responses []lint.Response) []byte { // with two Armenian full stops which are indistinguishable to my eye. runeThatLooksLikeTwoColonsButIsActuallyTwoArmenianFullStops := "։։" title := strings.ReplaceAll(string(problem.RuleID), "::", runeThatLooksLikeTwoColonsButIsActuallyTwoArmenianFullStops) - message := strings.ReplaceAll(problem.Message, "\n", "\\n") + message := strings.ReplaceAll(problem.Message, "\n", "%0A") uri := problem.GetRuleURI() if uri != "" { - message += "\\n\\n" + uri + message += "%0A%0A" + uri } fmt.Fprintf(&buf, ",title=%s::%s\n", title, message) } diff --git a/cmd/api-linter/github_actions_test.go b/cmd/api-linter/github_actions_test.go index 48e43b9d7..5a46b2906 100644 --- a/cmd/api-linter/github_actions_test.go +++ b/cmd/api-linter/github_actions_test.go @@ -53,27 +53,11 @@ func TestFormatGitHubActionOutput(t *testing.T) { Span: []int32{5, 6, 7}, }, }, - { - RuleID: "line::col", - Message: "Line and column", - Location: &descriptorpb.SourceCodeInfo_Location{ - Span: []int32{5, 6}, - }, - }, - { - RuleID: "line", - Message: "Line only", - Location: &descriptorpb.SourceCodeInfo_Location{ - Span: []int32{5}, - }, - }, }, }, }, - want: `::error file=example.proto,endColumn=8,endLine=7,col=6,line=5,title=line։։col։։endLine։։endColumn::line, column, endline, and endColumn -::error file=example.proto,endLine=7,col=6,line=5,title=line։։col։։endLine::Line, column, and endline -::error file=example.proto,col=6,line=5,title=line։։col::Line and column -::error file=example.proto,line=5,title=line::Line only + want: `::error file=example.proto,line=6,endLine=8,col=7,endColumn=8,title=line։։col։։endLine։։endColumn::line, column, endline, and endColumn +::error file=example.proto,line=6,endLine=6,col=7,endColumn=7,title=line։։col։։endLine::Line, column, and endline `, }, { @@ -90,7 +74,7 @@ func TestFormatGitHubActionOutput(t *testing.T) { }, { RuleID: "core::naming_formats::field_names", - Message: "multi\nline\ncomment", + Message: "multi%0Aline%0Acomment", Location: &descriptorpb.SourceCodeInfo_Location{ Span: []int32{5, 6, 7, 8}, }, @@ -98,8 +82,8 @@ func TestFormatGitHubActionOutput(t *testing.T) { }, }, }, - want: `::error file=example.proto,endColumn=4,endLine=3,col=2,line=1,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names -::error file=example.proto,endColumn=8,endLine=7,col=6,line=5,title=core։։naming_formats։։field_names::multi\nline\ncomment\n\nhttps://linter.aip.dev/naming_formats/field_names + want: `::error file=example.proto,line=2,endLine=4,col=3,endColumn=4,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names +::error file=example.proto,line=6,endLine=8,col=7,endColumn=8,title=core։։naming_formats։։field_names::multi%0Aline%0Acomment%0A%0Ahttps://linter.aip.dev/naming_formats/field_names `, }, { @@ -133,13 +117,13 @@ func TestFormatGitHubActionOutput(t *testing.T) { }, }, }, - want: `::error file=example.proto,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names -::error file=example.proto,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names -::error file=example2.proto,title=core։։0131։։request_message։։name::\n\nhttps://linter.aip.dev/131/request_message/name -::error file=example2.proto,title=core։։0132։։response_message։։name::\n\nhttps://linter.aip.dev/132/response_message/name -::error file=example3.proto,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names -::error file=example4.proto,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names -::error file=example4.proto,title=core։։0132։։response_message։։name::\n\nhttps://linter.aip.dev/132/response_message/name + want: `::error file=example.proto,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names +::error file=example.proto,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names +::error file=example2.proto,title=core։։0131։։request_message։։name::%0A%0Ahttps://linter.aip.dev/131/request_message/name +::error file=example2.proto,title=core։։0132։։response_message։։name::%0A%0Ahttps://linter.aip.dev/132/response_message/name +::error file=example3.proto,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names +::error file=example4.proto,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names +::error file=example4.proto,title=core։։0132։։response_message։։name::%0A%0Ahttps://linter.aip.dev/132/response_message/name `, }, } diff --git a/lint/problem.go b/lint/problem.go index ea7d24b2c..a203993e3 100644 --- a/lint/problem.go +++ b/lint/problem.go @@ -76,36 +76,36 @@ func (p Problem) MarshalYAML() (interface{}, error) { // Marshal defines how to represent a serialized Problem. func (p Problem) marshal() interface{} { - var fl fileLocation + var fl FileLocation if p.Location != nil { // If Location is set, use it. - fl = fileLocationFromPBLocation(p.Location, p.Descriptor) + fl = FileLocationFromPBLocation(p.Location, p.Descriptor) } else if p.Descriptor != nil { // Otherwise, use the descriptor's location. // This is the protobuf-go idiomatic way to get the source location. // Note: ParentFile() called on a FileDescriptor returns itself. loc := p.Descriptor.ParentFile().SourceLocations().ByDescriptor(p.Descriptor) - fl = fileLocation{ + fl = FileLocation{ Path: p.Descriptor.ParentFile().Path(), - Start: position{ + Start: Position{ Line: loc.StartLine + 1, Column: loc.StartColumn + 1, }, - End: position{ + End: Position{ Line: loc.EndLine + 1, Column: loc.EndColumn, }, } } else { // Default location if no descriptor. - fl = fileLocationFromPBLocation(nil, nil) + fl = FileLocationFromPBLocation(nil, nil) } // Return a marshal-able structure. return struct { Message string `json:"message" yaml:"message"` Suggestion string `json:"suggestion,omitempty" yaml:"suggestion,omitempty"` - Location fileLocation `json:"location" yaml:"location"` + Location FileLocation `json:"location" yaml:"location"` RuleID RuleName `json:"rule_id" yaml:"rule_id"` RuleDocURI string `json:"rule_doc_uri" yaml:"rule_doc_uri"` Category string `json:"category,omitempty" yaml:"category,omitempty"` @@ -124,35 +124,35 @@ func (p Problem) GetRuleURI() string { return getRuleURL(string(p.RuleID), ruleURLMappings) } -// position describes a one-based position in a source code file. +// Position describes a one-based Position in a source code file. // They are one-indexed, as a human counts lines or columns. -type position struct { +type Position struct { Line int `json:"line_number" yaml:"line_number"` Column int `json:"column_number" yaml:"column_number"` } -// fileLocation describes a location in a source code file. +// FileLocation describes a location in a source code file. // // Note: Positions are one-indexed, as a human counts lines or columns // in a file. -type fileLocation struct { - Start position `json:"start_position" yaml:"start_position"` - End position `json:"end_position" yaml:"end_position"` +type FileLocation struct { + Start Position `json:"start_position" yaml:"start_position"` + End Position `json:"end_position" yaml:"end_position"` Path string `json:"path" yaml:"path"` } -// fileLocationFromPBLocation returns a new fileLocation object based on a +// FileLocationFromPBLocation returns a new fileLocation object based on a // protocol buffer SourceCodeInfo_Location -func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d protoreflect.Descriptor) fileLocation { +func FileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d protoreflect.Descriptor) FileLocation { // Spans are guaranteed by protobuf to have either three or four ints. span := []int32{0, 0, 1} if l != nil { span = l.Span } - var fl fileLocation + var fl FileLocation if d != nil { - fl = fileLocation{Path: d.ParentFile().Path()} + fl = FileLocation{Path: d.ParentFile().Path()} } // If `span` has four ints; they correspond to @@ -161,11 +161,11 @@ func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d protoreflect.D // We add one because spans are zero-indexed, but not to the end column // because we want the ending position to be inclusive and not exclusive. if len(span) == 4 { - fl.Start = position{ + fl.Start = Position{ Line: int(span[0]) + 1, Column: int(span[1]) + 1, } - fl.End = position{ + fl.End = Position{ Line: int(span[2]) + 1, Column: int(span[3]), } @@ -177,11 +177,11 @@ func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d protoreflect.D // // We add one because spans are zero-indexed, but not to the end column // because we want the ending position to be inclusive and not exclusive. - fl.Start = position{ + fl.Start = Position{ Line: int(span[0]) + 1, Column: int(span[1]) + 1, } - fl.End = position{ + fl.End = Position{ Line: int(span[0]) + 1, Column: int(span[2]), } From bac5bc6d5e664a5b6b412e45ce9553056a2da25d Mon Sep 17 00:00:00 2001 From: SoMuchForSubtlety Date: Thu, 9 Apr 2026 20:20:33 +0200 Subject: [PATCH 2/2] fix docstrings --- lint/problem.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lint/problem.go b/lint/problem.go index a203993e3..9cb2cdd12 100644 --- a/lint/problem.go +++ b/lint/problem.go @@ -124,7 +124,7 @@ func (p Problem) GetRuleURI() string { return getRuleURL(string(p.RuleID), ruleURLMappings) } -// Position describes a one-based Position in a source code file. +// Position describes a one-based position in a source code file. // They are one-indexed, as a human counts lines or columns. type Position struct { Line int `json:"line_number" yaml:"line_number"` @@ -141,7 +141,7 @@ type FileLocation struct { Path string `json:"path" yaml:"path"` } -// FileLocationFromPBLocation returns a new fileLocation object based on a +// FileLocationFromPBLocation returns a new FileLocation object based on a // protocol buffer SourceCodeInfo_Location func FileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d protoreflect.Descriptor) FileLocation { // Spans are guaranteed by protobuf to have either three or four ints.