From c02d0de871a99b08d013bd79941889fca53ae3c4 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 17 Jul 2024 18:05:21 -0400 Subject: [PATCH] Account for structs that have unused params in nested types in fields --- compiler/rustc_hir_analysis/messages.ftl | 1 + .../rustc_hir_analysis/src/check/check.rs | 1 + .../rustc_hir_analysis/src/check/wfcheck.rs | 115 ++++++++++++++---- compiler/rustc_hir_analysis/src/errors.rs | 2 + .../inherent-impls-overflow.next.stderr | 23 ++-- .../inherent-impls-overflow.rs | 4 +- .../ui/variance/variance-unused-type-param.rs | 5 + .../variance-unused-type-param.stderr | 14 ++- 8 files changed, 128 insertions(+), 37 deletions(-) diff --git a/compiler/rustc_hir_analysis/messages.ftl b/compiler/rustc_hir_analysis/messages.ftl index ac46f981d98..b43559f4225 100644 --- a/compiler/rustc_hir_analysis/messages.ftl +++ b/compiler/rustc_hir_analysis/messages.ftl @@ -553,6 +553,7 @@ 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}` diff --git a/compiler/rustc_hir_analysis/src/check/check.rs b/compiler/rustc_hir_analysis/src/check/check.rs index bf8ef18c04f..dbc265ad3ff 100644 --- a/compiler/rustc_hir_analysis/src/check/check.rs +++ b/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); diff --git a/compiler/rustc_hir_analysis/src/check/wfcheck.rs b/compiler/rustc_hir_analysis/src/check/wfcheck.rs index ffe3e61d9cf..809427f86ee 100644 --- a/compiler/rustc_hir_analysis/src/check/wfcheck.rs +++ b/compiler/rustc_hir_analysis/src/check/wfcheck.rs @@ -1921,33 +1921,102 @@ fn report_bivariance<'tcx>( item, ); - if usage_spans.is_empty() { - let const_param_help = - matches!(param.kind, hir::GenericParamKind::Type { .. } if !has_explicit_bounds) - .then_some(()); + 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 mut diag = tcx.dcx().create_err(errors::UnusedGenericParameter { - span: param.span, - param_name, - param_def_kind: tcx.def_descr(param.def_id.to_def_id()), - help, - const_param_help, - }); - diag.code(E0392); - diag.emit() - } else { - 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: (), - }); - diag.emit() + let const_param_help = + matches!(param.kind, hir::GenericParamKind::Type { .. } if !has_explicit_bounds) + .then_some(()); + + let mut diag = tcx.dcx().create_err(errors::UnusedGenericParameter { + 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, +} + +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> 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, param_def_id: DefId, diff --git a/compiler/rustc_hir_analysis/src/errors.rs b/compiler/rustc_hir_analysis/src/errors.rs index 2edd8f54adb..cec295e1c15 100644 --- a/compiler/rustc_hir_analysis/src/errors.rs +++ b/compiler/rustc_hir_analysis/src/errors.rs @@ -1597,6 +1597,8 @@ 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, #[subdiagnostic] pub help: UnusedGenericParameterHelp, #[help(hir_analysis_const_param_help)] diff --git a/tests/ui/lazy-type-alias/inherent-impls-overflow.next.stderr b/tests/ui/lazy-type-alias/inherent-impls-overflow.next.stderr index 192b5eebdaa..48e7da96612 100644 --- a/tests/ui/lazy-type-alias/inherent-impls-overflow.next.stderr +++ b/tests/ui/lazy-type-alias/inherent-impls-overflow.next.stderr @@ -4,27 +4,27 @@ error[E0275]: overflow evaluating the requirement `Loop == _` LL | impl Loop {} | ^^^^ -error: type parameter `T` is only used recursively - --> $DIR/inherent-impls-overflow.rs:14:24 +error[E0392]: type parameter `T` is never used + --> $DIR/inherent-impls-overflow.rs:14:12 | LL | type Poly0 = Poly1<(T,)>; - | - ^ + | ^ - `T` is named here, but is likely unused in the containing type | | - | type parameter must be used non-recursively in the definition + | unused type parameter | = help: consider removing `T` or referring to it in the body of the type alias - = note: all type parameters must be used in a non-recursive way in order to constrain their variance + = help: if you intended `T` to be a const parameter, use `const T: /* Type */` instead -error: type parameter `T` is only used recursively - --> $DIR/inherent-impls-overflow.rs:17:24 +error[E0392]: type parameter `T` is never used + --> $DIR/inherent-impls-overflow.rs:17:12 | LL | type Poly1 = Poly0<(T,)>; - | - ^ + | ^ - `T` is named here, but is likely unused in the containing type | | - | type parameter must be used non-recursively in the definition + | unused type parameter | = help: consider removing `T` or referring to it in the body of the type alias - = note: all type parameters must be used in a non-recursive way in order to constrain their variance + = help: if you intended `T` to be a const parameter, use `const T: /* Type */` instead error[E0275]: overflow evaluating the requirement `Poly0<()> == _` --> $DIR/inherent-impls-overflow.rs:21:6 @@ -36,4 +36,5 @@ LL | impl Poly0<()> {} error: aborting due to 4 previous errors -For more information about this error, try `rustc --explain E0275`. +Some errors have detailed explanations: E0275, E0392. +For more information about an error, try `rustc --explain E0275`. diff --git a/tests/ui/lazy-type-alias/inherent-impls-overflow.rs b/tests/ui/lazy-type-alias/inherent-impls-overflow.rs index 1397695a3fe..98f0d811a47 100644 --- a/tests/ui/lazy-type-alias/inherent-impls-overflow.rs +++ b/tests/ui/lazy-type-alias/inherent-impls-overflow.rs @@ -13,10 +13,10 @@ impl Loop {} type Poly0 = Poly1<(T,)>; //[current]~^ ERROR overflow normalizing the type alias `Poly0<(((((((...,),),),),),),)>` -//[next]~^^ ERROR type parameter `T` is only used recursively +//[next]~^^ ERROR type parameter `T` is never used type Poly1 = Poly0<(T,)>; //[current]~^ ERROR overflow normalizing the type alias `Poly1<(((((((...,),),),),),),)>` -//[next]~^^ ERROR type parameter `T` is only used recursively +//[next]~^^ ERROR type parameter `T` is never used impl Poly0<()> {} //[current]~^ ERROR overflow normalizing the type alias `Poly1<(((((((...,),),),),),),)>` diff --git a/tests/ui/variance/variance-unused-type-param.rs b/tests/ui/variance/variance-unused-type-param.rs index ada57ab0d09..ef3c41ca556 100644 --- a/tests/ui/variance/variance-unused-type-param.rs +++ b/tests/ui/variance/variance-unused-type-param.rs @@ -28,4 +28,9 @@ struct WithWhereBounds where T: Sized {} struct WithOutlivesBounds {} //~^ ERROR parameter `T` is never used +struct DoubleNothing { +//~^ ERROR parameter `T` is never used + s: SomeStruct, +} + fn main() {} diff --git a/tests/ui/variance/variance-unused-type-param.stderr b/tests/ui/variance/variance-unused-type-param.stderr index 1a45bcba45a..c747532e628 100644 --- a/tests/ui/variance/variance-unused-type-param.stderr +++ b/tests/ui/variance/variance-unused-type-param.stderr @@ -62,6 +62,18 @@ LL | struct WithOutlivesBounds {} | = help: consider removing `T`, referring to it in a field, or using a marker such as `PhantomData` -error: aborting due to 7 previous errors +error[E0392]: type parameter `T` is never used + --> $DIR/variance-unused-type-param.rs:31:22 + | +LL | struct DoubleNothing { + | ^ unused type parameter +LL | +LL | s: SomeStruct, + | - `T` is named here, but is likely unused in the containing type + | + = 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 + +error: aborting due to 8 previous errors For more information about this error, try `rustc --explain E0392`.