Ignore arguments when looking for IndexMut for subsequent mut obligation

[estebank]

Given code like v[&field].boo(); where field: String and
.boo(&mut self), typeck will have decided that v is accessed using
Index, but when boo adds a new mut obligation,
convert_place_op_to_mutable is called. When this happens, for some
reason the arguments' dereference adjustments are completely ignored
causing an error saying that IndexMut is not satisfied:

error[E0596]: cannot borrow data in an index of `Indexable` as mutable
  --> src/main.rs:30:5
   |
30 |     v[&field].boo();
   |     ^^^^^^^^^ cannot borrow as mutable
   |
   = help: trait `IndexMut` is required to modify indexed content, but it is not implemented for `Indexable`

This is not true, but by changing try_overloaded_place_op to retry
when given Needs::MutPlace without passing the argument types, the
example successfully compiles.

I believe there might be more appropriate ways to deal with this.

Fix #72002.

[rust-highfive]

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

[estebank]

CC @eddyb, @oli-obk, @nikomatsakis who will likely be interested in checking this out for correctness.

@davidtwco don't approve without input from them as I fear this might not be sound for some reason.

[davidtwco]

r? @oli-obk

[oli-obk]

Just from looking at the function you changed, I would have said the bug is in the lookup_method_in_trait function, as the resolution works for the immutable trait. You can try to do some detailed logging of typeck during the compilation of (&mut v[&field]).boo(); and of v[&field].boo() and analyze what is being done differently.

[nikomatsakis]

Skimming the diff, I agree with @oli-obk that this doesn't seem like the right approach. I think I would want to see the logs coming from this call:

rust/src/librustc_typeck/check/method/mod.rs

Line 331 in 3fe4dd2

if !self.predicate_may_hold(&obligation) {

There must be some underlying bug here.

[estebank]

It seems to me that the difference in behavior is that when try_overloaded_place_op is first called in try_index_step in the (&mut foo[bar]).baz() case bar hasn't been resolved/coerced to a derefed &str yet (substs=[Indexable, _]), while when try_overloaded_place_op is called from convert_place_op_to_mutable bar is resolved to &String. When doing convert_place_op_to_mutable it fails always but writing(&mut v[&field]) precludes it being called.

[DEBUG rustc_typeck::check] try_index_step(expr=Expr { hir_id: HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 28 }, kind: Index(Expr { hir_id: HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 24 }, kind: Path(Resolved(None, Path { span: src/test/ui/issues/issue-72002.rs:28:5: 28:6, res: Local(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 1 }), segments: [PathSegment { ident: v#0, hir_id: Some(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 23 }), res: Some(Local(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 1 })), args: None, infer_args: true }] })), attrs: ThinVec(None), span: src/test/ui/issues/issue-72002.rs:28:5: 28:6 }, Expr { hir_id: HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 27 }, kind: AddrOf(Ref, Not, Expr { hir_id: HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 26 }, kind: Path(Resolved(None, Path { span: src/test/ui/issues/issue-72002.rs:28:8: 28:13, res: Local(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 2 }), segments: [PathSegment { ident: field#0, hir_id: Some(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 25 }), res: Some(Local(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 2 })), args: None, infer_args: true }] })), attrs: ThinVec(None), span: src/test/ui/issues/issue-72002.rs:28:8: 28:13 }), attrs: ThinVec(None), span: src/test/ui/issues/issue-72002.rs:28:7: 28:13 }), attrs: ThinVec(None), span: src/test/ui/issues/issue-72002.rs:28:5: 28:14 }, base_expr=Expr { hir_id: HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 24 }, kind: Path(Resolved(None, Path { span: src/test/ui/issues/issue-72002.rs:28:5: 28:6, res: Local(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 1 }), segments: [PathSegment { ident: v#0, hir_id: Some(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 23 }), res: Some(Local(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 1 })), args: None, infer_args: true }] })), attrs: ThinVec(None), span: src/test/ui/issues/issue-72002.rs:28:5: 28:6 }, adjusted_ty=Indexable, index_ty=&std::string::String)
[DEBUG rustc_typeck::check] try_overloaded_place_op(src/test/ui/issues/issue-72002.rs:28:5: 28:14,Indexable,None,Index)
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted(self_ty=Indexable, m_name=index, trait_def_id=DefId(2:2146 ~ core[4753]::ops[0]::index[0]::Index[0]))
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted: substs=[Indexable, _]
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted: obligation=Obligation(predicate=Binder(TraitPredicate(<Indexable as std::ops::Index<_>>)), depth=0) trait_ref=<Indexable as std::ops::Index<_>>
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted: method_item=AssocItem { def_id: DefId(2:2149 ~ core[4753]::ops[0]::index[0]::Index[0]::index[0]), ident: index#0, kind: Fn, vis: Public, defaultness: Default { has_value: false }, container: TraitContainer(DefId(2:2146 ~ core[4753]::ops[0]::index[0]::Index[0])), fn_has_self_parameter: true }
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted: matched method method_ty=fn(&Indexable, _) -> &Indexable obligation=Obligation(predicate=Binder(TraitPredicate(<Indexable as std::ops::Index<_>>)), depth=0)
[DEBUG rustc_typeck::check::method] callee = MethodCallee { def_id: DefId(2:2149 ~ core[4753]::ops[0]::index[0]::Index[0]::index[0]), substs: [Indexable, _], sig: ([&Indexable, _]; c_variadic: false)->&Indexable }
[DEBUG rustc_typeck::check] try_index_step: success, using overloaded indexing
[DEBUG rustc_typeck::check] register_predicate(Obligation(predicate=Binder(TraitPredicate(<Indexable as std::ops::Index<&str>>)), depth=0))
[DEBUG rustc_typeck::check] register_predicate(Obligation(predicate=WellFormed(fn(&Indexable, _) -> &Indexable), depth=0))
[DEBUG rustc_typeck::check] apply_adjustments(expr=Expr { hir_id: HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 24 }, kind: Path(Resolved(None, Path { span: src/test/ui/issues/issue-72002.rs:28:5: 28:6, res: Local(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 1 }), segments: [PathSegment { ident: v#0, hir_id: Some(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 23 }), res: Some(Local(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 1 })), args: None, infer_args: true }] })), attrs: ThinVec(None), span: src/test/ui/issues/issue-72002.rs:28:5: 28:6 }, adj=[Borrow(Ref('_#13r, Not)) -> &Indexable])
[DEBUG rustc_typeck::check] write_method_call(hir_id=HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 28 }, method=MethodCallee { def_id: DefId(2:2149 ~ core[4753]::ops[0]::index[0]::Index[0]::index[0]), substs: [Indexable, _], sig: ([&Indexable, _]; c_variadic: false)->&Indexable })
[DEBUG rustc_typeck::check] write_substs(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 28 }, [Indexable, _]) in fcx 0x700007e38178
[DEBUG rustc_typeck::check] resolve_vars_with_obligations(ty=_)
[DEBUG rustc_typeck::check] resolve_vars_with_obligations: ty=&str
[DEBUG rustc_typeck::check] resolve_vars_with_obligations(ty=&std::string::String)
[DEBUG rustc_typeck::check] resolve_vars_with_obligations: ty=&std::string::String
[DEBUG rustc_typeck::check::coercion] coercion::try(Expr { hir_id: HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 27 }, kind: AddrOf(Ref, Not, Expr { hir_id: HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 26 }, kind: Path(Resolved(None, Path { span: src/test/ui/issues/issue-72002.rs:28:8: 28:13, res: Local(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 2 }), segments: [PathSegment { ident: field#0, hir_id: Some(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 25 }), res: Some(Local(HirId { owner: DefId(0:12 ~ issue_72002[317d]::main[0]), local_id: 2 })), args: None, infer_args: true }] })), attrs: ThinVec(None), span: src/test/ui/issues/issue-72002.rs:28:8: 28:13 }), attrs: ThinVec(None), span: src/test/ui/issues/issue-72002.rs:28:7: 28:13 }: &std::string::String -> &str)
[DEBUG rustc_typeck::check::coercion] Coerce.tys(&std::string::String => &str)
[DEBUG rustc_typeck::check::coercion] coerce_unsized(source=&std::string::String, target=&str)
[DEBUG rustc_typeck::check::coercion] unify(a: _, b: &str, use_lub: false)
[DEBUG rustc_typeck::check::coercion] coerce_unsized resolve step: Obligation(predicate=Binder(TraitPredicate(<&std::string::String as std::ops::CoerceUnsized<_>>)), depth=0)
[DEBUG rustc_typeck::check::coercion] coerce_unsized resolve step: Obligation(predicate=Binder(OutlivesPredicate('_#19r, '_#18r)), depth=1)
[DEBUG rustc_typeck::check::coercion] coerce_unsized resolve step: Obligation(predicate=Binder(TraitPredicate(<std::string::String as std::marker::Unsize<str>>)), depth=1)
[DEBUG rustc_typeck::check::coercion] coerce_unsized: early return - can't prove obligation
[DEBUG rustc_typeck::check::coercion] coerce: unsize failed
[DEBUG rustc_typeck::check::coercion] coerce_borrowed_pointer(a=&std::string::String, b=&str)
[DEBUG rustc_typeck::check::autoderef] autoderef: steps=[], cur_ty=&std::string::String
[DEBUG rustc_typeck::check::autoderef] autoderef stage #0 is &std::string::String
[DEBUG rustc_typeck::check::autoderef] autoderef: steps=[], cur_ty=&std::string::String
[DEBUG rustc_typeck::check::autoderef] autoderef stage #1 is std::string::String from (&std::string::String, Builtin)
[DEBUG rustc_typeck::check::coercion] unify(a: &std::string::String, b: &str, use_lub: false)
[DEBUG rustc_typeck::check::autoderef] autoderef: steps=[(&std::string::String, Builtin)], cur_ty=std::string::String
[DEBUG rustc_typeck::check::autoderef] overloaded_deref_ty(std::string::String)
[DEBUG rustc_typeck::check::autoderef] overloaded_deref_ty(std::string::String) = (str, [])
[DEBUG rustc_typeck::check::autoderef] autoderef stage #2 is str from (std::string::String, Overloaded)
[DEBUG rustc_typeck::check::coercion] unify(a: &str, b: &str, use_lub: false)
[DEBUG rustc_typeck::check] try_overloaded_place_op(src/test/ui/issues/issue-72002.rs:28:7: 28:13,std::string::String,None,Deref)
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted(self_ty=std::string::String, m_name=deref, trait_def_id=DefId(2:2065 ~ core[4753]::ops[0]::deref[0]::Deref[0]))
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted: substs=[std::string::String]
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted: obligation=Obligation(predicate=Binder(TraitPredicate(<std::string::String as std::ops::Deref>)), depth=0) trait_ref=<std::string::String as std::ops::Deref>
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted: method_item=AssocItem { def_id: DefId(2:2067 ~ core[4753]::ops[0]::deref[0]::Deref[0]::deref[0]), ident: deref#0, kind: Fn, vis: Public, defaultness: Default { has_value: false }, container: TraitContainer(DefId(2:2065 ~ core[4753]::ops[0]::deref[0]::Deref[0])), fn_has_self_parameter: true }
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted: matched method method_ty=fn(&std::string::String) -> &str obligation=Obligation(predicate=Binder(TraitPredicate(<std::string::String as std::ops::Deref>)), depth=0)

[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted(self_ty=Indexable, m_name=index_mut, trait_def_id=DefId(2:2150 ~ core[4753]::ops[0]::index[0]::IndexMut[0]))
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted: substs=[Indexable, &std::string::String]
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted: obligation=Obligation(predicate=Binder(TraitPredicate(<Indexable as std::ops::IndexMut<&std::string::String>>)), depth=0) trait_ref=<Indexable as std::ops::IndexMut<&std::string::String>>
[DEBUG rustc_typeck::check::method] --> Cannot match obligation
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted(self_ty=Indexable, m_name=index, trait_def_id=DefId(2:2146 ~ core[4753]::ops[0]::index[0]::Index[0]))
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted: substs=[Indexable, &std::string::String]
[DEBUG rustc_typeck::check::method] lookup_in_trait_adjusted: obligation=Obligation(predicate=Binder(TraitPredicate(<Indexable as std::ops::Index<&std::string::String>>)), depth=0) trait_ref=<Indexable as std::ops::Index<&std::string::String>>
[DEBUG rustc_typeck::check::method] --> Cannot match obligation

Also, changing convert_place_op_to_mutable to never pass the argument also doesn't always work, as we'd get "annotations needed" errors.

It might be that what needs to change is the call to self.node_ty here

rust/src/librustc_typeck/check/method/confirm.rs

Lines 470 to 478 in 4282776

	hir::ExprKind::Index(ref base_expr, ref index_expr) => {
	let index_expr_ty = self.node_ty(index_expr.hir_id);
	self.convert_place_op_to_mutable(
	PlaceOp::Index,
	expr,
	base_expr,
	&[index_expr_ty],
	);
	}

in order to get the correctly resolved type for the index expr, with all the previously accepted derefs applied. I'd tried doing the same thing I saw above,

rust/src/librustc_typeck/check/method/confirm.rs

Lines 447 to 467 in 4282776

	let mut source = self.node_ty(expr.hir_id);
	// Do not mutate adjustments in place, but rather take them,
	// and replace them after mutating them, to avoid having the
	// tables borrowed during (`deref_mut`) method resolution.
	let previous_adjustments =
	self.tables.borrow_mut().adjustments_mut().remove(expr.hir_id);
	if let Some(mut adjustments) = previous_adjustments {
	let needs = Needs::MutPlace;
	for adjustment in &mut adjustments {
	if let Adjust::Deref(Some(ref mut deref)) = adjustment.kind {
	if let Some(ok) = self.try_overloaded_deref(expr.span, source, needs) {
	let method = self.register_infer_ok_obligations(ok);
	if let ty::Ref(region, _, mutbl) = method.sig.output().kind {
	*deref = OverloadedDeref { region, mutbl };
	}
	}
	}
	source = adjustment.target;
	}
	self.tables.borrow_mut().adjustments_mut().insert(expr.hir_id, adjustments);
	}

navigating the adjustments and changing the type being passed in, but that was giving me MIR errors afterwards (about finding &String but expecting &str).

Looking at that code with fresh eyes, I see that I was cargo-culting the update, using borrow().expr_ty_adjusted(index_expr) was the correct behavior here. Is there a reason expr_ty_adjusted isn't used more prevalently (and as discoverable as) node_ty?

[oli-obk]

Is there a reason expr_ty_adjusted isn't used more prevalently (and as discoverable as) node_ty?

I don't know, but it's easy to test by changing the tables access in node_ty to using expr_ty_adjusted and checking how that goes.

Anyway, yes this looks like the right fix now, and makes me wonder if we have similar issues flying around. Maybe we should rename node_ty to node_ty_unadjusted to help with making expr_ty_adjusted more discoverable.

[estebank]

I made a quick check and from the looks of it we only get issues around binops in const scopes (complaining about not yet stable comp fns) and some borrow check errors in binop assignments talking about borrowing, but the output is not necessarily worse in all cases. It seems like this change is something we should consider doing soon.

[estebank]

@oli-obk let me know if I should make any further changes on this PR (which would be nice to backport soon) and we can change how we call node_ty in a subsequent PR.

[nikomatsakis]

I agree that, in most cases, invoking node_ty is a bug, and expr_ty_adjusted is the correct operation.

[oli-obk]

@bors r+

[bors]

📌 Commit 0dcde02 has been approved by oli-obk

[oli-obk]

(which would be nice to backport soon)

the issue isn't marked as a stable/beta regression. Should it be?

and we can change how we call node_ty in a subsequent PR.

This seems like a fairly mechanical change. Maybe open an E-mentor issue for it?

[estebank]

(which would be nice to backport soon)

the issue isn't marked as a stable/beta regression. Should it be?

It's not a regression because it's always been broken, but given how low risk it is I thought it could jump on the running train. That being said, I guess I'm getting too happy to do that and from a policy stand point we shouldn't do so and let it wait in the station for a few weeks won't hurt anyone (beyond the people I saw horribly confused a few weeks back when they hit this and couldn't understand why it didn't work, but that's always true for stable users).

and we can change how we call node_ty in a subsequent PR.

This seems like a fairly mechanical change. Maybe open an E-mentor issue for it?

I agree that, in most cases, invoking node_ty is a bug, and expr_ty_adjusted is the correct operation.

👍