buf_lint_test: unused import is not reported

[alexeagle]

This may be the same root cause as #22

I made this patch to repro at HEAD:

commit f120b0bb66df0399a1f7e5ded672fc3cd22e3e5b (HEAD -> main)
Author: Alex Eagle <alex@aspect.dev>
Date:   Tue Dec 6 13:59:53 2022 -0800

    repro: no unused warning

diff --git a/examples/single_module/foo/v1/BUILD.bazel b/examples/single_module/foo/v1/BUILD.bazel
index f6db6ee..371d4c0 100644
--- a/examples/single_module/foo/v1/BUILD.bazel
+++ b/examples/single_module/foo/v1/BUILD.bazel
@@ -17,7 +17,10 @@ load("@rules_proto//proto:defs.bzl", "proto_library")
 
 proto_library(
     name = "foo_proto",
-    srcs = ["foo.proto"],
+    srcs = [
+        "foo.proto",
+        "other.proto",
+    ],
     visibility = ["//visibility:public"],
 )
 
diff --git a/examples/single_module/foo/v1/foo.proto b/examples/single_module/foo/v1/foo.proto
index 4e5227b..4f96307 100644
--- a/examples/single_module/foo/v1/foo.proto
+++ b/examples/single_module/foo/v1/foo.proto
@@ -16,6 +16,8 @@ syntax = "proto3";
 
 package foo.v1;
 
+import "foo/v1/other.proto";
+
 message Foo {
   string id = 1;
   string name = 2;
diff --git a/examples/single_module/foo/v1/other.proto b/examples/single_module/foo/v1/other.proto
new file mode 100644
index 0000000..790082b
--- /dev/null
+++ b/examples/single_module/foo/v1/other.proto
@@ -0,0 +1,3 @@
+syntax = "proto3";
+
+package foo.v1;

but the buf_lint_test still passes:

% bazel test foo/...                       
INFO: Analyzed 2 targets (0 packages loaded, 0 targets configured).
INFO: Found 1 target and 1 test target...
INFO: From Generating Descriptor Set proto_library //foo/v1:foo_proto:
foo/v1/foo.proto:19:1: warning: Import foo/v1/other.proto is unused.
INFO: Elapsed time: 0.288s, Critical Path: 0.17s
INFO: 3 processes: 3 darwin-sandbox.
//foo/v1:foo_proto_lint                                                  PASSED in 0.1s

Note that the proto_library rule wrote a warning in the action that created the Descriptor Set, but there's no test failure so the warning is easily ignored by developers.

[sushain97]

@alexeagle Has this been fixed? Not finding any recent changes that would indicate that it has been.

[alexeagle]

Ha thats funny, in a private repo I commented "when they fix " and GitHub triggered the auto close when that merged

[jhump]

@sushain97, @alexeagle: this is still an issue. Sadly, the protoc-gen-buf-lint plugin that the rule uses behaves differently than buf lint due to how the compiled descriptors are produced and provided to the plugin. With buf lint, we rely on the compiler emitting a warning about the unused import and then convey that as a lint error if such a warning is present. With protoc-gen-buf-lint, it is provided a compiled descriptor, so there is no compiler warning output available to it.

We could reproduce the unused import checks that the compiler does (though it's not necessarily trivial to implement due to how public imports work, and it is more cumbersome to implement with a compiled descriptor vs. with source due to how custom options are represented). But another thing we're looking into is how to possibly plug buf into the proto_library rule in place of protoc; buf can then include metadata about unused imports in the file descriptors that the plugin receives (just as it does when using buf lint).

There is actually a similar issue related to the warning about a file missing a syntax declaration. Unfortunately, this one cannot be solved by the plugin trying to reproduce a check that the compiler performs. This is a check that only the compiler can detect because a descriptor always has a syntax value, even if the original source file did not. So the approach we're exploring for unused imports would also allow us to address this (since the buf compiler can include metadata in the file descriptor set regarding whether the syntax declaration actually existed in source).

[smocherla-brex]

Based on the above comment, I happened to peek into the underlying Buf compiler code here and was able to write a custom test rule prototype that does unused import checks (example here). Sharing it here in case anyone else would find it helpful (note: my example was quickly put together, so might be rough around the edges)

[alexeagle]

@jhump does your team want to take that solution and upstream it? Otherwise anyone finding this issue will use @smocherla-brex solution which is ready-to-go (thanks for posting that!!)

[jhump]

@alexeagle, I'm looking into a fix in the protoc-gen-buf-lint plugin itself, to do this check in a more integrated/transparent way. The linked solution is rather heavy-weight, re-doing the compile from source. It should be much more efficient to instead examine the edges of the graph in the descriptors provided in the code gen request.

[sushain97]

FYI - this is fixed for us. I recently used it to eliminate well over 7 million log lines of spam 🎉

[jhump]

Yeah, sorry I didn't post here a few months ago. This was fixed in the protoc-gen-buf-lint plugin in bufbuild/buf#1835 and then released in v1.16.0