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

Jump to bottom

attributes: fix handling of inner attributes #2307

Merged
merged 6 commits into from Oct 6, 2022
Merged

attributes: fix handling of inner attributes #2307

merged 6 commits into from Oct 6, 2022

Conversation

e-nomem
Copy link
Contributor

@e-nomem e-nomem commented Sep 16, 2022

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 impl From<&'_ ItemFn> for MaybeItemFnRef<'_, Box<Block>> due to lifetime issues and error handling (converting the ItemFn requires me to parse the item.block for inner attributes which is a local that I can't return, and the parsing could fail), so instead we now have an impl From<ItemFn> for MaybeItemFn. I can't think of any way around this without removing the usages of ItemFn entirely though.

@e-nomem
attributes: fix handling of inner attributes
39f8fe9 
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
@e-nomem e-nomem requested review from hawkw, davidbarsky and a team as code owners September 16, 2022 10:19
@e-nomem
Copy link
Contributor Author

e-nomem commented Sep 16, 2022 •
edited

I haven't worked through all the logic for async fn handling yet, but I suspect that gen_block needs updates as well

Edit: No it doesn't. I think this PR is good to go?

hawkw
hawkw requested changes Oct 3, 2022
View reviewed changes
Copy link
Member

@hawkw hawkw left a 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.

tracing-attributes/tests/async_fn.rs Show resolved Hide resolved
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
tracing-attributes/tests/instrument.rs Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Oct 6, 2022

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!

hawkw
hawkw approved these changes Oct 6, 2022
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Okay, I went ahead and made the changes I suggested in commits a113fde and 5a27105, so I think this should be good to merge. Thanks for working on this!

@hawkw hawkw enabled auto-merge (squash) October 6, 2022 17:51
@e-nomem
Copy link
Contributor Author

e-nomem commented Oct 6, 2022

Ah, thanks for that. I was going to get to it this weekend but I don't want to hold up the release 😅

@hawkw
Copy link
Member

hawkw commented Oct 6, 2022

No worries, it was a pretty straightforward change. Thanks again for the fix!

@hawkw hawkw merged commit 4848d7d into tokio-rs:master Oct 6, 2022
hawkw added a commit that referenced this pull request Oct 6, 2022
@e-nomem @hawkw
attributes: fix handling of inner attributes (#2307)
92cb2f0 
## 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>
hawkw added a commit that referenced this pull request Oct 6, 2022
@hawkw
attributes: prepare to release v0.1.23
63f0f55 
# 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
@hawkw hawkw mentioned this pull request Oct 6, 2022
Merged
hawkw added a commit that referenced this pull request Oct 6, 2022
@hawkw
attributes: prepare to release v0.1.23 (#2340)
64b221d 
# 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
hawkw added a commit that referenced this pull request Oct 6, 2022
@hawkw
tracing: prepare to release v0.1.37
baf5718 
# 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/
@hawkw hawkw mentioned this pull request Oct 6, 2022
Merged
hawkw added a commit that referenced this pull request Oct 6, 2022
@hawkw
tracing: prepare to release v0.1.37 (#2341)
8e35927 
# 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/
@davidbarsky davidbarsky mentioned this pull request Sep 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw hawkw approved these changes

@davidbarsky davidbarsky Awaiting requested review from davidbarsky davidbarsky is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

instrument attribute incompatible with inner attributes inside function
2 participants
@e-nomem @hawkw