style(proto): enforce clippy:let_unit_value

[cratelyn]

see #4071.

see https://rust-lang.github.io/rust-clippy/master/index.html#let_unit_value.

this commit addresses the let_unit_value clippy lint. only two sites
are affected, relating to tracing spans.

[dswij]

trace_span returns a guard when tracing features is enabled. So the binding here actually serves a purpose.

[cratelyn]

yikes, thank you for spotting that @dswij. faf0de4 reverts this change to the trace_span! call-sites.

this is tricky because clippy warns about let_unit_value if the tracing flag isn't active, but as you point out, we must bind a variable here because a RAII guard is returned when the flag is active.

there's a "simple" solution here, wherein we add something like this to the assorted call-sites:

+#[cfg_attr(not(feature = "tracing"), allow(clippy::let_unit_value))]
 let _entered = trace_span!("instrumented");

i don't love how this introduces friction to adding telemetry though, it strikes me as something that would become a recurring papercut in CI, and let_unit_value seems like a worthy lint to enforce in general.

i thought about this for a bit today, and spun up a little playground to explore some possible solutions: https://github.com/cratelyn/hyper-span-macro-example.

that led me to the idea of introducing a type to stand-in as a disabled guard when the tracing flag isn't active. how does 804e64e look to you?

there's more information in the commit message there, but the nice part about this approach is that it avoids the need to decorate call-sites for these macros. what do you think?

[seanmonstar]

Would a new unit struct make the lint happy?

[cratelyn]

yes, it does! that's the core of what 804e64e is doing.

[dswij]

I think unit struct is the way to go, that's my first thought too. Thanks for exploring that!

Just one nit on the commit message though: I guess we're not introducing a mock guard, but a ZST marker as an alternative to the unit type (since we're not really mocking any of the guard functionality).

[dswij]

Also since they can just be dropped immediately without triggering a warning, do {}_span! macros not have #[must_use]?

[cratelyn]

Just one nit on the commit message though: I guess we're not introducing a mock guard, but a ZST marker as an alternative to the unit type (since we're not really mocking any of the guard functionality).

that's a good point, thank you. i polished the doc comment for this type to refer to it as a ZST marker rather than a guard in 7149e65.

[cratelyn]

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 Entered is marked with this attribute: https://docs.rs/tracing/0.1.44/src/tracing/span.rs.html#387

and i was also able to confirm that a warning appears when building the errant commit b236593b with 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_code and unused_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.

[cratelyn]

filed #4232 to track the conversation about enforcing warnings like this in the future. 🔖