attributes: support destructuring in arguments #397
Conversation
To enable the recursion needed to extract all the various destructured parameter types.
This is to test the combination of all the supported function argument destructuring types.
couchand
changed the title
There was a problem hiding this comment.
This is great, thank you so much!
It looks like the CI build for this branch is failing because rustfmt needs to be run. Would you mind running cargo fmt and pushing the results? Then I can go ahead and merge this branch. Thanks!
Edit: haha, looks like you ran rustfmt while I was writing this comment. Thanks again!
tracing-attributes/src/lib.rs
Outdated
| _ => None, | ||
| .flat_map(|param| match param { | ||
| FnArg::Typed(PatType { pat, .. }) => param_names(*pat), | ||
| _ => Box::new(iter::empty()), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That makes sense! I thought briefly about that but decided to keep this PR focused.
There was a problem hiding this comment.
I went ahead and added it, since it was only a few lines plus test.
| 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()), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
Since function signatures can only use irrefutable patterns, we can be confident that no legal code will have any other variant of `syn::Pat` than the ones we match on. However, to prevent anyone else from going down the rabbit hole of wondering about it, we add a helpful comment.
|
This looks great to me, so I went ahead and merged it. Thanks for all your work on this @couchand! I'll put together a release so we can get these changes into the next |
Added: - Support for destructuring in arguments to `#[instrument]`ed functions (#397) - Generated field for `self` parameters when `#[instrument]`ing methods (#397) Signed-off-by: Eliza Weisman <[email protected]>
Added: - Support for destructuring in arguments to `#[instrument]`ed functions (#397) - Generated field for `self` parameters when `#[instrument]`ing methods (#397) Signed-off-by: Eliza Weisman <[email protected]>
Added - Support for destructuring in arguments to `#[instrument]`ed functions (#397) - Generated field for `self` parameters when `#[instrument]`ing methods (#397) - Optional `skip` argument to `#[instrument]` for excluding function parametersfrom generated spans (#359) - Added `dispatcher::set_default` and `subscriber::set_default` APIs, which return a drop guard (#388) Fixed - Some minor documentation errors (#356, #370) Signed-off-by: Eliza Weisman <[email protected]>
* tracing: bump attrs and core versions Signed-off-by: Eliza Weisman <[email protected]> * tracing: prepare to release 0.1.10 Added - Support for destructuring in arguments to `#[instrument]`ed functions (#397) - Generated field for `self` parameters when `#[instrument]`ing methods (#397) - Optional `skip` argument to `#[instrument]` for excluding function parametersfrom generated spans (#359) - Added `dispatcher::set_default` and `subscriber::set_default` APIs, which return a drop guard (#388) Fixed - Some minor documentation errors (#356, #370) Signed-off-by: Eliza Weisman <[email protected]>
This pull request adds support to the
#[instrument]macro for destructured arguments.Motivation
The
#[instrument]macro automatically adds function arguments as fields on theSpan. Previously, it would only do so for "plain"Identargument types, but would ignore destructured arguments.Solution
By recursively pattern matching each argument to find all identifiers it creates, we can construct an authoritative list of all input names.