Rollup merge of #91416 - compiler-errors:infinite-ty-option-box, r=estebank

Specialize infinite-type "insert some indirection" suggestion for Option

Suggest `Option<Box<_>>` instead of `Box<Option<_>>` for infinitely-recursive members of a struct.

Not sure if I can get the span of the generic subty of the Option so I can make this a `+++`-style suggestion. The current output is a tiny bit less fancy looking than the original suggestion.

Should I limit the specialization to just `Option<Box<TheOuterStruct>>`? Because right now it applies to all `Option` members in the struct that are returned by `Representability::SelfRecursive`.

Fixes #91402

r? `@estebank`
(since you wrote the original suggestion and are definitely most familiar with it!)

Author: Dylan-DPC

Diff for: 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<Span>,
+    spans: Vec<(Span, Option<hir::HirId>)>,
 ) {
     assert!(type_def_id.is_local());
     let span = tcx.hir().span_if_local(type_def_id).unwrap();
@@ -2297,24 +2297,33 @@ 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!(
         "insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `{}` representable",
         path,
     );
     if spans.len() <= 4 {
+        // FIXME(compiler-errors): This suggestion might be erroneous if Box is 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,34 @@ pub fn recursive_type_with_infinite_size_error(
     err.emit();
 }
 
+/// Extract the span for the generic type `T` of `Option<T>` in a field definition
+fn get_option_generic_from_field_id(tcx: TyCtxt<'_>, field_id: Option<hir::HirId>) -> Option<Span> {
+    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.last()?.args?.args.get(0)?;
+
+    // Take the span out of the type, if it's a type
+    if let hir::GenericArg::Type(generic_ty) = generic_arg { Some(generic_ty.span) } else { None }
+}
+
 /// Summarizes information
 #[derive(Clone)]
 pub enum ArgKind {

Diff for: compiler/rustc_ty_utils/src/representability.rs

@@ -17,12 +17,20 @@ use std::cmp;
 pub enum Representability {
     Representable,
     ContainsRecursive,
-    SelfRecursive(Vec<Span>),
+    /// 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<hir::HirId>)>),
 }
 
 /// 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<hir::HirId>,
+) -> 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<It: Iterator<Item = Representability>>(iter: It) -> Representabilit
 
 fn are_inner_types_recursive<'tcx>(
     tcx: TyCtxt<'tcx>,
-    sp: Span,
     seen: &mut Vec<Ty<'tcx>>,
     shadow_seen: &mut Vec<ty::AdtDef<'tcx>>,
     representable_cache: &mut FxHashMap<Ty<'tcx>, Representability>,
     ty: Ty<'tcx>,
+    sp: Span,
+    field_id: Option<hir::HirId>,
     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<Ty<'tcx>>,
     shadow_seen: &mut Vec<ty::AdtDef<'tcx>>,
     representable_cache: &mut FxHashMap<Ty<'tcx>, Representability>,
     ty: Ty<'tcx>,
+    sp: Span,
+    field_id: Option<hir::HirId>,
     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<Ty<'tcx>>,
     shadow_seen: &mut Vec<ty::AdtDef<'tcx>>,
     representable_cache: &mut FxHashMap<Ty<'tcx>, Representability>,
     ty: Ty<'tcx>,
+    sp: Span,
+    field_id: Option<hir::HirId>,
     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 for: 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 for: src/test/ui/issues/issue-17431-1.stderr

@@ -8,8 +8,8 @@ LL | struct Foo { foo: Option<Option<Foo>> }
    |
 help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Foo` representable
    |
-LL | struct Foo { foo: Box<Option<Option<Foo>>> }
-   |                   ++++                   +
+LL | struct Foo { foo: Option<Box<Option<Foo>>> }
+   |                          ++++           +
 
 error: aborting due to previous error
 

Diff for: src/test/ui/issues/issue-17431-2.stderr

@@ -8,8 +8,8 @@ LL | struct Baz { q: Option<Foo> }
    |
 help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Baz` representable
    |
-LL | struct Baz { q: Box<Option<Foo>> }
-   |                 ++++           +
+LL | struct Baz { q: Option<Box<Foo>> }
+   |                        ++++   +
 
 error[E0072]: recursive type `Foo` has infinite size
   --> $DIR/issue-17431-2.rs:4:1
@@ -21,8 +21,8 @@ LL | struct Foo { q: Option<Baz> }
    |
 help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Foo` representable
    |
-LL | struct Foo { q: Box<Option<Baz>> }
-   |                 ++++           +
+LL | struct Foo { q: Option<Box<Baz>> }
+   |                        ++++   +
 
 error: aborting due to 2 previous errors
 

Diff for: src/test/ui/issues/issue-17431-4.stderr

@@ -8,8 +8,8 @@ LL | struct Foo<T> { foo: Option<Option<Foo<T>>>, marker: marker::PhantomData<T>
    |
 help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Foo` representable
    |
-LL | struct Foo<T> { foo: Box<Option<Option<Foo<T>>>>, marker: marker::PhantomData<T> }
-   |                      ++++                      +
+LL | struct Foo<T> { foo: Option<Box<Option<Foo<T>>>>, marker: marker::PhantomData<T> }
+   |                             ++++              +
 
 error: aborting due to previous error
 

Diff for: src/test/ui/issues/issue-17431-7.stderr

@@ -8,8 +8,8 @@ LL | enum Foo { Voo(Option<Option<Foo>>) }
    |
 help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Foo` representable
    |
-LL | enum Foo { Voo(Box<Option<Option<Foo>>>) }
-   |                ++++                   +
+LL | enum Foo { Voo(Option<Box<Option<Foo>>>) }
+   |                       ++++           +
 
 error: aborting due to previous error
 

Diff for: src/test/ui/issues/issue-3779.stderr

@@ -9,8 +9,8 @@ LL |     element: Option<S>
    |
 help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `S` representable
    |
-LL |     element: Box<Option<S>>
-   |              ++++         +
+LL |     element: Option<Box<S>>
+   |                     ++++ +
 
 error: aborting due to previous error