Skip to content

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jdonszelmann
Copy link
Contributor

r? @fmease

@rustbot

This comment was marked as resolved.

@rustbot

This comment was marked as resolved.

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 2, 2025
@jdonszelmann jdonszelmann assigned fmease and unassigned BoxyUwU May 2, 2025
@jdonszelmann
Copy link
Contributor Author

wait 1s for the rebase, it makes some things different @fmease. Might fix the issue already

Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//@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.

Copy link
Member

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),
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Member

@fmease fmease May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, right. It was 'unintentionally' fixed in #124141 which is part of 1.88 and since we need a hotfix for 1.87 (current beta) and certainly don't want to backport #124141, we'd need a PR targeting the beta branch. Lemme think.

Copy link
Member

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
Copy link
Member

@fmease fmease May 2, 2025

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ugh, alright ty

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

Copy link
Member

@fmease fmease May 2, 2025

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. 🤷

@fmease fmease 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 May 2, 2025
@jieyouxu
Copy link
Member

jieyouxu commented May 3, 2025

Just a heads up: I split #137687 into two issues:

I believe this PR is indeed trying to fix #137687.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants