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

fix: level! macros with constant field names in the first position #2883

Merged
merged 3 commits into from
Mar 1, 2024

Conversation

mladedav
Copy link
Contributor

@mladedav mladedav commented Feb 18, 2024

Motivation

#2837

Const argumetns in level! macros do not work when in the first positoin.

This also seems to have fixed #2748 where literals for fields names like info!("foo" = 2) could not be used outside the event! macro.

Solution

Previsously, level!($(args:tt)) was forwarded to event!(target: ..., level: ..., { $(args:tt) }) but the added curly braces seem to have prevented the event macro from correctly understanding the arguments and it tried to pass them to format!.

With this change there may have some performance impact when expanding the macros in most cases where the braces could have been added as it will take one more step.

These are the two relevant event! blocks I believe, the new tests used to expand to the first one (with empty fields), now they expand to the latter:

    (target: $target:expr, $lvl:expr, { $($fields:tt)* }, $($arg:tt)+ ) => (
        $crate::event!(
            target: $target,
            $lvl,
            { message = $crate::__macro_support::format_args!($($arg)+), $($fields)* }
        )
    );
    (target: $target:expr, $lvl:expr, $($arg:tt)+ ) => (
        $crate::event!(target: $target, $lvl, { $($arg)+ })
    );

@mladedav mladedav requested review from hawkw, davidbarsky and a team as code owners February 18, 2024 11:55
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.

Thanks for fixing this! This looks good to me!

@hawkw hawkw merged commit 908cc43 into tokio-rs:master Mar 1, 2024
55 checks passed
@mladedav mladedav deleted the dm/const-info branch March 1, 2024 23:07
@mladedav
Copy link
Contributor Author

mladedav commented Mar 2, 2024

@hawkw This (commit 908cc43 after squashing and merging into master) should also be cherry-picked onto v0.1.x. For future reference, if it applies to both, should I mention that in the description?

If so I can also try and change the PR template so that it's always clear which branches a PR should apply for and future contributors now to base things on master even if a patch is also needed on the v0.1.x branch.

dpc pushed a commit to dpc/tracing that referenced this pull request Apr 20, 2024
…tion (tokio-rs#2883)


## Motivation

Const argumetns in `level!` macros do not work when in the first
position.

This also seems to have fixed tokio-rs#2748 where literals for fields names like
`info!("foo" = 2)` could not be used outside the `event!` macro.


Fixes tokio-rs#2837
Fixes tokio-rs#2738

## Solution

Previsously, `level!($(args:tt))` was forwarded to `event!(target: ...,
level: ..., { $(args:tt) })` but the added curly braces seem to have
prevented the `event` macro from correctly understanding the arguments
and it tried to pass them to `format!`.

With this change there may have some performance impact when expanding
the macros in most cases where the braces could have been added as it
will take one more step.

These are the two relevant `event!` blocks I believe, the new tests used
to expand to the first one (with empty fields), now they expand to the
latter:
```
    (target: $target:expr, $lvl:expr, { $($fields:tt)* }, $($arg:tt)+ ) => (
        $crate::event!(
            target: $target,
            $lvl,
            { message = $crate::__macro_support::format_args!($($arg)+), $($fields)* }
        )
    );
    (target: $target:expr, $lvl:expr, $($arg:tt)+ ) => (
        $crate::event!(target: $target, $lvl, { $($arg)+ })
    );
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

string literal field names don't compile for level! event macros
2 participants