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

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

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
Collaborator

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
Contributor

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
Contributor

bors commented Jun 7, 2020

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

@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.