Port #[should_panic] to the new attribute parsing infrastructure

[JonathanBrouwer]

Ports #[should_panic] to the new attribute parsing infrastructure for #131229 (comment)

r? @jdonszelmann

[rustbot]

jdonszelmann is currently at their maximum review capacity.
They may take a while to respond.

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in compiler/rustc_attr_data_structures

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

[JonathanBrouwer]

This is a breaking change! Malformed should_panic attributes have had the following warning since 2016:

warning: argument must be of the form: `expected = "error message"`
 --> src/main.rs:5:1
  |
5 | #[should_panic(expected = "foo", bar)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: errors in this attribute were erroneously allowed and will become a hard error in a future release

This PR makes this an error.

As discussed in #142838 (comment), we can make breaking changes as long as we do a crater run. So this PR needs a crater run.

[JonathanBrouwer]

This issue is currently NOT solved by this PR.
We can do this in a future PR or now, but we should decide whether this should be a warning or error:
#143799

[bors]

☔ The latest upstream changes (presumably #143810) made this pull request unmergeable. Please resolve the merge conflicts.

[JonathanBrouwer]

^ Rebased on #143796
@rustbot ready

I'd propose to do #143799 in a separate PR, as I think it's more likely to fail crater than the change in this PR

[bors]

☔ The latest upstream changes (presumably #143779) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #144044) made this pull request unmergeable. Please resolve the merge conflicts.

[jdonszelmann]

@bors try

[bors]

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

1. git checkout should_panic_parser (switch to your branch)
2. git fetch upstream master (retrieve the latest master)
3. git rebase upstream/master -p (rebase on top of it)
4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
5. git push self should_panic_parser --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging compiler/rustc_passes/src/check_attr.rs
Auto-merging compiler/rustc_parse/src/validate_attr.rs
Auto-merging compiler/rustc_hir/src/hir.rs
Auto-merging compiler/rustc_builtin_macros/src/test.rs
CONFLICT (content): Merge conflict in compiler/rustc_builtin_macros/src/test.rs
Auto-merging compiler/rustc_attr_parsing/src/context.rs
Auto-merging compiler/rustc_attr_data_structures/src/encode_cross_crate.rs
Auto-merging compiler/rustc_attr_data_structures/src/attributes.rs
Automatic merge failed; fix conflicts and then commit the result.

[jdonszelmann]

@bors try

[JonathanBrouwer]

Crater analysis:

• One crate uses the invalid #[should_panic("Expecting char. Identifier found")]
• One crate used the invalid #[should_panic()]
• One crate uses the invalid #[should_panic()]
• I'm a little confused on this one, it does not seem like the kind of error that would be spurious, but also completely unrelated to the PR

So 3 regressions, the rest is all spurious.

@jdonszelmann I think calling in the lang team for a decision is next?

[jdonszelmann]

@rust-lang/lang ?

[traviscross]

If we could please make PRs to the three affected crates, that'd be good, as it's something the team might ask to see.

Otherwise, this sounds right to me, so let's...

@rfcbot fcp merge

cc @rust-lang/lang-docs

[JonathanBrouwer]

@traviscross
I've submitted the PRs:
cre-chan/MCIR#1
KijitoraFinch/Fuchu#5
masaimahapa/rust-simple-die#1

[rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[joshtriplett]

This seems fine, and I'm checking my box here.

I'm wondering, though: how difficult is it, in the new attribute parsing infrastructure, be to accept and ignore should_panic() and treat it as equivalent to should_panic?

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[JonathanBrouwer]

@joshtriplett
This is very easy to do, if this is desired

[traviscross]

Thanks. We'll discuss with the nomination.

For our notes, the applicable Reference text: "The should_panic attribute".

[traviscross]

We talked about this after the meeting, and we weren't motivated to change what is here and under FCP.