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

macros: Remove 'r#' prefix from raw identifiers in field names #3130

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dtolnay
Copy link

@dtolnay dtolnay commented Nov 4, 2024

Motivation

Currently a call like info!(..., r#type = "...") ends up emitting a field named "r#type", which is never desirable. Previous work in #2925 made the desired behavior expressible as info!(..., type = "..."), and even before that PR one could write it as info!(..., "type" = "...").

However, when dealing with macro-generated use of tracing, we do still sometimes end up with raw identifiers. For example, if user-provided identifiers are used for both declaring (or instantiating, or accessing) a struct, and also for tracing. Something like this:

example!(name, r#type);

// expands to:

...
info!(..., name = GLOBAL.name, r#type = GLOBAL.r#type);
...

If we ask the user of example! to pass type (unraw), there is no way for the macro to access GLOBAL.r#type from that. But if we ask the user of example! to pass r#type (raw), there is no straightforward way for the macro to trace the field name as "type". The best option would have been something like:

info!(
    ...
    $(
        {
            (const {
                if let Some((b"r#", unraw)) = stringify!($field).as_bytes().split_first_chunk() {
                    unsafe { core::str::from_utf8_unchecked(unraw) }
                } else {
                    stringify!($field)
                }
            })
        } = GLOBAL.$field,
    )*
);

but this gets even more ridiculous when dealing with field names that are not just a single identifier. (prefix.r#keyword.suffix).

Solution

r#keyword = $value is made equivalent to keyword = $value. The field will be emitted with the name "keyword", not "r#keyword" as before.

@dtolnay dtolnay requested review from hawkw, davidbarsky and a team as code owners November 4, 2024 05:20
Copy link

@kadenlnelson kadenlnelson left a comment

Choose a reason for hiding this comment

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

Interesting... I'm ignorant to this so forgive me if I'm understanding this wrong.

Assuming that example! is an external macro, is it possible that this behavior could be solved within the example macro? Entertain me for a minute, but it appears that the tracing macros (ie, info!) are behaving as expected. Since the example macro renders what is ultimately fed into info!, that is where I feel the behavior should be addressed.

Now, that is my 2c when viewing this as a bug, and I'm still not certain whether I would personally classify this as a bug or a UX enhancement.

@davidbarsky davidbarsky enabled auto-merge (squash) December 10, 2024 17:39
@dtolnay
Copy link
Author

dtolnay commented Dec 10, 2024

CI does not appear to be planning to run. Let me try rebasing.

auto-merge was automatically disabled December 10, 2024 19:39

Head branch was pushed to by a user without write access

@dtolnay
Copy link
Author

dtolnay commented Dec 13, 2024

Fixed clippy lint.

warning: transmute from a `&[u8]` to a `&str`
    --> tracing/src/lib.rs:1228:22
     |
1228 |             unsafe { mem::transmute(self.0.as_slice()) }
     |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::str::from_utf8_unchecked(self.0.as_slice())`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#transmute_bytes_to_str
     = note: `#[warn(clippy::transmute_bytes_to_str)]` on by default

@davidbarsky
Copy link
Member

Thanks; looks like I had to go into a hidden UI to approve running actions.

@davidbarsky davidbarsky enabled auto-merge (squash) December 13, 2024 21:27
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.

3 participants