Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

attributes: support destructuring in arguments #397

Merged
38 changes: 30 additions & 8 deletions tracing-attributes/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,14 @@
extern crate proc_macro;

use std::collections::HashSet;
use std::iter;

use proc_macro::TokenStream;
use quote::{quote, quote_spanned, ToTokens};
use syn::{
spanned::Spanned, AttributeArgs, FnArg, Ident, ItemFn, Lit, LitInt, Meta, MetaList,
MetaNameValue, NestedMeta, Pat, PatIdent, PatType, Signature,
spanned::Spanned, AttributeArgs, FieldPat, FnArg, Ident, ItemFn, Lit, LitInt, Meta, MetaList,
MetaNameValue, NestedMeta, Pat, PatIdent, PatReference, PatStruct, PatTuple, PatTupleStruct,
PatType, Signature,
};

/// Instruments a function to create and enter a `tracing` [span] every time
Expand Down Expand Up @@ -154,12 +156,9 @@ pub fn instrument(args: TokenStream, item: TokenStream) -> TokenStream {
let param_names: Vec<Ident> = params
.clone()
.into_iter()
.filter_map(|param| match param {
FnArg::Typed(PatType { pat, .. }) => match *pat {
Pat::Ident(PatIdent { ident, .. }) => Some(ident),
_ => None,
},
_ => None,
.flat_map(|param| match param {
FnArg::Typed(PatType { pat, .. }) => param_names(*pat),
FnArg::Receiver(_) => Box::new(iter::once(Ident::new("self", param.span()))),
})
.filter(|ident| !skips.contains(ident))
.collect();
Expand Down Expand Up @@ -212,6 +211,29 @@ pub fn instrument(args: TokenStream, item: TokenStream) -> TokenStream {
.into()
}

fn param_names(pat: Pat) -> Box<dyn Iterator<Item = Ident>> {
match pat {
Pat::Ident(PatIdent { ident, .. }) => Box::new(iter::once(ident)),
Pat::Reference(PatReference { pat, .. }) => param_names(*pat),
Pat::Struct(PatStruct { fields, .. }) => Box::new(
fields
.into_iter()
.flat_map(|FieldPat { pat, .. }| param_names(*pat)),
),
Pat::Tuple(PatTuple { elems, .. }) => Box::new(elems.into_iter().flat_map(param_names)),
Pat::TupleStruct(PatTupleStruct {
pat: PatTuple { elems, .. },
..
}) => Box::new(elems.into_iter().flat_map(param_names)),

// The above *should* cover all cases of irrefutable patterns,
// but we purposefully don't do any funny business here
// (such as panicking) because that would obscure rustc's
// much more informative error message.
_ => Box::new(iter::empty()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this case matches, it's probably a syntax error, right? Can any pattern types we don't currently match be used in function params? I think it might be good to have a comment hereif that's true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about changing that to unreachable!() but wasn't sure if my assumptions were valid. I can't seem to find authoritative documentation on the matter, but it does seem like this covers all irrefutable patterns.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I think that if the remaining cases are something that will cause rustc to error anyway, it's probably best for the proc macro not to panic, so that rustc can print the correct diagnostics for the syntax error (rather than a more surprising "proc macro panicked!" message)? But, it might be good to have a comment noting that. Not a blocker though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason I was operating under the assumption that this would be a compiler error before the proc macro was invoked. There is a class of errors that will bail before trying to invoke the proc macro, but clearly it doesn't include this (I double checked just to be sure). I think the exact details would be found in RFC #550, but I really don't have the head space to digest that RFC right now (and it's also not at all useful for the purposes of this PR 😆). A comment does seem useful, if for no other reason than to keep others from going down this rabbit hole.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the reason this would error after the proc-macro is evaluated is because the macro could rewrite the function definition. Thanks for adding the comment!

}
}

fn skips(args: &AttributeArgs) -> Result<HashSet<Ident>, impl ToTokens> {
let mut skips = args.iter().filter_map(|arg| match arg {
NestedMeta::Meta(Meta::List(MetaList {
Expand Down
215 changes: 215 additions & 0 deletions tracing-attributes/tests/destructuring.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
mod support;
use support::*;

use tracing::subscriber::with_default;
use tracing_attributes::instrument;

#[test]
fn destructure_tuples() {
#[instrument]
fn my_fn((arg1, arg2): (usize, usize)) {}

let span = span::mock().named("my_fn");

let (subscriber, handle) = subscriber::mock()
.new_span(
span.clone().with_field(
field::mock("arg1")
.with_value(&format_args!("1"))
.and(field::mock("arg2").with_value(&format_args!("2")))
.only(),
),
)
.enter(span.clone())
.exit(span.clone())
.drop_span(span)
.done()
.run_with_handle();

with_default(subscriber, || {
my_fn((1, 2));
});

handle.assert_finished();
}

#[test]
fn destructure_nested_tuples() {
#[instrument]
fn my_fn(((arg1, arg2), (arg3, arg4)): ((usize, usize), (usize, usize))) {}

let span = span::mock().named("my_fn");

let (subscriber, handle) = subscriber::mock()
.new_span(
span.clone().with_field(
field::mock("arg1")
.with_value(&format_args!("1"))
.and(field::mock("arg2").with_value(&format_args!("2")))
.and(field::mock("arg3").with_value(&format_args!("3")))
.and(field::mock("arg4").with_value(&format_args!("4")))
.only(),
),
)
.enter(span.clone())
.exit(span.clone())
.drop_span(span)
.done()
.run_with_handle();

with_default(subscriber, || {
my_fn(((1, 2), (3, 4)));
});

handle.assert_finished();
}

#[test]
fn destructure_refs() {
#[instrument]
fn my_fn(&arg1: &usize) {}

let span = span::mock().named("my_fn");

let (subscriber, handle) = subscriber::mock()
.new_span(
span.clone()
.with_field(field::mock("arg1").with_value(&format_args!("1")).only()),
)
.enter(span.clone())
.exit(span.clone())
.drop_span(span)
.done()
.run_with_handle();

with_default(subscriber, || {
my_fn(&1);
});

handle.assert_finished();
}

#[test]
fn destructure_tuple_structs() {
struct Foo(usize, usize);

#[instrument]
fn my_fn(Foo(arg1, arg2): Foo) {}

let span = span::mock().named("my_fn");

let (subscriber, handle) = subscriber::mock()
.new_span(
span.clone().with_field(
field::mock("arg1")
.with_value(&format_args!("1"))
.and(field::mock("arg2").with_value(&format_args!("2")))
.only(),
),
)
.enter(span.clone())
.exit(span.clone())
.drop_span(span)
.done()
.run_with_handle();

with_default(subscriber, || {
my_fn(Foo(1, 2));
});

handle.assert_finished();
}

#[test]
fn destructure_structs() {
struct Foo {
bar: usize,
baz: usize,
}

#[instrument]
fn my_fn(
Foo {
bar: arg1,
baz: arg2,
}: Foo,
) {
let _ = (arg1, arg2);
}

let span = span::mock().named("my_fn");

let (subscriber, handle) = subscriber::mock()
.new_span(
span.clone().with_field(
field::mock("arg1")
.with_value(&format_args!("1"))
.and(field::mock("arg2").with_value(&format_args!("2")))
.only(),
),
)
.enter(span.clone())
.exit(span.clone())
.drop_span(span)
.done()
.run_with_handle();

with_default(subscriber, || {
my_fn(Foo { bar: 1, baz: 2 });
});

handle.assert_finished();
}

#[test]
fn destructure_everything() {
struct Foo {
bar: Bar,
baz: (usize, usize),
qux: NoDebug,
}
struct Bar((usize, usize));
struct NoDebug;

#[instrument]
fn my_fn(
&Foo {
bar: Bar((arg1, arg2)),
baz: (arg3, arg4),
..
}: &Foo,
) {
let _ = (arg1, arg2, arg3, arg4);
}

let span = span::mock().named("my_fn");

let (subscriber, handle) = subscriber::mock()
.new_span(
span.clone().with_field(
field::mock("arg1")
.with_value(&format_args!("1"))
.and(field::mock("arg2").with_value(&format_args!("2")))
.and(field::mock("arg3").with_value(&format_args!("3")))
.and(field::mock("arg4").with_value(&format_args!("4")))
.only(),
),
)
.enter(span.clone())
.exit(span.clone())
.drop_span(span)
.done()
.run_with_handle();

with_default(subscriber, || {
let foo = Foo {
bar: Bar((1, 2)),
baz: (3, 4),
qux: NoDebug,
};
let _ = foo.qux; // to eliminate unused field warning
my_fn(&foo);
});

handle.assert_finished();
}
34 changes: 34 additions & 0 deletions tracing-attributes/tests/instrument.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,3 +166,37 @@ fn generics() {

handle.assert_finished();
}

#[test]
fn methods() {
#[derive(Debug)]
struct Foo;

impl Foo {
#[instrument]
fn my_fn(&self, arg1: usize) {}
}

let span = span::mock().named("my_fn");

let (subscriber, handle) = subscriber::mock()
.new_span(
span.clone().with_field(
field::mock("self")
.with_value(&format_args!("Foo"))
.and(field::mock("arg1").with_value(&format_args!("42"))),
),
)
.enter(span.clone())
.exit(span.clone())
.drop_span(span)
.done()
.run_with_handle();

with_default(subscriber, || {
let foo = Foo;
foo.my_fn(42);
});

handle.assert_finished();
}