Rollup merge of rust-lang#127871 - compiler-errors:recursive, r=estebank

Mention that type parameters are used recursively on bivariance error

Right now when a type parameter is used recursively, even with indirection (so it has a finite size) we say that the type parameter is unused:

```
struct B<T>(Box<B<T>>);
```

This is confusing, because the type parameter is *used*, it just doesn't have its variance constrained. This PR tweaks that message to mention that it must be used *non-recursively*.

Not sure if we should actually mention "variance" here, but also I'd somewhat prefer we don't keep the power users in the dark w.r.t the real underlying issue, which is that the variance isn't constrained. That technical detail is reserved for a note, though.

cc `@fee1-dead`

Fixes rust-lang#118976
Fixes rust-lang#26283
Fixes rust-lang#53191
Fixes rust-lang#105740
Fixes rust-lang#110466

Author: matthiaskrgr

compiler/rustc_hir_analysis/messages.ftl

@@ -382,6 +382,10 @@ hir_analysis_placeholder_not_allowed_item_signatures = the placeholder `_` is no
 hir_analysis_precise_capture_self_alias = `Self` can't be captured in `use<...>` precise captures list, since it is an alias
     .label = `Self` is not a generic argument, but an alias to the type of the {$what}
 
+hir_analysis_recursive_generic_parameter = {$param_def_kind} `{$param_name}` is only used recursively
+    .label = {$param_def_kind} must be used non-recursively in the definition
+    .note = all type parameters must be used in a non-recursive way in order to constrain their variance
+
 hir_analysis_redundant_lifetime_args = unnecessary lifetime parameter `{$victim}`
     .note = you can use the `{$candidate}` lifetime directly, in place of `{$victim}`
 
@@ -549,6 +553,8 @@ hir_analysis_unused_generic_parameter =
     {$param_def_kind} `{$param_name}` is never used
     .label = unused {$param_def_kind}
     .const_param_help = if you intended `{$param_name}` to be a const parameter, use `const {$param_name}: /* Type */` instead
+    .usage_spans = `{$param_name}` is named here, but is likely unused in the containing type
+
 hir_analysis_unused_generic_parameter_adt_help =
     consider removing `{$param_name}`, referring to it in a field, or using a marker such as `{$phantom_data}`
 hir_analysis_unused_generic_parameter_adt_no_phantom_data_help =

compiler/rustc_hir_analysis/src/check/check.rs

@@ -1572,6 +1572,7 @@ fn check_type_alias_type_params_are_used<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalD
                 param_name,
                 param_def_kind: tcx.def_descr(param.def_id),
                 help: errors::UnusedGenericParameterHelp::TyAlias { param_name },
+                usage_spans: vec![],
                 const_param_help,
             });
             diag.code(E0091);

compiler/rustc_hir_analysis/src/check/wfcheck.rs

@@ -4,12 +4,12 @@ use crate::constrained_generic_params::{identify_constrained_generic_params, Par
 use crate::errors;
 use crate::fluent_generated as fluent;
 
-use hir::intravisit::Visitor;
+use hir::intravisit::{self, Visitor};
 use rustc_ast as ast;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexSet};
 use rustc_errors::{codes::*, pluralize, struct_span_code_err, Applicability, ErrorGuaranteed};
 use rustc_hir as hir;
-use rustc_hir::def::DefKind;
+use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::{DefId, LocalDefId, LocalModDefId};
 use rustc_hir::lang_items::LangItem;
 use rustc_hir::ItemKind;
@@ -1799,7 +1799,7 @@ fn receiver_is_implemented<'tcx>(
 
 fn check_variances_for_type_defn<'tcx>(
     tcx: TyCtxt<'tcx>,
-    item: &hir::Item<'tcx>,
+    item: &'tcx hir::Item<'tcx>,
     hir_generics: &hir::Generics<'tcx>,
 ) {
     let identity_args = ty::GenericArgs::identity_for_item(tcx, item.owner_id);
@@ -1886,21 +1886,21 @@ fn check_variances_for_type_defn<'tcx>(
             hir::ParamName::Error => {}
             _ => {
                 let has_explicit_bounds = explicitly_bounded_params.contains(&parameter);
-                report_bivariance(tcx, hir_param, has_explicit_bounds, item.kind);
+                report_bivariance(tcx, hir_param, has_explicit_bounds, item);
             }
         }
     }
 }
 
-fn report_bivariance(
-    tcx: TyCtxt<'_>,
-    param: &rustc_hir::GenericParam<'_>,
+fn report_bivariance<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    param: &'tcx hir::GenericParam<'tcx>,
     has_explicit_bounds: bool,
-    item_kind: ItemKind<'_>,
+    item: &'tcx hir::Item<'tcx>,
 ) -> ErrorGuaranteed {
     let param_name = param.name.ident();
 
-    let help = match item_kind {
+    let help = match item.kind {
         ItemKind::Enum(..) | ItemKind::Struct(..) | ItemKind::Union(..) => {
             if let Some(def_id) = tcx.lang_items().phantom_data() {
                 errors::UnusedGenericParameterHelp::Adt {
@@ -1915,6 +1915,49 @@ fn report_bivariance(
         item_kind => bug!("report_bivariance: unexpected item kind: {item_kind:?}"),
     };
 
+    let mut usage_spans = vec![];
+    intravisit::walk_item(
+        &mut CollectUsageSpans { spans: &mut usage_spans, param_def_id: param.def_id.to_def_id() },
+        item,
+    );
+
+    if !usage_spans.is_empty() {
+        // First, check if the ADT is (probably) cyclical. We say probably here, since
+        // we're not actually looking into substitutions, just walking through fields.
+        // And we only recurse into the fields of ADTs, and not the hidden types of
+        // opaques or anything else fancy.
+        let item_def_id = item.owner_id.to_def_id();
+        let is_probably_cyclical = if matches!(
+            tcx.def_kind(item_def_id),
+            DefKind::Struct | DefKind::Union | DefKind::Enum
+        ) {
+            IsProbablyCyclical { tcx, adt_def_id: item_def_id, seen: Default::default() }
+                .visit_all_fields(tcx.adt_def(item_def_id))
+                .is_break()
+        } else {
+            false
+        };
+        // If the ADT is cyclical, then if at least one usage of the type parameter or
+        // the `Self` alias is present in the, then it's probably a cyclical struct, and
+        // we should call those parameter usages recursive rather than just saying they're
+        // unused...
+        //
+        // We currently report *all* of the parameter usages, since computing the exact
+        // subset is very involved, and the fact we're mentioning recursion at all is
+        // likely to guide the user in the right direction.
+        if is_probably_cyclical {
+            let diag = tcx.dcx().create_err(errors::RecursiveGenericParameter {
+                spans: usage_spans,
+                param_span: param.span,
+                param_name,
+                param_def_kind: tcx.def_descr(param.def_id.to_def_id()),
+                help,
+                note: (),
+            });
+            return diag.emit();
+        }
+    }
+
     let const_param_help =
         matches!(param.kind, hir::GenericParamKind::Type { .. } if !has_explicit_bounds)
             .then_some(());
@@ -1923,13 +1966,85 @@ fn report_bivariance(
         span: param.span,
         param_name,
         param_def_kind: tcx.def_descr(param.def_id.to_def_id()),
+        usage_spans,
         help,
         const_param_help,
     });
     diag.code(E0392);
     diag.emit()
 }
 
+/// Detects cases where an ADT is trivially cyclical -- we want to detect this so
+/// /we only mention that its parameters are used cyclically if the ADT is truly
+/// cyclical.
+///
+/// Notably, we don't consider substitutions here, so this may have false positives.
+struct IsProbablyCyclical<'tcx> {
+    tcx: TyCtxt<'tcx>,
+    adt_def_id: DefId,
+    seen: FxHashSet<DefId>,
+}
+
+impl<'tcx> IsProbablyCyclical<'tcx> {
+    fn visit_all_fields(&mut self, adt_def: ty::AdtDef<'tcx>) -> ControlFlow<(), ()> {
+        for field in adt_def.all_fields() {
+            self.tcx.type_of(field.did).instantiate_identity().visit_with(self)?;
+        }
+
+        ControlFlow::Continue(())
+    }
+}
+
+impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for IsProbablyCyclical<'tcx> {
+    type Result = ControlFlow<(), ()>;
+
+    fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<(), ()> {
+        if let Some(adt_def) = t.ty_adt_def() {
+            if adt_def.did() == self.adt_def_id {
+                return ControlFlow::Break(());
+            }
+
+            if self.seen.insert(adt_def.did()) {
+                self.visit_all_fields(adt_def)?;
+            }
+        }
+
+        t.super_visit_with(self)
+    }
+}
+
+/// Collect usages of the `param_def_id` and `Res::SelfTyAlias` in the HIR.
+///
+/// This is used to report places where the user has used parameters in a
+/// non-variance-constraining way for better bivariance errors.
+struct CollectUsageSpans<'a> {
+    spans: &'a mut Vec<Span>,
+    param_def_id: DefId,
+}
+
+impl<'tcx> Visitor<'tcx> for CollectUsageSpans<'_> {
+    type Result = ();
+
+    fn visit_generics(&mut self, _g: &'tcx rustc_hir::Generics<'tcx>) -> Self::Result {
+        // Skip the generics. We only care about fields, not where clause/param bounds.
+    }
+
+    fn visit_ty(&mut self, t: &'tcx hir::Ty<'tcx>) -> Self::Result {
+        if let hir::TyKind::Path(hir::QPath::Resolved(None, qpath)) = t.kind {
+            if let Res::Def(DefKind::TyParam, def_id) = qpath.res
+                && def_id == self.param_def_id
+            {
+                self.spans.push(t.span);
+                return;
+            } else if let Res::SelfTyAlias { .. } = qpath.res {
+                self.spans.push(t.span);
+                return;
+            }
+        }
+        intravisit::walk_ty(self, t);
+    }
+}
+
 impl<'tcx> WfCheckingCtxt<'_, 'tcx> {
     /// Feature gates RFC 2056 -- trivial bounds, checking for global bounds that
     /// aren't true.

compiler/rustc_hir_analysis/src/errors.rs

@@ -1597,12 +1597,29 @@ pub(crate) struct UnusedGenericParameter {
     pub span: Span,
     pub param_name: Ident,
     pub param_def_kind: &'static str,
+    #[label(hir_analysis_usage_spans)]
+    pub usage_spans: Vec<Span>,
     #[subdiagnostic]
     pub help: UnusedGenericParameterHelp,
     #[help(hir_analysis_const_param_help)]
     pub const_param_help: Option<()>,
 }
 
+#[derive(Diagnostic)]
+#[diag(hir_analysis_recursive_generic_parameter)]
+pub(crate) struct RecursiveGenericParameter {
+    #[primary_span]
+    pub spans: Vec<Span>,
+    #[label]
+    pub param_span: Span,
+    pub param_name: Ident,
+    pub param_def_kind: &'static str,
+    #[subdiagnostic]
+    pub help: UnusedGenericParameterHelp,
+    #[note]
+    pub note: (),
+}
+
 #[derive(Subdiagnostic)]
 pub(crate) enum UnusedGenericParameterHelp {
     #[help(hir_analysis_unused_generic_parameter_adt_help)]

tests/ui/lazy-type-alias/inherent-impls-overflow.next.stderr

@@ -8,7 +8,9 @@ error[E0392]: type parameter `T` is never used
   --> $DIR/inherent-impls-overflow.rs:14:12
    |
 LL | type Poly0<T> = Poly1<(T,)>;
-   |            ^ unused type parameter
+   |            ^           - `T` is named here, but is likely unused in the containing type
+   |            |
+   |            unused type parameter
    |
    = help: consider removing `T` or referring to it in the body of the type alias
    = help: if you intended `T` to be a const parameter, use `const T: /* Type */` instead
@@ -17,7 +19,9 @@ error[E0392]: type parameter `T` is never used
   --> $DIR/inherent-impls-overflow.rs:17:12
    |
 LL | type Poly1<T> = Poly0<(T,)>;
-   |            ^ unused type parameter
+   |            ^           - `T` is named here, but is likely unused in the containing type
+   |            |
+   |            unused type parameter
    |
    = help: consider removing `T` or referring to it in the body of the type alias
    = help: if you intended `T` to be a const parameter, use `const T: /* Type */` instead

tests/ui/traits/issue-105231.rs

@@ -1,9 +1,9 @@
 //~ ERROR overflow evaluating the requirement `A<A<A<A<A<A<A<...>>>>>>>: Send`
 struct A<T>(B<T>);
 //~^ ERROR recursive types `A` and `B` have infinite size
-//~| ERROR `T` is never used
+//~| ERROR `T` is only used recursively
 struct B<T>(A<A<T>>);
-//~^ ERROR `T` is never used
+//~^ ERROR `T` is only used recursively
 trait Foo {}
 impl<T> Foo for T where T: Send {}
 impl Foo for B<u8> {}

tests/ui/traits/issue-105231.stderr

@@ -15,23 +15,27 @@ LL |
 LL ~ struct B<T>(Box<A<A<T>>>);
    |
 
-error[E0392]: type parameter `T` is never used
-  --> $DIR/issue-105231.rs:2:10
+error: type parameter `T` is only used recursively
+  --> $DIR/issue-105231.rs:2:15
    |
 LL | struct A<T>(B<T>);
-   |          ^ unused type parameter
+   |          -    ^
+   |          |
+   |          type parameter must be used non-recursively in the definition
    |
    = help: consider removing `T`, referring to it in a field, or using a marker such as `PhantomData`
-   = help: if you intended `T` to be a const parameter, use `const T: /* Type */` instead
+   = note: all type parameters must be used in a non-recursive way in order to constrain their variance
 
-error[E0392]: type parameter `T` is never used
-  --> $DIR/issue-105231.rs:5:10
+error: type parameter `T` is only used recursively
+  --> $DIR/issue-105231.rs:5:17
    |
 LL | struct B<T>(A<A<T>>);
-   |          ^ unused type parameter
+   |          -      ^
+   |          |
+   |          type parameter must be used non-recursively in the definition
    |
    = help: consider removing `T`, referring to it in a field, or using a marker such as `PhantomData`
-   = help: if you intended `T` to be a const parameter, use `const T: /* Type */` instead
+   = note: all type parameters must be used in a non-recursive way in order to constrain their variance
 
 error[E0275]: overflow evaluating the requirement `A<A<A<A<A<A<A<...>>>>>>>: Send`
    |
@@ -44,5 +48,5 @@ LL | struct B<T>(A<A<T>>);
 
 error: aborting due to 4 previous errors
 
-Some errors have detailed explanations: E0072, E0275, E0392.
+Some errors have detailed explanations: E0072, E0275.
 For more information about an error, try `rustc --explain E0072`.

tests/ui/variance/variance-unused-type-param.rs

@@ -11,11 +11,14 @@ enum SomeEnum<A> { Nothing }
 
 // Here T might *appear* used, but in fact it isn't.
 enum ListCell<T> {
-//~^ ERROR parameter `T` is never used
     Cons(Box<ListCell<T>>),
+    //~^ ERROR parameter `T` is only used recursively
     Nil
 }
 
+struct SelfTyAlias<T>(Box<Self>);
+//~^ ERROR parameter `T` is only used recursively
+
 struct WithBounds<T: Sized> {}
 //~^ ERROR parameter `T` is never used
 
@@ -25,4 +28,9 @@ struct WithWhereBounds<T> where T: Sized {}
 struct WithOutlivesBounds<T: 'static> {}
 //~^ ERROR parameter `T` is never used
 
+struct DoubleNothing<T> {
+//~^ ERROR parameter `T` is never used
+    s: SomeStruct<T>,
+}
+
 fn main() {}