Re-land PR #72388: Recursively expand TokenKind::Interpolated in probably_equal_for_proc_macro#73084
Conversation
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
|
Let's a try build for a Crater run, so that we can verify that no new crates are broken by this change: @bors try |
|
⌛ Trying commit e2242b8217ed26f58df0d9adf30de04c61d1a2f4 with merge 9be3f2265fdeef8522c7f7b149329a178eac5317... |
This comment has been minimized.
This comment has been minimized.
|
@bors try |
|
⌛ Trying commit 6329f808c468086795b6d2cc14f439f172907393 with merge 879b8cb7dc2ad9102994457e73cf78d124926ea5... |
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-azure |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
|
Marking as waiting on author to triage the regressions. |
|
A large chunk of the regressions seems to be from crates still using an outdated Unfortunately, many of the regressions are legitimate. It looks like the |
The method `Span.join` will return `None` if the start and end `Span`s are from different files. This is currently difficult to observe in practice due to rust-lang/rust#43081, which causes span information (including file information) to be lost in many cases. However, PR rust-lang/rust#73084 will cause `Spans` to be properly preserved in many more cases. This will cause `rocket` to to stop compiling, as this code will end up getting hit with spans from different files (one from rocket, and one from the `result` ident defined in the `pear_try!` macro. To reproduce the issue: 1. Checkout SergioBenitez/Rocket#63a4ae048540a6232c3c7c186e9d027081940382 2. Install https://github.com/kennytm/rustup-toolchain-install-master if you don't already have it 3. Run `rustup-toolchain-install-master 879b8cb7dc2ad9102994457e73cf78d124926ea5` (this corresponds to the latest commit from rust-lang/rust#73084) 4. From the `Rocket` checkout, run `cargo +879b8cb7dc2ad9102994457e73cf78d124926ea5 build` 5. Observe the panic from `Pear` I've based this PR on the commit for `Pear 0.1.2`, since the master branch has many breaking changes. I would recommend merging this change into a separate branch, and making a new `0.1.3` point release.
In `struct-opt-derive`, a function is generated with a parameter named `matches`. Since `quote!` is used to generate the function, the `matches` token will be resolved using `Span::call_site`. However, the literal identifier `matches` is also used inside several `quote_spanned!` expressions. Such a `matches` identifier will be resolved using the `Span` passed to `quote_spanned!`, which may not be the same as `Span::call_site`. Currently, this is difficult to observe in practice, due to rust-lang/rust#43081 . However, once PR rust-lang/rust#73084 is merged, proc macros will see properly spanned tokens in more cases, which will cause these incorrect uses of `quote_spanned!` to break. This PR uses `quote! { matches }` to generate a correctly spanned `matches` token, which is then include in the `quote_spanned!` expressions using `#matches`.
|
@petrochenkov: I've addressed your comments |
|
Thanks! |
Fixes rust-lang#68430 This is a re-attempt of PR rust-lang#72388, which was previously reverted due to a large number of breakages. All of the known breakages should now be patched upstream.
|
I don't know what the process is for tagging something with |
|
@bors r=petrochenkov |
|
📌 Commit 0fcad9c has been approved by |
|
@bors retry |
|
☀️ Test successful - checks-actions, checks-azure |
|
This also regressed a proc macro in the Fuchsia tree which was using proc_macro_hack inside of a macro_rules macro. 😰 I'm fine with updating it not to use proc_macro_hack anymore, which seems to fix the regression. Not sure if this was ever "correct" or not. Errors looked like this: |
|
@tmandry Which version of proc-macro-hack do you depend on? (Probably it can be fixed by updating the version of proc-macro-hack.) |
|
@taiki-e Ah, we're on 0.5.15 so right before that fix was published. That's |
PR #72388 allowed us to preserve the original
TokenStreamin more cases during proc-macro expansion, but had to be reverted due to a large number of regressions (See #72545 and #72622). These regressions fell into two categoriesGroups withDelimiter::None, which are inserted duringmacro_rules!expansion (but are lost during stringification and re-parsing). A large number of these regressions were due tosynandproc-macro-hack, but several crates needed changes to their own proc-macro code.quote_spanned!). However, two crates had intentionally written unhygenicmacro_rules!macros, which were able to access identifiers that were not passed as arguments (see Crater run for PR #72388 - Recursively expandTokenKind::Interpolatedinprobably_equal_for_proc_macro#72622 (comment)).All but one of the Crater regressions have now been fixed upstream (see https://hackmd.io/ItrXWRaSSquVwoJATPx3PQ?both). The remaining crate (which has a PR pending at sammhicks/face-generator#1) is not on
crates.io, and is a Yew application that seems unlikely to have any reverse dependencies.As @petrochenkov mentioned in #72545 (comment), not re-landing PR #72388 allows more crates to write unhygenic
macro_rules!macros, which will eventually stop compiling. Since there is only one Crater regression remaining, since additional crates could write unhygenicmacro_rules!macros in the time it takes that PR to be merged.