Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Check if call return type is visibly uninhabited when building MIR #93313
Check if call return type is visibly uninhabited when building MIR #93313
Changes from all commits
6f8a1ee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 is a somewhat unfortunate reduction in test coverage for validity (we now error much earlier during evaluation)... ideally this would be adjusted to transmute to a type that is not 'visibly' uninhabited so that we still test the same code paths as before.
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 did the same in a Miri test so that type definition can probably just be copied.
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 behavior change seems correct, but I wonder if we need to be concerned from a compatibility perspective?
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.
To be clear: Wesley's assumption is that the unused
mut
lint started firing here (because the mutation ofresult
falls after the call tonever()
below, and so that's why the PR author has removed themut
.And Wesley's question is: Do we need to worry about this? Considering e.g. code that might have
#![deny(unused_mut)]
, and now it starts seeing that as an error.(My personal answer is that lints are allowed to change their behavior in this manner. I'm currently double-checking/asking around for examples from Rust's past.)
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.
Additional discussion in https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/lints.2C.20flow-sensitivity.2C.20and.20stability
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.
The lint side of things seems fine to me from compatibility perspective. It is
not unusual for us to adjust how lints work, and practical impact is limited by
cargo capping the lint level for dependencies.
The fact that we would now accept more programs seems more significant, since
it would be a breaking change to revert. Though, as far as I can see, we did in
fact accept those programs after #54125 landed, but have since regressed after
switching to MIR borrowck.
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.
It also seems surprising that this
mut
would be unused, sinceresult
is mutated in later code -- in a regular function, we are not that smart wrt unused-mut warnings, or are we?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 was talking about this with @scottmcm a little while back and he found this example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=26220b66188903b1dd8f3325960f75af
That version compiles even though
x
is mutated, since the mutation comes after the panic. Although, for me making itlet mut x
doesn't raise an unused mut warning. Anyway, I think this shows prior art for being clever about things being mutated only in unreachable code, so this change seems to make things more uniform.