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.

Sign up for GitHub

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
merged 10 commits into from
Oct 22, 2019
Merged

attributes: support destructuring in arguments #397

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0f88b64
attributes: extract param_names extractor
couchand Oct 22, 2019
ab19e89
attributes: support destructured tuple arguments
couchand Oct 22, 2019
aad290d
attributes: support destructured ref arguments
couchand Oct 22, 2019
880ad02
attributes: support destructured tuple struct arguments
couchand Oct 22, 2019
60f514b
attributes: support destructured struct arguments
couchand Oct 22, 2019
26afae1
attributes: add crazy destructuring test
couchand Oct 22, 2019
37b10a2
attributes: move destructuring tests to their own file
couchand Oct 22, 2019
49a430d
attributes: apply cargo fmt
couchand Oct 22, 2019
1e0ce4c
attributes: add helpful comment to pattern pattern match
couchand Oct 22, 2019
b47e709
attributes: support self argument as span field
couchand Oct 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions tracing-attributes/src/lib.rs
View file
Open in desktop
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, PatType, PatTuple, PatTupleStruct, 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),
_ => 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.

I think we should probably match the FnArg::Receiver case here as well, so that we can start including self in the generated spans. We can add that in a follow-up PR though, it's not a blocker for this branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense! I thought briefly about that but decided to keep this PR focused.

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 went ahead and added it, since it was only a few lines plus test.

})
.filter(|ident| !skips.contains(ident))
.collect();
Expand Down Expand Up @@ -212,6 +211,19 @@ 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)),
_ => 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
195 changes: 195 additions & 0 deletions tracing-attributes/tests/destructuring.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
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();
}