Rollup merge of #121720 - tmandry:split-refining, r=compiler-errors

Split refining_impl_trait lint into _reachable, _internal variants

As discussed in #119535 (comment):

> We discussed this today in triage and developed a consensus to:
>
> * Add a separate lint against impls that refine a return type defined with RPITIT even when the trait is not crate public.
> * Place that in a lint group along with the analogous crate public lint.
> * Create an issue to solicit feedback on these lints (or perhaps two separate ones).
> * Have the warnings displayed with each lint reference this issue in a similar manner to how we do that today with the required `Self: '0'` bound on GATs.
> * Make a note to review this feedback on 2-3 release cycles.

This points users to #121718 to leave feedback.

Author: fmease

compiler/rustc_hir_analysis/messages.ftl

@@ -351,6 +351,7 @@ hir_analysis_rpitit_refined = impl trait in impl method signature does not match
     .label = return type from trait method defined here
     .unmatched_bound_label = this bound is stronger than that defined on the trait
     .note = add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate
+    .feedback_note = we are soliciting feedback, see issue #121718 <https://github.com/rust-lang/rust/issues/121718> for more information
 
 hir_analysis_self_in_impl_self =
     `Self` is not valid in the self type of an impl block

compiler/rustc_hir_analysis/src/check/compare_impl_item/refine.rs

@@ -2,7 +2,7 @@ use rustc_data_structures::fx::FxIndexSet;
 use rustc_hir as hir;
 use rustc_hir::def_id::DefId;
 use rustc_infer::infer::{outlives::env::OutlivesEnvironment, TyCtxtInferExt};
-use rustc_lint_defs::builtin::REFINING_IMPL_TRAIT;
+use rustc_lint_defs::builtin::{REFINING_IMPL_TRAIT_INTERNAL, REFINING_IMPL_TRAIT_REACHABLE};
 use rustc_middle::traits::{ObligationCause, Reveal};
 use rustc_middle::ty::{
     self, Ty, TyCtxt, TypeFoldable, TypeFolder, TypeSuperVisitable, TypeVisitable, TypeVisitor,
@@ -23,26 +23,23 @@ pub(super) fn check_refining_return_position_impl_trait_in_trait<'tcx>(
     if !tcx.impl_method_has_trait_impl_trait_tys(impl_m.def_id) {
         return;
     }
+
     // unreachable traits don't have any library guarantees, there's no need to do this check.
-    if trait_m
+    let is_internal = trait_m
         .container_id(tcx)
         .as_local()
         .is_some_and(|trait_def_id| !tcx.effective_visibilities(()).is_reachable(trait_def_id))
-    {
-        return;
-    }
+        // If a type in the trait ref is private, then there's also no reason to do this check.
+        || impl_trait_ref.args.iter().any(|arg| {
+            if let Some(ty) = arg.as_type()
+                && let Some(self_visibility) = type_visibility(tcx, ty)
+            {
+                return !self_visibility.is_public();
+            }
+            false
+        });
 
-    // If a type in the trait ref is private, then there's also no reason to do this check.
     let impl_def_id = impl_m.container_id(tcx);
-    for arg in impl_trait_ref.args {
-        if let Some(ty) = arg.as_type()
-            && let Some(self_visibility) = type_visibility(tcx, ty)
-            && !self_visibility.is_public()
-        {
-            return;
-        }
-    }
-
     let impl_m_args = ty::GenericArgs::identity_for_item(tcx, impl_m.def_id);
     let trait_m_to_impl_m_args = impl_m_args.rebase_onto(tcx, impl_def_id, impl_trait_ref.args);
     let bound_trait_m_sig = tcx.fn_sig(trait_m.def_id).instantiate(tcx, trait_m_to_impl_m_args);
@@ -85,6 +82,7 @@ pub(super) fn check_refining_return_position_impl_trait_in_trait<'tcx>(
                 trait_m.def_id,
                 impl_m.def_id,
                 None,
+                is_internal,
             );
             return;
         };
@@ -104,6 +102,7 @@ pub(super) fn check_refining_return_position_impl_trait_in_trait<'tcx>(
                 trait_m.def_id,
                 impl_m.def_id,
                 None,
+                is_internal,
             );
             return;
         }
@@ -198,6 +197,7 @@ pub(super) fn check_refining_return_position_impl_trait_in_trait<'tcx>(
                 trait_m.def_id,
                 impl_m.def_id,
                 Some(span),
+                is_internal,
             );
             return;
         }
@@ -235,6 +235,7 @@ fn report_mismatched_rpitit_signature<'tcx>(
     trait_m_def_id: DefId,
     impl_m_def_id: DefId,
     unmatched_bound: Option<Span>,
+    is_internal: bool,
 ) {
     let mapping = std::iter::zip(
         tcx.fn_sig(trait_m_def_id).skip_binder().bound_vars(),
@@ -287,7 +288,7 @@ fn report_mismatched_rpitit_signature<'tcx>(
 
     let span = unmatched_bound.unwrap_or(span);
     tcx.emit_node_span_lint(
-        REFINING_IMPL_TRAIT,
+        if is_internal { REFINING_IMPL_TRAIT_INTERNAL } else { REFINING_IMPL_TRAIT_REACHABLE },
         tcx.local_def_id_to_hir_id(impl_m_def_id.expect_local()),
         span,
         crate::errors::ReturnPositionImplTraitInTraitRefined {

compiler/rustc_hir_analysis/src/errors.rs

@@ -1072,6 +1072,7 @@ pub struct UnusedAssociatedTypeBounds {
 #[derive(LintDiagnostic)]
 #[diag(hir_analysis_rpitit_refined)]
 #[note]
+#[note(hir_analysis_feedback_note)]
 pub(crate) struct ReturnPositionImplTraitInTraitRefined<'tcx> {
     #[suggestion(applicability = "maybe-incorrect", code = "{pre}{return_ty}{post}")]
     pub impl_return_span: Span,

compiler/rustc_lint/src/lib.rs

@@ -313,6 +313,12 @@ fn register_builtins(store: &mut LintStore) {
                                        // MACRO_USE_EXTERN_CRATE
     );
 
+    add_lint_group!(
+        "refining_impl_trait",
+        REFINING_IMPL_TRAIT_REACHABLE,
+        REFINING_IMPL_TRAIT_INTERNAL
+    );
+
     // Register renamed and removed lints.
     store.register_renamed("single_use_lifetime", "single_use_lifetimes");
     store.register_renamed("elided_lifetime_in_path", "elided_lifetimes_in_paths");

compiler/rustc_lint_defs/src/builtin.rs

@@ -79,7 +79,8 @@ declare_lint_pass! {
         PROC_MACRO_BACK_COMPAT,
         PROC_MACRO_DERIVE_RESOLUTION_FALLBACK,
         PUB_USE_OF_PRIVATE_EXTERN_CRATE,
-        REFINING_IMPL_TRAIT,
+        REFINING_IMPL_TRAIT_INTERNAL,
+        REFINING_IMPL_TRAIT_REACHABLE,
         RENAMED_AND_REMOVED_LINTS,
         REPR_TRANSPARENT_EXTERNAL_PRIVATE_FIELDS,
         RUST_2021_INCOMPATIBLE_CLOSURE_CAPTURES,
@@ -4402,8 +4403,10 @@ declare_lint! {
 }
 
 declare_lint! {
-    /// The `refining_impl_trait` lint detects usages of return-position impl
-    /// traits in trait signatures which are refined by implementations.
+    /// The `refining_impl_trait_reachable` lint detects `impl Trait` return
+    /// types in method signatures that are refined by a publically reachable
+    /// trait implementation, meaning the implementation adds information about
+    /// the return type that is not present in the trait.
     ///
     /// ### Example
     ///
@@ -4425,21 +4428,88 @@ declare_lint! {
     /// fn main() {
     ///     // users can observe that the return type of
     ///     // `<&str as AsDisplay>::as_display()` is `&str`.
-    ///     let x: &str = "".as_display();
+    ///     let _x: &str = "".as_display();
     /// }
     /// ```
     ///
     /// {{produces}}
     ///
     /// ### Explanation
     ///
-    /// Return-position impl trait in traits (RPITITs) desugar to associated types,
-    /// and callers of methods for types where the implementation is known are
+    /// Callers of methods for types where the implementation is known are
     /// able to observe the types written in the impl signature. This may be
-    /// intended behavior, but may also pose a semver hazard for authors of libraries
-    /// who do not wish to make stronger guarantees about the types than what is
-    /// written in the trait signature.
-    pub REFINING_IMPL_TRAIT,
+    /// intended behavior, but may also lead to implementation details being
+    /// revealed unintentionally. In particular, it may pose a semver hazard
+    /// for authors of libraries who do not wish to make stronger guarantees
+    /// about the types than what is written in the trait signature.
+    ///
+    /// `refining_impl_trait` is a lint group composed of two lints:
+    ///
+    /// * `refining_impl_trait_reachable`, for refinements that are publically
+    ///   reachable outside a crate, and
+    /// * `refining_impl_trait_internal`, for refinements that are only visible
+    ///    within a crate.
+    ///
+    /// We are seeking feedback on each of these lints; see issue
+    /// [#121718](https://github.com/rust-lang/rust/issues/121718) for more
+    /// information.
+    pub REFINING_IMPL_TRAIT_REACHABLE,
+    Warn,
+    "impl trait in impl method signature does not match trait method signature",
+}
+
+declare_lint! {
+    /// The `refining_impl_trait_internal` lint detects `impl Trait` return
+    /// types in method signatures that are refined by a trait implementation,
+    /// meaning the implementation adds information about the return type that
+    /// is not present in the trait.
+    ///
+    /// ### Example
+    ///
+    /// ```rust,compile_fail
+    /// #![deny(refining_impl_trait)]
+    ///
+    /// use std::fmt::Display;
+    ///
+    /// trait AsDisplay {
+    ///     fn as_display(&self) -> impl Display;
+    /// }
+    ///
+    /// impl<'s> AsDisplay for &'s str {
+    ///     fn as_display(&self) -> Self {
+    ///         *self
+    ///     }
+    /// }
+    ///
+    /// fn main() {
+    ///     // users can observe that the return type of
+    ///     // `<&str as AsDisplay>::as_display()` is `&str`.
+    ///     let _x: &str = "".as_display();
+    /// }
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// Callers of methods for types where the implementation is known are
+    /// able to observe the types written in the impl signature. This may be
+    /// intended behavior, but may also lead to implementation details being
+    /// revealed unintentionally. In particular, it may pose a semver hazard
+    /// for authors of libraries who do not wish to make stronger guarantees
+    /// about the types than what is written in the trait signature.
+    ///
+    /// `refining_impl_trait` is a lint group composed of two lints:
+    ///
+    /// * `refining_impl_trait_reachable`, for refinements that are publically
+    ///   reachable outside a crate, and
+    /// * `refining_impl_trait_internal`, for refinements that are only visible
+    ///    within a crate.
+    ///
+    /// We are seeking feedback on each of these lints; see issue
+    /// [#121718](https://github.com/rust-lang/rust/issues/121718) for more
+    /// information.
+    pub REFINING_IMPL_TRAIT_INTERNAL,
     Warn,
     "impl trait in impl method signature does not match trait method signature",
 }

src/tools/lint-docs/src/groups.rs

@@ -16,6 +16,10 @@ static GROUP_DESCRIPTIONS: &[(&str, &str)] = &[
     ("rust-2018-compatibility", "Lints used to transition code from the 2015 edition to 2018"),
     ("rust-2021-compatibility", "Lints used to transition code from the 2018 edition to 2021"),
     ("rust-2024-compatibility", "Lints used to transition code from the 2021 edition to 2024"),
+    (
+        "refining-impl-trait",
+        "Detects refinement of `impl Trait` return types by trait implementations",
+    ),
 ];
 
 type LintGroups = BTreeMap<String, BTreeSet<String>>;

tests/ui/async-await/in-trait/async-example-desugared-boxed.stderr

@@ -8,11 +8,13 @@ LL |     fn foo(&self) -> Pin<Box<dyn Future<Output = i32> + '_>> {
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate
+   = note: we are soliciting feedback, see issue #121718 <https://github.com/rust-lang/rust/issues/121718> for more information
 note: the lint level is defined here
   --> $DIR/async-example-desugared-boxed.rs:13:12
    |
 LL |     #[warn(refining_impl_trait)]
    |            ^^^^^^^^^^^^^^^^^^^
+   = note: `#[warn(refining_impl_trait_reachable)]` implied by `#[warn(refining_impl_trait)]`
 help: replace the return type so that it matches the trait
    |
 LL |     fn foo(&self) -> impl Future<Output = i32> {

tests/ui/async-await/in-trait/async-example-desugared-manual.stderr

@@ -8,11 +8,13 @@ LL |     fn foo(&self) -> MyFuture {
    |                      ^^^^^^^^
    |
    = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate
+   = note: we are soliciting feedback, see issue #121718 <https://github.com/rust-lang/rust/issues/121718> for more information
 note: the lint level is defined here
   --> $DIR/async-example-desugared-manual.rs:21:12
    |
 LL |     #[warn(refining_impl_trait)]
    |            ^^^^^^^^^^^^^^^^^^^
+   = note: `#[warn(refining_impl_trait_reachable)]` implied by `#[warn(refining_impl_trait)]`
 help: replace the return type so that it matches the trait
    |
 LL |     fn foo(&self) -> impl Future<Output = i32> {

tests/ui/impl-trait/in-trait/bad-item-bound-within-rpitit.stderr

@@ -22,7 +22,8 @@ LL |     fn iter(&self) -> impl 'a + Iterator<Item = I::Item<'a>> {
    |                            ^^ this bound is stronger than that defined on the trait
    |
    = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate
-   = note: `#[warn(refining_impl_trait)]` on by default
+   = note: we are soliciting feedback, see issue #121718 <https://github.com/rust-lang/rust/issues/121718> for more information
+   = note: `#[warn(refining_impl_trait_reachable)]` on by default
 help: replace the return type so that it matches the trait
    |
 LL |     fn iter(&self) -> impl Iterator<Item = <Self as Iterable>::Item<'_>> + '_ {

tests/ui/impl-trait/in-trait/foreign.rs

@@ -17,9 +17,29 @@ impl Foo for Local {
     }
 }
 
-struct LocalIgnoreRefining;
-impl Foo for LocalIgnoreRefining {
-    #[deny(refining_impl_trait)]
+struct LocalOnlyRefiningA;
+impl Foo for LocalOnlyRefiningA {
+    #[warn(refining_impl_trait)]
+    fn bar(self) -> Arc<String> {
+        //~^ WARN impl method signature does not match trait method signature
+        Arc::new(String::new())
+    }
+}
+
+struct LocalOnlyRefiningB;
+impl Foo for LocalOnlyRefiningB {
+    #[warn(refining_impl_trait)]
+    #[allow(refining_impl_trait_reachable)]
+    fn bar(self) -> Arc<String> {
+        //~^ WARN impl method signature does not match trait method signature
+        Arc::new(String::new())
+    }
+}
+
+struct LocalOnlyRefiningC;
+impl Foo for LocalOnlyRefiningC {
+    #[warn(refining_impl_trait)]
+    #[allow(refining_impl_trait_internal)]
     fn bar(self) -> Arc<String> {
         Arc::new(String::new())
     }
@@ -34,5 +54,7 @@ fn main() {
     let &() = Foreign.bar();
 
     let x: Arc<String> = Local.bar();
-    let x: Arc<String> = LocalIgnoreRefining.bar();
+    let x: Arc<String> = LocalOnlyRefiningA.bar();
+    let x: Arc<String> = LocalOnlyRefiningB.bar();
+    let x: Arc<String> = LocalOnlyRefiningC.bar();
 }