Skip to content
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

Merged
merged 1 commit into from
Nov 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions compiler/rustc_error_messages/locales/en-US/hir_typeck.ftl
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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
hir_typeck_fru_expr2 = ... so this is interpreted as a `..` range expression, instead of functional record update syntax
hir_typeck_fru_expr2 = ... so this is interpreted as a `..` range expression, instead of functional record update syntax, used to set the remaining fields of a struct

Copy link
Member Author

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.

Copy link
Member Author

@compiler-errors compiler-errors Nov 16, 2022

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"

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.

Copy link
Member

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.

hir_typeck_fru_suggestion =
to set the remaining fields{$expr ->
[NONE]{""}
*[other] {" "}from `{$expr}`
}, separate the last named field with a comma
1 change: 1 addition & 0 deletions compiler/rustc_error_messages/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ fluent_messages! {
errors => "../locales/en-US/errors.ftl",
expand => "../locales/en-US/expand.ftl",
hir_analysis => "../locales/en-US/hir_analysis.ftl",
hir_typeck => "../locales/en-US/hir_typeck.ftl",
infer => "../locales/en-US/infer.ftl",
interface => "../locales/en-US/interface.ftl",
lint => "../locales/en-US/lint.ftl",
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,9 @@ pub enum StashKey {
/// When an invalid lifetime e.g. `'2` should be reinterpreted
/// as a char literal in the parser
LifetimeIsChar,
/// Maybe there was a typo where a comma was forgotten before
/// FRU syntax
MaybeFruTypo,
}

fn default_track_diagnostic(_: &Diagnostic) {}
Expand Down
39 changes: 39 additions & 0 deletions compiler/rustc_hir_typeck/src/errors.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
//! Errors emitted by `rustc_hir_analysis`.
use rustc_errors::{AddToDiagnostic, Applicability, Diagnostic, MultiSpan, SubdiagnosticMessage};
use rustc_macros::{Diagnostic, Subdiagnostic};
use rustc_middle::ty::Ty;
use rustc_span::{symbol::Ident, Span};
Expand Down Expand Up @@ -133,3 +134,41 @@ pub struct OpMethodGenericParams {
pub span: Span,
pub method_name: String,
}

pub struct TypeMismatchFruTypo {
/// Span of the LHS of the range
pub expr_span: Span,
/// Span of the `..RHS` part of the range
pub fru_span: Span,
/// Rendered expression of the RHS of the range
pub expr: Option<String>,
}

impl AddToDiagnostic for TypeMismatchFruTypo {
fn add_to_diagnostic_with<F>(self, diag: &mut Diagnostic, _: F)
where
F: Fn(&mut Diagnostic, SubdiagnosticMessage) -> SubdiagnosticMessage,
{
diag.set_arg("expr", self.expr.as_deref().unwrap_or("NONE"));

// Only explain that `a ..b` is a range if it's split up
if self.expr_span.between(self.fru_span).is_empty() {
diag.span_note(
self.expr_span.to(self.fru_span),
rustc_errors::fluent::hir_typeck_fru_note,
);
} else {
let mut multispan: MultiSpan = vec![self.expr_span, self.fru_span].into();
multispan.push_span_label(self.expr_span, rustc_errors::fluent::hir_typeck_fru_expr);
multispan.push_span_label(self.fru_span, rustc_errors::fluent::hir_typeck_fru_expr2);
diag.span_note(multispan, rustc_errors::fluent::hir_typeck_fru_note);
}

diag.span_suggestion(
self.expr_span.shrink_to_hi(),
rustc_errors::fluent::hir_typeck_fru_suggestion,
", ",
Applicability::MaybeIncorrect,
);
}
}
51 changes: 39 additions & 12 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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();
}
}

Expand Down Expand Up @@ -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()));

Choose a reason for hiding this comment

The 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 s, and why do we filter it like this? Why 25?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 $EXPR"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

25 is an arbitrary number.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And is_control is to avoid rendering something like:

to set the remaining fields in `a
.b`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense :) Perhaps leave a comment to this effect?

Copy link
Member Author

Choose a reason for hiding this comment

The 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,
});
}
}

Expand Down
22 changes: 22 additions & 0 deletions src/test/ui/structs/multi-line-fru-suggestion.rs
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()
};
}
25 changes: 25 additions & 0 deletions src/test/ui/structs/multi-line-fru-suggestion.stderr
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`.
7 changes: 3 additions & 4 deletions src/test/ui/structs/struct-record-suggestion.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@ struct A {
}

fn a() {
let q = A { c: 5,..Default::default() };
//~^ ERROR mismatched types
//~| ERROR missing fields
let q = A { c: 5, ..Default::default() };
//~^ ERROR missing fields
//~| HELP separate the last named field with a comma
let r = A { c: 5, ..Default::default() };
assert_eq!(q, r);
Expand All @@ -21,7 +20,7 @@ struct B {
}

fn b() {
let q = B { b: 1,..Default::default() };
let q = B { b: 1, ..Default::default() };
//~^ ERROR mismatched types
//~| HELP separate the last named field with a comma
let r = B { b: 1 };
Expand Down
3 changes: 1 addition & 2 deletions src/test/ui/structs/struct-record-suggestion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ struct A {

fn a() {
let q = A { c: 5..Default::default() };
//~^ ERROR mismatched types
//~| ERROR missing fields
//~^ ERROR missing fields
//~| HELP separate the last named field with a comma
let r = A { c: 5, ..Default::default() };
assert_eq!(q, r);
Expand Down
27 changes: 14 additions & 13 deletions src/test/ui/structs/struct-record-suggestion.stderr
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`.