-
Notifications
You must be signed in to change notification settings - Fork 13.3k
add test to reproduce #137687 and add a hotfix #140584
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
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
wait 1s for the rebase, it makes some things different @fmease. Might fix the issue already |
92abfe5
to
09a9535
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to split this into two tests or remove the final #[crate_name] mod foo {}
if the latter is not a regression test for some issue?
Where the first one is:
// Regression test for former ICE: <https://github.com/rust-lang/rust/issues/137687>.
// Still, this should not pass but get rejected.
//@ known-bug: rust-lang/rust#NNN
//@ check-pass
#[crate_name = concat!("Cloneb")]
macro_rules! inline {
() => {};
}
Where NNN
is the relevant issue, otherwise use //@ known-bug: unknown
.
@@ -0,0 +1,8 @@ | |||
//@check-pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//@check-pass | |
// FIXME: This should fail. | |
//@ known-bug: rust-lang/rust#NNN | |
//@ check-pass |
with NNN
being the relavant issue or //@ known-bug: unknown
otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you link to the issue from within this test?
@@ -176,7 +176,7 @@ impl<'a> ArgParser<'a> { | |||
pub enum MetaItemOrLitParser<'a> { | |||
MetaItemParser(MetaItemParser<'a>), | |||
Lit(MetaItemLit), | |||
Err(Span, ErrorGuaranteed), | |||
Err(Span), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is MetaItemOrLitParser::Err
no longer constructed on master
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this may be true, actually! lets see
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @nnethercote 's recent work did that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'd need a PR targeting the beta branch
Opened #140601.
() => {}; | ||
} | ||
|
||
#[crate_name] //~ ERROR malformed `crate_name` attribute input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, this test case should definitely not reside in the same file as the one above as it masks the delayed_span_bug! If you remove the one below, the one above still ICEs on master: In compiler/rustc_attr_parsing/src/context.rs:337:43
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh, alright ty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear because it's getting slightly complicated ^^', you can either hotfix this issue (#137687) in this PR as you said you might and then we can beta-nominate it or we just ignore the ICE hitting stable (because it's fuzzer-generated and might not affect real users but who knows) and wait for "that lint buffering PR" to get merged which would unblock your real fix in #137729. 🤷
Just a heads up: I split #137687 into two issues:
I believe this PR is indeed trying to fix #137687. |
r? @fmease