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

instrument attribute incompatible with inner attributes inside function #2294

Closed
dtolnay opened this issue Aug 31, 2022 · 2 comments · Fixed by #2307
Closed

instrument attribute incompatible with inner attributes inside function #2294

dtolnay opened this issue Aug 31, 2022 · 2 comments · Fixed by #2307
Labels
crate/attributes Related to the `tracing-attributes` crate good first issue Good for newcomers kind/bug Something isn't working

Comments

@dtolnay
Copy link

dtolnay commented Aug 31, 2022

Bug Report

Version

└── tracing v0.1.36
    ├── tracing-attributes v0.1.22 (proc-macro)
    └── tracing-core v0.1.29

Platform

Linux rust 5.15.0-46-generic #49-Ubuntu SMP Thu Aug 4 18:03:25 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Description

use tracing::instrument;

#[instrument]
fn foo() {
    #![allow(dead_code)]

    struct Struct {}
}
$ cargo check

error: an inner attribute is not permitted in this context
 --> src/main.rs:5:5
  |
3 | #[instrument]
  | ------------- the inner attribute doesn't annotate this function
4 | fn foo() {
5 |     #![allow(dead_code)]
  |     ^^^^^^^^^^^^^^^^^^^^
  |
  = note: inner attributes, like `#![no_std]`, annotate the item enclosing them, and are usually found at the beginning of source files
help: to annotate the function, change the attribute from inner to outer style
  |
5 -     #![allow(dead_code)]
5 +     #[allow(dead_code)]
  |

This works without the instrument macro and it seems like it should be reasonable for instrument to support.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9c6afbe0ad161308d88f9cf02dfd1178

@hawkw hawkw added kind/bug Something isn't working crate/attributes Related to the `tracing-attributes` crate labels Aug 31, 2022
@hawkw
Copy link
Member

hawkw commented Aug 31, 2022

Thanks for the report, it definitely should be possible to use inner attributes!

My guess is that the issue is because #[instrument] currently just captures the entire function body as a block, and generates a function body that consists of code generated by #[instrument] followed by the entire contents of the function body. If the function body begins with inner attributes, that would mean that we insert the code generated by #[instrument] before the inner attributes, making them invalid.

To fix this, we would need to handle the case where the function body begins with inner attributes by capturing them separately and inserting them before the code added by #[instrument].

@hawkw hawkw added the good first issue Good for newcomers label Aug 31, 2022
@hawkw
Copy link
Member

hawkw commented Aug 31, 2022

This would probably be a fairly straightforward fix for a new contributor to tracing who has some familiarity with proc-macros in general, so I'm adding the good first issue label. Happy to provide guidance for anyone who wants to look into this!

e-nomem added a commit to e-nomem/tracing that referenced this issue Sep 16, 2022
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
@hawkw hawkw closed this as completed in 4848d7d Oct 6, 2022
hawkw added a commit that referenced this issue Oct 6, 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 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/attributes Related to the `tracing-attributes` crate good first issue Good for newcomers kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants