googleapis / googleapis/api-linter

GitHub Action output format generates incorrect line numbers

Open
#1,071 0 comments 0 reactions 0 assignees View on GitHub
priority: p2 type: bug
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.