Skip to content

Commit 193b59c

Browse files
GTimothyGTimothy
committed
second version
- properly propose methods depending of the type relationship between the index used and the map key. - use whether key type is copy/clone to orient propositions.
1 parent 5d997ef commit 193b59c

5 files changed

Lines changed: 105 additions & 117 deletions

File tree

‎compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs‎

Lines changed: 88 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -682,64 +682,100 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
682682
}
683683
};
684684

685-
/// Taken straight from https://doc.rust-lang.org/nightly/nightly-rustc/clippy_utils/fn.peel_hir_ty_refs.html
686-
/// Adapted to mid using https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.Ty.html#method.peel_refs
687-
/// Simplified to counting only
688-
/// Peels off all references on the type. Returns the number of references
689-
/// removed.
690-
fn count_ty_refs<'tcx>(ty: Ty<'tcx>) -> usize {
691-
let mut count = 0;
692-
let mut ty = ty;
693-
while let ty::Ref(_, inner_ty, _) = ty.kind() {
694-
ty = *inner_ty;
695-
count += 1;
685+
// Lets see:
686+
// because of TypeChecking and indexing, we know: index is &Q
687+
// with K: Eq + Hash + Borrow<Q>,
688+
// with Q: Eq + Hash + ?Sized,index is a &Q with K:Borrow<Q>,
689+
//
690+
//
691+
//
692+
// there is no other constraint on the types, therefore we need to look at the
693+
// constraints of the suggestions:
694+
// pub fn get_mut<Q>(&mut self, k: &Q) -> Option<&mut V>
695+
// where
696+
// K: Borrow<Q>,
697+
// Q: Hash + Eq + ?Sized,
698+
//
699+
// pub fn insert(&mut self, k: K, v: V) -> Option<V>
700+
// pub fn entry(&mut self, key: K) -> Entry<'_, K, V>
701+
//
702+
// But lets note that there could be also, if imported from hashbrown:
703+
// pub fn entry_ref<'a, 'b, Q>(
704+
// &'a mut self,
705+
// key: &'b Q,
706+
// ) -> EntryRef<'a, 'b, K, Q, V, S, A>
707+
// where
708+
// Q: Hash + Equivalent<K> + ?Sized,
709+
710+
/// testing if index is &K:
711+
fn index_is_borrowed_key<'tcx>(index: Ty<'tcx>, key: Ty<'tcx>) -> bool {
712+
if let ty::Ref(_, inner_ty, _) = index.kind() {
713+
*inner_ty == key
714+
} else {
715+
false
696716
}
697-
count
717+
}
718+
/// checking if the key is copy clone
719+
fn key_is_copyclone<'tcx>(key: Ty<'tcx>) -> bool {
720+
key.is_trivially_pure_clone_copy()
698721
}
699722

700723
// we know ty is a map, with a key type at walk distance 2.
701724
let key_type = self.ty.walk().nth(1).unwrap().expect_ty();
702-
let key_ref_depth = count_ty_refs(key_type);
703725

704726
if let hir::ExprKind::Assign(place, rv, _sp) = expr.kind
705727
&& let hir::ExprKind::Index(val, index, _) = place.kind
706728
&& (expr.span == self.assign_span || place.span == self.assign_span)
707729
{
708-
let (prefix, gm_prefix) = {
709-
let ref_depth_difference: usize;
710-
711-
if let Some(index_ty) =
712-
self.infcx.tcx.typeck(val.hir_id.owner.def_id).expr_ty_opt(index)
713-
{
714-
let index_ref_depth = count_ty_refs(index_ty);
715-
ref_depth_difference = index_ref_depth - key_ref_depth; //index should
716-
//be deeper than key
717-
} else {
718-
// no type ?
719-
// FIXME: unsure how to handle this case
720-
return;
721-
};
722-
723-
// remove the excessive referencing if necessary, but get_mut requires a ref
724-
match ref_depth_difference {
725-
0 => (String::new(), String::from("&")),
726-
n => ("*".repeat(n), "*".repeat(n - 1)),
727-
}
728-
};
729-
730-
self.err.multipart_suggestion(
731-
format!("use `.insert()` to insert a value into a `{}`", self.ty),
732-
vec![
733-
// val.insert(index, rv);
734-
(
735-
val.span.shrink_to_hi().with_hi(index.span.lo()),
736-
format!(".insert({prefix}"),
730+
let index_ty =
731+
self.infcx.tcx.typeck(val.hir_id.owner.def_id).expr_ty(index);
732+
// only suggest `insert` and `entry` if K is copy/clone because of the signature.
733+
if index_is_borrowed_key(index_ty, key_type) && key_is_copyclone(key_type) {
734+
self.err.multipart_suggestion(
735+
format!("use `.insert()` to insert a value into a `{}`", self.ty),
736+
vec![
737+
// val.insert(index, rv);
738+
(
739+
val.span
740+
.shrink_to_hi()
741+
.with_hi(index.span.lo() + BytePos(1)), //remove the
742+
//leading &
743+
format!(".insert("),
744+
),
745+
(
746+
index.span.shrink_to_hi().with_hi(rv.span.lo()),
747+
", ".to_string(),
748+
),
749+
(rv.span.shrink_to_hi(), ")".to_string()),
750+
],
751+
Applicability::MaybeIncorrect,
752+
);
753+
self.err.multipart_suggestion(
754+
format!(
755+
"use the entry API to modify a `{}` for more flexibility",
756+
self.ty
737757
),
738-
(index.span.shrink_to_hi().with_hi(rv.span.lo()), ", ".to_string()),
739-
(rv.span.shrink_to_hi(), ")".to_string()),
740-
],
741-
Applicability::MaybeIncorrect,
742-
);
758+
vec![
759+
// let x = val.entry(index).insert_entry(rv);
760+
(val.span.shrink_to_lo(), "let val = ".to_string()),
761+
(
762+
val.span
763+
.shrink_to_hi()
764+
.with_hi(index.span.lo() + BytePos(1)), //remove the
765+
//leading &
766+
format!(".entry("),
767+
),
768+
(
769+
index.span.shrink_to_hi().with_hi(rv.span.lo()),
770+
").insert_entry(".to_string(),
771+
),
772+
(rv.span.shrink_to_hi(), ")".to_string()),
773+
],
774+
Applicability::MaybeIncorrect,
775+
);
776+
}
777+
// in all cases, suggest get_mut because K:Borrow<K> or Q:Borrow<K> as a
778+
// requirement of indexing.
743779
self.err.multipart_suggestion(
744780
format!(
745781
"use `.get_mut()` to modify an existing key in a `{}`",
@@ -750,7 +786,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
750786
(val.span.shrink_to_lo(), "if let Some(val) = ".to_string()),
751787
(
752788
val.span.shrink_to_hi().with_hi(index.span.lo()),
753-
format!(".get_mut({gm_prefix}"),
789+
".get_mut(".to_string(),
754790
),
755791
(
756792
index.span.shrink_to_hi().with_hi(place.span.hi()),
@@ -760,60 +796,22 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
760796
],
761797
Applicability::MaybeIncorrect,
762798
);
763-
self.err.multipart_suggestion(
764-
format!(
765-
"use the entry API to modify a `{}` for more flexibility",
766-
self.ty
767-
),
768-
vec![
769-
// let x = val.entry(index).insert_entry(rv);
770-
(val.span.shrink_to_lo(), "let val = ".to_string()),
771-
(
772-
val.span.shrink_to_hi().with_hi(index.span.lo()),
773-
format!(".entry({prefix}"),
774-
),
775-
(
776-
index.span.shrink_to_hi().with_hi(rv.span.lo()),
777-
").insert_entry(".to_string(),
778-
),
779-
(rv.span.shrink_to_hi(), ")".to_string()),
780-
],
781-
Applicability::MaybeIncorrect,
782-
);
799+
//FIXME: in the future, include the ref_entry suggestion here if it is added
800+
//to std.
801+
783802
self.suggested = true;
784803
} else if let hir::ExprKind::MethodCall(_path, receiver, _, sp) = expr.kind
785804
&& let hir::ExprKind::Index(val, index, _) = receiver.kind
786805
&& receiver.span == self.assign_span
787806
{
788-
let gm_prefix = {
789-
let ref_depth_difference: usize;
790-
791-
if let Some(index_ty) =
792-
self.infcx.tcx.typeck(val.hir_id.owner.def_id).expr_ty_opt(index)
793-
{
794-
let index_ref_depth = count_ty_refs(index_ty);
795-
ref_depth_difference = index_ref_depth - key_ref_depth; //index should
796-
//be deeper than key
797-
} else {
798-
// no type ?
799-
// FIXME: unsure how to handle this case
800-
return;
801-
};
802-
803-
// remove the excessive referencing if necessary, but get_mut requires a ref
804-
match ref_depth_difference {
805-
0 => String::from("&"),
806-
n => "*".repeat(n - 1),
807-
}
808-
};
809807
// val[index].path(args..);
810808
self.err.multipart_suggestion(
811809
format!("to modify a `{}` use `.get_mut()`", self.ty),
812810
vec![
813811
(val.span.shrink_to_lo(), "if let Some(val) = ".to_string()),
814812
(
815813
val.span.shrink_to_hi().with_hi(index.span.lo()),
816-
format!(".get_mut({gm_prefix}"),
814+
format!(".get_mut("),
817815
),
818816
(
819817
index.span.shrink_to_hi().with_hi(receiver.span.hi()),

‎tests/ui/borrowck/index-mut-help.stderr‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ LL | map["peter"].clear();
88
help: to modify a `HashMap<&str, String>` use `.get_mut()`
99
|
1010
LL - map["peter"].clear();
11-
LL + if let Some(val) = map.get_mut(&"peter") { val.clear(); };
11+
LL + if let Some(val) = map.get_mut("peter") { val.clear(); };
1212
|
1313

1414
error[E0594]: cannot assign to data in an index of `HashMap<&str, String>`
@@ -18,20 +18,10 @@ LL | map["peter"] = "0".to_string();
1818
| ^^^^^^^^^^^^ cannot assign
1919
|
2020
= help: trait `IndexMut` is required to modify indexed content, but it is not implemented for `HashMap<&str, String>`
21-
help: use `.insert()` to insert a value into a `HashMap<&str, String>`
22-
|
23-
LL - map["peter"] = "0".to_string();
24-
LL + map.insert("peter", "0".to_string());
25-
|
2621
help: use `.get_mut()` to modify an existing key in a `HashMap<&str, String>`
2722
|
2823
LL - map["peter"] = "0".to_string();
29-
LL + if let Some(val) = map.get_mut(&"peter") { *val = "0".to_string(); };
30-
|
31-
help: use the entry API to modify a `HashMap<&str, String>` for more flexibility
32-
|
33-
LL - map["peter"] = "0".to_string();
34-
LL + let val = map.entry("peter").insert_entry("0".to_string());
24+
LL + if let Some(val) = map.get_mut("peter") { *val = "0".to_string(); };
3525
|
3626

3727
error[E0596]: cannot borrow data in an index of `HashMap<&str, String>` as mutable

‎tests/ui/collections/btreemap/btreemap-index-mut-2.stderr‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,17 @@ LL | map[&0] = 1;
88
help: use `.insert()` to insert a value into a `BTreeMap<u32, u32>`
99
|
1010
LL - map[&0] = 1;
11-
LL + map.insert(*&0, 1);
11+
LL + map.insert(0, 1);
1212
|
13-
help: use `.get_mut()` to modify an existing key in a `BTreeMap<u32, u32>`
13+
help: use the entry API to modify a `BTreeMap<u32, u32>` for more flexibility
1414
|
1515
LL - map[&0] = 1;
16-
LL + if let Some(val) = map.get_mut(&0) { *val = 1; };
16+
LL + let val = map.entry(0).insert_entry(1);
1717
|
18-
help: use the entry API to modify a `BTreeMap<u32, u32>` for more flexibility
18+
help: use `.get_mut()` to modify an existing key in a `BTreeMap<u32, u32>`
1919
|
2020
LL - map[&0] = 1;
21-
LL + let val = map.entry(*&0).insert_entry(1);
21+
LL + if let Some(val) = map.get_mut(&0) { *val = 1; };
2222
|
2323

2424
error: aborting due to 1 previous error

‎tests/ui/collections/btreemap/btreemap-index-mut.stderr‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,17 @@ LL | map[&0] = 1;
88
help: use `.insert()` to insert a value into a `BTreeMap<u32, u32>`
99
|
1010
LL - map[&0] = 1;
11-
LL + map.insert(*&0, 1);
11+
LL + map.insert(0, 1);
1212
|
13-
help: use `.get_mut()` to modify an existing key in a `BTreeMap<u32, u32>`
13+
help: use the entry API to modify a `BTreeMap<u32, u32>` for more flexibility
1414
|
1515
LL - map[&0] = 1;
16-
LL + if let Some(val) = map.get_mut(&0) { *val = 1; };
16+
LL + let val = map.entry(0).insert_entry(1);
1717
|
18-
help: use the entry API to modify a `BTreeMap<u32, u32>` for more flexibility
18+
help: use `.get_mut()` to modify an existing key in a `BTreeMap<u32, u32>`
1919
|
2020
LL - map[&0] = 1;
21-
LL + let val = map.entry(*&0).insert_entry(1);
21+
LL + if let Some(val) = map.get_mut(&0) { *val = 1; };
2222
|
2323

2424
error: aborting due to 1 previous error

‎tests/ui/collections/hashmap/hashmap-index-mut.stderr‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,17 @@ LL | map[&0] = 1;
88
help: use `.insert()` to insert a value into a `HashMap<u32, u32>`
99
|
1010
LL - map[&0] = 1;
11-
LL + map.insert(*&0, 1);
11+
LL + map.insert(0, 1);
1212
|
13-
help: use `.get_mut()` to modify an existing key in a `HashMap<u32, u32>`
13+
help: use the entry API to modify a `HashMap<u32, u32>` for more flexibility
1414
|
1515
LL - map[&0] = 1;
16-
LL + if let Some(val) = map.get_mut(&0) { *val = 1; };
16+
LL + let val = map.entry(0).insert_entry(1);
1717
|
18-
help: use the entry API to modify a `HashMap<u32, u32>` for more flexibility
18+
help: use `.get_mut()` to modify an existing key in a `HashMap<u32, u32>`
1919
|
2020
LL - map[&0] = 1;
21-
LL + let val = map.entry(*&0).insert_entry(1);
21+
LL + if let Some(val) = map.get_mut(&0) { *val = 1; };
2222
|
2323

2424
error: aborting due to 1 previous error

0 commit comments

Comments
 (0)