From 6a3176ca4ce3c59de2ece1c3240daade887da024 Mon Sep 17 00:00:00 2001 From: lcnr Date: Fri, 24 Mar 2023 10:28:50 +0100 Subject: [PATCH 1/2] do not allow inference in `predicate_must_hold` --- compiler/rustc_infer/src/infer/mod.rs | 128 ++++++++++-------- compiler/rustc_middle/src/ty/sty.rs | 4 +- .../rustc_trait_selection/src/traits/mod.rs | 56 +------- .../src/traits/query/evaluate_obligation.rs | 18 +-- .../src/traits/select/mod.rs | 11 +- tests/ui/suggestions/into-convert.rs | 1 - tests/ui/suggestions/into-convert.stderr | 5 - tests/ui/traits/copy-guessing.rs | 8 +- tests/ui/traits/copy-guessing.stderr | 23 ++++ 9 files changed, 120 insertions(+), 134 deletions(-) create mode 100644 tests/ui/traits/copy-guessing.stderr diff --git a/compiler/rustc_infer/src/infer/mod.rs b/compiler/rustc_infer/src/infer/mod.rs index aeb4ddb421259..776bf3dc81f12 100644 --- a/compiler/rustc_infer/src/infer/mod.rs +++ b/compiler/rustc_infer/src/infer/mod.rs @@ -1603,6 +1603,73 @@ impl<'tcx> InferCtxt<'tcx> { } } + /// Replaces substs that reference param or infer variables with suitable + /// placeholders. This function is meant to remove these param and infer + /// substs when they're not actually needed to evaluate a constant. + /// + /// This loses information about inference variables so it should only be used + /// if incorrectly failing to prove or equate something is acceptable. + pub fn replace_infer_vars_with_placeholders>>( + &self, + value: T, + ) -> T { + struct ReplaceInferVarsWithPlaceholders<'a, 'tcx> { + infcx: &'a InferCtxt<'tcx>, + map: FxHashMap, u32>, + } + + impl<'a, 'tcx> TypeFolder> for ReplaceInferVarsWithPlaceholders<'a, 'tcx> { + fn interner(&self) -> TyCtxt<'tcx> { + self.infcx.tcx + } + + fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> { + // This replaces integer and float inference vars with placeholders, losing + // information. + if let ty::Infer(_) = t.kind() { + let new_idx = self.map.len() as u32; + let idx = *self.map.entry(t.into()).or_insert(new_idx); + self.interner().mk_placeholder(ty::PlaceholderType { + universe: ty::UniverseIndex::ROOT, + name: ty::BoundTyKind::Anon(idx), + }) + } else { + t.super_fold_with(self) + } + } + + fn fold_const(&mut self, c: ty::Const<'tcx>) -> ty::Const<'tcx> { + if let ty::ConstKind::Infer(_) = c.kind() { + let ty = c.ty(); + if !ty.is_global() { + bug!("const `{c}`'s type should not reference params or types"); + } + let new_idx = self.map.len() as u32; + let idx = *self.map.entry(c.into()).or_insert(new_idx); + self.interner().mk_const( + ty::PlaceholderConst { + universe: ty::UniverseIndex::ROOT, + name: ty::BoundVar::from_u32(idx), + }, + ty, + ) + } else { + c.super_fold_with(self) + } + } + } + + let value = self.resolve_vars_if_possible(value); + if value.has_non_region_infer() { + value.fold_with(&mut ReplaceInferVarsWithPlaceholders { + infcx: self, + map: Default::default(), + }) + } else { + value + } + } + /// Resolves and evaluates a constant. /// /// The constant can be located on a trait like `::C`, in which case the given @@ -1636,7 +1703,7 @@ impl<'tcx> InferCtxt<'tcx> { } else if ct.has_non_region_infer() || ct.has_non_region_param() { return Err(ErrorHandled::TooGeneric); } else { - substs = replace_param_and_infer_substs_with_placeholder(tcx, substs); + substs = self.replace_infer_vars_with_placeholders(substs); } } else { substs = InternalSubsts::identity_for_item(tcx, unevaluated.def.did); @@ -2087,62 +2154,3 @@ impl RegionVariableOrigin { } } } - -/// Replaces substs that reference param or infer variables with suitable -/// placeholders. This function is meant to remove these param and infer -/// substs when they're not actually needed to evaluate a constant. -fn replace_param_and_infer_substs_with_placeholder<'tcx>( - tcx: TyCtxt<'tcx>, - substs: SubstsRef<'tcx>, -) -> SubstsRef<'tcx> { - struct ReplaceParamAndInferWithPlaceholder<'tcx> { - tcx: TyCtxt<'tcx>, - idx: u32, - } - - impl<'tcx> TypeFolder> for ReplaceParamAndInferWithPlaceholder<'tcx> { - fn interner(&self) -> TyCtxt<'tcx> { - self.tcx - } - - fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> { - if let ty::Infer(_) = t.kind() { - self.tcx.mk_placeholder(ty::PlaceholderType { - universe: ty::UniverseIndex::ROOT, - name: ty::BoundTyKind::Anon({ - let idx = self.idx; - self.idx += 1; - idx - }), - }) - } else { - t.super_fold_with(self) - } - } - - fn fold_const(&mut self, c: ty::Const<'tcx>) -> ty::Const<'tcx> { - if let ty::ConstKind::Infer(_) = c.kind() { - let ty = c.ty(); - // If the type references param or infer then ICE ICE ICE - if ty.has_non_region_param() || ty.has_non_region_infer() { - bug!("const `{c}`'s type should not reference params or types"); - } - self.tcx.mk_const( - ty::PlaceholderConst { - universe: ty::UniverseIndex::ROOT, - name: ty::BoundVar::from_u32({ - let idx = self.idx; - self.idx += 1; - idx - }), - }, - ty, - ) - } else { - c.super_fold_with(self) - } - } - } - - substs.fold_with(&mut ReplaceParamAndInferWithPlaceholder { tcx, idx: 0 }) -} diff --git a/compiler/rustc_middle/src/ty/sty.rs b/compiler/rustc_middle/src/ty/sty.rs index 2a0536a1af72d..eecf60c383ec2 100644 --- a/compiler/rustc_middle/src/ty/sty.rs +++ b/compiler/rustc_middle/src/ty/sty.rs @@ -2339,9 +2339,7 @@ impl<'tcx> Ty<'tcx> { _ => bug!("cannot convert type `{:?}` to a closure kind", self), }, - // "Bound" types appear in canonical queries when the - // closure type is not yet known - Bound(..) | Infer(_) => None, + Infer(_) | Placeholder(_) => None, Error(_) => Some(ty::ClosureKind::Fn), diff --git a/compiler/rustc_trait_selection/src/traits/mod.rs b/compiler/rustc_trait_selection/src/traits/mod.rs index b27a39290781a..7315baaba63f1 100644 --- a/compiler/rustc_trait_selection/src/traits/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/mod.rs @@ -119,61 +119,19 @@ pub fn predicates_for_generics<'tcx>( }) } -/// Determines whether the type `ty` is known to meet `bound` and -/// returns true if so. Returns false if `ty` either does not meet -/// `bound` or is not known to meet bound (note that this is -/// conservative towards *no impl*, which is the opposite of the -/// `evaluate` methods). +/// Returns whether the type `ty` is known to meet `bound`. pub fn type_known_to_meet_bound_modulo_regions<'tcx>( infcx: &InferCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>, ty: Ty<'tcx>, def_id: DefId, ) -> bool { - let trait_ref = ty::Binder::dummy(infcx.tcx.mk_trait_ref(def_id, [ty])); - pred_known_to_hold_modulo_regions(infcx, param_env, trait_ref.without_const()) -} - -/// FIXME(@lcnr): this function doesn't seem right and shouldn't exist? -/// -/// Ping me on zulip if you want to use this method and need help with finding -/// an appropriate replacement. -#[instrument(level = "debug", skip(infcx, param_env, pred), ret)] -fn pred_known_to_hold_modulo_regions<'tcx>( - infcx: &InferCtxt<'tcx>, - param_env: ty::ParamEnv<'tcx>, - pred: impl ToPredicate<'tcx> + TypeVisitable>, -) -> bool { - let has_non_region_infer = pred.has_non_region_infer(); - let obligation = Obligation::new(infcx.tcx, ObligationCause::dummy(), param_env, pred); - - let result = infcx.evaluate_obligation_no_overflow(&obligation); - debug!(?result); - - if result.must_apply_modulo_regions() && !has_non_region_infer { - true - } else if result.may_apply() { - // Because of inference "guessing", selection can sometimes claim - // to succeed while the success requires a guess. To ensure - // this function's result remains infallible, we must confirm - // that guess. While imperfect, I believe this is sound. - - // The handling of regions in this area of the code is terrible, - // see issue #29149. We should be able to improve on this with - // NLL. - let ocx = ObligationCtxt::new(infcx); - ocx.register_obligation(obligation); - let errors = ocx.select_all_or_error(); - match errors.as_slice() { - [] => true, - errors => { - debug!(?errors); - false - } - } - } else { - false - } + infcx.predicate_must_hold_modulo_regions(&Obligation::new( + infcx.tcx, + ObligationCause::dummy(), + param_env, + infcx.tcx.mk_trait_ref(def_id, [ty]), + )) } #[instrument(level = "debug", skip(tcx, elaborated_env))] diff --git a/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs b/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs index f84b2f4428d1a..83c0997935905 100644 --- a/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs +++ b/compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs @@ -39,8 +39,8 @@ impl<'tcx> InferCtxtExt<'tcx> for InferCtxt<'tcx> { } /// Evaluates whether the predicate can be satisfied in the given - /// `ParamEnv`, and returns `false` if not certain. However, this is - /// not entirely accurate if inference variables are involved. + /// `ParamEnv`, and returns `false` if not certain. If this returns + /// true then the given `obligation` is guaranteed to hold. /// /// This version may conservatively fail when outlives obligations /// are required. @@ -48,16 +48,18 @@ impl<'tcx> InferCtxtExt<'tcx> for InferCtxt<'tcx> { &self, obligation: &PredicateObligation<'tcx>, ) -> bool { - self.evaluate_obligation_no_overflow(obligation).must_apply_considering_regions() + let obligation = obligation + .with(self.tcx, self.replace_infer_vars_with_placeholders(obligation.predicate)); + self.evaluate_obligation_no_overflow(&obligation).must_apply_considering_regions() } /// Evaluates whether the predicate can be satisfied in the given - /// `ParamEnv`, and returns `false` if not certain. However, this is - /// not entirely accurate if inference variables are involved. - /// - /// This version ignores all outlives constraints. + /// `ParamEnv`. If this returns true then the given `obligation` is + /// guaranteed to hold except for region constraints. fn predicate_must_hold_modulo_regions(&self, obligation: &PredicateObligation<'tcx>) -> bool { - self.evaluate_obligation_no_overflow(obligation).must_apply_modulo_regions() + let obligation = obligation + .with(self.tcx, self.replace_infer_vars_with_placeholders(obligation.predicate)); + self.evaluate_obligation_no_overflow(&obligation).must_apply_modulo_regions() } /// Evaluate a given predicate, capturing overflow and propagating it back. diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index b8758ad93231d..affb0fd4fe3cb 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -2250,11 +2250,12 @@ impl<'tcx> SelectionContext<'_, 'tcx> { ty::Closure(_, substs) => { // (*) binder moved here let ty = self.infcx.shallow_resolve(substs.as_closure().tupled_upvars_ty()); - if let ty::Infer(ty::TyVar(_)) = ty.kind() { - // Not yet resolved. - Ambiguous - } else { - Where(obligation.predicate.rebind(substs.as_closure().upvar_tys().collect())) + match ty.kind() { + ty::Infer(ty::TyVar(_)) => Ambiguous, + ty::Placeholder(_) => None, + _ => Where( + obligation.predicate.rebind(substs.as_closure().upvar_tys().collect()), + ), } } diff --git a/tests/ui/suggestions/into-convert.rs b/tests/ui/suggestions/into-convert.rs index 1c9a9e0aaf561..cdf40c70def23 100644 --- a/tests/ui/suggestions/into-convert.rs +++ b/tests/ui/suggestions/into-convert.rs @@ -13,7 +13,6 @@ fn main() { let z: AtomicU32 = 1; //~^ ERROR mismatched types - //~| HELP call `Into::into` on this expression to convert `{integer}` into `AtomicU32` } struct A; diff --git a/tests/ui/suggestions/into-convert.stderr b/tests/ui/suggestions/into-convert.stderr index 704b280a985f4..67f4542536fb5 100644 --- a/tests/ui/suggestions/into-convert.stderr +++ b/tests/ui/suggestions/into-convert.stderr @@ -33,11 +33,6 @@ LL | let z: AtomicU32 = 1; | --------- ^ expected `AtomicU32`, found integer | | | expected due to this - | -help: call `Into::into` on this expression to convert `{integer}` into `AtomicU32` - | -LL | let z: AtomicU32 = 1.into(); - | +++++++ error: aborting due to 3 previous errors diff --git a/tests/ui/traits/copy-guessing.rs b/tests/ui/traits/copy-guessing.rs index f031dd9ca48f6..ec6e111e15456 100644 --- a/tests/ui/traits/copy-guessing.rs +++ b/tests/ui/traits/copy-guessing.rs @@ -1,4 +1,3 @@ -// run-pass #![allow(dead_code)] // "guessing" in trait selection can affect `copy_or_move`. Check that this // is correctly handled. I am not sure what is the "correct" behaviour, @@ -10,7 +9,10 @@ struct U([u8; 1337]); struct S<'a,T:'a>(&'a T); impl<'a, T> Clone for S<'a, T> { fn clone(&self) -> Self { S(self.0) } } -/// This impl triggers inference "guessing" - S<_>: Copy => _ = U +/// This impl could trigger inference "guessing" - S<_>: Copy => _ = U +/// +/// The current behavior is that due to inference fallback we end up with +/// `S>` instead. impl<'a> Copy for S<'a, Option> {} fn assert_impls_fnR>(_: &T){} @@ -18,7 +20,7 @@ fn assert_impls_fnR>(_: &T){} fn main() { let n = None; let e = S(&n); - let f = || { + let f = || { //~ ERROR expected a closure that implements the `Fn` trait // S being copy is critical for this to work drop(e); mem::size_of_val(e.0) diff --git a/tests/ui/traits/copy-guessing.stderr b/tests/ui/traits/copy-guessing.stderr new file mode 100644 index 0000000000000..50c0ea1206c70 --- /dev/null +++ b/tests/ui/traits/copy-guessing.stderr @@ -0,0 +1,23 @@ +error[E0525]: expected a closure that implements the `Fn` trait, but this closure only implements `FnOnce` + --> $DIR/copy-guessing.rs:23:13 + | +LL | let f = || { + | ^^ this closure implements `FnOnce`, not `Fn` +LL | // S being copy is critical for this to work +LL | drop(e); + | - closure is `FnOnce` because it moves the variable `e` out of its environment +... +LL | assert_impls_fn(&f); + | --------------- -- the requirement to implement `Fn` derives from here + | | + | required by a bound introduced by this call + | +note: required by a bound in `assert_impls_fn` + --> $DIR/copy-guessing.rs:18:25 + | +LL | fn assert_impls_fnR>(_: &T){} + | ^^^^^^^ required by this bound in `assert_impls_fn` + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0525`. From ff9e7e2233fbc60e6681c8eb3f93545e113d39a2 Mon Sep 17 00:00:00 2001 From: lcnr Date: Fri, 24 Mar 2023 11:36:41 +0100 Subject: [PATCH 2/2] update clippy --- src/tools/clippy/tests/ui/crashes/ice-6250.stderr | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/tools/clippy/tests/ui/crashes/ice-6250.stderr b/src/tools/clippy/tests/ui/crashes/ice-6250.stderr index 4506d1550bd4b..db34e6bfa7b2f 100644 --- a/src/tools/clippy/tests/ui/crashes/ice-6250.stderr +++ b/src/tools/clippy/tests/ui/crashes/ice-6250.stderr @@ -12,11 +12,6 @@ LL | for reference in vec![1, 2, 3] { ... LL | Some(reference) = cache.data.get(key) { | ^^^^^^^^^ expected integer, found `&i32` - | -help: consider dereferencing the borrow - | -LL | Some(*reference) = cache.data.get(key) { - | + error[E0308]: mismatched types --> $DIR/ice-6250.rs:12:9