Skip to content

Commit

Permalink
attributes: remove unnecessary quote_spanned! (#1617)
Browse files Browse the repository at this point in the history
## Motivation

Apparently, using `quote_spanned!` can trigger a Clippy bug where the
text `else`, even inside a comment, _may_ cause the
`suspicious_else_formatting` lint to be triggered incorrectly (see
rust-lang/rust-clippy#7760 and rust-lang/rust-clippy#6249). This causes
the lint to fire in some cases when the `#[instrument]` attribute is
used on `async fn`s. See issue #1613 for details.

## Solution

It turns out that some of the uses of `quote_spanned!` in the
`tracing-attributes` code generation are not needed. We really only need
`quote_spanned!` when actually interpolating the user provided code into
a block, not in the `tracing-attributes` code that inserts the generated
code for producing the span etc. Replacing some of these
`quote_spanned!` uses with the normal `quote!` macro still generates
correct location diagnostics for errors in the user code, but fixes the
incorrect clippy lint.

I've added a few test cases that should reproduce the bug.

Fixes #1613

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
  • Loading branch information
hawkw committed Oct 5, 2021
1 parent 243a3e2 commit 7dda7f5
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 3 deletions.
6 changes: 3 additions & 3 deletions tracing-attributes/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,7 @@ fn gen_block(
)
};

return quote_spanned!(block.span()=>
return quote!(
let __tracing_attr_span = #span;
let __tracing_instrument_future = #mk_fut;
if !__tracing_attr_span.is_disabled() {
Expand All @@ -745,7 +745,7 @@ fn gen_block(
);
}

let span = quote_spanned!(block.span()=>
let span = quote!(
// These variables are left uninitialized and initialized only
// if the tracing level is statically enabled at this point.
// While the tracing level is also checked at span creation
Expand Down Expand Up @@ -779,7 +779,7 @@ fn gen_block(
);
}

quote_spanned!(block.span()=>
quote_spanned!(block.span() =>
// Because `quote` produces a stream of tokens _without_ whitespace, the
// `if` and the block will appear directly next to each other. This
// generates a clippy lint about suspicious `if/else` formatting.
Expand Down
22 changes: 22 additions & 0 deletions tracing-attributes/tests/async_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,28 @@ async fn test_ret_impl_trait_err(n: i32) -> Result<impl Iterator<Item = i32>, &'
#[instrument]
async fn test_async_fn_empty() {}

// Reproduces https://github.com/tokio-rs/tracing/issues/1613
#[instrument]
// LOAD-BEARING `#[rustfmt::skip]`! This is necessary to reproduce the bug;
// with the rustfmt-generated formatting, the lint will not be triggered!
#[rustfmt::skip]
#[deny(clippy::suspicious_else_formatting)]
async fn repro_1613(var: bool) {
println!(
"{}",
if var { "true" } else { "false" }
);
}

// Reproduces https://github.com/tokio-rs/tracing/issues/1613
// and https://github.com/rust-lang/rust-clippy/issues/7760
#[instrument]
#[deny(clippy::suspicious_else_formatting)]
async fn repro_1613_2() {
// hello world
// else
}

#[test]
fn async_fn_only_enters_for_polls() {
let (subscriber, handle) = subscriber::mock()
Expand Down

0 comments on commit 7dda7f5

Please sign in to comment.