Repository navigation
bug(ci): some warnings are not enforced by features step #4232
Description
in #4231, we noticed that the features step in the CI pipeline did not properly catch some warnings when running cargo hack against various feature combinations.
i tracked that down to a root cause of discrepancy between what warnings we enforce when running cargo test --features full, here:
Also since they can just be dropped immediately without triggering a warning, do
{}_span!macros not have#[must_use]?i was really curious about this as well. i think i figured this out:
first, i confirmed that
Enteredis marked with this attribute: https://docs.rs/tracing/0.1.44/src/tracing/span.rs.html#387and i was also able to confirm that a warning appears when building the errant commit
b236593bwith a nightly toolchain, with the tracing feature active.moreover, i even found that these did generate warnings in CI, here: https://github.com/hyperium/hyper/actions/runs/38023510701/job/114129390066#step:7:217
however, note the specific warnings we deny when running
cargo hack, here:- name: check --feature-powerset with tracing feature run: cargo hack --no-dev-deps check --feature-powerset --depth 2 --features tracing --skip ffi env: RUSTFLAGS: "--cfg hyper_unstable_tracing -D dead_code -D unused_imports"...as compared to the compiler warning:
warning: unused `EnteredSpan` that must be used --> src/proto/h1/role.rs:86:5 | 86 | trace_span!("parse_headers"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: once a span has been entered, it should be exited = note: `#[warn(unused_must_use)]` (part of `#[warn(unused)]`) on by default help: use `let _ = ...` to ignore the resulting value | 86 | let _ = trace_span!("parse_headers");; | +++++++ +we are only denying
dead\_codeandunused\_imports, and this is a separate linting rule.we don't enforce warnings as strictly when running
cargo hack, because we only deny warnings in tests here:// src/lib.rs #![cfg_attr(all(test, feature = "full"), deny(unreachable_pub))] #![cfg_attr(all(test, feature = "full"), deny(warnings))] #![cfg_attr(all(test, feature = "nightly"), feature(test))]https://github.com/cratelyn/hyper/blob/b236593b60195dd25b53fefbb6b3f47b422b2db9/src/lib.rs#L4-L6
i think we can fix this easily, but i wanted to write this up here to (a) explain in-band why we didn't see a CI failure previously, and (b) act as a comment i can spin out into a separate issue.
thank you again for your thoughtful review, @/dswij, i appreciate it very much.
we should bring these into alignment. it can be somewhat abrasive to develop code in a feature that always enforces warnings however, so it would be nice to keep this blanket enforcement of all warnings cordoned off to specific builds in CI, as opposed promoting that to an unpredicated #![deny(warnings)].
Activity