Skip to content

Commit 76a76d4

Browse files
SoMuchForSubtletySoMuchForSubtlety
committed
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
1 parent 05fc4f6 commit 76a76d4

3 files changed

Lines changed: 38 additions & 67 deletions

File tree

‎cmd/api-linter/github_actions.go‎

Lines changed: 5 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -32,34 +32,21 @@ func formatGitHubActionOutput(responses []lint.Response) []byte {
3232
// https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-an-error-message
3333

3434
fmt.Fprintf(&buf, "::error file=%s", response.FilePath)
35+
3536
if problem.Location != nil {
36-
// Some findings are *line level* and only have start positions but no
37-
// starting column. Construct a switch fallthrough to emit as many of
38-
// the location indicators are included.
39-
switch len(problem.Location.Span) {
40-
case 4:
41-
fmt.Fprintf(&buf, ",endColumn=%d", problem.Location.Span[3])
42-
fallthrough
43-
case 3:
44-
fmt.Fprintf(&buf, ",endLine=%d", problem.Location.Span[2])
45-
fallthrough
46-
case 2:
47-
fmt.Fprintf(&buf, ",col=%d", problem.Location.Span[1])
48-
fallthrough
49-
case 1:
50-
fmt.Fprintf(&buf, ",line=%d", problem.Location.Span[0])
51-
}
37+
loc := lint.FileLocationFromPBLocation(problem.Location, nil)
38+
fmt.Fprintf(&buf, ",line=%d,endLine=%d,col=%d,endColumn=%d", loc.Start.Line, loc.End.Line, loc.Start.Column, loc.End.Column)
5239
}
5340

5441
// GitHub uses :: as control characters (which are also used to delimit
5542
// linter rules. In order to prevent confusion, replace the double colon
5643
// with two Armenian full stops which are indistinguishable to my eye.
5744
runeThatLooksLikeTwoColonsButIsActuallyTwoArmenianFullStops := "։։"
5845
title := strings.ReplaceAll(string(problem.RuleID), "::", runeThatLooksLikeTwoColonsButIsActuallyTwoArmenianFullStops)
59-
message := strings.ReplaceAll(problem.Message, "\n", "\\n")
46+
message := strings.ReplaceAll(problem.Message, "\n", "%0A")
6047
uri := problem.GetRuleURI()
6148
if uri != "" {
62-
message += "\\n\\n" + uri
49+
message += "%0A%0A" + uri
6350
}
6451
fmt.Fprintf(&buf, ",title=%s::%s\n", title, message)
6552
}

‎cmd/api-linter/github_actions_test.go‎

Lines changed: 12 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -53,27 +53,11 @@ func TestFormatGitHubActionOutput(t *testing.T) {
5353
Span: []int32{5, 6, 7},
5454
},
5555
},
56-
{
57-
RuleID: "line::col",
58-
Message: "Line and column",
59-
Location: &descriptorpb.SourceCodeInfo_Location{
60-
Span: []int32{5, 6},
61-
},
62-
},
63-
{
64-
RuleID: "line",
65-
Message: "Line only",
66-
Location: &descriptorpb.SourceCodeInfo_Location{
67-
Span: []int32{5},
68-
},
69-
},
7056
},
7157
},
7258
},
73-
want: `::error file=example.proto,endColumn=8,endLine=7,col=6,line=5,title=line։։col։։endLine։։endColumn::line, column, endline, and endColumn
74-
::error file=example.proto,endLine=7,col=6,line=5,title=line։։col։։endLine::Line, column, and endline
75-
::error file=example.proto,col=6,line=5,title=line։։col::Line and column
76-
::error file=example.proto,line=5,title=line::Line only
59+
want: `::error file=example.proto,line=6,endLine=8,col=7,endColumn=8,title=line։։col։։endLine։։endColumn::line, column, endline, and endColumn
60+
::error file=example.proto,line=6,endLine=6,col=7,endColumn=7,title=line։։col։։endLine::Line, column, and endline
7761
`,
7862
},
7963
{
@@ -90,16 +74,16 @@ func TestFormatGitHubActionOutput(t *testing.T) {
9074
},
9175
{
9276
RuleID: "core::naming_formats::field_names",
93-
Message: "multi\nline\ncomment",
77+
Message: "multi%0Aline%0Acomment",
9478
Location: &descriptorpb.SourceCodeInfo_Location{
9579
Span: []int32{5, 6, 7, 8},
9680
},
9781
},
9882
},
9983
},
10084
},
101-
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
102-
::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
85+
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
86+
::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
10387
`,
10488
},
10589
{
@@ -133,13 +117,13 @@ func TestFormatGitHubActionOutput(t *testing.T) {
133117
},
134118
},
135119
},
136-
want: `::error file=example.proto,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names
137-
::error file=example.proto,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names
138-
::error file=example2.proto,title=core։։0131։։request_message։։name::\n\nhttps://linter.aip.dev/131/request_message/name
139-
::error file=example2.proto,title=core։։0132։։response_message։։name::\n\nhttps://linter.aip.dev/132/response_message/name
140-
::error file=example3.proto,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names
141-
::error file=example4.proto,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names
142-
::error file=example4.proto,title=core։։0132։։response_message։։name::\n\nhttps://linter.aip.dev/132/response_message/name
120+
want: `::error file=example.proto,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names
121+
::error file=example.proto,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names
122+
::error file=example2.proto,title=core։։0131։։request_message։։name::%0A%0Ahttps://linter.aip.dev/131/request_message/name
123+
::error file=example2.proto,title=core։։0132։։response_message։։name::%0A%0Ahttps://linter.aip.dev/132/response_message/name
124+
::error file=example3.proto,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names
125+
::error file=example4.proto,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names
126+
::error file=example4.proto,title=core։։0132։։response_message։։name::%0A%0Ahttps://linter.aip.dev/132/response_message/name
143127
`,
144128
},
145129
}

‎lint/problem.go‎

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -76,36 +76,36 @@ func (p Problem) MarshalYAML() (interface{}, error) {
7676

7777
// Marshal defines how to represent a serialized Problem.
7878
func (p Problem) marshal() interface{} {
79-
var fl fileLocation
79+
var fl FileLocation
8080
if p.Location != nil {
8181
// If Location is set, use it.
82-
fl = fileLocationFromPBLocation(p.Location, p.Descriptor)
82+
fl = FileLocationFromPBLocation(p.Location, p.Descriptor)
8383
} else if p.Descriptor != nil {
8484
// Otherwise, use the descriptor's location.
8585
// This is the protobuf-go idiomatic way to get the source location.
8686
// Note: ParentFile() called on a FileDescriptor returns itself.
8787
loc := p.Descriptor.ParentFile().SourceLocations().ByDescriptor(p.Descriptor)
88-
fl = fileLocation{
88+
fl = FileLocation{
8989
Path: p.Descriptor.ParentFile().Path(),
90-
Start: position{
90+
Start: Position{
9191
Line: loc.StartLine + 1,
9292
Column: loc.StartColumn + 1,
9393
},
94-
End: position{
94+
End: Position{
9595
Line: loc.EndLine + 1,
9696
Column: loc.EndColumn,
9797
},
9898
}
9999
} else {
100100
// Default location if no descriptor.
101-
fl = fileLocationFromPBLocation(nil, nil)
101+
fl = FileLocationFromPBLocation(nil, nil)
102102
}
103103

104104
// Return a marshal-able structure.
105105
return struct {
106106
Message string `json:"message" yaml:"message"`
107107
Suggestion string `json:"suggestion,omitempty" yaml:"suggestion,omitempty"`
108-
Location fileLocation `json:"location" yaml:"location"`
108+
Location FileLocation `json:"location" yaml:"location"`
109109
RuleID RuleName `json:"rule_id" yaml:"rule_id"`
110110
RuleDocURI string `json:"rule_doc_uri" yaml:"rule_doc_uri"`
111111
Category string `json:"category,omitempty" yaml:"category,omitempty"`
@@ -124,35 +124,35 @@ func (p Problem) GetRuleURI() string {
124124
return getRuleURL(string(p.RuleID), ruleURLMappings)
125125
}
126126

127-
// position describes a one-based position in a source code file.
127+
// Position describes a one-based Position in a source code file.
128128
// They are one-indexed, as a human counts lines or columns.
129-
type position struct {
129+
type Position struct {
130130
Line int `json:"line_number" yaml:"line_number"`
131131
Column int `json:"column_number" yaml:"column_number"`
132132
}
133133

134-
// fileLocation describes a location in a source code file.
134+
// FileLocation describes a location in a source code file.
135135
//
136136
// Note: Positions are one-indexed, as a human counts lines or columns
137137
// in a file.
138-
type fileLocation struct {
139-
Start position `json:"start_position" yaml:"start_position"`
140-
End position `json:"end_position" yaml:"end_position"`
138+
type FileLocation struct {
139+
Start Position `json:"start_position" yaml:"start_position"`
140+
End Position `json:"end_position" yaml:"end_position"`
141141
Path string `json:"path" yaml:"path"`
142142
}
143143

144-
// fileLocationFromPBLocation returns a new fileLocation object based on a
144+
// FileLocationFromPBLocation returns a new fileLocation object based on a
145145
// protocol buffer SourceCodeInfo_Location
146-
func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d protoreflect.Descriptor) fileLocation {
146+
func FileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d protoreflect.Descriptor) FileLocation {
147147
// Spans are guaranteed by protobuf to have either three or four ints.
148148
span := []int32{0, 0, 1}
149149
if l != nil {
150150
span = l.Span
151151
}
152152

153-
var fl fileLocation
153+
var fl FileLocation
154154
if d != nil {
155-
fl = fileLocation{Path: d.ParentFile().Path()}
155+
fl = FileLocation{Path: d.ParentFile().Path()}
156156
}
157157

158158
// If `span` has four ints; they correspond to
@@ -161,11 +161,11 @@ func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d protoreflect.D
161161
// We add one because spans are zero-indexed, but not to the end column
162162
// because we want the ending position to be inclusive and not exclusive.
163163
if len(span) == 4 {
164-
fl.Start = position{
164+
fl.Start = Position{
165165
Line: int(span[0]) + 1,
166166
Column: int(span[1]) + 1,
167167
}
168-
fl.End = position{
168+
fl.End = Position{
169169
Line: int(span[2]) + 1,
170170
Column: int(span[3]),
171171
}
@@ -177,11 +177,11 @@ func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d protoreflect.D
177177
//
178178
// We add one because spans are zero-indexed, but not to the end column
179179
// because we want the ending position to be inclusive and not exclusive.
180-
fl.Start = position{
180+
fl.Start = Position{
181181
Line: int(span[0]) + 1,
182182
Column: int(span[1]) + 1,
183183
}
184-
fl.End = position{
184+
fl.End = Position{
185185
Line: int(span[0]) + 1,
186186
Column: int(span[2]),
187187
}

0 commit comments

Comments
 (0)