Skip to content

Port #[should_panic] to the new attribute parsing infrastructure #143808

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JonathanBrouwer
Copy link
Contributor

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

r? @jdonszelmann

@rustbot
Copy link
Collaborator

rustbot commented Jul 11, 2025

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

@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 Jul 11, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 11, 2025

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

match BUILTIN_ATTRIBUTE_MAP.get(name) {
// checked below
Some(BuiltinAttribute { type_: AttributeType::CrateLevel, .. }) => {}
Some(_) => {
if rest.len() > 0 && AttributeParser::<Late>::is_parsed_attribute(slice::from_ref(name)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently blocked on #143796, is in the merge queue right now should be fixed soon
@rustbot blocked

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 11, 2025
@@ -1,4 +1,3 @@
//@ check-pass
Copy link
Contributor Author

@JonathanBrouwer JonathanBrouwer Jul 11, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@JonathanBrouwer JonathanBrouwer left a comment

Choose a reason for hiding this comment

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

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

bors commented Jul 12, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) S-blocked Status: Blocked on something else such as an RFC or other implementation work. 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.

4 participants