From 54571407b2ac4bd29f6509b680a19b49d7fbaa16 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 4 Jan 2023 20:55:37 +0000 Subject: [PATCH 1/2] Bump IMPLIED_BOUNDS_ENTAILMENT to Deny + ReportNow --- compiler/rustc_lint_defs/src/builtin.rs | 4 ++-- ...plied-bounds-compatibility-unnormalized.stderr | 15 +++++++++++++++ .../impl-implied-bounds-compatibility.stderr | 15 +++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 28317d6cea02a..6cdf50970836a 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -4033,10 +4033,10 @@ declare_lint! { /// /// This can be used to implement an unsound API if used incorrectly. pub IMPLIED_BOUNDS_ENTAILMENT, - Warn, + Deny, "impl method assumes more implied bounds than its corresponding trait method", @future_incompatible = FutureIncompatibleInfo { reference: "issue #105572 ", - reason: FutureIncompatibilityReason::FutureReleaseError, + reason: FutureIncompatibilityReason::FutureReleaseErrorReportNow, }; } diff --git a/tests/ui/implied-bounds/impl-implied-bounds-compatibility-unnormalized.stderr b/tests/ui/implied-bounds/impl-implied-bounds-compatibility-unnormalized.stderr index 0ac31c642eb12..961ff573e5040 100644 --- a/tests/ui/implied-bounds/impl-implied-bounds-compatibility-unnormalized.stderr +++ b/tests/ui/implied-bounds/impl-implied-bounds-compatibility-unnormalized.stderr @@ -14,3 +14,18 @@ LL | #![deny(implied_bounds_entailment)] error: aborting due to previous error +Future incompatibility report: Future breakage diagnostic: +error: impl method assumes more implied bounds than the corresponding trait method + --> $DIR/impl-implied-bounds-compatibility-unnormalized.rs:13:5 + | +LL | fn get<'s>(s: &'s str, _: <&'static &'s () as Project>::Ty) -> &'static str { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #105572 +note: the lint level is defined here + --> $DIR/impl-implied-bounds-compatibility-unnormalized.rs:1:9 + | +LL | #![deny(implied_bounds_entailment)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + diff --git a/tests/ui/implied-bounds/impl-implied-bounds-compatibility.stderr b/tests/ui/implied-bounds/impl-implied-bounds-compatibility.stderr index 0dfa8167a9966..4841f2b179932 100644 --- a/tests/ui/implied-bounds/impl-implied-bounds-compatibility.stderr +++ b/tests/ui/implied-bounds/impl-implied-bounds-compatibility.stderr @@ -14,3 +14,18 @@ LL | #![deny(implied_bounds_entailment)] error: aborting due to previous error +Future incompatibility report: Future breakage diagnostic: +error: impl method assumes more implied bounds than the corresponding trait method + --> $DIR/impl-implied-bounds-compatibility.rs:14:5 + | +LL | fn listeners<'b>(&'b self) -> &'a MessageListeners<'b> { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #105572 +note: the lint level is defined here + --> $DIR/impl-implied-bounds-compatibility.rs:1:9 + | +LL | #![deny(implied_bounds_entailment)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + From eaa7cc84d358a0113c063d445e5d0d7caeeea94c Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 12 Jan 2023 20:43:44 +0000 Subject: [PATCH 2/2] Add logic to make IMPLIED_BOUNDS_ENTAILMENT easier to understand --- .../src/check/compare_impl_item.rs | 160 ++++++++++++++++-- ...d-bounds-compatibility-unnormalized.stderr | 8 +- .../impl-implied-bounds-compatibility.stderr | 8 +- 3 files changed, 156 insertions(+), 20 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/check/compare_impl_item.rs b/compiler/rustc_hir_analysis/src/check/compare_impl_item.rs index 7af89934d1420..2cdf75794713f 100644 --- a/compiler/rustc_hir_analysis/src/check/compare_impl_item.rs +++ b/compiler/rustc_hir_analysis/src/check/compare_impl_item.rs @@ -2,7 +2,9 @@ use super::potentially_plural_count; use crate::errors::LifetimesOrBoundsMismatchOnTrait; use hir::def_id::{DefId, LocalDefId}; use rustc_data_structures::fx::{FxHashMap, FxIndexSet}; -use rustc_errors::{pluralize, struct_span_err, Applicability, DiagnosticId, ErrorGuaranteed}; +use rustc_errors::{ + pluralize, struct_span_err, Applicability, DiagnosticId, ErrorGuaranteed, MultiSpan, +}; use rustc_hir as hir; use rustc_hir::def::{DefKind, Res}; use rustc_hir::intravisit; @@ -320,15 +322,6 @@ fn compare_method_predicate_entailment<'tcx>( ty::Binder::dummy(ty::PredicateKind::WellFormed(unnormalized_impl_fty.into())), )); } - let emit_implied_wf_lint = || { - infcx.tcx.struct_span_lint_hir( - rustc_session::lint::builtin::IMPLIED_BOUNDS_ENTAILMENT, - impl_m_hir_id, - infcx.tcx.def_span(impl_m.def_id), - "impl method assumes more implied bounds than the corresponding trait method", - |lint| lint, - ); - }; // Check that all obligations are satisfied by the implementation's // version. @@ -346,7 +339,7 @@ fn compare_method_predicate_entailment<'tcx>( ) .map(|()| { // If the skip-mode was successful, emit a lint. - emit_implied_wf_lint(); + emit_implied_wf_lint(infcx.tcx, impl_m, impl_m_hir_id, vec![]); }); } CheckImpliedWfMode::Skip => { @@ -382,8 +375,16 @@ fn compare_method_predicate_entailment<'tcx>( CheckImpliedWfMode::Skip, ) .map(|()| { + let bad_args = extract_bad_args_for_implies_lint( + tcx, + &errors, + (trait_m, trait_sig), + // Unnormalized impl sig corresponds to the HIR types written + (impl_m, unnormalized_impl_sig), + impl_m_hir_id, + ); // If the skip-mode was successful, emit a lint. - emit_implied_wf_lint(); + emit_implied_wf_lint(tcx, impl_m, impl_m_hir_id, bad_args); }); } CheckImpliedWfMode::Skip => { @@ -400,6 +401,141 @@ fn compare_method_predicate_entailment<'tcx>( Ok(()) } +fn extract_bad_args_for_implies_lint<'tcx>( + tcx: TyCtxt<'tcx>, + errors: &[infer::RegionResolutionError<'tcx>], + (trait_m, trait_sig): (&ty::AssocItem, ty::FnSig<'tcx>), + (impl_m, impl_sig): (&ty::AssocItem, ty::FnSig<'tcx>), + hir_id: hir::HirId, +) -> Vec<(Span, Option)> { + let mut blame_generics = vec![]; + for error in errors { + // Look for the subregion origin that contains an input/output type + let origin = match error { + infer::RegionResolutionError::ConcreteFailure(o, ..) => o, + infer::RegionResolutionError::GenericBoundFailure(o, ..) => o, + infer::RegionResolutionError::SubSupConflict(_, _, o, ..) => o, + infer::RegionResolutionError::UpperBoundUniverseConflict(.., o, _) => o, + }; + // Extract (possible) input/output types from origin + match origin { + infer::SubregionOrigin::Subtype(trace) => { + if let Some((a, b)) = trace.values.ty() { + blame_generics.extend([a, b]); + } + } + infer::SubregionOrigin::RelateParamBound(_, ty, _) => blame_generics.push(*ty), + infer::SubregionOrigin::ReferenceOutlivesReferent(ty, _) => blame_generics.push(*ty), + _ => {} + } + } + + let fn_decl = tcx.hir().fn_decl_by_hir_id(hir_id).unwrap(); + let opt_ret_ty = match fn_decl.output { + hir::FnRetTy::DefaultReturn(_) => None, + hir::FnRetTy::Return(ty) => Some(ty), + }; + + // Map late-bound regions from trait to impl, so the names are right. + let mapping = std::iter::zip( + tcx.fn_sig(trait_m.def_id).bound_vars(), + tcx.fn_sig(impl_m.def_id).bound_vars(), + ) + .filter_map(|(impl_bv, trait_bv)| { + if let ty::BoundVariableKind::Region(impl_bv) = impl_bv + && let ty::BoundVariableKind::Region(trait_bv) = trait_bv + { + Some((impl_bv, trait_bv)) + } else { + None + } + }) + .collect(); + + // For each arg, see if it was in the "blame" of any of the region errors. + // If so, then try to produce a suggestion to replace the argument type with + // one from the trait. + let mut bad_args = vec![]; + for (idx, (ty, hir_ty)) in + std::iter::zip(impl_sig.inputs_and_output, fn_decl.inputs.iter().chain(opt_ret_ty)) + .enumerate() + { + let expected_ty = trait_sig.inputs_and_output[idx] + .fold_with(&mut RemapLateBound { tcx, mapping: &mapping }); + if blame_generics.iter().any(|blame| ty.contains(*blame)) { + let expected_ty_sugg = expected_ty.to_string(); + bad_args.push(( + hir_ty.span, + // Only suggest something if it actually changed. + (expected_ty_sugg != ty.to_string()).then_some(expected_ty_sugg), + )); + } + } + + bad_args +} + +struct RemapLateBound<'a, 'tcx> { + tcx: TyCtxt<'tcx>, + mapping: &'a FxHashMap, +} + +impl<'tcx> TypeFolder<'tcx> for RemapLateBound<'_, 'tcx> { + fn tcx(&self) -> TyCtxt<'tcx> { + self.tcx + } + + fn fold_region(&mut self, r: ty::Region<'tcx>) -> ty::Region<'tcx> { + if let ty::ReFree(fr) = *r { + self.tcx.mk_region(ty::ReFree(ty::FreeRegion { + bound_region: self + .mapping + .get(&fr.bound_region) + .copied() + .unwrap_or(fr.bound_region), + ..fr + })) + } else { + r + } + } +} + +fn emit_implied_wf_lint<'tcx>( + tcx: TyCtxt<'tcx>, + impl_m: &ty::AssocItem, + hir_id: hir::HirId, + bad_args: Vec<(Span, Option)>, +) { + let span: MultiSpan = if bad_args.is_empty() { + tcx.def_span(impl_m.def_id).into() + } else { + bad_args.iter().map(|(span, _)| *span).collect::>().into() + }; + tcx.struct_span_lint_hir( + rustc_session::lint::builtin::IMPLIED_BOUNDS_ENTAILMENT, + hir_id, + span, + "impl method assumes more implied bounds than the corresponding trait method", + |lint| { + let bad_args: Vec<_> = + bad_args.into_iter().filter_map(|(span, sugg)| Some((span, sugg?))).collect(); + if !bad_args.is_empty() { + lint.multipart_suggestion( + format!( + "replace {} type{} to make the impl signature compatible", + pluralize!("this", bad_args.len()), + pluralize!(bad_args.len()) + ), + bad_args, + Applicability::MaybeIncorrect, + ); + } + lint + }, + ); +} + #[derive(Debug, PartialEq, Eq)] enum CheckImpliedWfMode { /// Checks implied well-formedness of the impl method. If it fails, we will diff --git a/tests/ui/implied-bounds/impl-implied-bounds-compatibility-unnormalized.stderr b/tests/ui/implied-bounds/impl-implied-bounds-compatibility-unnormalized.stderr index 961ff573e5040..ebe07027d2fa1 100644 --- a/tests/ui/implied-bounds/impl-implied-bounds-compatibility-unnormalized.stderr +++ b/tests/ui/implied-bounds/impl-implied-bounds-compatibility-unnormalized.stderr @@ -1,8 +1,8 @@ error: impl method assumes more implied bounds than the corresponding trait method - --> $DIR/impl-implied-bounds-compatibility-unnormalized.rs:13:5 + --> $DIR/impl-implied-bounds-compatibility-unnormalized.rs:13:31 | LL | fn get<'s>(s: &'s str, _: <&'static &'s () as Project>::Ty) -> &'static str { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace this type to make the impl signature compatible: `()` | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue #105572 @@ -16,10 +16,10 @@ error: aborting due to previous error Future incompatibility report: Future breakage diagnostic: error: impl method assumes more implied bounds than the corresponding trait method - --> $DIR/impl-implied-bounds-compatibility-unnormalized.rs:13:5 + --> $DIR/impl-implied-bounds-compatibility-unnormalized.rs:13:31 | LL | fn get<'s>(s: &'s str, _: <&'static &'s () as Project>::Ty) -> &'static str { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace this type to make the impl signature compatible: `()` | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue #105572 diff --git a/tests/ui/implied-bounds/impl-implied-bounds-compatibility.stderr b/tests/ui/implied-bounds/impl-implied-bounds-compatibility.stderr index 4841f2b179932..43d3e058ffeb3 100644 --- a/tests/ui/implied-bounds/impl-implied-bounds-compatibility.stderr +++ b/tests/ui/implied-bounds/impl-implied-bounds-compatibility.stderr @@ -1,8 +1,8 @@ error: impl method assumes more implied bounds than the corresponding trait method - --> $DIR/impl-implied-bounds-compatibility.rs:14:5 + --> $DIR/impl-implied-bounds-compatibility.rs:14:35 | LL | fn listeners<'b>(&'b self) -> &'a MessageListeners<'b> { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace this type to make the impl signature compatible: `&'b MessageListeners<'b>` | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue #105572 @@ -16,10 +16,10 @@ error: aborting due to previous error Future incompatibility report: Future breakage diagnostic: error: impl method assumes more implied bounds than the corresponding trait method - --> $DIR/impl-implied-bounds-compatibility.rs:14:5 + --> $DIR/impl-implied-bounds-compatibility.rs:14:35 | LL | fn listeners<'b>(&'b self) -> &'a MessageListeners<'b> { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace this type to make the impl signature compatible: `&'b MessageListeners<'b>` | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue #105572