Skip to content

Commit 0dab65b

Browse files
borsbors
committed
Auto merge of rust-lang#119934 - compiler-errors:could-impl, r=jackh726
Make `InferCtxtExt::could_impl_trait` more precise, less ICEy The implementation for `InferCtxtExt::could_impl_trait` was very wrong. Along with being pretty poorly named, way too specific to ADTs, it was also doing impl substitution wrong -- this caused an ICE (rust-lang#119915). This PR generalizes that code, gives it a clearer name, makes it stop using the new trait solver (lol), and fixes some fallout bad suggestions that are made worse with the code fix. Fixes rust-lang#119915
2 parents aa5f781 + 7724ba7 commit 0dab65b

15 files changed

+128
-101
lines changed

Diff for: ‎compiler/rustc_borrowck/src/diagnostics/mod.rs

+5-3
Original file line numberDiff line numberDiff line change
@@ -1178,9 +1178,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
11781178
} else {
11791179
vec![(move_span.shrink_to_hi(), ".clone()".to_string())]
11801180
};
1181-
if let Some(errors) =
1182-
self.infcx.could_impl_trait(clone_trait, ty, self.param_env)
1183-
&& !has_sugg
1181+
if let Some(errors) = self.infcx.type_implements_trait_shallow(
1182+
clone_trait,
1183+
ty,
1184+
self.param_env,
1185+
) && !has_sugg
11841186
{
11851187
let msg = match &errors[..] {
11861188
[] => "you can `clone` the value and consume it, but this \

Diff for: ‎compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

+13-10
Original file line numberDiff line numberDiff line change
@@ -1217,19 +1217,22 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
12171217
{
12181218
match self
12191219
.infcx
1220-
.could_impl_trait(clone_trait, ty.peel_refs(), self.param_env)
1220+
.type_implements_trait_shallow(
1221+
clone_trait,
1222+
ty.peel_refs(),
1223+
self.param_env,
1224+
)
12211225
.as_deref()
12221226
{
12231227
Some([]) => {
1224-
// The type implements Clone.
1225-
err.span_help(
1226-
expr.span,
1227-
format!(
1228-
"you can `clone` the `{}` value and consume it, but this \
1229-
might not be your desired behavior",
1230-
ty.peel_refs(),
1231-
),
1232-
);
1228+
// FIXME: This error message isn't useful, since we're just
1229+
// vaguely suggesting to clone a value that already
1230+
// implements `Clone`.
1231+
//
1232+
// A correct suggestion here would take into account the fact
1233+
// that inference may be affected by missing types on bindings,
1234+
// etc., to improve "tests/ui/borrowck/issue-91206.stderr", for
1235+
// example.
12331236
}
12341237
None => {
12351238
if let hir::ExprKind::MethodCall(segment, _rcvr, [], span) =

Diff for: ‎compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1623,7 +1623,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
16231623
);
16241624
} else {
16251625
if let Some(errors) =
1626-
self.could_impl_trait(clone_trait_did, expected_ty, self.param_env)
1626+
self.type_implements_trait_shallow(clone_trait_did, expected_ty, self.param_env)
16271627
{
16281628
match &errors[..] {
16291629
[] => {}

Diff for: ‎compiler/rustc_trait_selection/src/infer.rs

+33-58
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
1-
use crate::solve::FulfillmentCtxt;
21
use crate::traits::query::evaluate_obligation::InferCtxtExt as _;
3-
use crate::traits::{self, DefiningAnchor, ObligationCtxt};
2+
use crate::traits::{self, DefiningAnchor, ObligationCtxt, SelectionContext};
43

4+
use crate::traits::TraitEngineExt as _;
55
use rustc_hir::def_id::DefId;
66
use rustc_hir::lang_items::LangItem;
7-
use rustc_infer::traits::{TraitEngine, TraitEngineExt};
7+
use rustc_infer::traits::{Obligation, TraitEngine, TraitEngineExt as _};
88
use rustc_middle::arena::ArenaAllocatable;
99
use rustc_middle::infer::canonical::{Canonical, CanonicalQueryResponse, QueryResponse};
1010
use rustc_middle::traits::query::NoSolution;
11+
use rustc_middle::traits::ObligationCause;
1112
use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable, TypeVisitableExt};
1213
use rustc_middle::ty::{GenericArg, ToPredicate};
1314
use rustc_span::DUMMY_SP;
@@ -21,24 +22,36 @@ pub trait InferCtxtExt<'tcx> {
2122

2223
fn type_is_sized_modulo_regions(&self, param_env: ty::ParamEnv<'tcx>, ty: Ty<'tcx>) -> bool;
2324

24-
/// Check whether a `ty` implements given trait(trait_def_id).
25+
/// Check whether a `ty` implements given trait(trait_def_id) without side-effects.
26+
///
2527
/// The inputs are:
2628
///
2729
/// - the def-id of the trait
2830
/// - the type parameters of the trait, including the self-type
2931
/// - the parameter environment
3032
///
3133
/// Invokes `evaluate_obligation`, so in the event that evaluating
32-
/// `Ty: Trait` causes overflow, EvaluatedToErrStackDependent (or EvaluatedToAmbigStackDependent)
33-
/// will be returned.
34+
/// `Ty: Trait` causes overflow, EvaluatedToErrStackDependent
35+
/// (or EvaluatedToAmbigStackDependent) will be returned.
3436
fn type_implements_trait(
3537
&self,
3638
trait_def_id: DefId,
3739
params: impl IntoIterator<Item: Into<GenericArg<'tcx>>>,
3840
param_env: ty::ParamEnv<'tcx>,
3941
) -> traits::EvaluationResult;
4042

41-
fn could_impl_trait(
43+
/// Returns `Some` if a type implements a trait shallowly, without side-effects,
44+
/// along with any errors that would have been reported upon further obligation
45+
/// processing.
46+
///
47+
/// - If this returns `Some([])`, then the trait holds modulo regions.
48+
/// - If this returns `Some([errors..])`, then the trait has an impl for
49+
/// the self type, but some nested obligations do not hold.
50+
/// - If this returns `None`, no implementation that applies could be found.
51+
///
52+
/// FIXME(-Znext-solver): Due to the recursive nature of the new solver,
53+
/// this will probably only ever return `Some([])` or `None`.
54+
fn type_implements_trait_shallow(
4255
&self,
4356
trait_def_id: DefId,
4457
ty: Ty<'tcx>,
@@ -86,64 +99,26 @@ impl<'tcx> InferCtxtExt<'tcx> for InferCtxt<'tcx> {
8699
self.evaluate_obligation(&obligation).unwrap_or(traits::EvaluationResult::EvaluatedToErr)
87100
}
88101

89-
fn could_impl_trait(
102+
fn type_implements_trait_shallow(
90103
&self,
91104
trait_def_id: DefId,
92105
ty: Ty<'tcx>,
93106
param_env: ty::ParamEnv<'tcx>,
94107
) -> Option<Vec<traits::FulfillmentError<'tcx>>> {
95108
self.probe(|_snapshot| {
96-
if let ty::Adt(def, args) = ty.kind()
97-
&& let Some((impl_def_id, _)) = self
98-
.tcx
99-
.all_impls(trait_def_id)
100-
.filter_map(|impl_def_id| {
101-
self.tcx.impl_trait_ref(impl_def_id).map(|r| (impl_def_id, r))
102-
})
103-
.map(|(impl_def_id, imp)| (impl_def_id, imp.skip_binder()))
104-
.find(|(_, imp)| match imp.self_ty().peel_refs().kind() {
105-
ty::Adt(i_def, _) if i_def.did() == def.did() => true,
106-
_ => false,
107-
})
108-
{
109-
let mut fulfill_cx = FulfillmentCtxt::new(self);
110-
// We get all obligations from the impl to talk about specific
111-
// trait bounds.
112-
let obligations = self
113-
.tcx
114-
.predicates_of(impl_def_id)
115-
.instantiate(self.tcx, args)
116-
.into_iter()
117-
.map(|(clause, span)| {
118-
traits::Obligation::new(
119-
self.tcx,
120-
traits::ObligationCause::dummy_with_span(span),
121-
param_env,
122-
clause,
123-
)
124-
})
125-
.collect::<Vec<_>>();
126-
fulfill_cx.register_predicate_obligations(self, obligations);
127-
let trait_ref = ty::TraitRef::new(self.tcx, trait_def_id, [ty]);
128-
let obligation = traits::Obligation::new(
129-
self.tcx,
130-
traits::ObligationCause::dummy(),
131-
param_env,
132-
trait_ref,
133-
);
134-
fulfill_cx.register_predicate_obligation(self, obligation);
135-
let mut errors = fulfill_cx.select_all_or_error(self);
136-
// We remove the last predicate failure, which corresponds to
137-
// the top-level obligation, because most of the type we only
138-
// care about the other ones, *except* when it is the only one.
139-
// This seems to only be relevant for arbitrary self-types.
140-
// Look at `tests/ui/moves/move-fn-self-receiver.rs`.
141-
if errors.len() > 1 {
142-
errors.truncate(errors.len() - 1);
109+
let mut selcx = SelectionContext::new(self);
110+
match selcx.select(&Obligation::new(
111+
self.tcx,
112+
ObligationCause::dummy(),
113+
param_env,
114+
ty::TraitRef::new(self.tcx, trait_def_id, [ty]),
115+
)) {
116+
Ok(Some(selection)) => {
117+
let mut fulfill_cx = <dyn TraitEngine<'tcx>>::new(self);
118+
fulfill_cx.register_predicate_obligations(self, selection.nested_obligations());
119+
Some(fulfill_cx.select_all_or_error(self))
143120
}
144-
Some(errors)
145-
} else {
146-
None
121+
Ok(None) | Err(_) => None,
147122
}
148123
})
149124
}

Diff for: ‎compiler/rustc_trait_selection/src/solve/fulfill.rs

+5
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,11 @@ pub struct FulfillmentCtxt<'tcx> {
3636

3737
impl<'tcx> FulfillmentCtxt<'tcx> {
3838
pub fn new(infcx: &InferCtxt<'tcx>) -> FulfillmentCtxt<'tcx> {
39+
assert!(
40+
infcx.next_trait_solver(),
41+
"new trait solver fulfillment context created when \
42+
infcx is set up for old trait solver"
43+
);
3944
FulfillmentCtxt { obligations: Vec::new(), usable_in_snapshot: infcx.num_open_snapshots() }
4045
}
4146
}

Diff for: ‎compiler/rustc_trait_selection/src/traits/fulfill.rs

+5
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,11 @@ static_assert_size!(PendingPredicateObligation<'_>, 72);
8080
impl<'tcx> FulfillmentContext<'tcx> {
8181
/// Creates a new fulfillment context.
8282
pub(super) fn new(infcx: &InferCtxt<'tcx>) -> FulfillmentContext<'tcx> {
83+
assert!(
84+
!infcx.next_trait_solver(),
85+
"old trait solver fulfillment context created when \
86+
infcx is set up for new trait solver"
87+
);
8388
FulfillmentContext {
8489
predicates: ObligationForest::new(),
8590
usable_in_snapshot: infcx.num_open_snapshots(),

Diff for: ‎tests/ui/borrowck/issue-119915-bad-clone-suggestion.rs

+28
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
use std::marker::PhantomData;
2+
3+
struct Example<E, FakeParam>(PhantomData<(fn(E), fn(FakeParam))>);
4+
5+
struct NoLifetime;
6+
struct Immutable<'a>(PhantomData<&'a ()>);
7+
8+
impl<'a, E: 'a> Copy for Example<E, Immutable<'a>> {}
9+
impl<'a, E: 'a> Clone for Example<E, Immutable<'a>> {
10+
fn clone(&self) -> Self {
11+
*self
12+
}
13+
}
14+
15+
impl<E, FakeParam> Example<E, FakeParam> {
16+
unsafe fn change<NewFakeParam>(self) -> Example<E, NewFakeParam> {
17+
Example(PhantomData)
18+
}
19+
}
20+
21+
impl<E> Example<E, NoLifetime> {
22+
fn the_ice(&mut self) -> Example<E, Immutable<'_>> {
23+
unsafe { self.change() }
24+
//~^ ERROR cannot move out of `*self` which is behind a mutable reference
25+
}
26+
}
27+
28+
fn main() {}

Diff for: ‎tests/ui/borrowck/issue-119915-bad-clone-suggestion.stderr

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
error[E0507]: cannot move out of `*self` which is behind a mutable reference
2+
--> $DIR/issue-119915-bad-clone-suggestion.rs:23:18
3+
|
4+
LL | unsafe { self.change() }
5+
| ^^^^ -------- `*self` moved due to this method call
6+
| |
7+
| move occurs because `*self` has type `Example<E, NoLifetime>`, which does not implement the `Copy` trait
8+
|
9+
note: `Example::<E, FakeParam>::change` takes ownership of the receiver `self`, which moves `*self`
10+
--> $DIR/issue-119915-bad-clone-suggestion.rs:16:36
11+
|
12+
LL | unsafe fn change<NewFakeParam>(self) -> Example<E, NewFakeParam> {
13+
| ^^^^
14+
15+
error: aborting due to 1 previous error
16+
17+
For more information about this error, try `rustc --explain E0507`.

Diff for: ‎tests/ui/borrowck/issue-85765-closure.rs

-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ fn main() {
33
let mut test = Vec::new();
44
let rofl: &Vec<Vec<i32>> = &mut test;
55
//~^ HELP consider changing this binding's type
6-
//~| HELP you can `clone` the `Vec<Vec<i32>>` value and consume it, but this might not be your desired behavior
76
rofl.push(Vec::new());
87
//~^ ERROR cannot borrow `*rofl` as mutable, as it is behind a `&` reference
98
//~| NOTE `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable

Diff for: ‎tests/ui/borrowck/issue-85765-closure.stderr

+4-9
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,16 @@
11
error[E0596]: cannot borrow `*rofl` as mutable, as it is behind a `&` reference
2-
--> $DIR/issue-85765-closure.rs:7:9
2+
--> $DIR/issue-85765-closure.rs:6:9
33
|
44
LL | rofl.push(Vec::new());
55
| ^^^^ `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable
66
|
7-
help: you can `clone` the `Vec<Vec<i32>>` value and consume it, but this might not be your desired behavior
8-
--> $DIR/issue-85765-closure.rs:4:36
9-
|
10-
LL | let rofl: &Vec<Vec<i32>> = &mut test;
11-
| ^^^^^^^^^
127
help: consider changing this binding's type
138
|
149
LL | let rofl: &mut Vec<Vec<i32>> = &mut test;
1510
| ~~~~~~~~~~~~~~~~~~
1611

1712
error[E0594]: cannot assign to `*r`, which is behind a `&` reference
18-
--> $DIR/issue-85765-closure.rs:14:9
13+
--> $DIR/issue-85765-closure.rs:13:9
1914
|
2015
LL | *r = 0;
2116
| ^^^^^^ `r` is a `&` reference, so the data it refers to cannot be written
@@ -26,7 +21,7 @@ LL | let r = &mut mutvar;
2621
| +++
2722

2823
error[E0594]: cannot assign to `*x`, which is behind a `&` reference
29-
--> $DIR/issue-85765-closure.rs:21:9
24+
--> $DIR/issue-85765-closure.rs:20:9
3025
|
3126
LL | *x = 1;
3227
| ^^^^^^ `x` is a `&` reference, so the data it refers to cannot be written
@@ -37,7 +32,7 @@ LL | let x: &mut usize = &mut{0};
3732
| ~~~~~~~~~~
3833

3934
error[E0594]: cannot assign to `*y`, which is behind a `&` reference
40-
--> $DIR/issue-85765-closure.rs:28:9
35+
--> $DIR/issue-85765-closure.rs:27:9
4136
|
4237
LL | *y = 1;
4338
| ^^^^^^ `y` is a `&` reference, so the data it refers to cannot be written

Diff for: ‎tests/ui/borrowck/issue-85765.rs

-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ fn main() {
22
let mut test = Vec::new();
33
let rofl: &Vec<Vec<i32>> = &mut test;
44
//~^ HELP consider changing this binding's type
5-
//~| HELP you can `clone` the `Vec<Vec<i32>>` value and consume it, but this might not be your desired behavior
65
rofl.push(Vec::new());
76
//~^ ERROR cannot borrow `*rofl` as mutable, as it is behind a `&` reference
87
//~| NOTE `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable

Diff for: ‎tests/ui/borrowck/issue-85765.stderr

+4-9
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,16 @@
11
error[E0596]: cannot borrow `*rofl` as mutable, as it is behind a `&` reference
2-
--> $DIR/issue-85765.rs:6:5
2+
--> $DIR/issue-85765.rs:5:5
33
|
44
LL | rofl.push(Vec::new());
55
| ^^^^ `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable
66
|
7-
help: you can `clone` the `Vec<Vec<i32>>` value and consume it, but this might not be your desired behavior
8-
--> $DIR/issue-85765.rs:3:32
9-
|
10-
LL | let rofl: &Vec<Vec<i32>> = &mut test;
11-
| ^^^^^^^^^
127
help: consider changing this binding's type
138
|
149
LL | let rofl: &mut Vec<Vec<i32>> = &mut test;
1510
| ~~~~~~~~~~~~~~~~~~
1611

1712
error[E0594]: cannot assign to `*r`, which is behind a `&` reference
18-
--> $DIR/issue-85765.rs:13:5
13+
--> $DIR/issue-85765.rs:12:5
1914
|
2015
LL | *r = 0;
2116
| ^^^^^^ `r` is a `&` reference, so the data it refers to cannot be written
@@ -26,7 +21,7 @@ LL | let r = &mut mutvar;
2621
| +++
2722

2823
error[E0594]: cannot assign to `*x`, which is behind a `&` reference
29-
--> $DIR/issue-85765.rs:20:5
24+
--> $DIR/issue-85765.rs:19:5
3025
|
3126
LL | *x = 1;
3227
| ^^^^^^ `x` is a `&` reference, so the data it refers to cannot be written
@@ -37,7 +32,7 @@ LL | let x: &mut usize = &mut{0};
3732
| ~~~~~~~~~~
3833

3934
error[E0594]: cannot assign to `*y`, which is behind a `&` reference
40-
--> $DIR/issue-85765.rs:27:5
35+
--> $DIR/issue-85765.rs:26:5
4136
|
4237
LL | *y = 1;
4338
| ^^^^^^ `y` is a `&` reference, so the data it refers to cannot be written

Diff for: ‎tests/ui/borrowck/issue-91206.rs

-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ fn main() {
1010
let client = TestClient;
1111
let inner = client.get_inner_ref();
1212
//~^ HELP consider specifying this binding's type
13-
//~| HELP you can `clone` the `Vec<usize>` value and consume it, but this might not be your desired behavior
1413
inner.clear();
1514
//~^ ERROR cannot borrow `*inner` as mutable, as it is behind a `&` reference [E0596]
1615
//~| NOTE `inner` is a `&` reference, so the data it refers to cannot be borrowed as mutable

Diff for: ‎tests/ui/borrowck/issue-91206.stderr

+1-6
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,9 @@
11
error[E0596]: cannot borrow `*inner` as mutable, as it is behind a `&` reference
2-
--> $DIR/issue-91206.rs:14:5
2+
--> $DIR/issue-91206.rs:13:5
33
|
44
LL | inner.clear();
55
| ^^^^^ `inner` is a `&` reference, so the data it refers to cannot be borrowed as mutable
66
|
7-
help: you can `clone` the `Vec<usize>` value and consume it, but this might not be your desired behavior
8-
--> $DIR/issue-91206.rs:11:17
9-
|
10-
LL | let inner = client.get_inner_ref();
11-
| ^^^^^^^^^^^^^^^^^^^^^^
127
help: consider specifying this binding's type
138
|
149
LL | let inner: &mut Vec<usize> = client.get_inner_ref();

0 commit comments

Comments
 (0)