[Feature Request] Make Clippy aspect apply to transitive dependencies

[blorente]

Currently, the clippy aspect doesn't traverse through deps edges. Therefore, if you build a rust_binary that depends on a rust_library and the rust_library has clippy issues, these won't be reported unless you also build the library.

To reproduce:

1. Create this target in test/clippy/BUILD.bazel:

rust_binary(
    name = "bad_transitive_dep",
    srcs = ["src/main.rs"],
    edition = "2018",
    deps = [":bad_library"],
)

2. Then, remove the noclippy tag from :bad_library.
3. Run clippy:

• bazel build --config=clippy test/clippy:bad_transitive_dep -> Succeeds
• bazel build --config=clippy test/clippy:bad_library -> Fails
• bazel build --config=clippy test/clippy:bad_library test/clippy:bad_transitive_dep -> Fails

I would have expected building :bad_transitive_dep to fail, since one of their dependencies fails.

I'm happy to implement the traversal functionality, but I wanted to check first before going deep into it: Is there any context I should be aware of? Is this against the philosophy of the the aspect?

I wasn't able to find anything in the Bazel slack or in the open issues, sorry if I missed anything.

[illicitonion]

I think in general lint-style aspects are expected to only lint the targets they're passed? If I asked rustfmt whether my library was formatted and it told me one of my dependencies was poorly formatted, I'd be surprised...

How is this being called at the moment? If e.g. the result of a query, is it possible to add a deps() call around the query?

IIRC the clippy aspect is special-cased so that if it's called on a non-rust target it will just no-op, so you don't need to worry about filtering like kind(rust_library|rust_binary|..., deps(...)) - you should be able to just deps() around your targets.

[blorente]

If I asked rustfmt whether my library was formatted and it told me one of my dependencies was poorly formatted, I'd be surprised...

True. I think the main disconnect is that a clippy failure can make the bazel build fail, so it's more akin to a static check by the compiler (like asan), but I gues the same could be said for a very strict rustfmt check?

As an alternative, would you take a refactor that exposes the "run clippy on this target" action in the public API, so that other rulesets that do traverse dependencies (like rules_lint) can consume it?

[blackliner]

clippy should not traverse the upstream deps by itself, agreed, but the bazel aspect should make sure it runs on each of the (changed) targets

[illicitonion]

would you take a refactor that exposes the "run clippy on this target" action in the public API, so that other rulesets that do traverse dependencies (like rules_lint) can consume it?

Absolutely!

[blorente]

Absolutely!

Thanks! I'll get to that.

In the meantime, I've opened https://github.com/bazelbuild/rules_rust/pull/3612/files to show how an optional flag to toggle this behaviour could look like, without changing the default behaviour. I understand if this is not the direction rules_rust wants to go in, but I figured I might as well try and see if the change would be a welcome one. No rush on looking at it though!

[blackliner]

To make the case for viewing it as a static linter: our expectations is that clippy has to pass, if it doesn't, it is considered just as bad as a compilation error. It's a really bad user experience if they iterate on code and have green builds locally, but CI fails later because//... checks more.

Maybe a toggle to enable this strict mode that we can flip like suggested in the PR above makes most sense.