Skip to content

Commit de04c05

Browse files
compiler-errorscompiler-errors
committed
Specialize suggestion for Option<T>
1 parent df20355 commit de04c05

File tree

14 files changed

+231
-63
lines changed

14 files changed

+231
-63
lines changed

‎compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

+52-11
Original file line numberDiff line numberDiff line change
@@ -2285,10 +2285,10 @@ impl<'v> Visitor<'v> for FindTypeParam {
22852285
}
22862286
}
22872287

2288-
pub fn recursive_type_with_infinite_size_error(
2289-
tcx: TyCtxt<'_>,
2288+
pub fn recursive_type_with_infinite_size_error<'tcx>(
2289+
tcx: TyCtxt<'tcx>,
22902290
type_def_id: DefId,
2291-
spans: Vec<Span>,
2291+
spans: Vec<(Span, Option<hir::HirId>)>,
22922292
) {
22932293
assert!(type_def_id.is_local());
22942294
let span = tcx.hir().span_if_local(type_def_id).unwrap();
@@ -2297,24 +2297,33 @@ pub fn recursive_type_with_infinite_size_error(
22972297
let mut err =
22982298
struct_span_err!(tcx.sess, span, E0072, "recursive type `{}` has infinite size", path);
22992299
err.span_label(span, "recursive type has infinite size");
2300-
for &span in &spans {
2300+
for &(span, _) in &spans {
23012301
err.span_label(span, "recursive without indirection");
23022302
}
23032303
let msg = format!(
23042304
"insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `{}` representable",
23052305
path,
23062306
);
23072307
if spans.len() <= 4 {
2308+
// FIXME: This suggestion might be erroneous if Option or Box are shadowed
23082309
err.multipart_suggestion(
23092310
&msg,
23102311
spans
2311-
.iter()
2312-
.flat_map(|&span| {
2313-
[
2314-
(span.shrink_to_lo(), "Box<".to_string()),
2315-
(span.shrink_to_hi(), ">".to_string()),
2316-
]
2317-
.into_iter()
2312+
.into_iter()
2313+
.flat_map(|(span, field_id)| {
2314+
if let Some(generic_span) = get_option_generic_from_field_id(tcx, field_id) {
2315+
// If we match an `Option` and can grab the span of the Option's generic, then
2316+
// suggest boxing the generic arg for a non-null niche optimization.
2317+
vec![
2318+
(generic_span.shrink_to_lo(), "Box<".to_string()),
2319+
(generic_span.shrink_to_hi(), ">".to_string()),
2320+
]
2321+
} else {
2322+
vec![
2323+
(span.shrink_to_lo(), "Box<".to_string()),
2324+
(span.shrink_to_hi(), ">".to_string()),
2325+
]
2326+
}
23182327
})
23192328
.collect(),
23202329
Applicability::HasPlaceholders,
@@ -2325,6 +2334,38 @@ pub fn recursive_type_with_infinite_size_error(
23252334
err.emit();
23262335
}
23272336

2337+
/// Extract the span for the generic type `T` of `Option<T>` in a field definition
2338+
fn get_option_generic_from_field_id(tcx: TyCtxt<'_>, field_id: Option<hir::HirId>) -> Option<Span> {
2339+
let node = tcx.hir().find(field_id?);
2340+
2341+
// Expect a field from our field_id
2342+
let Some(hir::Node::Field(field_def)) = node
2343+
else { bug!("Expected HirId corresponding to FieldDef, found: {:?}", node) };
2344+
2345+
// Match a type that is a simple QPath with no Self
2346+
let hir::TyKind::Path(hir::QPath::Resolved(None, path)) = &field_def.ty.kind
2347+
else { return None };
2348+
2349+
// Check if the path we're checking resolves to Option
2350+
let hir::def::Res::Def(_, did) = path.res
2351+
else { return None };
2352+
2353+
// Bail if this path doesn't describe `::core::option::Option`
2354+
if !tcx.is_diagnostic_item(sym::Option, did) {
2355+
return None;
2356+
}
2357+
2358+
// Match a single generic arg in the 0th path segment
2359+
let generic_arg = path.segments.get(0)?.args?.args.get(0);
2360+
2361+
// Take the span out of the type, if it's a type
2362+
if let Some(hir::GenericArg::Type(generic_ty)) = generic_arg {
2363+
Some(generic_ty.span)
2364+
} else {
2365+
None
2366+
}
2367+
}
2368+
23282369
/// Summarizes information
23292370
#[derive(Clone)]
23302371
pub enum ArgKind {

‎compiler/rustc_ty_utils/src/representability.rs

+47-23
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,20 @@ use std::cmp;
1717
pub enum Representability {
1818
Representable,
1919
ContainsRecursive,
20-
SelfRecursive(Vec<Span>),
20+
/// Return a list of types that are included in themselves:
21+
/// the spans where they are self-included, and (if found)
22+
/// the HirId of the FieldDef that defines the self-inclusion.
23+
SelfRecursive(Vec<(Span, Option<hir::HirId>)>),
2124
}
2225

2326
/// Check whether a type is representable. This means it cannot contain unboxed
2427
/// structural recursion. This check is needed for structs and enums.
25-
pub fn ty_is_representable<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, sp: Span) -> Representability {
28+
pub fn ty_is_representable<'tcx>(
29+
tcx: TyCtxt<'tcx>,
30+
ty: Ty<'tcx>,
31+
sp: Span,
32+
field_id: Option<hir::HirId>,
33+
) -> Representability {
2634
debug!("is_type_representable: {:?}", ty);
2735
// To avoid a stack overflow when checking an enum variant or struct that
2836
// 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
3846
let mut force_result = false;
3947
let r = is_type_structurally_recursive(
4048
tcx,
41-
sp,
4249
&mut seen,
4350
&mut shadow_seen,
4451
&mut representable_cache,
4552
ty,
53+
sp,
54+
field_id,
4655
&mut force_result,
4756
);
4857
debug!("is_type_representable: {:?} is {:?}", ty, r);
@@ -61,11 +70,12 @@ fn fold_repr<It: Iterator<Item = Representability>>(iter: It) -> Representabilit
6170

6271
fn are_inner_types_recursive<'tcx>(
6372
tcx: TyCtxt<'tcx>,
64-
sp: Span,
6573
seen: &mut Vec<Ty<'tcx>>,
6674
shadow_seen: &mut Vec<ty::AdtDef<'tcx>>,
6775
representable_cache: &mut FxHashMap<Ty<'tcx>, Representability>,
6876
ty: Ty<'tcx>,
77+
sp: Span,
78+
field_id: Option<hir::HirId>,
6979
force_result: &mut bool,
7080
) -> Representability {
7181
debug!("are_inner_types_recursive({:?}, {:?}, {:?})", ty, seen, shadow_seen);
@@ -75,11 +85,12 @@ fn are_inner_types_recursive<'tcx>(
7585
fold_repr(fields.iter().map(|ty| {
7686
is_type_structurally_recursive(
7787
tcx,
78-
sp,
7988
seen,
8089
shadow_seen,
8190
representable_cache,
8291
ty,
92+
sp,
93+
field_id,
8394
force_result,
8495
)
8596
}))
@@ -88,20 +99,26 @@ fn are_inner_types_recursive<'tcx>(
8899
// FIXME(#11924) Behavior undecided for zero-length vectors.
89100
ty::Array(ty, _) => is_type_structurally_recursive(
90101
tcx,
91-
sp,
92102
seen,
93103
shadow_seen,
94104
representable_cache,
95105
*ty,
106+
sp,
107+
field_id,
96108
force_result,
97109
),
98110
ty::Adt(def, substs) => {
99111
// Find non representable fields with their spans
100112
fold_repr(def.all_fields().map(|field| {
101113
let ty = field.ty(tcx, substs);
102-
let span = match field.did.as_local().and_then(|id| tcx.hir().find_by_def_id(id)) {
103-
Some(hir::Node::Field(field)) => field.ty.span,
104-
_ => sp,
114+
let (sp, field_id) = match field
115+
.did
116+
.as_local()
117+
.map(|id| tcx.hir().local_def_id_to_hir_id(id))
118+
.and_then(|id| tcx.hir().find(id))
119+
{
120+
Some(hir::Node::Field(field)) => (field.ty.span, Some(field.hir_id)),
121+
_ => (sp, field_id),
105122
};
106123

107124
let mut result = None;
@@ -130,7 +147,7 @@ fn are_inner_types_recursive<'tcx>(
130147
// result without adjusting).
131148
if shadow_seen.len() > seen.len() && shadow_seen.first() == Some(def) {
132149
*force_result = true;
133-
result = Some(Representability::SelfRecursive(vec![span]));
150+
result = Some(Representability::SelfRecursive(vec![(sp, field_id)]));
134151
}
135152

136153
if result == None {
@@ -161,16 +178,17 @@ fn are_inner_types_recursive<'tcx>(
161178
result = Some(
162179
match is_type_structurally_recursive(
163180
tcx,
164-
span,
165181
&mut nested_seen,
166182
shadow_seen,
167183
representable_cache,
168184
raw_adt_ty,
185+
sp,
186+
field_id,
169187
force_result,
170188
) {
171189
Representability::SelfRecursive(_) => {
172190
if *force_result {
173-
Representability::SelfRecursive(vec![span])
191+
Representability::SelfRecursive(vec![(sp, field_id)])
174192
} else {
175193
Representability::ContainsRecursive
176194
}
@@ -208,15 +226,16 @@ fn are_inner_types_recursive<'tcx>(
208226
result = Some(
209227
match is_type_structurally_recursive(
210228
tcx,
211-
span,
212229
seen,
213230
shadow_seen,
214231
representable_cache,
215232
ty,
233+
sp,
234+
field_id,
216235
force_result,
217236
) {
218237
Representability::SelfRecursive(_) => {
219-
Representability::SelfRecursive(vec![span])
238+
Representability::SelfRecursive(vec![(sp, field_id)])
220239
}
221240
x => x,
222241
},
@@ -247,29 +266,31 @@ fn same_adt<'tcx>(ty: Ty<'tcx>, def: ty::AdtDef<'tcx>) -> bool {
247266
// contain any types on stack `seen`?
248267
fn is_type_structurally_recursive<'tcx>(
249268
tcx: TyCtxt<'tcx>,
250-
sp: Span,
251269
seen: &mut Vec<Ty<'tcx>>,
252270
shadow_seen: &mut Vec<ty::AdtDef<'tcx>>,
253271
representable_cache: &mut FxHashMap<Ty<'tcx>, Representability>,
254272
ty: Ty<'tcx>,
273+
sp: Span,
274+
field_id: Option<hir::HirId>,
255275
force_result: &mut bool,
256276
) -> Representability {
257-
debug!("is_type_structurally_recursive: {:?} {:?}", ty, sp);
277+
debug!("is_type_structurally_recursive: {:?} {:?} {:?}", ty, sp, field_id);
258278
if let Some(representability) = representable_cache.get(&ty) {
259279
debug!(
260-
"is_type_structurally_recursive: {:?} {:?} - (cached) {:?}",
261-
ty, sp, representability
280+
"is_type_structurally_recursive: {:?} {:?} {:?} - (cached) {:?}",
281+
ty, sp, field_id, representability
262282
);
263283
return representability.clone();
264284
}
265285

266286
let representability = is_type_structurally_recursive_inner(
267287
tcx,
268-
sp,
269288
seen,
270289
shadow_seen,
271290
representable_cache,
272291
ty,
292+
sp,
293+
field_id,
273294
force_result,
274295
);
275296

@@ -279,11 +300,12 @@ fn is_type_structurally_recursive<'tcx>(
279300

280301
fn is_type_structurally_recursive_inner<'tcx>(
281302
tcx: TyCtxt<'tcx>,
282-
sp: Span,
283303
seen: &mut Vec<Ty<'tcx>>,
284304
shadow_seen: &mut Vec<ty::AdtDef<'tcx>>,
285305
representable_cache: &mut FxHashMap<Ty<'tcx>, Representability>,
286306
ty: Ty<'tcx>,
307+
sp: Span,
308+
field_id: Option<hir::HirId>,
287309
force_result: &mut bool,
288310
) -> Representability {
289311
match ty.kind() {
@@ -305,7 +327,7 @@ fn is_type_structurally_recursive_inner<'tcx>(
305327
if let Some(&seen_adt) = iter.next() {
306328
if same_adt(seen_adt, *def) {
307329
debug!("SelfRecursive: {:?} contains {:?}", seen_adt, ty);
308-
return Representability::SelfRecursive(vec![sp]);
330+
return Representability::SelfRecursive(vec![(sp, field_id)]);
309331
}
310332
}
311333

@@ -335,11 +357,12 @@ fn is_type_structurally_recursive_inner<'tcx>(
335357
shadow_seen.push(*def);
336358
let out = are_inner_types_recursive(
337359
tcx,
338-
sp,
339360
seen,
340361
shadow_seen,
341362
representable_cache,
342363
ty,
364+
sp,
365+
field_id,
343366
force_result,
344367
);
345368
shadow_seen.pop();
@@ -350,11 +373,12 @@ fn is_type_structurally_recursive_inner<'tcx>(
350373
// No need to push in other cases.
351374
are_inner_types_recursive(
352375
tcx,
353-
sp,
354376
seen,
355377
shadow_seen,
356378
representable_cache,
357379
ty,
380+
sp,
381+
field_id,
358382
force_result,
359383
)
360384
}

‎compiler/rustc_typeck/src/check/check.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1045,7 +1045,7 @@ pub(super) fn check_representable(tcx: TyCtxt<'_>, sp: Span, item_def_id: LocalD
10451045
// recursive type. It is only necessary to throw an error on those that
10461046
// contain themselves. For case 2, there must be an inner type that will be
10471047
// caught by case 1.
1048-
match representability::ty_is_representable(tcx, rty, sp) {
1048+
match representability::ty_is_representable(tcx, rty, sp, None) {
10491049
Representability::SelfRecursive(spans) => {
10501050
recursive_type_with_infinite_size_error(tcx, item_def_id.to_def_id(), spans);
10511051
return false;

‎src/test/ui/issues/issue-17431-1.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ LL | struct Foo { foo: Option<Option<Foo>> }
88
|
99
help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Foo` representable
1010
|
11-
LL | struct Foo { foo: Box<Option<Option<Foo>>> }
12-
| ++++ +
11+
LL | struct Foo { foo: Option<Box<Option<Foo>>> }
12+
| ++++ +
1313

1414
error: aborting due to previous error
1515

‎src/test/ui/issues/issue-17431-2.stderr

+4-4
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ LL | struct Baz { q: Option<Foo> }
88
|
99
help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Baz` representable
1010
|
11-
LL | struct Baz { q: Box<Option<Foo>> }
12-
| ++++ +
11+
LL | struct Baz { q: Option<Box<Foo>> }
12+
| ++++ +
1313

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

2727
error: aborting due to 2 previous errors
2828

‎src/test/ui/issues/issue-17431-4.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ LL | struct Foo<T> { foo: Option<Option<Foo<T>>>, marker: marker::PhantomData<T>
88
|
99
help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Foo` representable
1010
|
11-
LL | struct Foo<T> { foo: Box<Option<Option<Foo<T>>>>, marker: marker::PhantomData<T> }
12-
| ++++ +
11+
LL | struct Foo<T> { foo: Option<Box<Option<Foo<T>>>>, marker: marker::PhantomData<T> }
12+
| ++++ +
1313

1414
error: aborting due to previous error
1515

‎src/test/ui/issues/issue-17431-7.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ LL | enum Foo { Voo(Option<Option<Foo>>) }
88
|
99
help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `Foo` representable
1010
|
11-
LL | enum Foo { Voo(Box<Option<Option<Foo>>>) }
12-
| ++++ +
11+
LL | enum Foo { Voo(Option<Box<Option<Foo>>>) }
12+
| ++++ +
1313

1414
error: aborting due to previous error
1515

0 commit comments

Comments
 (0)