Skip to content

Suggest use .get_mut instead of &mut when overloaded index type not impl IndexMut #144018

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions compiler/rustc_borrowck/src/diagnostics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -889,7 +889,7 @@ pub(super) enum BorrowedContentSource<'tcx> {
DerefMutableRef,
DerefSharedRef,
OverloadedDeref(Ty<'tcx>),
OverloadedIndex(Ty<'tcx>),
OverloadedIndex(Ty<'tcx>, Ty<'tcx>),
}

impl<'tcx> BorrowedContentSource<'tcx> {
Expand All @@ -905,7 +905,7 @@ impl<'tcx> BorrowedContentSource<'tcx> {
_ => None,
})
.unwrap_or_else(|| format!("dereference of `{ty}`")),
BorrowedContentSource::OverloadedIndex(ty) => format!("index of `{ty}`"),
BorrowedContentSource::OverloadedIndex(ty, _) => format!("index of `{ty}`"),
}
}

Expand All @@ -917,7 +917,7 @@ impl<'tcx> BorrowedContentSource<'tcx> {
// Overloaded deref and index operators should be evaluated into a
// temporary. So we don't need a description here.
BorrowedContentSource::OverloadedDeref(_)
| BorrowedContentSource::OverloadedIndex(_) => None,
| BorrowedContentSource::OverloadedIndex(_, _) => None,
}
}

Expand All @@ -935,7 +935,7 @@ impl<'tcx> BorrowedContentSource<'tcx> {
_ => None,
})
.unwrap_or_else(|| format!("dereference of `{ty}`")),
BorrowedContentSource::OverloadedIndex(ty) => format!("an index of `{ty}`"),
BorrowedContentSource::OverloadedIndex(ty, _) => format!("an index of `{ty}`"),
}
}

Expand All @@ -951,7 +951,7 @@ impl<'tcx> BorrowedContentSource<'tcx> {
} else if tcx.is_lang_item(trait_id, LangItem::Index)
|| tcx.is_lang_item(trait_id, LangItem::IndexMut)
{
Some(BorrowedContentSource::OverloadedIndex(args.type_at(0)))
Some(BorrowedContentSource::OverloadedIndex(args.type_at(0), args.type_at(1)))
} else {
None
}
Expand Down
81 changes: 79 additions & 2 deletions compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
but it is not implemented for `{ty}`",
));
}
Some(BorrowedContentSource::OverloadedIndex(ty)) => {
Some(BorrowedContentSource::OverloadedIndex(ty, _)) => {
err.help(format!(
"trait `IndexMut` is required to modify indexed content, \
but it is not implemented for `{ty}`",
Expand Down Expand Up @@ -1196,7 +1196,11 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
None
}
None => {
if name != kw::SelfLower {
let (should_suggest_ref_mut, suggest_get_mut) =
self.suggest_get_mut_or_ref_mut(local, opt_assignment_rhs_span);
if !should_suggest_ref_mut {
suggest_get_mut
} else if name != kw::SelfLower {
suggest_ampmut(
self.infcx.tcx,
local_decl.ty,
Expand Down Expand Up @@ -1414,6 +1418,79 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
None => {}
}
}

/// check if the RHS is an overloaded index expression whose source type
/// doesn't implement `IndexMut`. This category contains two sub-categories:
/// 1. HashMap or BTreeMap
/// 2. Other types
/// For the first sub-category, we suggest using `.get_mut()` instead of `&mut`,
/// For the second sub-category, we still suggest use `&mut`.
/// See issue #143732.
///
/// the first element of return value is a boolean indicating
/// if we should suggest use `&mut`
fn suggest_get_mut_or_ref_mut(
&self,
local: Local,
opt_assignment_rhs_span: Option<Span>,
) -> (bool, Option<AmpMutSugg>) {
self.find_assignments(local)
.first()
.map(|&location| {
if let Some(mir::Statement {
source_info: _,
kind: mir::StatementKind::Assign(box (_, mir::Rvalue::Ref(_, _, place))),
..
}) = self.body[location.block].statements.get(location.statement_index)
&& let BorrowedContentSource::OverloadedIndex(ty, index_ty) =
self.borrowed_content_source(place.as_ref())
&& let Some(index_mut_trait) = self.infcx.tcx.lang_items().index_mut_trait()
&& !self
.infcx
.type_implements_trait(
index_mut_trait,
[ty, index_ty],
self.infcx.param_env,
)
.must_apply_modulo_regions()
Comment on lines +1450 to +1455
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 use type_implements_trait to judge if the type impl IndexMut.

{
if let Some(adt) = ty.ty_adt_def()
&& (self.infcx.tcx.is_diagnostic_item(sym::HashMap, adt.did())
|| self.infcx.tcx.is_diagnostic_item(sym::BTreeMap, adt.did()))
&& let Some(rhs_span) = opt_assignment_rhs_span
&& let Ok(rhs_str) =
self.infcx.tcx.sess.source_map().span_to_snippet(rhs_span)
&& let Some(content) = rhs_str.strip_prefix('&')
&& content.contains('[')
&& content.contains(']')
&& let Some(bracket_start) = content.find('[')
&& let Some(bracket_end) = content.rfind(']')
&& bracket_start < bracket_end
{
let map_part = &content[..bracket_start];
let key_part = &content[bracket_start + 1..bracket_end];

(
false,
Some(AmpMutSugg {
has_sugg: true,
span: rhs_span,
suggestion: format!("{}.get_mut({}).unwrap()", map_part, key_part),
additional: None,
}),
)
} else {
// Type not implemented `IndexMut`,
// and we did not suggest because of HashMap or BTreeMap
(false, None)
}
} else {
// Type Impl IndexMut, we should suggest use `&mut`
(true, None)
}
})
.unwrap_or((true, None))
}
}

struct BindingFinder {
Expand Down
44 changes: 44 additions & 0 deletions tests/ui/borrowck/suggest-use-as-mut-for-map.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// We don't suggest change `&` to `&mut`
// instead we suggest using .get_mut() instead of &mut, see issue #143732

// Custom type that implements Index<usize> but not IndexMut
struct ReadOnlyVec<T> {
data: Vec<T>,
}

impl<T> ReadOnlyVec<T> {
fn new(data: Vec<T>) -> Self {
ReadOnlyVec { data }
}
}

impl<T> std::ops::Index<usize> for ReadOnlyVec<T> {
type Output = T;

fn index(&self, index: usize) -> &Self::Output {
&self.data[index]
}
}

fn main() {
let mut map = std::collections::BTreeMap::new();
map.insert(0, "string".to_owned());

let string = &map[&0];
string.push_str("test"); //~ ERROR cannot borrow `*string` as mutable, as it is behind a `&` reference [E0596]

let mut map = std::collections::HashMap::new();
map.insert(0, "string".to_owned());

let string = &map[&0];
string.push_str("test"); //~ ERROR cannot borrow `*string` as mutable, as it is behind a `&` reference [E0596]

let mut vec = vec![String::new(), String::new()];
let string = &vec[0];
string.push_str("test"); //~ ERROR cannot borrow `*string` as mutable, as it is behind a `&` reference [E0596]
Comment on lines +36 to +38
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 add a case that type impls IndexMut, i.e. Vec.


// Example with our custom ReadOnlyVec type
let read_only_vec = ReadOnlyVec::new(vec![String::new(), String::new()]);
let string_ref = &read_only_vec[0];
string_ref.push_str("test"); //~ ERROR cannot borrow `*string_ref` as mutable, as it is behind a `&` reference [E0596]
}
44 changes: 44 additions & 0 deletions tests/ui/borrowck/suggest-use-as-mut-for-map.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
error[E0596]: cannot borrow `*string` as mutable, as it is behind a `&` reference
--> $DIR/suggest-use-as-mut-for-map.rs:28:5
|
LL | string.push_str("test");
| ^^^^^^ `string` is a `&` reference, so the data it refers to cannot be borrowed as mutable
|
help: consider changing this to be a mutable reference
|
LL - let string = &map[&0];
LL + let string = map.get_mut(&0).unwrap();
|

error[E0596]: cannot borrow `*string` as mutable, as it is behind a `&` reference
--> $DIR/suggest-use-as-mut-for-map.rs:34:5
|
LL | string.push_str("test");
| ^^^^^^ `string` is a `&` reference, so the data it refers to cannot be borrowed as mutable
|
help: consider changing this to be a mutable reference
|
LL - let string = &map[&0];
LL + let string = map.get_mut(&0).unwrap();
|

error[E0596]: cannot borrow `*string` as mutable, as it is behind a `&` reference
--> $DIR/suggest-use-as-mut-for-map.rs:38:5
|
LL | string.push_str("test");
| ^^^^^^ `string` is a `&` reference, so the data it refers to cannot be borrowed as mutable
|
help: consider changing this to be a mutable reference
|
LL | let string = &mut vec[0];
| +++

error[E0596]: cannot borrow `*string_ref` as mutable, as it is behind a `&` reference
--> $DIR/suggest-use-as-mut-for-map.rs:43:5
|
LL | string_ref.push_str("test");
| ^^^^^^^^^^ `string_ref` is a `&` reference, so the data it refers to cannot be borrowed as mutable

Comment on lines +36 to +41
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 added a testcase by defining a type that impl Index but not impl IndexMut. We can see the test changes in the second commit.

error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0596`.
Loading