From 04a6fd241bc18391b3f1f6ece1b109acd19842f1 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sat, 13 Jan 2024 17:09:33 +0000 Subject: [PATCH 1/2] Make InferCtxtExt::could_impl_trait less messed up --- .../rustc_borrowck/src/diagnostics/mod.rs | 8 +- .../src/diagnostics/mutability_errors.rs | 23 +++-- .../src/fn_ctxt/suggestions.rs | 2 +- compiler/rustc_trait_selection/src/infer.rs | 91 +++++++------------ .../issue-119915-bad-clone-suggestion.rs | 28 ++++++ .../issue-119915-bad-clone-suggestion.stderr | 17 ++++ tests/ui/borrowck/issue-85765-closure.rs | 1 - tests/ui/borrowck/issue-85765-closure.stderr | 13 +-- tests/ui/borrowck/issue-85765.rs | 1 - tests/ui/borrowck/issue-85765.stderr | 13 +-- tests/ui/borrowck/issue-91206.rs | 1 - tests/ui/borrowck/issue-91206.stderr | 7 +- tests/ui/moves/move-fn-self-receiver.stderr | 14 ++- 13 files changed, 118 insertions(+), 101 deletions(-) create mode 100644 tests/ui/borrowck/issue-119915-bad-clone-suggestion.rs create mode 100644 tests/ui/borrowck/issue-119915-bad-clone-suggestion.stderr diff --git a/compiler/rustc_borrowck/src/diagnostics/mod.rs b/compiler/rustc_borrowck/src/diagnostics/mod.rs index b31325485db..bb802c4eb46 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mod.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mod.rs @@ -1178,9 +1178,11 @@ fn explain_captures( } else { vec![(move_span.shrink_to_hi(), ".clone()".to_string())] }; - if let Some(errors) = - self.infcx.could_impl_trait(clone_trait, ty, self.param_env) - && !has_sugg + if let Some(errors) = self.infcx.type_implements_trait_shallow( + clone_trait, + ty, + self.param_env, + ) && !has_sugg { let msg = match &errors[..] { [] => "you can `clone` the value and consume it, but this \ diff --git a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs index 3b3d440df97..60164e84d54 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs @@ -1217,19 +1217,22 @@ fn visit_stmt(&mut self, s: &'tcx hir::Stmt<'tcx>) { { match self .infcx - .could_impl_trait(clone_trait, ty.peel_refs(), self.param_env) + .type_implements_trait_shallow( + clone_trait, + ty.peel_refs(), + self.param_env, + ) .as_deref() { Some([]) => { - // The type implements Clone. - err.span_help( - expr.span, - format!( - "you can `clone` the `{}` value and consume it, but this \ - might not be your desired behavior", - ty.peel_refs(), - ), - ); + // FIXME: This error message isn't useful, since we're just + // vaguely suggesting to clone a value that already + // implements `Clone`. + // + // A correct suggestion here would take into account the fact + // that inference may be affected by missing types on bindings, + // etc., to improve "tests/ui/borrowck/issue-91206.stderr", for + // example. } None => { if let hir::ExprKind::MethodCall(segment, _rcvr, [], span) = diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs index b542132d71c..6944b5eb176 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs @@ -1623,7 +1623,7 @@ pub(crate) fn note_type_is_not_clone( ); } else { if let Some(errors) = - self.could_impl_trait(clone_trait_did, expected_ty, self.param_env) + self.type_implements_trait_shallow(clone_trait_did, expected_ty, self.param_env) { match &errors[..] { [] => {} diff --git a/compiler/rustc_trait_selection/src/infer.rs b/compiler/rustc_trait_selection/src/infer.rs index 251f0628a71..ef4a0f52f9e 100644 --- a/compiler/rustc_trait_selection/src/infer.rs +++ b/compiler/rustc_trait_selection/src/infer.rs @@ -1,13 +1,14 @@ -use crate::solve::FulfillmentCtxt; use crate::traits::query::evaluate_obligation::InferCtxtExt as _; -use crate::traits::{self, DefiningAnchor, ObligationCtxt}; +use crate::traits::{self, DefiningAnchor, ObligationCtxt, SelectionContext}; +use crate::traits::TraitEngineExt as _; use rustc_hir::def_id::DefId; use rustc_hir::lang_items::LangItem; -use rustc_infer::traits::{TraitEngine, TraitEngineExt}; +use rustc_infer::traits::{Obligation, TraitEngine, TraitEngineExt as _}; use rustc_middle::arena::ArenaAllocatable; use rustc_middle::infer::canonical::{Canonical, CanonicalQueryResponse, QueryResponse}; use rustc_middle::traits::query::NoSolution; +use rustc_middle::traits::ObligationCause; use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable, TypeVisitableExt}; use rustc_middle::ty::{GenericArg, ToPredicate}; use rustc_span::DUMMY_SP; @@ -21,7 +22,8 @@ pub trait InferCtxtExt<'tcx> { fn type_is_sized_modulo_regions(&self, param_env: ty::ParamEnv<'tcx>, ty: Ty<'tcx>) -> bool; - /// Check whether a `ty` implements given trait(trait_def_id). + /// Check whether a `ty` implements given trait(trait_def_id) without side-effects. + /// /// The inputs are: /// /// - the def-id of the trait @@ -29,8 +31,8 @@ pub trait InferCtxtExt<'tcx> { /// - the parameter environment /// /// Invokes `evaluate_obligation`, so in the event that evaluating - /// `Ty: Trait` causes overflow, EvaluatedToErrStackDependent (or EvaluatedToAmbigStackDependent) - /// will be returned. + /// `Ty: Trait` causes overflow, EvaluatedToErrStackDependent + /// (or EvaluatedToAmbigStackDependent) will be returned. fn type_implements_trait( &self, trait_def_id: DefId, @@ -38,7 +40,18 @@ fn type_implements_trait( param_env: ty::ParamEnv<'tcx>, ) -> traits::EvaluationResult; - fn could_impl_trait( + /// Returns `Some` if a type implements a trait shallowly, without side-effects, + /// along with any errors that would have been reported upon further obligation + /// processing. + /// + /// - If this returns `Some([])`, then the trait holds modulo regions. + /// - If this returns `Some([errors..])`, then the trait has an impl for + /// the self type, but some nested obligations do not hold. + /// - If this returns `None`, no implementation that applies could be found. + /// + /// FIXME(-Znext-solver): Due to the recursive nature of the new solver, + /// this will probably only ever return `Some([])` or `None`. + fn type_implements_trait_shallow( &self, trait_def_id: DefId, ty: Ty<'tcx>, @@ -86,64 +99,26 @@ fn type_implements_trait( self.evaluate_obligation(&obligation).unwrap_or(traits::EvaluationResult::EvaluatedToErr) } - fn could_impl_trait( + fn type_implements_trait_shallow( &self, trait_def_id: DefId, ty: Ty<'tcx>, param_env: ty::ParamEnv<'tcx>, ) -> Option>> { self.probe(|_snapshot| { - if let ty::Adt(def, args) = ty.kind() - && let Some((impl_def_id, _)) = self - .tcx - .all_impls(trait_def_id) - .filter_map(|impl_def_id| { - self.tcx.impl_trait_ref(impl_def_id).map(|r| (impl_def_id, r)) - }) - .map(|(impl_def_id, imp)| (impl_def_id, imp.skip_binder())) - .find(|(_, imp)| match imp.self_ty().peel_refs().kind() { - ty::Adt(i_def, _) if i_def.did() == def.did() => true, - _ => false, - }) - { - let mut fulfill_cx = FulfillmentCtxt::new(self); - // We get all obligations from the impl to talk about specific - // trait bounds. - let obligations = self - .tcx - .predicates_of(impl_def_id) - .instantiate(self.tcx, args) - .into_iter() - .map(|(clause, span)| { - traits::Obligation::new( - self.tcx, - traits::ObligationCause::dummy_with_span(span), - param_env, - clause, - ) - }) - .collect::>(); - fulfill_cx.register_predicate_obligations(self, obligations); - let trait_ref = ty::TraitRef::new(self.tcx, trait_def_id, [ty]); - let obligation = traits::Obligation::new( - self.tcx, - traits::ObligationCause::dummy(), - param_env, - trait_ref, - ); - fulfill_cx.register_predicate_obligation(self, obligation); - let mut errors = fulfill_cx.select_all_or_error(self); - // We remove the last predicate failure, which corresponds to - // the top-level obligation, because most of the type we only - // care about the other ones, *except* when it is the only one. - // This seems to only be relevant for arbitrary self-types. - // Look at `tests/ui/moves/move-fn-self-receiver.rs`. - if errors.len() > 1 { - errors.truncate(errors.len() - 1); + let mut selcx = SelectionContext::new(self); + match selcx.select(&Obligation::new( + self.tcx, + ObligationCause::dummy(), + param_env, + ty::TraitRef::new(self.tcx, trait_def_id, [ty]), + )) { + Ok(Some(selection)) => { + let mut fulfill_cx = >::new(self); + fulfill_cx.register_predicate_obligations(self, selection.nested_obligations()); + Some(fulfill_cx.select_all_or_error(self)) } - Some(errors) - } else { - None + Ok(None) | Err(_) => None, } }) } diff --git a/tests/ui/borrowck/issue-119915-bad-clone-suggestion.rs b/tests/ui/borrowck/issue-119915-bad-clone-suggestion.rs new file mode 100644 index 00000000000..0b0ac9448db --- /dev/null +++ b/tests/ui/borrowck/issue-119915-bad-clone-suggestion.rs @@ -0,0 +1,28 @@ +use std::marker::PhantomData; + +struct Example(PhantomData<(fn(E), fn(FakeParam))>); + +struct NoLifetime; +struct Immutable<'a>(PhantomData<&'a ()>); + +impl<'a, E: 'a> Copy for Example> {} +impl<'a, E: 'a> Clone for Example> { + fn clone(&self) -> Self { + *self + } +} + +impl Example { + unsafe fn change(self) -> Example { + Example(PhantomData) + } +} + +impl Example { + fn the_ice(&mut self) -> Example> { + unsafe { self.change() } + //~^ ERROR cannot move out of `*self` which is behind a mutable reference + } +} + +fn main() {} diff --git a/tests/ui/borrowck/issue-119915-bad-clone-suggestion.stderr b/tests/ui/borrowck/issue-119915-bad-clone-suggestion.stderr new file mode 100644 index 00000000000..ab42205d510 --- /dev/null +++ b/tests/ui/borrowck/issue-119915-bad-clone-suggestion.stderr @@ -0,0 +1,17 @@ +error[E0507]: cannot move out of `*self` which is behind a mutable reference + --> $DIR/issue-119915-bad-clone-suggestion.rs:23:18 + | +LL | unsafe { self.change() } + | ^^^^ -------- `*self` moved due to this method call + | | + | move occurs because `*self` has type `Example`, which does not implement the `Copy` trait + | +note: `Example::::change` takes ownership of the receiver `self`, which moves `*self` + --> $DIR/issue-119915-bad-clone-suggestion.rs:16:36 + | +LL | unsafe fn change(self) -> Example { + | ^^^^ + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0507`. diff --git a/tests/ui/borrowck/issue-85765-closure.rs b/tests/ui/borrowck/issue-85765-closure.rs index edc9eeaffb5..f2d1dd0fbc3 100644 --- a/tests/ui/borrowck/issue-85765-closure.rs +++ b/tests/ui/borrowck/issue-85765-closure.rs @@ -3,7 +3,6 @@ fn main() { let mut test = Vec::new(); let rofl: &Vec> = &mut test; //~^ HELP consider changing this binding's type - //~| HELP you can `clone` the `Vec>` value and consume it, but this might not be your desired behavior rofl.push(Vec::new()); //~^ ERROR cannot borrow `*rofl` as mutable, as it is behind a `&` reference //~| NOTE `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable diff --git a/tests/ui/borrowck/issue-85765-closure.stderr b/tests/ui/borrowck/issue-85765-closure.stderr index 4a6a0e94bec..936ddd67bcd 100644 --- a/tests/ui/borrowck/issue-85765-closure.stderr +++ b/tests/ui/borrowck/issue-85765-closure.stderr @@ -1,21 +1,16 @@ error[E0596]: cannot borrow `*rofl` as mutable, as it is behind a `&` reference - --> $DIR/issue-85765-closure.rs:7:9 + --> $DIR/issue-85765-closure.rs:6:9 | LL | rofl.push(Vec::new()); | ^^^^ `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable | -help: you can `clone` the `Vec>` value and consume it, but this might not be your desired behavior - --> $DIR/issue-85765-closure.rs:4:36 - | -LL | let rofl: &Vec> = &mut test; - | ^^^^^^^^^ help: consider changing this binding's type | LL | let rofl: &mut Vec> = &mut test; | ~~~~~~~~~~~~~~~~~~ error[E0594]: cannot assign to `*r`, which is behind a `&` reference - --> $DIR/issue-85765-closure.rs:14:9 + --> $DIR/issue-85765-closure.rs:13:9 | LL | *r = 0; | ^^^^^^ `r` is a `&` reference, so the data it refers to cannot be written @@ -26,7 +21,7 @@ LL | let r = &mut mutvar; | +++ error[E0594]: cannot assign to `*x`, which is behind a `&` reference - --> $DIR/issue-85765-closure.rs:21:9 + --> $DIR/issue-85765-closure.rs:20:9 | LL | *x = 1; | ^^^^^^ `x` is a `&` reference, so the data it refers to cannot be written @@ -37,7 +32,7 @@ LL | let x: &mut usize = &mut{0}; | ~~~~~~~~~~ error[E0594]: cannot assign to `*y`, which is behind a `&` reference - --> $DIR/issue-85765-closure.rs:28:9 + --> $DIR/issue-85765-closure.rs:27:9 | LL | *y = 1; | ^^^^^^ `y` is a `&` reference, so the data it refers to cannot be written diff --git a/tests/ui/borrowck/issue-85765.rs b/tests/ui/borrowck/issue-85765.rs index ce5740bc0e7..76e0b517354 100644 --- a/tests/ui/borrowck/issue-85765.rs +++ b/tests/ui/borrowck/issue-85765.rs @@ -2,7 +2,6 @@ fn main() { let mut test = Vec::new(); let rofl: &Vec> = &mut test; //~^ HELP consider changing this binding's type - //~| HELP you can `clone` the `Vec>` value and consume it, but this might not be your desired behavior rofl.push(Vec::new()); //~^ ERROR cannot borrow `*rofl` as mutable, as it is behind a `&` reference //~| NOTE `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable diff --git a/tests/ui/borrowck/issue-85765.stderr b/tests/ui/borrowck/issue-85765.stderr index 4889f774afa..57900bfb612 100644 --- a/tests/ui/borrowck/issue-85765.stderr +++ b/tests/ui/borrowck/issue-85765.stderr @@ -1,21 +1,16 @@ error[E0596]: cannot borrow `*rofl` as mutable, as it is behind a `&` reference - --> $DIR/issue-85765.rs:6:5 + --> $DIR/issue-85765.rs:5:5 | LL | rofl.push(Vec::new()); | ^^^^ `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable | -help: you can `clone` the `Vec>` value and consume it, but this might not be your desired behavior - --> $DIR/issue-85765.rs:3:32 - | -LL | let rofl: &Vec> = &mut test; - | ^^^^^^^^^ help: consider changing this binding's type | LL | let rofl: &mut Vec> = &mut test; | ~~~~~~~~~~~~~~~~~~ error[E0594]: cannot assign to `*r`, which is behind a `&` reference - --> $DIR/issue-85765.rs:13:5 + --> $DIR/issue-85765.rs:12:5 | LL | *r = 0; | ^^^^^^ `r` is a `&` reference, so the data it refers to cannot be written @@ -26,7 +21,7 @@ LL | let r = &mut mutvar; | +++ error[E0594]: cannot assign to `*x`, which is behind a `&` reference - --> $DIR/issue-85765.rs:20:5 + --> $DIR/issue-85765.rs:19:5 | LL | *x = 1; | ^^^^^^ `x` is a `&` reference, so the data it refers to cannot be written @@ -37,7 +32,7 @@ LL | let x: &mut usize = &mut{0}; | ~~~~~~~~~~ error[E0594]: cannot assign to `*y`, which is behind a `&` reference - --> $DIR/issue-85765.rs:27:5 + --> $DIR/issue-85765.rs:26:5 | LL | *y = 1; | ^^^^^^ `y` is a `&` reference, so the data it refers to cannot be written diff --git a/tests/ui/borrowck/issue-91206.rs b/tests/ui/borrowck/issue-91206.rs index c60ac62fa34..e062a253767 100644 --- a/tests/ui/borrowck/issue-91206.rs +++ b/tests/ui/borrowck/issue-91206.rs @@ -10,7 +10,6 @@ fn main() { let client = TestClient; let inner = client.get_inner_ref(); //~^ HELP consider specifying this binding's type - //~| HELP you can `clone` the `Vec` value and consume it, but this might not be your desired behavior inner.clear(); //~^ ERROR cannot borrow `*inner` as mutable, as it is behind a `&` reference [E0596] //~| NOTE `inner` is a `&` reference, so the data it refers to cannot be borrowed as mutable diff --git a/tests/ui/borrowck/issue-91206.stderr b/tests/ui/borrowck/issue-91206.stderr index e3dd65b6419..f96b0c7d9e1 100644 --- a/tests/ui/borrowck/issue-91206.stderr +++ b/tests/ui/borrowck/issue-91206.stderr @@ -1,14 +1,9 @@ error[E0596]: cannot borrow `*inner` as mutable, as it is behind a `&` reference - --> $DIR/issue-91206.rs:14:5 + --> $DIR/issue-91206.rs:13:5 | LL | inner.clear(); | ^^^^^ `inner` is a `&` reference, so the data it refers to cannot be borrowed as mutable | -help: you can `clone` the `Vec` value and consume it, but this might not be your desired behavior - --> $DIR/issue-91206.rs:11:17 - | -LL | let inner = client.get_inner_ref(); - | ^^^^^^^^^^^^^^^^^^^^^^ help: consider specifying this binding's type | LL | let inner: &mut Vec = client.get_inner_ref(); diff --git a/tests/ui/moves/move-fn-self-receiver.stderr b/tests/ui/moves/move-fn-self-receiver.stderr index 462deacbe8d..17f48f5f7bf 100644 --- a/tests/ui/moves/move-fn-self-receiver.stderr +++ b/tests/ui/moves/move-fn-self-receiver.stderr @@ -55,10 +55,15 @@ note: `Foo::use_box_self` takes ownership of the receiver `self`, which moves `b | LL | fn use_box_self(self: Box) {} | ^^^^ -help: you could `clone` the value and consume it, if the `Box: Clone` trait bound could be satisfied +help: you could `clone` the value and consume it, if the `Foo: Clone` trait bound could be satisfied | LL | boxed_foo.clone().use_box_self(); | ++++++++ +help: consider annotating `Foo` with `#[derive(Clone)]` + | +LL + #[derive(Clone)] +LL | struct Foo; + | error[E0382]: use of moved value: `pin_box_foo` --> $DIR/move-fn-self-receiver.rs:46:5 @@ -75,10 +80,15 @@ note: `Foo::use_pin_box_self` takes ownership of the receiver `self`, which move | LL | fn use_pin_box_self(self: Pin>) {} | ^^^^ -help: you could `clone` the value and consume it, if the `Box: Clone` trait bound could be satisfied +help: you could `clone` the value and consume it, if the `Foo: Clone` trait bound could be satisfied | LL | pin_box_foo.clone().use_pin_box_self(); | ++++++++ +help: consider annotating `Foo` with `#[derive(Clone)]` + | +LL + #[derive(Clone)] +LL | struct Foo; + | error[E0505]: cannot move out of `mut_foo` because it is borrowed --> $DIR/move-fn-self-receiver.rs:50:5 From 7724ba7bd5f2c947a7dc561c24b19bb8409a6ed6 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sat, 13 Jan 2024 21:53:42 +0000 Subject: [PATCH 2/2] assert that trait solver is only created in proper infcx --- compiler/rustc_trait_selection/src/solve/fulfill.rs | 5 +++++ compiler/rustc_trait_selection/src/traits/fulfill.rs | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/compiler/rustc_trait_selection/src/solve/fulfill.rs b/compiler/rustc_trait_selection/src/solve/fulfill.rs index 2139210b873..09f4f3e9702 100644 --- a/compiler/rustc_trait_selection/src/solve/fulfill.rs +++ b/compiler/rustc_trait_selection/src/solve/fulfill.rs @@ -36,6 +36,11 @@ pub struct FulfillmentCtxt<'tcx> { impl<'tcx> FulfillmentCtxt<'tcx> { pub fn new(infcx: &InferCtxt<'tcx>) -> FulfillmentCtxt<'tcx> { + assert!( + infcx.next_trait_solver(), + "new trait solver fulfillment context created when \ + infcx is set up for old trait solver" + ); FulfillmentCtxt { obligations: Vec::new(), usable_in_snapshot: infcx.num_open_snapshots() } } } diff --git a/compiler/rustc_trait_selection/src/traits/fulfill.rs b/compiler/rustc_trait_selection/src/traits/fulfill.rs index 045d7e444b6..472342f9898 100644 --- a/compiler/rustc_trait_selection/src/traits/fulfill.rs +++ b/compiler/rustc_trait_selection/src/traits/fulfill.rs @@ -80,6 +80,11 @@ pub struct PendingPredicateObligation<'tcx> { impl<'tcx> FulfillmentContext<'tcx> { /// Creates a new fulfillment context. pub(super) fn new(infcx: &InferCtxt<'tcx>) -> FulfillmentContext<'tcx> { + assert!( + !infcx.next_trait_solver(), + "old trait solver fulfillment context created when \ + infcx is set up for new trait solver" + ); FulfillmentContext { predicates: ObligationForest::new(), usable_in_snapshot: infcx.num_open_snapshots(),