search

aspect-build / rules_lint

Run static analysis tools with Bazel
Apache License 2.0
91 stars 50 forks source link

[Bug]: Buf linter reports incorrect source line numbers #344

Open sallustfire opened 4 months ago

sallustfire commented 4 months ago

What happened?

The reported line numbers from running the buf_lint_aspect are always <souce_path>:1:1<messaage>, which adds friction to resolving lint errors especially when the source file is large.

Version

Development (host) and target OS/architectures:

Output of bazel --version: 7.1.1

Version of the Aspect rules, or other relevant rules from your WORKSPACE or MODULE.bazel file: HEAD

How to reproduce

This can be reproduced with the file.proto source in example/

Via aspect:

bazel build --aspects=//tools/lint:linters.bzl%buf --@aspect_rules_lint//lint:fail_on_violation --output_groups=rules_lint_report //src:foo_proto
INFO: Analyzed target //src:foo_proto (0 packages loaded, 0 targets configured).

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
--buf-plugin_out: src/file.proto:1:1:Import "src/unused.proto" is unused.
Aspect //tools/lint:linters.bzl%buf of //src:foo_proto failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 0.168s, Critical Path: 0.03s
INFO: 2 processes: 2 internal.
ERROR: Build did NOT complete successfully

Via buf cli

bazel run @rules_buf_toolchains//:buf -- lint --config=buf.yaml src/file.proto
WARNING: Build option --@@aspect_rules_lint~//lint:fail_on_violation has changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed).
INFO: Analyzed target @@rules_buf~~buf~rules_buf_toolchains//:buf (0 packages loaded, 1 target configured).
INFO: Found 1 target...
INFO: Elapsed time: 0.224s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Running command line: /home/.cache/bazel/_bazel/d387874fc4e6234b1e2f92e13a10d0a3/external/rules_buf~~buf~rules_buf_toolchains/buf lint '--config=buf.yaml' src/file.proto
src/file.proto:3:1:Import "src/unused.proto" is unused.

As a sanity check aspect lint produces the same output:

aspect lint //src:foo_proto
INFO: Analyzed target //src:foo_proto (232 packages loaded, 9814 targets configured).
INFO: Found 1 target...
Aspect //tools/lint:linters.bzl%buf of //src:foo_proto up-to-date:
  bazel-bin/src/foo_proto.AspectRulesLintBuf.report
  bazel-bin/src/foo_proto.AspectRulesLintBuf.report.exit_code
INFO: Elapsed time: 4.369s, Critical Path: 0.03s
INFO: 2 processes: 1 internal, 1 linux-sandbox.
INFO: Build completed successfully, 2 total actions
Lint results for //src:foo_proto:

--buf-plugin_out: src/file.proto:1:1:Import "src/unused.proto" is unused.

Any other information?

I haven't more than glanced at the source code, but I believe this is a problem with the protoc plugin protoc-gen-but-lint and not rules_lint. The buf cli is a go program that lints the sources directly and not as a protoc plugin. It may be the case that using the protoc plugin also introduces subtle differences in the DX if IDE plugins are integrating with the buf cli directly.

sallustfire commented 4 months ago

@alexeagle Here's what I see, and I saw the same problem with rules_proto_grpc and their buf lint integration in the past. That makes me thing it's a general problem with the protoc plugin, but I don't know how to square that with the smoke test you performed the other day.

alexeagle commented 4 months ago

To your earlier point, I do think switching to call buf lint instead of protoc --plugin=some-buf-thing is likely more correct and easier to reproduce or get help from the Buf team. Worth investigation.