Correct Fixes for Options and Results in Certain Contexts #96537
Conversation
rustbot
added
the
T-compiler
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| // the suggestion format strangely, so we adjust the hi bound manually | ||
| span.with_hi(span.hi() - BytePos(1)) | ||
| } | ||
| _ => span, |
There was a problem hiding this comment.
I suspect that this case may never happen (as an Option or Result must always be unwraped, or expected, etc) but am not confident enough to make this a panic or some such. Can change at reviewer discretion
There was a problem hiding this comment.
I'm not sure, let's keep it as is.
(And set applicability to MaybeIncorrect for this case.)
| @@ -8,7 +8,7 @@ impl LipogramCorpora { | |||
| pub fn validate_all(&mut self) -> Result<(), char> { | |||
| for selection in &self.selections { | |||
| if selection.1.is_some() { | |||
| if selection.1.as_ref().unwrap().contains(selection.0) { | |||
| if selection.1.as_ref().as_mut().unwrap().contains(selection.0) { | |||
There was a problem hiding this comment.
I'm not sure how to stop it from doing this (applying both suggestions)
There was a problem hiding this comment.
What happens if you use span_suggestion_verbose twice instead of one span_suggestions?
| @@ -8,7 +8,7 @@ impl LipogramCorpora { | |||
| pub fn validate_all(&mut self) -> Result<(), char> { | |||
| for selection in &self.selections { | |||
| if selection.1.is_some() { | |||
| if selection.1.as_ref().unwrap().contains(selection.0) { | |||
| if selection.1.as_ref().as_mut().unwrap().contains(selection.0) { | |||
There was a problem hiding this comment.
What happens if you use span_suggestion_verbose twice instead of one span_suggestions?
|
|
||
| // Unfortunately the new span has a "." on the end which makes | ||
| // the suggestion format strangely, so we adjust the hi bound manually | ||
| span.with_hi(span.hi() - BytePos(1)) |
There was a problem hiding this comment.
If it has . on the end, then maybe we should suggest as_ref(). instead of .as_ref().
Span arithmetic is dangerous because macros can set spans arbitrarily and span - BytePos(1) can potentially underflow or point into the middle of a unicode character producing an ICE.
There was a problem hiding this comment.
Sounds good to me, I'll remove the span arithmetic. This + the previous suggestion gives the following output for issue-96438.rs:
error[E0507]: cannot move out of dereference of `RefMut<'_, Option<Vec<()>>>`
--> ./src/test/ui/borrowck/issue-96438.rs:4:5
|
4 | cell.borrow_mut().unwrap().pop().unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ move occurs because value has type `Option<Vec<()>>`, which does not implement the `Copy` trait
|
help: consider borrowing the `Option`'s content
|
4 | cell.borrow_mut().as_ref().unwrap().pop().unwrap();
| +++++++++
help: consider borrowing the `Option`'s content
|
4 | cell.borrow_mut().as_mut().unwrap().pop().unwrap();
| +++++++++
error: aborting due to previous error
For more information about this error, try `rustc --explain E0507`.Which is alright, however it doesn't seem to fix the double application in option-content-move.stderr unfortunately.
| // the suggestion format strangely, so we adjust the hi bound manually | ||
| span.with_hi(span.hi() - BytePos(1)) | ||
| } | ||
| _ => span, |
There was a problem hiding this comment.
I'm not sure, let's keep it as is.
(And set applicability to MaybeIncorrect for this case.)
petrochenkov
added
S-waiting-on-author
JohnCSimon
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #98066) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@George-lewis @rustbot label: +S-inactive |
rustbot
added
the
S-inactive
Corrects the suggested fixes for Options and Results in certain contexts as mentioned in #96438.
The new code adjusts the span before applying the suggestion so that it's applied in the correct place, and also suggests both
.as_refand.as_mut. There may be a way to analyze the use of the moved variable to determine which one is needed, but I hope that this is sufficient for now.Closes #96438