googleapis / googleapis/api-linter
GitHub Action output format generates incorrect line numbers
- Dominant language
- Go
- Stars
- 765
- Forks
- 181
- Avg merge
- 2d 12h
- Merged PRs (30d)
- 3
Description
When using the `--output-format github` switch, the resulting can place the `Span` values into the wrong places, e.g.
```json
[710, 32, 40]
```
produces
```
::error file=users/users.proto,endLine=40,col=32,line=710,title=core...
```
I believe the issue comes from [this switch in the formatter code](https://github.com/googleapis/api-linter/blob/main/cmd/api-linter/github_actions.go#L40-L50):
```go
// 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])
}
```
Which does not treat the two different span lengths the same way as the JSON/YAML [marshaling code](https://github.com/googleapis/api-linter/blob/main/lint/problem.go#L134-L166)
```go
// If `span` has four ints; they correspond to
// [start line, start column, end line, end column].
//
// 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 {
return fileLocation{
Start: position{
Line: int(span[0]) + 1,
Column: int(span[1]) + 1,
},
End: position{
Line: int(span[2]) + 1,
Column: int(span[3]),
},
}
}
// Okay, `span` has three ints; they correspond to
// [start line, start column, end column].
//
// 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.
return fileLocation{
Start: position{
Line: int(span[0]) + 1,
Column: int(span[1]) + 1,
},
End: position{
Line: int(span[0]) + 1,
Column: int(span[2]),
},
}
```
I think a focused fix for the formatter might look like
```go
startLine := problem.Location.Span[0] + 1
startColumn := problem.Location.Span[1] + 1
if len(problem.Location.Span) == 4 {
fmt.Fprintf(&buf, ",line=%d,col=%d,endColumn=%d,endLine=%d", startLine, startColumn, problem.Location.Span[3], problem.Location.Span[2] + 1)
} else {
fmt.Fprintf(&buf, "line=%d,col=%d,endColumn=%d", startLine, startColumn, problem.Location.Span[2] + 1)
}
```
I think it would correctly parse the `Span` data.
Obviously, though, it's somewhat awkward, and requires repeating the logic for converting the zero-indexed numbers from the protobuf location. A bigger refactor might look something taking the private structs in `lint/problem.go` for file locations, like `fileLocation` and `position` and exporting them, and some method that can use `fileLocationFromPBLocation` to enable relying the same logic.
#### Environment details
- Programming language: go
- OS: macos and ubuntu
- Language runtime version: go 1.19.5
- Package version: 1.39.5
#### Steps to reproduce
Using this test file:
```protobuf
syntax = "proto3";
package test;
option go_package = "test.code/test";
service TestService {
rpc GetTest(GetTestRequest) returns (Test);
}
message GetTestRequest {
string url = 1;
}
message Test {
string url = 1;
}
```
1. Run the linter `api-linter test.proto`, observe the final complaint in the output has the following location
```
location:
start_position:
line_number: 3
column_number: 1
end_position:
line_number: 3
column_number: 13
```
2. Run the linter with `api-linter --output-format github test.proto`, and compare:
```
::error file=test.proto,endLine=13,col=0,line=2,title=core։։0215։։versioned-packages::API components should be in versioned packages.\n\nhttps://linter.aip.dev/215/versioned-package
```
Contributor guide
Assessment
This issue has not been assessed yet.