-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a detailed note for missing comma typo w/ FRU syntax #104504
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
hir_typeck_fru_note = this expression may have been misinterpreted as a `..` range expression | ||
hir_typeck_fru_expr = this expression does not end in a comma... | ||
hir_typeck_fru_expr2 = ... so this is interpreted as a `..` range expression, instead of functional record update syntax | ||
hir_typeck_fru_suggestion = | ||
to set the remaining fields{$expr -> | ||
[NONE]{""} | ||
*[other] {" "}from `{$expr}` | ||
}, separate the last named field with a comma |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
use crate::cast; | ||
use crate::coercion::CoerceMany; | ||
use crate::coercion::DynamicCoerceMany; | ||
use crate::errors::TypeMismatchFruTypo; | ||
use crate::errors::{AddressOfTemporaryTaken, ReturnStmtOutsideOfFnBody, StructExprNonExhaustive}; | ||
use crate::errors::{ | ||
FieldMultiplySpecifiedInInitializer, FunctionalRecordUpdateOnNonStruct, | ||
|
@@ -1614,10 +1615,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |
self.demand_coerce_diag(&field.expr, ty, field_type, None, AllowTwoPhase::No); | ||
|
||
if let Some(mut diag) = diag { | ||
if idx == ast_fields.len() - 1 && remaining_fields.is_empty() { | ||
self.suggest_fru_from_range(field, variant, substs, &mut diag); | ||
if idx == ast_fields.len() - 1 { | ||
if remaining_fields.is_empty() { | ||
self.suggest_fru_from_range(field, variant, substs, &mut diag); | ||
diag.emit(); | ||
} else { | ||
diag.stash(field.span, StashKey::MaybeFruTypo); | ||
} | ||
} else { | ||
diag.emit(); | ||
} | ||
diag.emit(); | ||
} | ||
} | ||
|
||
|
@@ -1875,19 +1882,39 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |
.map(|adt| adt.did()) | ||
!= range_def_id | ||
{ | ||
let instead = self | ||
// Suppress any range expr type mismatches | ||
if let Some(mut diag) = self | ||
.tcx | ||
.sess | ||
.diagnostic() | ||
.steal_diagnostic(last_expr_field.span, StashKey::MaybeFruTypo) | ||
{ | ||
diag.delay_as_bug(); | ||
} | ||
|
||
// Use a (somewhat arbitrary) filtering heuristic to avoid printing | ||
// expressions that are either too long, or have control character | ||
//such as newlines in them. | ||
let expr = self | ||
.tcx | ||
.sess | ||
.source_map() | ||
.span_to_snippet(range_end.expr.span) | ||
.map(|s| format!(" from `{s}`")) | ||
.unwrap_or_default(); | ||
err.span_suggestion( | ||
range_start.span.shrink_to_hi(), | ||
&format!("to set the remaining fields{instead}, separate the last named field with a comma"), | ||
",", | ||
Applicability::MaybeIncorrect, | ||
); | ||
.ok() | ||
.filter(|s| s.len() < 25 && !s.contains(|c: char| c.is_control())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As someone new to this part of the codebase, this line is unclear to me. What's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to avoid rendering very long expressions in "to set the remaining fields in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 25 is an arbitrary number. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense :) Perhaps leave a comment to this effect? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure thing. |
||
|
||
let fru_span = self | ||
.tcx | ||
.sess | ||
.source_map() | ||
.span_extend_while(range_start.span, |c| c.is_whitespace()) | ||
.unwrap_or(range_start.span).shrink_to_hi().to(range_end.span); | ||
|
||
err.subdiagnostic(TypeMismatchFruTypo { | ||
expr_span: range_start.span, | ||
fru_span, | ||
expr, | ||
}); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
#[derive(Default)] | ||
struct Inner { | ||
a: u8, | ||
b: u8, | ||
} | ||
|
||
#[derive(Default)] | ||
struct Outer { | ||
inner: Inner, | ||
defaulted: u8, | ||
} | ||
|
||
fn main(){ | ||
Outer { | ||
//~^ ERROR missing field `defaulted` in initializer of `Outer` | ||
inner: Inner { | ||
a: 1, | ||
b: 2, | ||
} | ||
..Default::default() | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
error[E0063]: missing field `defaulted` in initializer of `Outer` | ||
--> $DIR/multi-line-fru-suggestion.rs:14:5 | ||
| | ||
LL | Outer { | ||
| ^^^^^ missing `defaulted` | ||
| | ||
note: this expression may have been misinterpreted as a `..` range expression | ||
--> $DIR/multi-line-fru-suggestion.rs:16:16 | ||
| | ||
LL | inner: Inner { | ||
| ________________^ | ||
LL | | a: 1, | ||
LL | | b: 2, | ||
LL | | } | ||
| |_________^ this expression does not end in a comma... | ||
LL | ..Default::default() | ||
| ^^^^^^^^^^^^^^^^^^^^ ... so this is interpreted as a `..` range expression, instead of functional record update syntax | ||
help: to set the remaining fields from `Default::default()`, separate the last named field with a comma | ||
| | ||
LL | }, | ||
| + | ||
|
||
error: aborting due to previous error | ||
|
||
For more information about this error, try `rustc --explain E0063`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,37 +1,38 @@ | ||
error[E0308]: mismatched types | ||
--> $DIR/struct-record-suggestion.rs:10:20 | ||
| | ||
LL | let q = A { c: 5..Default::default() }; | ||
| ^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found struct `std::ops::Range` | ||
| | ||
= note: expected type `u64` | ||
found struct `std::ops::Range<{integer}>` | ||
|
||
error[E0063]: missing fields `b` and `d` in initializer of `A` | ||
--> $DIR/struct-record-suggestion.rs:10:13 | ||
| | ||
LL | let q = A { c: 5..Default::default() }; | ||
| ^ missing `b` and `d` | ||
| | ||
note: this expression may have been misinterpreted as a `..` range expression | ||
--> $DIR/struct-record-suggestion.rs:10:20 | ||
| | ||
LL | let q = A { c: 5..Default::default() }; | ||
| ^^^^^^^^^^^^^^^^^^^^^ | ||
help: to set the remaining fields from `Default::default()`, separate the last named field with a comma | ||
| | ||
LL | let q = A { c: 5,..Default::default() }; | ||
LL | let q = A { c: 5, ..Default::default() }; | ||
| + | ||
|
||
error[E0308]: mismatched types | ||
--> $DIR/struct-record-suggestion.rs:24:20 | ||
--> $DIR/struct-record-suggestion.rs:23:20 | ||
| | ||
LL | let q = B { b: 1..Default::default() }; | ||
| ^^^^^^^^^^^^^^^^^^^^^ expected `u32`, found struct `std::ops::Range` | ||
| | ||
= note: expected type `u32` | ||
found struct `std::ops::Range<{integer}>` | ||
note: this expression may have been misinterpreted as a `..` range expression | ||
--> $DIR/struct-record-suggestion.rs:23:20 | ||
| | ||
LL | let q = B { b: 1..Default::default() }; | ||
| ^^^^^^^^^^^^^^^^^^^^^ | ||
help: to set the remaining fields from `Default::default()`, separate the last named field with a comma | ||
| | ||
LL | let q = B { b: 1,..Default::default() }; | ||
LL | let q = B { b: 1, ..Default::default() }; | ||
| + | ||
|
||
error: aborting due to 3 previous errors | ||
error: aborting due to 2 previous errors | ||
|
||
Some errors have detailed explanations: E0063, E0308. | ||
For more information about an error, try `rustc --explain E0063`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"functional record update syntax" is pretty heavy on jargon (and this is an error beginners will regularly see). Google search for it turns up nothing unless you add Rust, in which case you get this RFC: https://rust-lang.github.io/rfcs/2528-type-changing-struct-update-syntax.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the error message is already pretty long, and it doesn't wrap well -- if you're against the jargon, I would rather remove everything after
instead of
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not too keen on the duplication of "used to set the remaining fields of a struct" between the note and the suggestion's "to set the remaining fields"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see your point! I think it's better left as is then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "struct update syntax"? I've heard that quite often when talking about FRU.