From baae90b543ccc87f11ac5fc0efe6f2b3b0ff9a6f Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 8 Aug 2022 15:04:08 -0700 Subject: [PATCH 1/6] Add fake return to improve span on type error in tracing::instrument --- tracing-attributes/src/expand.rs | 91 +++++++++++++++++++++++++------- 1 file changed, 71 insertions(+), 20 deletions(-) diff --git a/tracing-attributes/src/expand.rs b/tracing-attributes/src/expand.rs index d4ef29fc7a..ee718d3720 100644 --- a/tracing-attributes/src/expand.rs +++ b/tracing-attributes/src/expand.rs @@ -2,10 +2,11 @@ use std::iter; use proc_macro2::TokenStream; use quote::{quote, quote_spanned, ToTokens}; +use syn::visit_mut::VisitMut; use syn::{ punctuated::Punctuated, spanned::Spanned, Block, Expr, ExprAsync, ExprCall, FieldPat, FnArg, Ident, Item, ItemFn, Pat, PatIdent, PatReference, PatStruct, PatTuple, PatTupleStruct, PatType, - Path, Signature, Stmt, Token, TypePath, + Path, ReturnType, Signature, Stmt, Token, Type, TypePath, }; use crate::{ @@ -18,7 +19,7 @@ pub(crate) fn gen_function<'a, B: ToTokens + 'a>( input: MaybeItemFnRef<'a, B>, args: InstrumentArgs, instrumented_function_name: &str, - self_type: Option<&syn::TypePath>, + self_type: Option<&TypePath>, ) -> proc_macro2::TokenStream { // these are needed ahead of time, as ItemFn contains the function body _and_ // isn't representable inside a quote!/quote_spanned! macro @@ -31,7 +32,7 @@ pub(crate) fn gen_function<'a, B: ToTokens + 'a>( } = input; let Signature { - output: return_type, + output, inputs: params, unsafety, asyncness, @@ -49,8 +50,37 @@ pub(crate) fn gen_function<'a, B: ToTokens + 'a>( let warnings = args.warnings(); + let block = if let ReturnType::Type(_, return_type) = &output { + let return_type = erase_impl_trait(return_type); + // Install a fake return statement as the first thing in the function + // body, so that we eagerly infer that the return type is what we + // declared in the async fn signature. + // The `#[allow(unreachable_code)]` is given because the return + // statement is unreachable, but does affect inference. + let fake_return_edge = quote_spanned! {return_type.span()=> + #[allow(unreachable_code)] + if false { + let __tracing_attr_fake_return: #return_type = + unreachable!("this is just for type inference, and is unreachable code"); + return __tracing_attr_fake_return; + } + }; + quote! { + { + #fake_return_edge + #block + } + } + } else { + quote! { + { + let _: () = #block; + } + } + }; + let body = gen_block( - block, + &block, params, asyncness.is_some(), args, @@ -60,7 +90,7 @@ pub(crate) fn gen_function<'a, B: ToTokens + 'a>( quote!( #(#attrs) * - #vis #constness #unsafety #asyncness #abi fn #ident<#gen_params>(#params) #return_type + #vis #constness #unsafety #asyncness #abi fn #ident<#gen_params>(#params) #output #where_clause { #warnings @@ -76,7 +106,7 @@ fn gen_block( async_context: bool, mut args: InstrumentArgs, instrumented_function_name: &str, - self_type: Option<&syn::TypePath>, + self_type: Option<&TypePath>, ) -> proc_macro2::TokenStream { // generate the span's name let span_name = args @@ -393,11 +423,11 @@ impl RecordType { "Wrapping", ]; - /// Parse `RecordType` from [syn::Type] by looking up + /// Parse `RecordType` from [Type] by looking up /// the [RecordType::TYPES_FOR_VALUE] array. - fn parse_from_ty(ty: &syn::Type) -> Self { + fn parse_from_ty(ty: &Type) -> Self { match ty { - syn::Type::Path(syn::TypePath { path, .. }) + Type::Path(TypePath { path, .. }) if path .segments .iter() @@ -410,9 +440,7 @@ impl RecordType { { RecordType::Value } - syn::Type::Reference(syn::TypeReference { elem, .. }) => { - RecordType::parse_from_ty(&*elem) - } + Type::Reference(syn::TypeReference { elem, .. }) => RecordType::parse_from_ty(&*elem), _ => RecordType::Debug, } } @@ -471,7 +499,7 @@ pub(crate) struct AsyncInfo<'block> { // statement that must be patched source_stmt: &'block Stmt, kind: AsyncKind<'block>, - self_type: Option, + self_type: Option, input: &'block ItemFn, } @@ -606,11 +634,11 @@ impl<'block> AsyncInfo<'block> { if ident == "_self" { let mut ty = *ty.ty.clone(); // extract the inner type if the argument is "&self" or "&mut self" - if let syn::Type::Reference(syn::TypeReference { elem, .. }) = ty { + if let Type::Reference(syn::TypeReference { elem, .. }) = ty { ty = *elem; } - if let syn::Type::Path(tp) = ty { + if let Type::Path(tp) = ty { self_type = Some(tp); break; } @@ -722,7 +750,7 @@ struct IdentAndTypesRenamer<'a> { idents: Vec<(Ident, Ident)>, } -impl<'a> syn::visit_mut::VisitMut for IdentAndTypesRenamer<'a> { +impl<'a> VisitMut for IdentAndTypesRenamer<'a> { // we deliberately compare strings because we want to ignore the spans // If we apply clippy's lint, the behavior changes #[allow(clippy::cmp_owned)] @@ -734,11 +762,11 @@ impl<'a> syn::visit_mut::VisitMut for IdentAndTypesRenamer<'a> { } } - fn visit_type_mut(&mut self, ty: &mut syn::Type) { + fn visit_type_mut(&mut self, ty: &mut Type) { for (type_name, new_type) in &self.types { - if let syn::Type::Path(TypePath { path, .. }) = ty { + if let Type::Path(TypePath { path, .. }) = ty { if path_to_string(path) == *type_name { - *ty = syn::Type::Path(new_type.clone()); + *ty = Type::Path(new_type.clone()); } } } @@ -751,10 +779,33 @@ struct AsyncTraitBlockReplacer<'a> { patched_block: Block, } -impl<'a> syn::visit_mut::VisitMut for AsyncTraitBlockReplacer<'a> { +impl<'a> VisitMut for AsyncTraitBlockReplacer<'a> { fn visit_block_mut(&mut self, i: &mut Block) { if i == self.block { *i = self.patched_block.clone(); } } } + +// Replaces any `impl Trait` with `_` so it can be used as the type in +// a `let` statement's LHS. +struct ImplTraitEraser; + +impl VisitMut for ImplTraitEraser { + fn visit_type_mut(&mut self, t: &mut Type) { + if let Type::ImplTrait(..) = t { + *t = syn::TypeInfer { + underscore_token: Token![_](t.span()), + } + .into(); + } else { + syn::visit_mut::visit_type_mut(self, t); + } + } +} + +fn erase_impl_trait(ty: &Type) -> Type { + let mut ty = ty.clone(); + ImplTraitEraser.visit_type_mut(&mut ty); + ty +} From 9aea0e5d76c783bd87ec894a5cba5fdfe06d7df1 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 8 Aug 2022 15:39:14 -0700 Subject: [PATCH 2/6] Add trybuild test for instrumented async fn type mismatch --- tracing-attributes/Cargo.toml | 2 + tracing-attributes/tests/ui.rs | 7 +++ .../tests/ui/async_instrument.rs | 31 ++++++++++ .../tests/ui/async_instrument.stderr | 56 +++++++++++++++++++ 4 files changed, 96 insertions(+) create mode 100644 tracing-attributes/tests/ui.rs create mode 100644 tracing-attributes/tests/ui/async_instrument.rs create mode 100644 tracing-attributes/tests/ui/async_instrument.stderr diff --git a/tracing-attributes/Cargo.toml b/tracing-attributes/Cargo.toml index b93ddec1ed..16d0e903e9 100644 --- a/tracing-attributes/Cargo.toml +++ b/tracing-attributes/Cargo.toml @@ -45,6 +45,8 @@ tokio-test = "0.4.2" tracing-core = { path = "../tracing-core", version = "0.2"} tracing-subscriber = { path = "../tracing-subscriber", version = "0.3", features = ["env-filter"] } async-trait = "0.1.56" +trybuild = "1.0.64" +rustversion = "1.0.9" [badges] maintenance = { status = "experimental" } diff --git a/tracing-attributes/tests/ui.rs b/tracing-attributes/tests/ui.rs new file mode 100644 index 0000000000..72858033f0 --- /dev/null +++ b/tracing-attributes/tests/ui.rs @@ -0,0 +1,7 @@ +// Only test on nightly, since UI tests are bound to change over time +#[rustversion::nightly] +#[test] +fn async_instrument() { + let t = trybuild::TestCases::new(); + t.compile_fail("tests/ui/async_instrument.rs"); +} diff --git a/tracing-attributes/tests/ui/async_instrument.rs b/tracing-attributes/tests/ui/async_instrument.rs new file mode 100644 index 0000000000..66581c66b7 --- /dev/null +++ b/tracing-attributes/tests/ui/async_instrument.rs @@ -0,0 +1,31 @@ +#![allow(unreachable_code)] + +#[tracing::instrument] +async fn unit() { + "" +} + +#[tracing::instrument] +async fn simple_mismatch() -> String { + "" +} + +// FIXME: this span is still pretty poor +#[tracing::instrument] +async fn opaque_unsatisfied() -> impl std::fmt::Display { + ("",) +} + +struct Wrapper(T); + +#[tracing::instrument] +async fn mismatch_with_opaque() -> Wrapper { + "" +} + +fn main() { + let _ = unit(); + let _ = simple_mismatch(); + let _ = opaque_unsatisfied(); + let _ = mismatch_with_opaque(); +} diff --git a/tracing-attributes/tests/ui/async_instrument.stderr b/tracing-attributes/tests/ui/async_instrument.stderr new file mode 100644 index 0000000000..967a3174ea --- /dev/null +++ b/tracing-attributes/tests/ui/async_instrument.stderr @@ -0,0 +1,56 @@ +error[E0308]: mismatched types + --> tests/ui/async_instrument.rs:5:5 + | +5 | "" + | ^^ expected `()`, found `&str` + +error[E0308]: mismatched types + --> tests/ui/async_instrument.rs:10:5 + | +10 | "" + | ^^- help: try using a conversion method: `.to_string()` + | | + | expected struct `String`, found `&str` + | +note: return type inferred to be `String` here + --> tests/ui/async_instrument.rs:9:31 + | +9 | async fn simple_mismatch() -> String { + | ^^^^^^ + +error[E0277]: `(&str,)` doesn't implement `std::fmt::Display` + --> tests/ui/async_instrument.rs:14:1 + | +14 | #[tracing::instrument] + | ^^^^^^^^^^^^^^^^^^^^^^ `(&str,)` cannot be formatted with the default formatter + | + = help: the trait `std::fmt::Display` is not implemented for `(&str,)` + = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead + = note: this error originates in the attribute macro `tracing::instrument` (in Nightly builds, run with -Z macro-backtrace for more info) + +error[E0277]: `(&str,)` doesn't implement `std::fmt::Display` + --> tests/ui/async_instrument.rs:15:34 + | +15 | async fn opaque_unsatisfied() -> impl std::fmt::Display { + | ^^^^^^^^^^^^^^^^^^^^^^ `(&str,)` cannot be formatted with the default formatter + | + = help: the trait `std::fmt::Display` is not implemented for `(&str,)` + = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead + +error[E0308]: mismatched types + --> tests/ui/async_instrument.rs:23:5 + | +23 | "" + | ^^ expected struct `Wrapper`, found `&str` + | + = note: expected struct `Wrapper<_>` + found reference `&'static str` +note: return type inferred to be `Wrapper<_>` here + --> tests/ui/async_instrument.rs:22:36 + | +22 | async fn mismatch_with_opaque() -> Wrapper { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: try wrapping the expression in `Wrapper` + | +23 | Wrapper("") + | ++++++++ + From 33354fdd06cc9a17659533270a9632cbfda16d57 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 8 Aug 2022 16:42:32 -0700 Subject: [PATCH 3/6] Suppress clippy divergence warning --- tracing-attributes/src/expand.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tracing-attributes/src/expand.rs b/tracing-attributes/src/expand.rs index ee718d3720..c3c42d352f 100644 --- a/tracing-attributes/src/expand.rs +++ b/tracing-attributes/src/expand.rs @@ -55,10 +55,11 @@ pub(crate) fn gen_function<'a, B: ToTokens + 'a>( // Install a fake return statement as the first thing in the function // body, so that we eagerly infer that the return type is what we // declared in the async fn signature. - // The `#[allow(unreachable_code)]` is given because the return - // statement is unreachable, but does affect inference. + // The `#[allow(..)]` is given because the return statement is + // unreachable, but does affect inference, so it needs to be written + // exactly that way for it to do its magic. let fake_return_edge = quote_spanned! {return_type.span()=> - #[allow(unreachable_code)] + #[allow(unreachable_code, clippy::diverging_sub_expression)] if false { let __tracing_attr_fake_return: #return_type = unreachable!("this is just for type inference, and is unreachable code"); From 6ce67146e5a9bb42f860afcc9d40ba97fd55021f Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 8 Aug 2022 17:08:09 -0700 Subject: [PATCH 4/6] Improve error for async fn implicitly returning unit --- tracing-attributes/src/expand.rs | 49 +++++++++---------- .../tests/ui/async_instrument.rs | 25 ++++++++-- .../tests/ui/async_instrument.stderr | 42 ++++++++++++++++ 3 files changed, 85 insertions(+), 31 deletions(-) diff --git a/tracing-attributes/src/expand.rs b/tracing-attributes/src/expand.rs index c3c42d352f..5a93f452fb 100644 --- a/tracing-attributes/src/expand.rs +++ b/tracing-attributes/src/expand.rs @@ -50,33 +50,30 @@ pub(crate) fn gen_function<'a, B: ToTokens + 'a>( let warnings = args.warnings(); - let block = if let ReturnType::Type(_, return_type) = &output { - let return_type = erase_impl_trait(return_type); - // Install a fake return statement as the first thing in the function - // body, so that we eagerly infer that the return type is what we - // declared in the async fn signature. - // The `#[allow(..)]` is given because the return statement is - // unreachable, but does affect inference, so it needs to be written - // exactly that way for it to do its magic. - let fake_return_edge = quote_spanned! {return_type.span()=> - #[allow(unreachable_code, clippy::diverging_sub_expression)] - if false { - let __tracing_attr_fake_return: #return_type = - unreachable!("this is just for type inference, and is unreachable code"); - return __tracing_attr_fake_return; - } - }; - quote! { - { - #fake_return_edge - #block - } - } + let (return_type, return_span) = if let ReturnType::Type(_, return_type) = &output { + (erase_impl_trait(return_type), return_type.span()) } else { - quote! { - { - let _: () = #block; - } + // Point at function name if we don't have an explicit return type + (syn::parse_quote! { () }, ident.span()) + }; + // Install a fake return statement as the first thing in the function + // body, so that we eagerly infer that the return type is what we + // declared in the async fn signature. + // The `#[allow(..)]` is given because the return statement is + // unreachable, but does affect inference, so it needs to be written + // exactly that way for it to do its magic. + let fake_return_edge = quote_spanned! {return_span=> + #[allow(unreachable_code, clippy::diverging_sub_expression)] + if false { + let __tracing_attr_fake_return: #return_type = + unreachable!("this is just for type inference, and is unreachable code"); + return __tracing_attr_fake_return; + } + }; + let block = quote! { + { + #fake_return_edge + #block } }; diff --git a/tracing-attributes/tests/ui/async_instrument.rs b/tracing-attributes/tests/ui/async_instrument.rs index 66581c66b7..5b245746a6 100644 --- a/tracing-attributes/tests/ui/async_instrument.rs +++ b/tracing-attributes/tests/ui/async_instrument.rs @@ -23,9 +23,24 @@ async fn mismatch_with_opaque() -> Wrapper { "" } -fn main() { - let _ = unit(); - let _ = simple_mismatch(); - let _ = opaque_unsatisfied(); - let _ = mismatch_with_opaque(); +#[tracing::instrument] +async fn early_return_unit() { + if true { + return ""; + } +} + +#[tracing::instrument] +async fn early_return() -> String { + if true { + return ""; + } + String::new() } + +#[tracing::instrument] +async fn extra_semicolon() -> i32 { + 1; +} + +fn main() {} diff --git a/tracing-attributes/tests/ui/async_instrument.stderr b/tracing-attributes/tests/ui/async_instrument.stderr index 967a3174ea..7916356518 100644 --- a/tracing-attributes/tests/ui/async_instrument.stderr +++ b/tracing-attributes/tests/ui/async_instrument.stderr @@ -3,6 +3,12 @@ error[E0308]: mismatched types | 5 | "" | ^^ expected `()`, found `&str` + | +note: return type inferred to be `()` here + --> tests/ui/async_instrument.rs:4:10 + | +4 | async fn unit() { + | ^^^^ error[E0308]: mismatched types --> tests/ui/async_instrument.rs:10:5 @@ -54,3 +60,39 @@ help: try wrapping the expression in `Wrapper` | 23 | Wrapper("") | ++++++++ + + +error[E0308]: mismatched types + --> tests/ui/async_instrument.rs:29:16 + | +29 | return ""; + | ^^ expected `()`, found `&str` + | +note: return type inferred to be `()` here + --> tests/ui/async_instrument.rs:27:10 + | +27 | async fn early_return_unit() { + | ^^^^^^^^^^^^^^^^^ + +error[E0308]: mismatched types + --> tests/ui/async_instrument.rs:36:16 + | +36 | return ""; + | ^^- help: try using a conversion method: `.to_string()` + | | + | expected struct `String`, found `&str` + | +note: return type inferred to be `String` here + --> tests/ui/async_instrument.rs:34:28 + | +34 | async fn early_return() -> String { + | ^^^^^^ + +error[E0308]: mismatched types + --> tests/ui/async_instrument.rs:42:35 + | +42 | async fn extra_semicolon() -> i32 { + | ___________________________________^ +43 | | 1; + | | - help: remove this semicolon +44 | | } + | |_^ expected `i32`, found `()` From 4d833c666e2ada148dc1a2d7eb07ab1693516175 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 9 Aug 2022 13:11:09 -0700 Subject: [PATCH 5/6] Suppress more clippy --- tracing-attributes/src/expand.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tracing-attributes/src/expand.rs b/tracing-attributes/src/expand.rs index 5a93f452fb..1e199efa06 100644 --- a/tracing-attributes/src/expand.rs +++ b/tracing-attributes/src/expand.rs @@ -63,7 +63,7 @@ pub(crate) fn gen_function<'a, B: ToTokens + 'a>( // unreachable, but does affect inference, so it needs to be written // exactly that way for it to do its magic. let fake_return_edge = quote_spanned! {return_span=> - #[allow(unreachable_code, clippy::diverging_sub_expression)] + #[allow(unreachable_code, clippy::diverging_sub_expression, clippy::let_unit_value)] if false { let __tracing_attr_fake_return: #return_type = unreachable!("this is just for type inference, and is unreachable code"); From 1e64ae08c0d9b0b649dac3ae0dfc493f6d607347 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sun, 14 Aug 2022 17:43:00 -0700 Subject: [PATCH 6/6] Pin trybuild test to stable instead --- tracing-attributes/tests/ui.rs | 2 +- tracing-attributes/tests/ui/async_instrument.stderr | 11 +---------- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/tracing-attributes/tests/ui.rs b/tracing-attributes/tests/ui.rs index 72858033f0..f11cc019eb 100644 --- a/tracing-attributes/tests/ui.rs +++ b/tracing-attributes/tests/ui.rs @@ -1,5 +1,5 @@ // Only test on nightly, since UI tests are bound to change over time -#[rustversion::nightly] +#[rustversion::stable] #[test] fn async_instrument() { let t = trybuild::TestCases::new(); diff --git a/tracing-attributes/tests/ui/async_instrument.stderr b/tracing-attributes/tests/ui/async_instrument.stderr index 7916356518..5214f92a7e 100644 --- a/tracing-attributes/tests/ui/async_instrument.stderr +++ b/tracing-attributes/tests/ui/async_instrument.stderr @@ -34,15 +34,6 @@ error[E0277]: `(&str,)` doesn't implement `std::fmt::Display` = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead = note: this error originates in the attribute macro `tracing::instrument` (in Nightly builds, run with -Z macro-backtrace for more info) -error[E0277]: `(&str,)` doesn't implement `std::fmt::Display` - --> tests/ui/async_instrument.rs:15:34 - | -15 | async fn opaque_unsatisfied() -> impl std::fmt::Display { - | ^^^^^^^^^^^^^^^^^^^^^^ `(&str,)` cannot be formatted with the default formatter - | - = help: the trait `std::fmt::Display` is not implemented for `(&str,)` - = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead - error[E0308]: mismatched types --> tests/ui/async_instrument.rs:23:5 | @@ -55,7 +46,7 @@ note: return type inferred to be `Wrapper<_>` here --> tests/ui/async_instrument.rs:22:36 | 22 | async fn mismatch_with_opaque() -> Wrapper { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^ help: try wrapping the expression in `Wrapper` | 23 | Wrapper("")