Skip to content

Re-land PR #72388: Recursively expand TokenKind::Interpolated in probably_equal_for_proc_macro #73084

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Aug 23, 2020

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Jun 7, 2020

PR #72388 allowed us to preserve the original TokenStream in 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 categories

  1. Missing handling for Groups with Delimiter::None, which are inserted during macro_rules! expansion (but are lost during stringification and re-parsing). A large number of these regressions were due to syn and proc-macro-hack, but several crates needed changes to their own proc-macro code.
  2. Legitimate hygiene issues that were previously being masked by stringification. Some of these were relatively benign (e.g. a compiliation error caused by misusing quote_spanned!). However, two crates had intentionally written unhygenic macro_rules! macros, which were able to access identifiers that were not passed as arguments (see Crater run for PR #72388 - Recursively expand TokenKind::Interpolated in probably_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 unhygenic macro_rules! macros in the time it takes that PR to be merged.

@rust-highfive
Copy link
Contributor

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 7, 2020
@Aaron1011
Copy link
Member Author

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned eddyb Jun 7, 2020
@Aaron1011
Copy link
Member Author

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

@bors
Copy link
Collaborator

bors commented Jun 7, 2020

⌛ Trying commit e2242b8217ed26f58df0d9adf30de04c61d1a2f4 with merge 9be3f2265fdeef8522c7f7b149329a178eac5317...

@rust-highfive

This comment has been minimized.

@Aaron1011
Copy link
Member Author

@bors try

@bors
Copy link
Collaborator

bors commented Jun 7, 2020

⌛ Trying commit 6329f808c468086795b6d2cc14f439f172907393 with merge 879b8cb7dc2ad9102994457e73cf78d124926ea5...

@rust-highfive

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Jun 7, 2020

☀️ Try build successful - checks-azure
Build commit: 879b8cb7dc2ad9102994457e73cf78d124926ea5 (879b8cb7dc2ad9102994457e73cf78d124926ea5)

@Aaron1011
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-73084 created and queued.
🤖 Automatically detected try build 879b8cb7dc2ad9102994457e73cf78d124926ea5
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 7, 2020
@craterbot
Copy link
Collaborator

🚧 Experiment pr-73084 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-73084 is completed!
📊 1158 regressed and 4 fixed (107755 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jun 11, 2020
@petrochenkov
Copy link
Contributor

Marking as waiting on author to triage the regressions.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2020
@Aaron1011
Copy link
Member Author

A large chunk of the regressions seems to be from crates still using an outdated js-sys version - the latest version compiles with this PR.

Unfortunately, many of the regressions are legitimate. It looks like the macro_rules! capture fix (needed because some crates would regress with just the first change) has set off another round of regressions, probably very similar to the first round caused by #72388.

Aaron1011 added a commit to Aaron1011/Pear that referenced this pull request Jun 11, 2020
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.
Aaron1011 added a commit to Aaron1011/structopt that referenced this pull request Jun 12, 2020
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`.
goldoneen added a commit to goldoneen/rust-front that referenced this pull request May 7, 2022
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-aura that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-atomic-swap that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-authorship that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-assets that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-babe that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-authority-discovery that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-benchmarking that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-balances that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-collective that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-democracy that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-elections-phragmen that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-contracts that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-executive that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-indices that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-grandpa that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-membership that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-im-online that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-identity that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-multisig that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-nicks that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-offences that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-proxy that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-recovery that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-scored-pool that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-society that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-session that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
taqtiqa-mark pushed a commit to subversive-upstream/polkadot-sdk-substrate-frame-staking that referenced this pull request Sep 14, 2023
This updates parity-scale-codec{-derive} to prepare for a rustc release
that would otherwise break the derive implementation:
rust-lang/rust#73084
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.