Skip to content

Commit ccdf2e9

Browse files
cratelynRafael-Conde
authored andcommitted
style(proto): enforce clippy:let_unit_value (#4231)
see #4071. this commit addresses the `let_unit_value` clippy lint. only two sites are affected, relating to tracing spans. this commit introduces a `hyper::trace::Disabled` type to act as a ZST marker in places that can sometimes conditionally compile to a `tracing::span::Entered` guard. this is based upon experimenting in <https://github.com/cratelyn/hyper-span-macro-example>. to restate some of those learnings here: ### background hyper contains a `trace` submodule, introduced in #3326, that contains macros like the following: ```rust macro_rules! trace { ($($arg:tt)*) => { #[cfg(feature = "tracing")] { tracing::trace!($($arg)+); } } } macro_rules! trace_span { ($($arg:tt)*) => { { #[cfg(feature = "tracing")] { let _span = tracing::trace_span!($($arg)+); _span.entered() } } } } ``` these thinly wrap `tracing`'s macros, to account for the fact that tracing support in hyper is opt-in. it is gated behind a feature flag, and additionally mandates that the `hyper_unstable_tracing` flag be enabled. however, this leads to violations of clippy's `let_unit_value` rule when compiled without the `tracing` feature flag is enabled. this is because `trace_span!` only returns a `tracing::span::Entered` when that flag is active, otherwise it returns `()`. thus, we *must* bind these calls to a variable, so that we properly hold on to the span guard when the tracing flag is active. however, we must contend with the fact that when this flag is not active, strict clippy linting yields us warnings like this: ```text cargo clippy --no-default-features Checking hyper-span-macro-example v0.1.0 error: this let-binding has unit value --> src/main.rs:52:5 | 52 | let _entered = trace_span!("instrumented"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.99.0/index.html#let_unit_value ``` ### why not use `#[allow(..)]` one might try to selectively apply `#[allow(clippy::let_unit_value)]` attributes to stifle this. however, applying attributes to expressions is still a nightly feature we cannot rely on yet, so call-sites for our macros must be updated rather than modifying the code generated by the macro. see rust-lang/rust#15701. this would require us to do something like this throughout the codebase, wherever we create tracing spans: ```rust #[cfg_attr(not(feature = "tracing"), allow(clippy::let_unit_value))] let _entered = trace_span!("instrumented"); ``` `#[cfg_attr]` is a bit esoteric, and if we require that instrumenting code in hyper require this song and dance every time we create a span, it will generally discourage instrumentation. ### changes this commit introduce a `Disabled` type to replace the empty `()` when tracing instrumentation is disabled. this satisfies clippy's `let_unit_value` rule because macros like `trace_span!` will never return `()` no matter which feature flags are active. this _also_ prevents accidentally writing code that would drop span guards once the tracing flag is enabled, because an inert `#[must_use]` guard acts as a stand-in when the flag isn't present. Signed-off-by: katelyn martin <git@katelyn.world>
1 parent 75d2355 commit ccdf2e9

2 files changed

Lines changed: 41 additions & 1 deletion

File tree

‎Cargo.toml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,6 @@ checked_conversions = "allow"
123123
else_if_without_else = "allow"
124124
float_arithmetic = "allow"
125125
indexing_slicing = "allow"
126-
let_unit_value = "allow"
127126
match_wild_err_arm = "allow"
128127
multiple_inherent_impl = "allow"
129128
multiple_unsafe_ops_per_block = "allow"

‎src/trace.rs‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,27 @@ compile_error!(
1010
"
1111
);
1212

13+
/// A type representing a RAII guard from a disabled tracing span.
14+
///
15+
/// When the `tracing` feature flag is not active we do not enter tracing
16+
/// spans. This type does nothing when dropped, and does not guard any
17+
/// resources.
18+
///
19+
/// This helps avoid expressions that conditionally compile to either `()` or
20+
/// `tracing::span::Entered` depending on which feature flags are enabled.
21+
//
22+
// This ZST (zero sized type) marker only exists when the tracing flag is not
23+
// active.
24+
#[cfg(not(feature = "tracing"))]
25+
// This module allows `unused_macros`. Similarly, we allow dead code in case
26+
// none of the macros exported by this module are used elsewhere.
27+
#[allow(dead_code)]
28+
// This type is marked as a `#[must_use]` type not for its own semantic
29+
// constraints, but to enforce consistency with code paths associated with
30+
// the `tracing` feature flag.
31+
#[must_use = "once a span has been entered, it should be exited"]
32+
pub(crate) struct Disabled;
33+
1334
macro_rules! debug {
1435
($($arg:tt)+) => {
1536
#[cfg(feature = "tracing")]
@@ -27,6 +48,10 @@ macro_rules! debug_span {
2748
let _span = tracing::debug_span!($($arg)+);
2849
_span.entered()
2950
}
51+
#[cfg(not(feature = "tracing"))]
52+
{
53+
crate::trace::Disabled
54+
}
3055
}
3156
}
3257
}
@@ -48,6 +73,10 @@ macro_rules! error_span {
4873
let _span = tracing::error_span!($($arg)+);
4974
_span.entered()
5075
}
76+
#[cfg(not(feature = "tracing"))]
77+
{
78+
crate::trace::Disabled
79+
}
5180
}
5281
}
5382
}
@@ -69,6 +98,10 @@ macro_rules! info_span {
6998
let _span = tracing::info_span!($($arg)+);
7099
_span.entered()
71100
}
101+
#[cfg(not(feature = "tracing"))]
102+
{
103+
crate::trace::Disabled
104+
}
72105
}
73106
}
74107
}
@@ -90,6 +123,10 @@ macro_rules! trace_span {
90123
let _span = tracing::trace_span!($($arg)+);
91124
_span.entered()
92125
}
126+
#[cfg(not(feature = "tracing"))]
127+
{
128+
crate::trace::Disabled
129+
}
93130
}
94131
}
95132
}
@@ -123,6 +160,10 @@ macro_rules! warn_span {
123160
let _span = tracing::warn_span!($($arg)+);
124161
_span.entered()
125162
}
163+
#[cfg(not(feature = "tracing"))]
164+
{
165+
crate::trace::Disabled
166+
}
126167
}
127168
}
128169
}

0 commit comments

Comments
 (0)