-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Make broken MIR a proper lint. #119260
base: master
Are you sure you want to change the base?
Make broken MIR a proper lint. #119260
Conversation
r? @TaKO8Ki (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
/// The `broken_mir` statically detects undefined behaviour in the MIR optimization pipeline. | ||
/// This is an internal lint, and not intended to be used directly. | ||
pub rustc::BROKEN_MIR, | ||
Deny, |
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.
Forbid maybe?
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.
One use case is to use expect
. And you cannot expect a forbidden lint.
crate::declare_tool_lint! { | ||
/// The `broken_mir` statically detects undefined behaviour in the MIR optimization pipeline. | ||
/// This is an internal lint, and not intended to be used directly. | ||
pub rustc::BROKEN_MIR, | ||
Deny, | ||
"broken MIR", | ||
report_in_external_macro: true | ||
} |
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.
Bike-shedding: The term broken MIR is already used when validation fails. It also overstates the severity of what is being reported. Maybe unusual MIR?
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 do believe broken is the right term here
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.
In what sense is this MIR broken?
The motivation behind moving those checks out of validator was the fact that there is nothing inherently wrong about violating them. Those reports should NOT be treated as bugs. It doesn't even mean that the user provided program has an undefined behaviour, since we don't know whether those instructions are ever executed.
☔ The latest upstream changes (presumably #119377) made this pull request unmergeable. Please resolve the merge conflicts. |
1176c87
to
0cbefd5
Compare
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #119621) made this pull request unmergeable. Please resolve the merge conflicts. |
@rustbot author |
@cjgillot any updates on this? thanks |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☔ The latest upstream changes (presumably #129261) made this pull request unmergeable. Please resolve the merge conflicts. |
ping from triage - can you post your status on this PR? There hasn't been an update in a few months, and there are build and merge conflicts. Thanks! FYI: when a PR is ready for review, send a message containing |
This allows to use
expect(rustc::broken_mir)
when we actually expect it to fire.cc @tmiasko
@matthiaskrgr this should address #119077 (comment)