Use proper span when emitting 'self' identifier #210
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Normally, the span of a field has the same hygiene context as `Span::call_site`. However, it's possible for a struct definition to be 'constructed' via a `macro_rules` macro such that the field has a different hygiene context. This will cause the expanded code to be unable to resolve any references to `self`, resulting in a compilation error. This pull request uses `quote!` instead of `quote_spanned!` when emitting a 'self' identifier. `quote_spanned!` is still used for everything else in the emitted method, meaning that error messages will still point to the proper field. I've included a test case which triggers this issue on Rust 1.43.1. It's current difficult to hit this issue other than in this artificial case, but that will change once rust-lang/rust#72622 is re-landed.
bkchr
approved these changes
May 30, 2020
|
@bkchr: Could you make a new point release? I'm planning to merge rust-lang/rust#73084, which will expose the bug fixed by this PR. |
bkchr
pushed a commit
that referenced
this pull request
Jul 28, 2020
Normally, the span of a field has the same hygiene context as `Span::call_site`. However, it's possible for a struct definition to be 'constructed' via a `macro_rules` macro such that the field has a different hygiene context. This will cause the expanded code to be unable to resolve any references to `self`, resulting in a compilation error. This pull request uses `quote!` instead of `quote_spanned!` when emitting a 'self' identifier. `quote_spanned!` is still used for everything else in the emitted method, meaning that error messages will still point to the proper field. I've included a test case which triggers this issue on Rust 1.43.1. It's current difficult to hit this issue other than in this artificial case, but that will change once rust-lang/rust#72622 is re-landed.
|
@Aaron1011 ty for reminding us! Done: https://crates.io/crates/parity-scale-codec-derive |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 23, 2020
…d, r=petrochenkov Re-land PR rust-lang#72388: Recursively expand `TokenKind::Interpolated` in `probably_equal_for_proc_macro` PR rust-lang#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 rust-lang#72545 and rust-lang#72622). These regressions fell into two categories 1. Missing handling for `Group`s 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](paritytech/parity-scale-codec#210) 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 rust-lang#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 rust-lang#72545 (comment), not re-landing PR rust-lang#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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Normally, the span of a field has the same hygiene context as
Span::call_site. However, it's possible for a struct definitionto be 'constructed' via a
macro_rulesmacro such that the fieldhas a different hygiene context. This will cause the expanded code
to be unable to resolve any references to
self, resulting in acompilation error.
This pull request uses
quote!instead ofquote_spanned!whenemitting a 'self' identifier.
quote_spanned!is still used foreverything else in the emitted method, meaning that error messages
will still point to the proper field.
I've included a test case which triggers this issue on
Rust 1.43.1. It's current difficult to hit this issue
other than in this artificial case, but that will change
once rust-lang/rust#72622 is re-landed.