From de04c05dea10fc03cb626338ce1d55f4c90f58b0 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 30 Nov 2021 21:30:05 -0800 Subject: [PATCH] Specialize suggestion for Option --- .../src/traits/error_reporting/mod.rs | 63 +++++++++++--- .../rustc_ty_utils/src/representability.rs | 70 ++++++++++----- compiler/rustc_typeck/src/check/check.rs | 2 +- src/test/ui/issues/issue-17431-1.stderr | 4 +- src/test/ui/issues/issue-17431-2.stderr | 8 +- src/test/ui/issues/issue-17431-4.stderr | 4 +- src/test/ui/issues/issue-17431-7.stderr | 4 +- src/test/ui/issues/issue-3779.stderr | 4 +- src/test/ui/sized-cycle-note.stderr | 8 +- src/test/ui/span/E0072.stderr | 4 +- src/test/ui/span/multiline-span-E0072.stderr | 4 +- .../struct-rec/mutual-struct-recursion.stderr | 8 +- src/test/ui/type/type-recursive.rs | 26 +++++- src/test/ui/type/type-recursive.stderr | 85 ++++++++++++++++++- 14 files changed, 231 insertions(+), 63 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs index 468c7a3c55b..53acffc5098 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs @@ -2285,10 +2285,10 @@ impl<'v> Visitor<'v> for FindTypeParam { } } -pub fn recursive_type_with_infinite_size_error( - tcx: TyCtxt<'_>, +pub fn recursive_type_with_infinite_size_error<'tcx>( + tcx: TyCtxt<'tcx>, type_def_id: DefId, - spans: Vec, + spans: Vec<(Span, Option)>, ) { assert!(type_def_id.is_local()); let span = tcx.hir().span_if_local(type_def_id).unwrap(); @@ -2297,7 +2297,7 @@ pub fn recursive_type_with_infinite_size_error( let mut err = struct_span_err!(tcx.sess, span, E0072, "recursive type `{}` has infinite size", path); err.span_label(span, "recursive type has infinite size"); - for &span in &spans { + for &(span, _) in &spans { err.span_label(span, "recursive without indirection"); } let msg = format!( @@ -2305,16 +2305,25 @@ pub fn recursive_type_with_infinite_size_error( path, ); if spans.len() <= 4 { + // FIXME: This suggestion might be erroneous if Option or Box are shadowed err.multipart_suggestion( &msg, spans - .iter() - .flat_map(|&span| { - [ - (span.shrink_to_lo(), "Box<".to_string()), - (span.shrink_to_hi(), ">".to_string()), - ] - .into_iter() + .into_iter() + .flat_map(|(span, field_id)| { + if let Some(generic_span) = get_option_generic_from_field_id(tcx, field_id) { + // If we match an `Option` and can grab the span of the Option's generic, then + // suggest boxing the generic arg for a non-null niche optimization. + vec![ + (generic_span.shrink_to_lo(), "Box<".to_string()), + (generic_span.shrink_to_hi(), ">".to_string()), + ] + } else { + vec![ + (span.shrink_to_lo(), "Box<".to_string()), + (span.shrink_to_hi(), ">".to_string()), + ] + } }) .collect(), Applicability::HasPlaceholders, @@ -2325,6 +2334,38 @@ pub fn recursive_type_with_infinite_size_error( err.emit(); } +/// Extract the span for the generic type `T` of `Option` in a field definition +fn get_option_generic_from_field_id(tcx: TyCtxt<'_>, field_id: Option) -> Option { + let node = tcx.hir().find(field_id?); + + // Expect a field from our field_id + let Some(hir::Node::Field(field_def)) = node + else { bug!("Expected HirId corresponding to FieldDef, found: {:?}", node) }; + + // Match a type that is a simple QPath with no Self + let hir::TyKind::Path(hir::QPath::Resolved(None, path)) = &field_def.ty.kind + else { return None }; + + // Check if the path we're checking resolves to Option + let hir::def::Res::Def(_, did) = path.res + else { return None }; + + // Bail if this path doesn't describe `::core::option::Option` + if !tcx.is_diagnostic_item(sym::Option, did) { + return None; + } + + // Match a single generic arg in the 0th path segment + let generic_arg = path.segments.get(0)?.args?.args.get(0); + + // Take the span out of the type, if it's a type + if let Some(hir::GenericArg::Type(generic_ty)) = generic_arg { + Some(generic_ty.span) + } else { + None + } +} + /// Summarizes information #[derive(Clone)] pub enum ArgKind { diff --git a/compiler/rustc_ty_utils/src/representability.rs b/compiler/rustc_ty_utils/src/representability.rs index b8f3efe6462..7efc82efd15 100644 --- a/compiler/rustc_ty_utils/src/representability.rs +++ b/compiler/rustc_ty_utils/src/representability.rs @@ -17,12 +17,20 @@ use std::cmp; pub enum Representability { Representable, ContainsRecursive, - SelfRecursive(Vec), + /// Return a list of types that are included in themselves: + /// the spans where they are self-included, and (if found) + /// the HirId of the FieldDef that defines the self-inclusion. + SelfRecursive(Vec<(Span, Option)>), } /// Check whether a type is representable. This means it cannot contain unboxed /// structural recursion. This check is needed for structs and enums. -pub fn ty_is_representable<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, sp: Span) -> Representability { +pub fn ty_is_representable<'tcx>( + tcx: TyCtxt<'tcx>, + ty: Ty<'tcx>, + sp: Span, + field_id: Option, +) -> Representability { debug!("is_type_representable: {:?}", ty); // To avoid a stack overflow when checking an enum variant or struct that // contains a different, structurally recursive type, maintain a stack of @@ -38,11 +46,12 @@ pub fn ty_is_representable<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, sp: Span) -> R let mut force_result = false; let r = is_type_structurally_recursive( tcx, - sp, &mut seen, &mut shadow_seen, &mut representable_cache, ty, + sp, + field_id, &mut force_result, ); debug!("is_type_representable: {:?} is {:?}", ty, r); @@ -61,11 +70,12 @@ fn fold_repr>(iter: It) -> Representabilit fn are_inner_types_recursive<'tcx>( tcx: TyCtxt<'tcx>, - sp: Span, seen: &mut Vec>, shadow_seen: &mut Vec>, representable_cache: &mut FxHashMap, Representability>, ty: Ty<'tcx>, + sp: Span, + field_id: Option, force_result: &mut bool, ) -> Representability { debug!("are_inner_types_recursive({:?}, {:?}, {:?})", ty, seen, shadow_seen); @@ -75,11 +85,12 @@ fn are_inner_types_recursive<'tcx>( fold_repr(fields.iter().map(|ty| { is_type_structurally_recursive( tcx, - sp, seen, shadow_seen, representable_cache, ty, + sp, + field_id, force_result, ) })) @@ -88,20 +99,26 @@ fn are_inner_types_recursive<'tcx>( // FIXME(#11924) Behavior undecided for zero-length vectors. ty::Array(ty, _) => is_type_structurally_recursive( tcx, - sp, seen, shadow_seen, representable_cache, *ty, + sp, + field_id, force_result, ), ty::Adt(def, substs) => { // Find non representable fields with their spans fold_repr(def.all_fields().map(|field| { let ty = field.ty(tcx, substs); - let span = match field.did.as_local().and_then(|id| tcx.hir().find_by_def_id(id)) { - Some(hir::Node::Field(field)) => field.ty.span, - _ => sp, + let (sp, field_id) = match field + .did + .as_local() + .map(|id| tcx.hir().local_def_id_to_hir_id(id)) + .and_then(|id| tcx.hir().find(id)) + { + Some(hir::Node::Field(field)) => (field.ty.span, Some(field.hir_id)), + _ => (sp, field_id), }; let mut result = None; @@ -130,7 +147,7 @@ fn are_inner_types_recursive<'tcx>( // result without adjusting). if shadow_seen.len() > seen.len() && shadow_seen.first() == Some(def) { *force_result = true; - result = Some(Representability::SelfRecursive(vec![span])); + result = Some(Representability::SelfRecursive(vec![(sp, field_id)])); } if result == None { @@ -161,16 +178,17 @@ fn are_inner_types_recursive<'tcx>( result = Some( match is_type_structurally_recursive( tcx, - span, &mut nested_seen, shadow_seen, representable_cache, raw_adt_ty, + sp, + field_id, force_result, ) { Representability::SelfRecursive(_) => { if *force_result { - Representability::SelfRecursive(vec![span]) + Representability::SelfRecursive(vec![(sp, field_id)]) } else { Representability::ContainsRecursive } @@ -208,15 +226,16 @@ fn are_inner_types_recursive<'tcx>( result = Some( match is_type_structurally_recursive( tcx, - span, seen, shadow_seen, representable_cache, ty, + sp, + field_id, force_result, ) { Representability::SelfRecursive(_) => { - Representability::SelfRecursive(vec![span]) + Representability::SelfRecursive(vec![(sp, field_id)]) } x => x, }, @@ -247,29 +266,31 @@ fn same_adt<'tcx>(ty: Ty<'tcx>, def: ty::AdtDef<'tcx>) -> bool { // contain any types on stack `seen`? fn is_type_structurally_recursive<'tcx>( tcx: TyCtxt<'tcx>, - sp: Span, seen: &mut Vec>, shadow_seen: &mut Vec>, representable_cache: &mut FxHashMap, Representability>, ty: Ty<'tcx>, + sp: Span, + field_id: Option, force_result: &mut bool, ) -> Representability { - debug!("is_type_structurally_recursive: {:?} {:?}", ty, sp); + debug!("is_type_structurally_recursive: {:?} {:?} {:?}", ty, sp, field_id); if let Some(representability) = representable_cache.get(&ty) { debug!( - "is_type_structurally_recursive: {:?} {:?} - (cached) {:?}", - ty, sp, representability + "is_type_structurally_recursive: {:?} {:?} {:?} - (cached) {:?}", + ty, sp, field_id, representability ); return representability.clone(); } let representability = is_type_structurally_recursive_inner( tcx, - sp, seen, shadow_seen, representable_cache, ty, + sp, + field_id, force_result, ); @@ -279,11 +300,12 @@ fn is_type_structurally_recursive<'tcx>( fn is_type_structurally_recursive_inner<'tcx>( tcx: TyCtxt<'tcx>, - sp: Span, seen: &mut Vec>, shadow_seen: &mut Vec>, representable_cache: &mut FxHashMap, Representability>, ty: Ty<'tcx>, + sp: Span, + field_id: Option, force_result: &mut bool, ) -> Representability { match ty.kind() { @@ -305,7 +327,7 @@ fn is_type_structurally_recursive_inner<'tcx>( if let Some(&seen_adt) = iter.next() { if same_adt(seen_adt, *def) { debug!("SelfRecursive: {:?} contains {:?}", seen_adt, ty); - return Representability::SelfRecursive(vec![sp]); + return Representability::SelfRecursive(vec![(sp, field_id)]); } } @@ -335,11 +357,12 @@ fn is_type_structurally_recursive_inner<'tcx>( shadow_seen.push(*def); let out = are_inner_types_recursive( tcx, - sp, seen, shadow_seen, representable_cache, ty, + sp, + field_id, force_result, ); shadow_seen.pop(); @@ -350,11 +373,12 @@ fn is_type_structurally_recursive_inner<'tcx>( // No need to push in other cases. are_inner_types_recursive( tcx, - sp, seen, shadow_seen, representable_cache, ty, + sp, + field_id, force_result, ) } diff --git a/compiler/rustc_typeck/src/check/check.rs b/compiler/rustc_typeck/src/check/check.rs index 5362ca8d719..7cb478d7888 100644 --- a/compiler/rustc_typeck/src/check/check.rs +++ b/compiler/rustc_typeck/src/check/check.rs @@ -1045,7 +1045,7 @@ pub(super) fn check_representable(tcx: TyCtxt<'_>, sp: Span, item_def_id: LocalD // recursive type. It is only necessary to throw an error on those that // contain themselves. For case 2, there must be an inner type that will be // caught by case 1. - match representability::ty_is_representable(tcx, rty, sp) { + match representability::ty_is_representable(tcx, rty, sp, None) { Representability::SelfRecursive(spans) => { recursive_type_with_infinite_size_error(tcx, item_def_id.to_def_id(), spans); return false; diff --git a/src/test/ui/issues/issue-17431-1.stderr b/src/test/ui/issues/issue-17431-1.stderr index 4d739a3823b..db32eb952ba 100644 --- a/src/test/ui/issues/issue-17431-1.stderr +++ b/src/test/ui/issues/issue-17431-1.stderr @@ -8,8 +8,8 @@ LL | struct Foo { foo: Option> } | help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Foo` representable | -LL | struct Foo { foo: Box>> } - | ++++ + +LL | struct Foo { foo: Option>> } + | ++++ + error: aborting due to previous error diff --git a/src/test/ui/issues/issue-17431-2.stderr b/src/test/ui/issues/issue-17431-2.stderr index e53134c5238..d23fd1474ac 100644 --- a/src/test/ui/issues/issue-17431-2.stderr +++ b/src/test/ui/issues/issue-17431-2.stderr @@ -8,8 +8,8 @@ LL | struct Baz { q: Option } | help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Baz` representable | -LL | struct Baz { q: Box> } - | ++++ + +LL | struct Baz { q: Option> } + | ++++ + error[E0072]: recursive type `Foo` has infinite size --> $DIR/issue-17431-2.rs:4:1 @@ -21,8 +21,8 @@ LL | struct Foo { q: Option } | help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Foo` representable | -LL | struct Foo { q: Box> } - | ++++ + +LL | struct Foo { q: Option> } + | ++++ + error: aborting due to 2 previous errors diff --git a/src/test/ui/issues/issue-17431-4.stderr b/src/test/ui/issues/issue-17431-4.stderr index f5aeeec7337..ddf669b8fd1 100644 --- a/src/test/ui/issues/issue-17431-4.stderr +++ b/src/test/ui/issues/issue-17431-4.stderr @@ -8,8 +8,8 @@ LL | struct Foo { foo: Option>>, marker: marker::PhantomData | help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Foo` representable | -LL | struct Foo { foo: Box>>>, marker: marker::PhantomData } - | ++++ + +LL | struct Foo { foo: Option>>>, marker: marker::PhantomData } + | ++++ + error: aborting due to previous error diff --git a/src/test/ui/issues/issue-17431-7.stderr b/src/test/ui/issues/issue-17431-7.stderr index aeaadaa6917..6f8a7e3867b 100644 --- a/src/test/ui/issues/issue-17431-7.stderr +++ b/src/test/ui/issues/issue-17431-7.stderr @@ -8,8 +8,8 @@ LL | enum Foo { Voo(Option>) } | help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Foo` representable | -LL | enum Foo { Voo(Box>>) } - | ++++ + +LL | enum Foo { Voo(Option>>) } + | ++++ + error: aborting due to previous error diff --git a/src/test/ui/issues/issue-3779.stderr b/src/test/ui/issues/issue-3779.stderr index 6a21472848a..e853d0f8c89 100644 --- a/src/test/ui/issues/issue-3779.stderr +++ b/src/test/ui/issues/issue-3779.stderr @@ -9,8 +9,8 @@ LL | element: Option | help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `S` representable | -LL | element: Box> - | ++++ + +LL | element: Option> + | ++++ + error: aborting due to previous error diff --git a/src/test/ui/sized-cycle-note.stderr b/src/test/ui/sized-cycle-note.stderr index 0726b16a919..536510814c5 100644 --- a/src/test/ui/sized-cycle-note.stderr +++ b/src/test/ui/sized-cycle-note.stderr @@ -8,8 +8,8 @@ LL | struct Baz { q: Option } | help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Baz` representable | -LL | struct Baz { q: Box> } - | ++++ + +LL | struct Baz { q: Option> } + | ++++ + error[E0072]: recursive type `Foo` has infinite size --> $DIR/sized-cycle-note.rs:11:1 @@ -21,8 +21,8 @@ LL | struct Foo { q: Option } | help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Foo` representable | -LL | struct Foo { q: Box> } - | ++++ + +LL | struct Foo { q: Option> } + | ++++ + error: aborting due to 2 previous errors diff --git a/src/test/ui/span/E0072.stderr b/src/test/ui/span/E0072.stderr index 98e92751360..882ed577cf3 100644 --- a/src/test/ui/span/E0072.stderr +++ b/src/test/ui/span/E0072.stderr @@ -9,8 +9,8 @@ LL | tail: Option, | help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `ListNode` representable | -LL | tail: Box>, - | ++++ + +LL | tail: Option>, + | ++++ + error: aborting due to previous error diff --git a/src/test/ui/span/multiline-span-E0072.stderr b/src/test/ui/span/multiline-span-E0072.stderr index cb71a55b09e..acfc60b51f3 100644 --- a/src/test/ui/span/multiline-span-E0072.stderr +++ b/src/test/ui/span/multiline-span-E0072.stderr @@ -12,8 +12,8 @@ LL | | } | help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `ListNode` representable | -LL | tail: Box>, - | ++++ + +LL | tail: Option>, + | ++++ + error: aborting due to previous error diff --git a/src/test/ui/structs-enums/struct-rec/mutual-struct-recursion.stderr b/src/test/ui/structs-enums/struct-rec/mutual-struct-recursion.stderr index c6d2434e424..80a494f3f65 100644 --- a/src/test/ui/structs-enums/struct-rec/mutual-struct-recursion.stderr +++ b/src/test/ui/structs-enums/struct-rec/mutual-struct-recursion.stderr @@ -37,8 +37,8 @@ LL | y: Option>>, | help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `C` representable | -LL | y: Box>>>, - | ++++ + +LL | y: Option>>>, + | ++++ + error[E0072]: recursive type `D` has infinite size --> $DIR/mutual-struct-recursion.rs:18:1 @@ -51,8 +51,8 @@ LL | z: Option>>, | help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `D` representable | -LL | z: Box>>>, - | ++++ + +LL | z: Option>>>, + | ++++ + error: aborting due to 4 previous errors diff --git a/src/test/ui/type/type-recursive.rs b/src/test/ui/type/type-recursive.rs index 8558f761ee8..e8084f0d082 100644 --- a/src/test/ui/type/type-recursive.rs +++ b/src/test/ui/type/type-recursive.rs @@ -1,6 +1,30 @@ struct T1 { //~ ERROR E0072 foo: isize, - foolish: T1 + foolish: T1, +} + +struct T2 { //~ ERROR E0072 + inner: Option, +} + +type OptionT3 = Option; + +struct T3 { //~ ERROR E0072 + inner: OptionT3, +} + +struct T4(Option); //~ ERROR E0072 + +enum T5 { //~ ERROR E0072 + Variant(Option), +} + +enum T6 { //~ ERROR E0072 + Variant{ field: Option }, +} + +struct T7 { //~ ERROR E0072 + foo: std::cell::Cell>, } fn main() { } diff --git a/src/test/ui/type/type-recursive.stderr b/src/test/ui/type/type-recursive.stderr index 5a94a0fd683..04392f7390d 100644 --- a/src/test/ui/type/type-recursive.stderr +++ b/src/test/ui/type/type-recursive.stderr @@ -4,14 +4,93 @@ error[E0072]: recursive type `T1` has infinite size LL | struct T1 { | ^^^^^^^^^ recursive type has infinite size LL | foo: isize, -LL | foolish: T1 +LL | foolish: T1, | -- recursive without indirection | help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `T1` representable | -LL | foolish: Box +LL | foolish: Box, | ++++ + -error: aborting due to previous error +error[E0072]: recursive type `T2` has infinite size + --> $DIR/type-recursive.rs:6:1 + | +LL | struct T2 { + | ^^^^^^^^^ recursive type has infinite size +LL | inner: Option, + | ---------- recursive without indirection + | +help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `T2` representable + | +LL | inner: Option>, + | ++++ + + +error[E0072]: recursive type `T3` has infinite size + --> $DIR/type-recursive.rs:12:1 + | +LL | struct T3 { + | ^^^^^^^^^ recursive type has infinite size +LL | inner: OptionT3, + | -------- recursive without indirection + | +help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `T3` representable + | +LL | inner: Box, + | ++++ + + +error[E0072]: recursive type `T4` has infinite size + --> $DIR/type-recursive.rs:16:1 + | +LL | struct T4(Option); + | ^^^^^^^^^^----------^^ + | | | + | | recursive without indirection + | recursive type has infinite size + | +help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `T4` representable + | +LL | struct T4(Option>); + | ++++ + + +error[E0072]: recursive type `T5` has infinite size + --> $DIR/type-recursive.rs:18:1 + | +LL | enum T5 { + | ^^^^^^^ recursive type has infinite size +LL | Variant(Option), + | ---------- recursive without indirection + | +help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `T5` representable + | +LL | Variant(Option>), + | ++++ + + +error[E0072]: recursive type `T6` has infinite size + --> $DIR/type-recursive.rs:22:1 + | +LL | enum T6 { + | ^^^^^^^ recursive type has infinite size +LL | Variant{ field: Option }, + | ---------- recursive without indirection + | +help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `T6` representable + | +LL | Variant{ field: Option> }, + | ++++ + + +error[E0072]: recursive type `T7` has infinite size + --> $DIR/type-recursive.rs:26:1 + | +LL | struct T7 { + | ^^^^^^^^^ recursive type has infinite size +LL | foo: std::cell::Cell>, + | --------------------------- recursive without indirection + | +help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `T7` representable + | +LL | foo: Box>>, + | ++++ + + +error: aborting due to 7 previous errors For more information about this error, try `rustc --explain E0072`.