Aaron1011
commented
4 years ago @craterbot run start=master#52b605c8cb2f730e607de0777a694cd1b9bb3e15 end=master#7726070fa755f660b5da3f82f46e07d9c6866f69 mode=check-only
Closed Aaron1011 closed 4 years ago
Aaron1011
commented
4 years ago @craterbot run start=master#52b605c8cb2f730e607de0777a694cd1b9bb3e15 end=master#7726070fa755f660b5da3f82f46e07d9c6866f69 mode=check-only
craterbot
commented
4 years ago :ok_hand: Experiment pr-72622 created and queued.
:mag: You can check out the queue and this experiment's details.
:information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more
craterbot
commented
4 years ago :construction: Experiment pr-72622 is now running
:information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more
craterbot
commented
4 years ago :tada: Experiment pr-72622 is completed!
:bar_chart: 709 regressed and 2 fixed (106133 total)
:newspaper: Open the full report.
:warning: If you notice any spurious failure please add them to the blacklist! :information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more
Aaron1011
commented
4 years ago 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?
jhpratt
commented
4 years ago 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 cargo update -p proc-macro-hack resolves the issue, so I don't think a new release is necessary — the user would still have to update to get the fix.
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.
wez
commented
4 years ago I'm not sure why I've been mentioned here; what am I missing?
dagit
commented
4 years ago Same. I think it's some sort of spam bot or error. Definitely unwelcome and annoying.
Aaron1011
commented
4 years ago @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.
Aaron1011
commented
4 years ago The fallout from this change is extensive - I started trying to fix async-graphql, but there appears to be several layers of issues (the async-graphql-derive crate needs to handle syn::Type::Group, but it's also emitting a #[async_trait] invocation that has the wrong SyntaxContext for a self token due to a reparse).
I think we should revert the original PR, and work on submitting PRs to the regressed crates.
Aaron1011
commented
4 years ago I've started triaging the regressed crates here: https://hackmd.io/ItrXWRaSSquVwoJATPx3PQ
daboross
commented
4 years ago I was trying to fix this in screeps-api, but I really can't figure out what's wrong with the code. Is this an unintended regression, or could you point out what I've done wrong here, or is this serde_derive's problem somehow?
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
error: expected identifier
--> src/lib.rs:15:17
|
15 | $field_vis $field: $type,
| ^^^^^^^^^^
...
21 | / regurgitate_struct_def! {
22 | | #[derive(serde_derive::Deserialize)]
23 | | pub struct Foo {
24 | | _phantom: (),
25 | | }
26 | | }
| |_- in this macro invocation
|
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
error: aborting due to previous error
error: could not compile `nightlytest`.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 serde_derive's problem and as long as they fix it I should be good to go?
CasualX
commented
4 years ago 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 #[attribute] const NAME: Type = VALUE; VALUE. Then the proc macro can produce the value via the attribute proc macro. There doesn't appear to be a fundamental problem with this.
Also proc macros expanding to statements and expressions when? :)
jhpratt
commented
4 years ago @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!
CasualX
commented
4 years ago @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.
dtolnay
commented
4 years ago @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.
leo60228
commented
4 years ago Wait, what? I don't think I've ever used the name testing for a proc macro, much less published it...
leo60228
commented
4 years ago Yeah, the testing crate isn't mine and isn't even a proc macro.
leo60228
commented
4 years ago Oh, it's this repo I made to help someone on Discord: https://github.com/leo60228/axiom-demo That's not a proc macro, though. No idea why it's a root.
Aaron1011
commented
4 years ago 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 macro_rules macro that effectively has call-site hygiene for local variables. The value_changed reference is resolved in the scope of the caller (in the example invocation, to the function parameter value_changed: Callback<()>. Note that this identifier is not passed into slider.
Prior to PR https://github.com/rust-lang/rust/pull/72388, we were losing span information when html! (a proc-macro) was invoked by slider. As a result, all of the expanded tokens ended up with SyntaxContext::root, allowing unhygienic access to local variables.
When https://github.com/rust-lang/rust/pull/72388 landed, we started retaining proper Spans, leading us to correctly fail to resolve value_changed in the context of the slider expansion.
It looks as though slider was written this way deliberately - the author may not have realized that this behavior was a bug.
What is the policy for rolling out this kind of change? I'll send a PR to fix the slider macro, but I'm concerned that some crates might have more complex macros that are relying on this bug.
cc @petrochenkov @dtolnay.
Aaron1011
commented
4 years ago I've found another unhygenic macro_rules! in serde-tcod-config-parser
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, visitor ends up getting resolved in the callsite, even though it should have def-site resolution.
petrochenkov
commented
4 years ago Closing in favor of https://github.com/rust-lang/rust/pull/73084.
Better late than never...