Clarify E0381 diagnostics for branch conditions#156603
Conversation
rustbot
added
S-waiting-on-review
|
r? @mati865 rustbot has assigned @mati865. Use Why was this reviewer chosen?The reviewer was selected based on:
|
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
eval-exec
force-pushed
the
e0381-diagnostic-condition-note
branch
from
b0e5f62 to
cb680f9
Compare
rustbot
added
S-waiting-on-review
rustbot
added
S-waiting-on-author
rustbot
added
S-waiting-on-review
|
@rustbot reroll |
There was a problem hiding this comment.
I'm not convinced the ConditionKind::Other variant makes sense. I think either we unconditionally give the disclaimer about the compiler not considering values for its reasoning, or we rephrase its messages to talk about then-branches and else-branches, without mentioning whether the if-condition might be true or false. That way we don't imply that the compiler cares about the value of the condition, but only that a certain branch lacks initialization.
| fn new(span: Span, kind: ConditionErrorKind, label: String) -> Self { | ||
| Self { span, label, kind } |
There was a problem hiding this comment.
Field ordering is not consistent between new and struct definition. Unless there is a good reason for that, I think it would be less surprising if the order were consistent.
| ConditionErrorKind::Other, | ||
| format!( | ||
| "if this pattern is matched, {} is not initialized", | ||
| self.name |
There was a problem hiding this comment.
Could not a pattern match lead to the same kind of diagnostic? If a variable is initialized as None, and I match on Some(_) and then don't initialize something in that branch, then we are in the same situation I think.
| ConditionErrorKind::Other, | ||
| format!( | ||
| "if the `for` loop runs 0 times, {} is not initialized", | ||
| self.name |
There was a problem hiding this comment.
What about the case where the for-loop loops i times and let i = 1;? Are we then not also in the situation where the compiler does not consider a value for its diagnostic?
| ConditionErrorKind::Other, | ||
| format!("an `else` arm might be missing here, initializing {}", self.name), | ||
| )); |
There was a problem hiding this comment.
The else-branch might be unreachable because of the values of variables in the program, which would mean the same kind of disclaimer would apply to this case as well.
|
@bors r+ |
rust-bors
Bot
added
the
S-waiting-on-bors
rust-bors
Bot
removed
the
S-waiting-on-review
|
@nikomatsakis Did you have an opinion about my review comments? I believe this is making a distinction that is not really there, as shown by the examples in said comments. |
…uwer Rollup of 6 pull requests Successful merges: - #155418 (transmute: fix check for whether newtypes have equal size) - #156603 (Clarify E0381 diagnostics for branch conditions) - #156643 (Document run-make external dependencies) - #157009 (Avoid `unreachable_code` on required return values) - #157308 (make typing mode exhaustive again...) - #157312 (disallow most attrs on eiis)
…uwer Rollup of 6 pull requests Successful merges: - #155418 (transmute: fix check for whether newtypes have equal size) - #156603 (Clarify E0381 diagnostics for branch conditions) - #156643 (Document run-make external dependencies) - #157009 (Avoid `unreachable_code` on required return values) - #157308 (make typing mode exhaustive again...) - #157312 (disallow most attrs on eiis)
this pr want to fix: #156494