Skip to content

Correct Fixes for Options and Results in Certain Contexts #96537

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Correct Fixes for Options and Results in Certain Contexts #96537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
28f8485
Simplify code, change applicability
George-lewis Apr 28, 2022
355dafa
Tidy
George-lewis Apr 28, 2022
aee411f
Lint
George-lewis Apr 28, 2022
58443e7
Delete dead code
George-lewis Apr 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions compiler/rustc_borrowck/src/diagnostics/move_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use rustc_middle::ty;
use rustc_mir_dataflow::move_paths::{
IllegalMoveOrigin, IllegalMoveOriginKind, LookupResult, MoveError, MovePathIndex,
};
use rustc_span::{sym, Span};
use rustc_span::{sym, BytePos, Span};

use crate::diagnostics::UseSpans;
use crate::prefixes::PrefixSet;
Expand Down Expand Up @@ -414,11 +414,23 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
&& use_spans.map_or(true, |v| !v.for_closure())
&& !has_complex_bindings
{
err.span_suggestion_verbose(
let span = match use_spans {
Some(UseSpans::FnSelfUse { var_span, fn_call_span, .. }) => {
// `fn_call_span` is the span of `unwrap()`
// This gets the 'variable' span excluding the unwrap
let span = var_span.until(fn_call_span);

// 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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@George-lewis George-lewis Apr 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}
_ => span,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, let's keep it as is.
(And set applicability to MaybeIncorrect for this case.)

};
err.span_suggestions(
span.shrink_to_hi(),
&format!("consider borrowing the `{}`'s content", diag_name.unwrap()),
".as_ref()".to_string(),
Applicability::MaybeIncorrect,
vec![".as_ref()".to_string(), ".as_mut()".to_string()].into_iter(),
Applicability::MachineApplicable,
);
} else if let Some(use_spans) = use_spans {
self.explain_captures(
Expand Down
13 changes: 13 additions & 0 deletions src/test/ui/borrowck/issue-96438.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
use std::cell::RefCell;

fn foo(cell: &RefCell<Option<Vec<()>>>) {
cell.borrow_mut().unwrap().pop().unwrap();
//~^ ERROR cannot move out of dereference of `RefMut<'_, Option<Vec<()>>>` [E0507]
//~| NOTE move occurs because value has type `Option<Vec<()>>`, which does not implement the `Copy` trait
//~| HELP consider borrowing the `Option`'s content
}

fn main() {
let cell = RefCell::new(None);
foo(&cell);
}
16 changes: 16 additions & 0 deletions src/test/ui/borrowck/issue-96438.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
error[E0507]: cannot move out of dereference of `RefMut<'_, Option<Vec<()>>>`
--> $DIR/issue-96438.rs:4:5
|
LL | 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
|
LL | cell.borrow_mut().as_mut().unwrap().pop().unwrap();
| +++++++++
LL | cell.borrow_mut().as_ref().unwrap().pop().unwrap();
| +++++++++

error: aborting due to previous error

For more information about this error, try `rustc --explain E0507`.
4 changes: 2 additions & 2 deletions src/test/ui/suggestions/option-content-move.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor Author

@George-lewis George-lewis Apr 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure how to stop it from doing this (applying both suggestions)

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you use span_suggestion_verbose twice instead of one span_suggestions?

//~^ ERROR cannot move out of `selection.1`
return Err(selection.0);
}
Expand All @@ -26,7 +26,7 @@ impl LipogramCorpora2 {
pub fn validate_all(&mut self) -> Result<(), char> {
for selection in &self.selections {
if selection.1.is_ok() {
if selection.1.as_ref().unwrap().contains(selection.0) {
if selection.1.as_ref().as_mut().unwrap().contains(selection.0) {
//~^ ERROR cannot move out of `selection.1`
return Err(selection.0);
}
Expand Down
4 changes: 4 additions & 0 deletions src/test/ui/suggestions/option-content-move.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ LL | if selection.1.unwrap().contains(selection.0) {
|
help: consider borrowing the `Option`'s content
|
LL | if selection.1.as_mut().unwrap().contains(selection.0) {
| +++++++++
LL | if selection.1.as_ref().unwrap().contains(selection.0) {
| +++++++++

Expand All @@ -17,6 +19,8 @@ LL | if selection.1.unwrap().contains(selection.0) {
|
help: consider borrowing the `Result`'s content
|
LL | if selection.1.as_mut().unwrap().contains(selection.0) {
| +++++++++
LL | if selection.1.as_ref().unwrap().contains(selection.0) {
| +++++++++

Expand Down