Crater run for PR #72388 - Recursively expand TokenKind::Interpolated in probably_equal_for_proc_macro
#72622
Labels
S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
Comments
|
@craterbot run start=master#52b605c8cb2f730e607de0777a694cd1b9bb3e15 end=master#7726070fa755f660b5da3f82f46e07d9c6866f69 mode=check-only |
|
👌 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 |
craterbot
added
the
S-waiting-on-crater
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
root: async-graphql - 4 (0 gh, 4 crates.io) detected crates which regressed due to this; cc @sunli829 root: ckb-fixed-hash-hack - 2 (2 gh, 0 crates.io) detected crates which regressed due to thisno owner? root: dwm-status - 2 (1 gh, 1 crates.io) detected crates which regressed due to this; cc @Gerschtli
root: fourier - 2 (1 gh, 1 crates.io) detected crates which regressed due to this; cc @calebzulawski root: frame-support - 16 (8 gh, 8 crates.io) detected crates which regressed due to this; cc @gnunicorn
root: js-sys - 468 (293 gh, 175 crates.io) detected crates which regressed due to this; cc @alexcrichton, @fitzgen
root: reproto-core - 8 (0 gh, 8 crates.io) detected crates which regressed due to this; cc @udoprog
root: srml-support - 6 (6 gh, 0 crates.io) detected crates which regressed due to thisno owner? root: time-macros-impl - 176 (119 gh, 57 crates.io) detected crates which regressed due to this; cc @jhpratt
root: vek - 5 (3 gh, 2 crates.io) detected crates which regressed due to this; cc @yoanlcq
root: wirefilter-engine - 2 (1 gh, 1 crates.io) detected crates which regressed due to this; cc @RReverser
root: unknown causes - 2 (1 gh, 1 crates.io) detected crates which regressed due to thisno owner? |
|
As the maintainer of the time crate, what should I do, if anything? It's one of the largest regressions. I've already received confirmation that running Realistically I don't even need this specific use, as it only saves me a little bit of code. But at the same time, I don't think removing the proc macro inside of the macro_rules will make anything better. |
|
I'm not sure why I've been mentioned here; what am I missing? |
|
Same. I think it's some sort of spam bot or error. Definitely unwelcome and annoying. |
|
@wez @dagit: I'm really sorry - the tool I used to generate the report automatically included mentions. I should have removed them before posting the report. EDIT: I failed to read the documentation for crater-generate-report - I thought that the option to generate pings was actually an option to control which crates were considered. |
|
The fallout from this change is extensive - I started trying to fix I think we should revert the original PR, and work on submitting PRs to the regressed crates. |
|
I've started triaging the regressed crates here: https://hackmd.io/ItrXWRaSSquVwoJATPx3PQ |
|
I was trying to fix this in macro_rules! regurgitate_struct_def {
(
$( #[$struct_attr:meta] )*
$item_vis:vis struct $name:ident {
$(
$( #[$field_attr:meta] )*
$field_vis:vis $field:ident : $type:ty,
)*
}
) => (
$( #[$struct_attr] )*
$item_vis struct $name {
$(
$( #[$field_attr] )*
$field_vis $field: $type,
)*
}
)
}
regurgitate_struct_def! {
#[derive(serde_derive::Deserialize)]
pub struct Foo {
_phantom: (),
}
}For reference, it errors with According to the rust reference, vis metavariables are allowed to be empty, so I would fully expect this to work. Edit: I looked at the link for the proper way to fix this from the hackmd, but it seems to be entirely about proc macros. Does this mean it is |
|
Hi, I'd love to fix any 'broken' code on my end but I'm unsure what's going on here. What exactly is this issue trying to do? I have the impression it's the proc macro hack workaround that is problematic but I'm not sure why? All I do is use a macro rules macro to expand to a Also proc macros expanding to statements and expressions when? :) |
|
@CasualX Have you tried updating the version of proc macro hack? That's all that was necessary for the time crate. And proc macros will be able to expand to expressions in 1.45, which is due to be released July 16! |
|
@jhpratt I don't use proc macro hack because it depends on syn and quote which takes forever to compile. I learned how proc macro hack worked and implemented the bare essentials myself. I'm not a fan of unnecessary and slow to compile dependencies. |
|
@CasualX fwiw proc-macro-hack does not depend on syn. It has no dependencies. It has dev-dependencies for compiling its test suite but downstream code does not build those. |
kornelski
added a commit
to crev-dev/cargo-crev
that referenced
this issue
May 29, 2020
rust-lang/rust#72622 also removes unused deps
|
Wait, what? I don't think I've ever used the name |
|
Yeah, the |
|
Oh, it's this repo I made to help someone on Discord: https://github.com/leo60228/axiom-demo |
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this issue
May 30, 2020
…, r=petrochenkov Revert recursive `TokenKind::Interpolated` expansion for now The crater run rust-lang#72622 revealed many root regressions, at least one of which is going to take some time to fix. For now, let's revert rust-lang#72388 to allow the 709 affected crates to continue building on the latest nightly.
yvt
added a commit
to yvt/try_match-rs
that referenced
this issue
May 30, 2020
Tested with: - nightly-2020-05-30 - nightly-2020-04-30 - 1.40.0 - 1.37.0 This commit fixes non-trivial patterns causing an error saying "unexpected token". This is done by parenthesizing `$p`. This might have something to do with an input pattern being wrapped by a `None`-delimited group and I suspect this might have been overlooked by the crater run <rust-lang/rust#72622>, but I'm not sure. This commit also implements a work-around for the issue <dtolnay/proc-macro-hack#46> by capturing the input expression by `$($in:tt)*`. The first work-around might have revealed this issue.
Aaron1011
added a commit
to Aaron1011/parity-scale-codec
that referenced
this issue
May 30, 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.
bkchr
pushed a commit
to paritytech/parity-scale-codec
that referenced
this issue
May 30, 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.
|
One of the failing crates has this concerning code: #[macro_export]
macro_rules! slider {
( $name:expr, $min:expr, $max:expr, $value:expr ) => {
html!(<Slider name=$name min=$min max=$max value=&mut $value as *mut Value value_changed=value_changed.clone() />)
};
}it's invoked like this: impl ScaledHead {
pub fn controls(&mut self, value_changed: Callback<()>) -> Html {
html!(
<>
<Group name="Grid">
{crate::slider!("Size", 8.0, 32.0, self.scale)}
</Group>
{self.head.controls(value_changed.clone())}
</>
)
}This is a Prior to PR #72388, we were losing span information when When #72388 landed, we started retaining proper It looks as though What is the policy for rolling out this kind of change? I'll send a PR to fix the |
Aaron1011
added a commit
to Aaron1011/utils
that referenced
this issue
May 31, 2020
Currently, rustc does not pass the exact original `TokenStream` to proc-macros in several cases. This has many undesirable effects, such as losing correct location information in error message. See rust-lang/rust#43081 for more details In the future, rustc will begin passing the correct `TokenStream` to proc-macros. As a result, some tokens may be wrapped in a `TokenTree::Group` with `Delimiter::None` (when the tokens originally came from a `macro_rules!`) macro expansion. I've determined that this change will cause your crate to stop working on some inputs. This PR updates `hex-literal-impl` to be compatible with both the old and new `TokenStream` contents. If you have any questions, feel free to ask me. See rust-lang/rust#72622 for more details
Aaron1011
added a commit
to Aaron1011/oauth1-request-rs
that referenced
this issue
May 31, 2020
Currently, rustc does not pass the exact original TokenStream to proc-macros in several cases. This has many undesirable effects, such as losing correct location information in error message. See rust-lang/rust#43081 for more details In the future, rustc will begin passing the correct TokenStream to proc-macros. As a result, `syn` may wrap a type in one or more `syn::Type::Group`s (if the proc-macro input came from a `macro_rules!` expansion). I've determined that this can cause `oauth1-request-derive` to fail to match a `Type::Path`. This PR should properly handle nested groups, allowing your crate to work with both old and new input. If you have any questions, feel free to ask me. See rust-lang/rust#72622 for more details.
|
I've found another unhygenic macro_rules! visit_number {
($l: expr, $to: ident, $ty: ident) => {
if $l.token == Token::$to {
paste::expr! {
let result = $l.slice().parse().unwrap();
$l.advance();
visitor.[<visit_$ty>](result)
}
} else {
unexpected_token!($l, "<number>")
}
};
}here, |
Aaron1011
added a commit
to Aaron1011/serde-tcod-config-parser
that referenced
this issue
May 31, 2020
In Rust, variable identifiers in a `macro_rules!` body are resolved
in the scope of tht body - they cannot see through to the caller's body.
For example, the following code errors:
```rust
macro_rules! weird_ident {
() => { my_ident; }
}
fn main() {
let my_ident = 1;
weird_ident!();
}
```
However, due to a compiler bug (rust-lang/rust#43081),
this kind of code current compiles when a procedural macro is invoked by
a `macro_rules!` macro. Eventually, this will cause a compilation error.
In the `visit_number!` macro, you're currently writing an expression
involving `visitor`, and passing it to a procedural macro (`paste!`).
However, `visitor` comes from the body of the caller of `visit_number!`,
so this code will stop compiling once the compiler bug is fixed.
Fortunately, the identifier of `visitor` can be passed into
`visit_number!`. This modified code will with the current version of
Rust, as well as future version that causes the old code into an error.
Feel free to ask me about any questions you may have. For more details,
see rust-lang/rust#72622
Aaron1011
added a commit
to Aaron1011/yaserde
that referenced
this issue
May 31, 2020
Currently, rustc does not pass the exact original TokenStream to proc-macros in several cases. This has many undesirable effects, such as losing correct location information in error message. See rust-lang/rust#43081 for more details In the future, rustc will begin passing the correct TokenStream to proc-macros. As a result, `syn` may wrap a type in one or more `syn::Type::Group`s (if the proc-macro input came from a `macro_rules!` expansion). I've determined that this can cause `yaserde-derive` to fail to match a `Type::Path`. This PR should properly handle nested groups, allowing your crate to work with both old and new input. If you have any questions, feel free to ask me. See rust-lang/rust#72622 for more details.
Aaron1011
added a commit
to Aaron1011/oauth1-request-rs
that referenced
this issue
May 31, 2020
Currently, rustc does not pass the exact original TokenStream to proc-macros in several cases. This has many undesirable effects, such as losing correct location information in error message. See rust-lang/rust#43081 for more details In the future, rustc will begin passing the correct TokenStream to proc-macros. As a result, `syn` may wrap a type in one or more `syn::Type::Group`s (if the proc-macro input came from a `macro_rules!` expansion). I've determined that this can cause `oauth1-request-derive` to fail to match a `Type::Path`. This PR should properly handle nested groups, allowing your crate to work with both old and new input. If you have any questions, feel free to ask me. See rust-lang/rust#72622 for more details.
Aaron1011
added a commit
to Aaron1011/oauth1-request-rs
that referenced
this issue
May 31, 2020
Currently, rustc does not pass the exact original TokenStream to proc-macros in several cases. This has many undesirable effects, such as losing correct location information in error message. See rust-lang/rust#43081 for more details In the future, rustc will begin passing the correct TokenStream to proc-macros. As a result, `syn` may wrap a type in one or more `syn::Type::Group`s (if the proc-macro input came from a `macro_rules!` expansion). I've determined that this can cause `oauth1-request-derive` to fail to match a `Type::Path`. This PR should properly handle nested groups, allowing your crate to work with both old and new input. If you have any questions, feel free to ask me. See rust-lang/rust#72622 for more details.
Aaron1011
added a commit
to Aaron1011/face-generator
that referenced
this issue
May 31, 2020
In Rust, variable identifiers in a `macro_rules!` body are resolved
in the scope of tht body - they cannot see through to the caller's body.
For example, the following code errors:
`rust
macro_rules! weird_ident {
() => { my_ident; }
}
fn main() {
let my_ident = 1;
weird_ident!();
}
`
However, due to a compiler bug (rust-lang/rust#43081),
this kind of code current compiles when a procedural macro is invoked by
a `macro_rules!` macro. Eventually, this will cause a compilation error.
In the `color!` and `slider!` macro, you're currently writing an expression
involving `value_changed`, and passing it to a procedural macro (`html!`).
However, `value_changed` comes from the body of the caller of `color!`/`slider!`,
so this code will stop compiling once the compiler bug is fixed.
Fortunately, the identifier of `value_changed` can be passed into
`color!` and `slider!`. This modified code will with the current version of
Rust, as well as future version that causes the old code into an error.
I also ran `cargo update` to bump the dependencies in your Cargo.lock.
This will allow your crate to continue to compile when the compiler bug is fixed,
as you are depending on some crates (e.g. `syn`) that have released updates
to address the issue.
Feel free to ask me about any questions you may have. For more details,
see rust-lang/rust#72622
newpavlov
pushed a commit
to RustCrypto/utils
that referenced
this issue
Jun 1, 2020
Currently, rustc does not pass the exact original `TokenStream` to proc-macros in several cases. This has many undesirable effects, such as losing correct location information in error message. See rust-lang/rust#43081 for more details In the future, rustc will begin passing the correct `TokenStream` to proc-macros. As a result, some tokens may be wrapped in a `TokenTree::Group` with `Delimiter::None` (when the tokens originally came from a `macro_rules!`) macro expansion. I've determined that this change will cause your crate to stop working on some inputs. This PR updates `hex-literal-impl` to be compatible with both the old and new `TokenStream` contents. If you have any questions, feel free to ask me. See rust-lang/rust#72622 for more details
bkchr
pushed a commit
to paritytech/parity-scale-codec
that referenced
this issue
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.
|
Closing in favor of #73084. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
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
Better late than never...
The text was updated successfully, but these errors were encountered: