Subtle change in how ty fragments get stringified

[workingjubilee]

Something in lexing, parsing, or even stringification or pretty-printing is now causing code that previously yielded strings like "Component1" to serde, based on the type name, to now emit strings like " Component1". This has been extracted from #128899 because many of the regressions there proved to be unrelated to each other, and it was hard to triage out what the "real" problem was. We have and most of them depended on unstable library details, but the bbarker/bevy_serde_macros case was distinct.

See @Nemo157's comment:

Equality doesn't care about which order you check the keys in, the debug output is different but that shouldn't affect the comparison result. There seem to be extra spaces in the keys on one side, which I assume means the test itself is broken. EDIT: looking at it a little more, seems to be related to not accounting for whitespace in stringify! of ty fragments, so tests :: Component1 gets split at the :: and given the name Component1 instead of the expected Component1, maybe #125174 related.

Originally posted by @Nemo157 in #128899 (comment)

[Nemo157]

Minimal example (playground):

macro_rules! test { ($t:ty) => { dbg!(stringify!($t)); } }
macro_rules! via_callback { () => { test!(foo::Bar); } }
fn main() { via_callback!(); }

on stable:

[src/main.rs:3:13] stringify!(foo::Bar) = "foo::Bar"

on beta and nightly:

[src/main.rs:3:13] stringify! (foo :: Bar) = "foo :: Bar"

[workingjubilee]

Thank you very much for that repro, @Nemo157!

[compiler-errors]

This regressed in #125174

[tgross35]

Cc @nnethercote from the above

[nnethercote]

Minimal example (playground):

macro_rules! test { ($t:ty) => { dbg!(stringify!($t)); } }
macro_rules! via_callback { () => { test!(foo::Bar); } }
fn main() { via_callback!(); }

on stable:

[src/main.rs:3:13] stringify!(foo::Bar) = "foo::Bar"

on beta and nightly:

[src/main.rs:3:13] stringify! (foo :: Bar) = "foo :: Bar"

Things like that aren't surprising. #125174 did change whitespace production in pretty printing output, the diff in the PR identifies quit a few other cases where output changed. These changes are valid.

• The docs for stringify! say "Note that the expanded results of the input tokens may change in the future. You should be careful if you rely on the output."
• Relatedly, the docs for impl Display for TokenStream say "Note: the exact form of the output is subject to change, e.g. there might be changes in the whitespace used between tokens. Therefore, you should not do any kind of simple substring matching on the output string (as produced by to_string) to implement a proc macro, because that matching might stop working if such changes happen. Instead, you should work at the TokenTree level, e.g. matching against TokenTree::Ident, TokenTree::Punct, or TokenTree::Literal."
• Of course people don't always follow that advice, so compilation failure with "cargo check --tests" for espidf #125714 did a crater run and found very small amounts of breakage.

My initial guess is that somebody has recently written a new proc macro that uses impl Display for TokenStream and doesn't account for possible whitespace changes. And that this was written since #125174, which is why it didn't show up in the crater run for that PR.

[nnethercote]

We have and most of them depended on unstable library details, but the bbarker/bevy_serde_macros case was distinct.

I overlooked this sentence before my earlier reply. I agree that, of the failures in #128899, bbarker/bevy_serde is probably caused by the changes in #125174.

[nnethercote]

The relevant lines in bevy_serde_macros are these two:

https://github.com/bbarker/bevy_serde_macros/blob/6865671e12e4ec555c4af6c031eaf8709eb40f6c/src/lib.rs#L74-L75

https://github.com/bbarker/bevy_serde_macros/blob/6865671e12e4ec555c4af6c031eaf8709eb40f6c/src/lib.rs#L154-L155

[workingjubilee]

@nnethercote Those lines date to November 25th, 2023, and your PR landed June 11, 2024. While it is technically possible to falsify git histories, I think that the real reason that your PR didn't reveal this in crater is that the "craterbot check" and "craterbot build-and-test" commands are non-identical, and beta crater runs use the build-and-test mode.

[nnethercote]

Thanks for the info! I think the proper fix here needs to be on the bevy_serde_macros side, given that it was relying on behaviour that is subject to change.

[apiraino]

Hope it's fine to this as WONTFIX (as per prev. comment)