Auto merge of #105575 - compiler-errors:impl-wf-lint, r=oli-obk

Add `IMPLIED_BOUNDS_ENTAILMENT` lint

Implements a lint (#105572) version of the hard-error introduced in #105483. Context is in that PR.

r? `@lcnr`
cc `@oli-obk` who had asked for this to be a lint first

Not sure if this needs to be an FCP, since it's a lint for now.

Author: bors

compiler/rustc_hir_analysis/src/check/compare_method.rs

@@ -71,8 +71,14 @@ pub(crate) fn compare_impl_method<'tcx>(
         return;
     }
 
-    if let Err(_) = compare_predicate_entailment(tcx, impl_m, impl_m_span, trait_m, impl_trait_ref)
-    {
+    if let Err(_) = compare_predicate_entailment(
+        tcx,
+        impl_m,
+        impl_m_span,
+        trait_m,
+        impl_trait_ref,
+        CheckImpliedWfMode::Check,
+    ) {
         return;
     }
 }
@@ -150,6 +156,7 @@ fn compare_predicate_entailment<'tcx>(
     impl_m_span: Span,
     trait_m: &ty::AssocItem,
     impl_trait_ref: ty::TraitRef<'tcx>,
+    check_implied_wf: CheckImpliedWfMode,
 ) -> Result<(), ErrorGuaranteed> {
     let trait_to_impl_substs = impl_trait_ref.substs;
 
@@ -255,15 +262,15 @@ fn compare_predicate_entailment<'tcx>(
 
     let mut wf_tys = FxIndexSet::default();
 
-    let impl_sig = infcx.replace_bound_vars_with_fresh_vars(
+    let unnormalized_impl_sig = infcx.replace_bound_vars_with_fresh_vars(
         impl_m_span,
         infer::HigherRankedType,
         tcx.fn_sig(impl_m.def_id),
     );
+    let unnormalized_impl_fty = tcx.mk_fn_ptr(ty::Binder::dummy(unnormalized_impl_sig));
 
     let norm_cause = ObligationCause::misc(impl_m_span, impl_m_hir_id);
-    let impl_sig = ocx.normalize(&norm_cause, param_env, impl_sig);
-    let impl_fty = tcx.mk_fn_ptr(ty::Binder::dummy(impl_sig));
+    let impl_fty = ocx.normalize(&norm_cause, param_env, unnormalized_impl_fty);
     debug!("compare_impl_method: impl_fty={:?}", impl_fty);
 
     let trait_sig = tcx.bound_fn_sig(trait_m.def_id).subst(tcx, trait_to_placeholder_substs);
@@ -304,29 +311,108 @@ fn compare_predicate_entailment<'tcx>(
         return Err(emitted);
     }
 
+    if check_implied_wf == CheckImpliedWfMode::Check {
+        // We need to check that the impl's args are well-formed given
+        // the hybrid param-env (impl + trait method where-clauses).
+        ocx.register_obligation(traits::Obligation::new(
+            infcx.tcx,
+            ObligationCause::dummy(),
+            param_env,
+            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.
     let errors = ocx.select_all_or_error();
     if !errors.is_empty() {
-        let reported = infcx.err_ctxt().report_fulfillment_errors(&errors, None);
-        return Err(reported);
+        match check_implied_wf {
+            CheckImpliedWfMode::Check => {
+                return compare_predicate_entailment(
+                    tcx,
+                    impl_m,
+                    impl_m_span,
+                    trait_m,
+                    impl_trait_ref,
+                    CheckImpliedWfMode::Skip,
+                )
+                .map(|()| {
+                    // If the skip-mode was successful, emit a lint.
+                    emit_implied_wf_lint();
+                });
+            }
+            CheckImpliedWfMode::Skip => {
+                let reported = infcx.err_ctxt().report_fulfillment_errors(&errors, None);
+                return Err(reported);
+            }
+        }
     }
 
     // Finally, resolve all regions. This catches wily misuses of
     // lifetime parameters.
-    let outlives_environment = OutlivesEnvironment::with_bounds(
+    let outlives_env = OutlivesEnvironment::with_bounds(
         param_env,
         Some(infcx),
-        infcx.implied_bounds_tys(param_env, impl_m_hir_id, wf_tys),
+        infcx.implied_bounds_tys(param_env, impl_m_hir_id, wf_tys.clone()),
     );
-    infcx.check_region_obligations_and_report_errors(
-        impl_m.def_id.expect_local(),
-        &outlives_environment,
+    infcx.process_registered_region_obligations(
+        outlives_env.region_bound_pairs(),
+        outlives_env.param_env,
     );
+    let errors = infcx.resolve_regions(&outlives_env);
+    if !errors.is_empty() {
+        // FIXME(compiler-errors): This can be simplified when IMPLIED_BOUNDS_ENTAILMENT
+        // becomes a hard error (i.e. ideally we'd just call `resolve_regions_and_report_errors`
+        match check_implied_wf {
+            CheckImpliedWfMode::Check => {
+                return compare_predicate_entailment(
+                    tcx,
+                    impl_m,
+                    impl_m_span,
+                    trait_m,
+                    impl_trait_ref,
+                    CheckImpliedWfMode::Skip,
+                )
+                .map(|()| {
+                    // If the skip-mode was successful, emit a lint.
+                    emit_implied_wf_lint();
+                });
+            }
+            CheckImpliedWfMode::Skip => {
+                if infcx.tainted_by_errors().is_none() {
+                    infcx.err_ctxt().report_region_errors(impl_m.def_id.expect_local(), &errors);
+                }
+                return Err(tcx
+                    .sess
+                    .delay_span_bug(rustc_span::DUMMY_SP, "error should have been emitted"));
+            }
+        }
+    }
 
     Ok(())
 }
 
+#[derive(Debug, PartialEq, Eq)]
+enum CheckImpliedWfMode {
+    /// Checks implied well-formedness of the impl method. If it fails, we will
+    /// re-check with `Skip`, and emit a lint if it succeeds.
+    Check,
+    /// Skips checking implied well-formedness of the impl method, but will emit
+    /// a lint if the `compare_predicate_entailment` succeeded. This means that
+    /// the reason that we had failed earlier during `Check` was due to the impl
+    /// having stronger requirements than the trait.
+    Skip,
+}
+
 fn compare_asyncness<'tcx>(
     tcx: TyCtxt<'tcx>,
     impl_m: &ty::AssocItem,

compiler/rustc_infer/src/infer/mod.rs

@@ -1693,7 +1693,7 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
         &self,
         generic_param_scope: LocalDefId,
         outlives_env: &OutlivesEnvironment<'tcx>,
-    ) {
+    ) -> Option<ErrorGuaranteed> {
         let errors = self.resolve_regions(outlives_env);
 
         if let None = self.tainted_by_errors() {
@@ -1704,6 +1704,10 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
             // errors from silly ones.
             self.report_region_errors(generic_param_scope, &errors);
         }
+
+        (!errors.is_empty()).then(|| {
+            self.tcx.sess.delay_span_bug(rustc_span::DUMMY_SP, "error should have been emitted")
+        })
     }
 
     // [Note-Type-error-reporting]

compiler/rustc_infer/src/infer/outlives/obligations.rs

@@ -68,6 +68,7 @@ use crate::infer::{
 };
 use crate::traits::{ObligationCause, ObligationCauseCode};
 use rustc_data_structures::undo_log::UndoLogs;
+use rustc_errors::ErrorGuaranteed;
 use rustc_hir::def_id::DefId;
 use rustc_hir::def_id::LocalDefId;
 use rustc_middle::mir::ConstraintCategory;
@@ -177,7 +178,7 @@ impl<'tcx> InferCtxt<'tcx> {
         &self,
         generic_param_scope: LocalDefId,
         outlives_env: &OutlivesEnvironment<'tcx>,
-    ) {
+    ) -> Option<ErrorGuaranteed> {
         self.process_registered_region_obligations(
             outlives_env.region_bound_pairs(),
             outlives_env.param_env,

compiler/rustc_lint_defs/src/builtin.rs

@@ -3311,6 +3311,7 @@ declare_lint_pass! {
         FFI_UNWIND_CALLS,
         REPR_TRANSPARENT_EXTERNAL_PRIVATE_FIELDS,
         NAMED_ARGUMENTS_USED_POSITIONALLY,
+        IMPLIED_BOUNDS_ENTAILMENT,
     ]
 }
 
@@ -3998,3 +3999,44 @@ declare_lint! {
     Warn,
     "named arguments in format used positionally"
 }
+
+declare_lint! {
+    /// The `implied_bounds_entailment` lint detects cases where the arguments of an impl method
+    /// have stronger implied bounds than those from the trait method it's implementing.
+    ///
+    /// ### Example
+    ///
+    /// ```rust,compile_fail
+    /// #![deny(implied_bounds_entailment)]
+    ///
+    /// trait Trait {
+    ///     fn get<'s>(s: &'s str, _: &'static &'static ()) -> &'static str;
+    /// }
+    ///
+    /// impl Trait for () {
+    ///     fn get<'s>(s: &'s str, _: &'static &'s ()) -> &'static str {
+    ///         s
+    ///     }
+    /// }
+    ///
+    /// let val = <() as Trait>::get(&String::from("blah blah blah"), &&());
+    /// println!("{}", val);
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// Neither the trait method, which provides no implied bounds about `'s`, nor the impl,
+    /// requires the main function to prove that 's: 'static, but the impl method is allowed
+    /// to assume that `'s: 'static` within its own body.
+    ///
+    /// This can be used to implement an unsound API if used incorrectly.
+    pub IMPLIED_BOUNDS_ENTAILMENT,
+    Warn,
+    "impl method assumes more implied bounds than its corresponding trait method",
+    @future_incompatible = FutureIncompatibleInfo {
+        reference: "issue #105572 <https://github.com/rust-lang/rust/issues/105572>",
+        reason: FutureIncompatibilityReason::FutureReleaseError,
+    };
+}

src/test/ui/implied-bounds/impl-implied-bounds-compatibility-unnormalized.rs

@@ -0,0 +1,22 @@
+#![deny(implied_bounds_entailment)]
+
+trait Project {
+    type Ty;
+}
+impl Project for &'_ &'_ () {
+    type Ty = ();
+}
+trait Trait {
+    fn get<'s>(s: &'s str, _: ()) -> &'static str;
+}
+impl Trait for () {
+    fn get<'s>(s: &'s str, _: <&'static &'s () as Project>::Ty) -> &'static str {
+        //~^ ERROR impl method assumes more implied bounds than the corresponding trait method
+        //~| WARN this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+        s
+    }
+}
+fn main() {
+    let val = <() as Trait>::get(&String::from("blah blah blah"), ());
+    println!("{}", val);
+}

src/test/ui/implied-bounds/impl-implied-bounds-compatibility-unnormalized.stderr

@@ -0,0 +1,16 @@
+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 <https://github.com/rust-lang/rust/issues/105572>
+note: the lint level is defined here
+  --> $DIR/impl-implied-bounds-compatibility-unnormalized.rs:1:9
+   |
+LL | #![deny(implied_bounds_entailment)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to previous error
+

src/test/ui/implied-bounds/impl-implied-bounds-compatibility.rs

@@ -0,0 +1,21 @@
+#![deny(implied_bounds_entailment)]
+
+use std::cell::RefCell;
+
+pub struct MessageListeners<'a> {
+    listeners: RefCell<Vec<Box<dyn FnMut(()) + 'a>>>,
+}
+
+pub trait MessageListenersInterface {
+    fn listeners<'c>(&'c self) -> &'c MessageListeners<'c>;
+}
+
+impl<'a> MessageListenersInterface for MessageListeners<'a> {
+    fn listeners<'b>(&'b self) -> &'a MessageListeners<'b> {
+        //~^ ERROR impl method assumes more implied bounds than the corresponding trait method
+        //~| WARN this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+        self
+    }
+}
+
+fn main() {}

src/test/ui/implied-bounds/impl-implied-bounds-compatibility.stderr

@@ -0,0 +1,16 @@
+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 <https://github.com/rust-lang/rust/issues/105572>
+note: the lint level is defined here
+  --> $DIR/impl-implied-bounds-compatibility.rs:1:9
+   |
+LL | #![deny(implied_bounds_entailment)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to previous error
+

src/tools/clippy/tests/ui/borrow_interior_mutable_const/others.rs

@@ -42,7 +42,7 @@ impl<T> StaticRef<T> {
 impl<T> std::ops::Deref for StaticRef<T> {
     type Target = T;
 
-    fn deref(&self) -> &'static T {
+    fn deref(&self) -> &T {
         unsafe { &*self.ptr }
     }
 }