-
Notifications
You must be signed in to change notification settings - Fork 726
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: fix handling of inner attributes #2307
Conversation
I updated the code to parse out the inner attributes fron the function body first and that wasn't too bad. I couldn't keep the `From<&'a ItemFn>` impl for `MaybeItemFnRef` due to lifetime issues though, so now it's `impl TryFrom<ItemFn> for MaybeItemFn`. Fixes: tokio-rs#2294
Edit: No it doesn't. I think this PR is good to go? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this definitely looks like it works!
My one suggested change is that it would be nice if we could do the ItemFn
to MaybeItemFn
conversion without having to re-parse the entire thing --- I left a comment on how we might be able to do that. If that approach isn't viable, though, I'm happy to move forward with the current implementation.
Hi @e-nomem, I'd like to be able to get this branch merged soon so we can publish a release with the fix. Are you able to make the changes I suggested? If you aren't, I'm happy to make them myself and merge the PR, but I don't want to step on your toes here. Thanks again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks for that. I was going to get to it this weekend but I don't want to hold up the release 😅 |
No worries, it was a pretty straightforward change. Thanks again for the fix! |
## Motivation When the `instrument` attribute is used on a function with inner attributes, the proc macro generates code above the attributes within the function block that causes compilation errors. These should be parsed out separately and handled. Fixes #2294 ## Solution I updated `MaybeItemFn` and `MaybeItemFnRef` to so they hold both the outer and inner attributes for the instrumented function and updated the codegen to inlcude them in the appropriate locations. I couldn't preserve the existing implementation of `From<&'_ ItemFn> for MaybeItemFnRef<'_, Box<Block>>`, because it is now necessary to separate the inner and outer attributes of the `ItemFn` into two separate `Vec`s. That implementation was replaced with a `From<ItemFn> for MaybeItemFn`, which uses `Iterator::partition` to separate out the inner and outer attributes. Co-authored-by: Eliza Weisman <eliza@buoyant.io>
# 0.1.23 (October 6, 2022) This release of `tracing-attributes` fixes a bug where compiler diagnostic spans for type errorsin `#[instrument]`ed `async fn`s have the location of the `#[instrument]` attribute rather than the location of the actual error, and a bug where inner attributes in `#[instrument]`ed functions would cause a compiler error. ### Fixed - Fix incorrect handling of inner attributes in `#[instrument]`ed functions ([#2307]) - Add fake return to improve spans generated for type errors in `async fn`s ([#2270]) - Updated `syn` dependency to fix compilation with `-Z minimal-versions` ([#2246]) Thanks to new contributors @compiler-errors and @e-nomem, as well as @CAD97, for contributing to this release! [#2307]: #2307 [#2270]: #2270 [#2246]: #2246
# 0.1.23 (October 6, 2022) This release of `tracing-attributes` fixes a bug where compiler diagnostic spans for type errors in `#[instrument]`ed `async fn`s have the location of the `#[instrument]` attribute rather than the location of the actual error, and a bug where inner attributes in `#[instrument]`ed functions would cause a compiler error. ### Fixed - Fix incorrect handling of inner attributes in `#[instrument]`ed functions ([#2307]) - Add fake return to improve spans generated for type errors in `async fn`s ([#2270]) - Updated `syn` dependency to fix compilation with `-Z minimal-versions` ([#2246]) Thanks to new contributors @compiler-errors and @e-nomem, as well as @CAD97, for contributing to this release! [#2307]: #2307 [#2270]: #2270 [#2246]: #2246
# 0.1.37 (October 6, 2022) This release of `tracing` incorporates changes from `tracing-core` [v0.1.30][core-0.1.30] and `tracing-attributes` [v0.1.23][0.1.23], including the new `Subscriber::on_register_dispatch` method for performing late initialization after a `Subscriber` is registered as a `Dispatch`, and bugfixes for the `#[instrument]` attribute. Additionally, it fixes instances of the `bare_trait_objects` lint, which is now a warning on `tracing`'s MSRV and will become an error in the next edition. ### Fixed - **attributes**: Incorrect handling of inner attributes in `#[instrument]`ed functions (#2307) - **attributes**: Incorrect location of compiler diagnostic spans generated for type errors in `#[instrument]`ed `async fn`s (#2270) - **attributes**: Updated `syn` dependency to fix compilation with `-Z minimal-versions` (#2246) - `bare_trait_objects` warning in `valueset!` macro expansion (#2308) ### Added - **core**: `Subscriber::on_register_dispatch` method (#2269) - **core**: `WeakDispatch` type and `Dispatch::downgrade()` function (#2293) ### Changed - `tracing-core`: updated to [0.1.30][core-0.1.30] - `tracing-attributes`: updated to [0.1.23][attrs-0.1.23] ### Documented - Added [`tracing-web`] and [`reqwest-tracing`] to related crates (#2283], [#2331) Thanks to new contributors @compiler-errors, @e-nomem, @WorldSEnder, @Xiami2012, and @tl-rodrigo-gryzinski, as well as @jswrenn and @CAD97, for contributing to this release! [core-0.1.30]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.30 [attrs-0.1.23]: https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.23 [`tracing-web`]: https://crates.io/crates/tracing-web/ [`reqwest-tracing`]: https://crates.io/crates/reqwest-tracing/
# 0.1.37 (October 6, 2022) This release of `tracing` incorporates changes from `tracing-core` [v0.1.30][core-0.1.30] and `tracing-attributes` [v0.1.23][attrs-0.1.23], including the new `Subscriber::on_register_dispatch` method for performing late initialization after a `Subscriber` is registered as a `Dispatch`, and bugfixes for the `#[instrument]` attribute. Additionally, it fixes instances of the `bare_trait_objects` lint, which is now a warning on `tracing`'s MSRV and will become an error in the next edition. ### Fixed - **attributes**: Incorrect handling of inner attributes in `#[instrument]`ed functions (#2307) - **attributes**: Incorrect location of compiler diagnostic spans generated for type errors in `#[instrument]`ed `async fn`s (#2270) - **attributes**: Updated `syn` dependency to fix compilation with `-Z minimal-versions` (#2246) - `bare_trait_objects` warning in `valueset!` macro expansion (#2308) ### Added - **core**: `Subscriber::on_register_dispatch` method (#2269) - **core**: `WeakDispatch` type and `Dispatch::downgrade()` function (#2293) ### Changed - `tracing-core`: updated to [0.1.30][core-0.1.30] - `tracing-attributes`: updated to [0.1.23][attrs-0.1.23] ### Documented - Added [`tracing-web`] and [`reqwest-tracing`] to related crates (#2283, #2331) Thanks to new contributors @compiler-errors, @e-nomem, @WorldSEnder, @Xiami2012, and @tl-rodrigo-gryzinski, as well as @jswrenn and @CAD97, for contributing to this release! [core-0.1.30]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.30 [attrs-0.1.23]: https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.23 [`tracing-web`]: https://crates.io/crates/tracing-web/ [`reqwest-tracing`]: https://crates.io/crates/reqwest-tracing/
# 0.1.23 (October 6, 2022) This release of `tracing-attributes` fixes a bug where compiler diagnostic spans for type errors in `#[instrument]`ed `async fn`s have the location of the `#[instrument]` attribute rather than the location of the actual error, and a bug where inner attributes in `#[instrument]`ed functions would cause a compiler error. ### Fixed - Fix incorrect handling of inner attributes in `#[instrument]`ed functions ([tokio-rs#2307]) - Add fake return to improve spans generated for type errors in `async fn`s ([tokio-rs#2270]) - Updated `syn` dependency to fix compilation with `-Z minimal-versions` ([tokio-rs#2246]) Thanks to new contributors @compiler-errors and @e-nomem, as well as @CAD97, for contributing to this release! [tokio-rs#2307]: tokio-rs#2307 [tokio-rs#2270]: tokio-rs#2270 [tokio-rs#2246]: tokio-rs#2246
# 0.1.37 (October 6, 2022) This release of `tracing` incorporates changes from `tracing-core` [v0.1.30][core-0.1.30] and `tracing-attributes` [v0.1.23][attrs-0.1.23], including the new `Subscriber::on_register_dispatch` method for performing late initialization after a `Subscriber` is registered as a `Dispatch`, and bugfixes for the `#[instrument]` attribute. Additionally, it fixes instances of the `bare_trait_objects` lint, which is now a warning on `tracing`'s MSRV and will become an error in the next edition. ### Fixed - **attributes**: Incorrect handling of inner attributes in `#[instrument]`ed functions (tokio-rs#2307) - **attributes**: Incorrect location of compiler diagnostic spans generated for type errors in `#[instrument]`ed `async fn`s (tokio-rs#2270) - **attributes**: Updated `syn` dependency to fix compilation with `-Z minimal-versions` (tokio-rs#2246) - `bare_trait_objects` warning in `valueset!` macro expansion (tokio-rs#2308) ### Added - **core**: `Subscriber::on_register_dispatch` method (tokio-rs#2269) - **core**: `WeakDispatch` type and `Dispatch::downgrade()` function (tokio-rs#2293) ### Changed - `tracing-core`: updated to [0.1.30][core-0.1.30] - `tracing-attributes`: updated to [0.1.23][attrs-0.1.23] ### Documented - Added [`tracing-web`] and [`reqwest-tracing`] to related crates (tokio-rs#2283, tokio-rs#2331) Thanks to new contributors @compiler-errors, @e-nomem, @WorldSEnder, @Xiami2012, and @tl-rodrigo-gryzinski, as well as @jswrenn and @CAD97, for contributing to this release! [core-0.1.30]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.30 [attrs-0.1.23]: https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.23 [`tracing-web`]: https://crates.io/crates/tracing-web/ [`reqwest-tracing`]: https://crates.io/crates/reqwest-tracing/
Motivation
When the
instrument
attribute is used on a function with inner attributes, the proc macro generates code above the attributes within the function block that causes compilation errors. These should be parsed out separately and handled.Fixes: #2294
Solution
I updated
MaybeItemFn
andMaybeItemFnRef
to so they hold both the outer and inner attributes for the instrumented function and updated the codegen to inlcude them in the appropriate locations.I couldn't preserve the existing
impl From<&'_ ItemFn> for MaybeItemFnRef<'_, Box<Block>>
due to lifetime issues and error handling (converting theItemFn
requires me to parse theitem.block
for inner attributes which is a local that I can't return, and the parsing could fail), so instead we now have animpl From<ItemFn> for MaybeItemFn
. I can't think of any way around this without removing the usages ofItemFn
entirely though.