Better error message for comma after base struct #45178

Badel2 · 2017-10-10T16:46:36Z

This adds a better error for commas after the base struct:

let foo = Foo {
    one: 111,
    ..Foo::default(), // This comma is a syntax error
};

The current error is a generic expected one of ... which isn't beginner-friendly. My error looks like this:

error: cannot use a comma after the base struct
  --> tmp/example.rs:26:9
   |
26 |         ..Foo::default(),
   |         ^^^^^^^^^^^^^^^^- help: remove this comma
   |
   = note: the base struct expansion must always be the last field

I even added a note for people who don't know why this isn't allowed.

rust-highfive · 2017-10-10T16:46:48Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

scottmcm

This looks like a great improvement!

I think it should have a test, though? I guess it'd be a parse-fail test?

scottmcm · 2017-10-10T21:39:43Z

src/libsyntax/parse/parser.rs

+                        "cannot use a comma after struct expansion",
+                    );
+                    err.span_suggestion_short(self.span, "remove this comma", "".to_owned());
+                    err.note("the struct expansion should always be the last field");


nitpick: It must be the last field, not just should, right?

Alright, thanks, I will add a test. Should I call it issue-41834.rs or struct-expansion.rs? (not sure what's the correct term for the ..default())

petrochenkov · 2017-10-10T23:31:04Z

src/test/parse-fail/struct-expansion.rs

@@ -0,0 +1,27 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT


Could you make this test an UI test?

petrochenkov · 2017-10-10T23:32:03Z

src/libsyntax/parse/parser.rs

@@ -2323,6 +2324,16 @@ impl<'a> Parser<'a> {
                        self.recover_stmt();
                    }
                }
+                if self.check(&token::Comma) {


Nit: check adds the token into the "expected tokens" set, self.token == &token::Comma should be enough here.

petrochenkov · 2017-10-10T23:37:16Z

src/libsyntax/parse/parser.rs

+                        "cannot use a comma after struct expansion",
+                    );
+                    err.span_suggestion_short(self.span, "remove this comma", "".to_owned());
+                    err.note("the struct expansion must always be the last field");


"the" shouldn't be here if we talk about struct expansions in general?

Actually, this is the first time I see the name "struct expansion".
Looks like it was a @Xion's invention in #41834.

I'm not even sure how this thing is called (..expr specifically, the whole Struct { field1, field2, ..expr } is usually called a "functional record update").
What the book/reference/etc say?

The book refers to this as "field init shorthand", or "field init syntax", however still not sure how we call the item. The first edition of the book refers to all this as "update syntax", so I'm not even sure if that is the official name.

Anyway I suggest we just call it the "base struct":
"cannot use a comma after the base struct"
"the base struct must always be the last field"
(I use "the" because there is only one base struct)

Ok, sounds good.

`let x = { ..default(), } // This comma is an error`

petrochenkov · 2017-10-11T08:27:58Z

@bors r+

bors · 2017-10-11T08:27:58Z

📌 Commit 72cfd20 has been approved by petrochenkov

bors · 2017-10-11T08:27:59Z

🌲 The tree is currently closed for pull requests below priority 2, this pull request will be tested once the tree is reopened

Mark-Simulacrum · 2017-10-13T14:38:38Z

@bors rollup

…henkov Better error message for comma after base struct rust-lang#41834 This adds a better error for commas after the base struct: ``` let foo = Foo { one: 111, ..Foo::default(), // This comma is a syntax error }; ``` The current error is a generic `expected one of ...` which isn't beginner-friendly. My error looks like this: ``` error: cannot use a comma after the base struct --> tmp/example.rs:26:9 | 26 | ..Foo::default(), | ^^^^^^^^^^^^^^^^- help: remove this comma | = note: the base struct expansion must always be the last field ``` I even added a note for people who don't know why this isn't allowed.

Rollup of 14 pull requests - Successful merges: #44855, #45110, #45122, #45133, #45173, #45178, #45189, #45203, #45209, #45221, #45236, #45240, #45245, #45253 - Failed merges:

rust-highfive assigned nikomatsakis Oct 10, 2017

petrochenkov assigned petrochenkov and unassigned nikomatsakis Oct 10, 2017

scottmcm reviewed Oct 10, 2017

View reviewed changes

Badel2 force-pushed the comma-after-struct branch 2 times, most recently from 4d5e257 to 1a0c551 Compare October 10, 2017 23:23

petrochenkov reviewed Oct 10, 2017

View reviewed changes

Badel2 force-pushed the comma-after-struct branch from 1a0c551 to ee46c26 Compare October 11, 2017 01:11

Add error for comma after base struct field

72cfd20

`let x = { ..default(), } // This comma is an error`

Badel2 force-pushed the comma-after-struct branch from ee46c26 to 72cfd20 Compare October 11, 2017 01:13

Badel2 changed the title ~~Better error message for comma after struct expansion~~ Better error message for comma after base struct Oct 11, 2017

aidanhs added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 12, 2017

kennytm mentioned this pull request Oct 13, 2017

Rollup of 14 pull requests #45261

Merged

bors merged commit 72cfd20 into rust-lang:master Oct 13, 2017

Phlosioneer mentioned this pull request Oct 16, 2017

Comma after struct expansion is a syntax error #41834

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error message for comma after base struct #45178

Better error message for comma after base struct #45178

Badel2 commented Oct 10, 2017 •

edited

Loading

rust-highfive commented Oct 10, 2017

scottmcm left a comment

scottmcm Oct 10, 2017

Badel2 Oct 10, 2017

petrochenkov Oct 10, 2017

petrochenkov Oct 10, 2017

petrochenkov Oct 10, 2017

petrochenkov Oct 10, 2017

petrochenkov Oct 10, 2017

Badel2 Oct 11, 2017 •

edited

Loading

petrochenkov Oct 11, 2017

petrochenkov commented Oct 11, 2017

bors commented Oct 11, 2017

bors commented Oct 11, 2017

Mark-Simulacrum commented Oct 13, 2017

		@@ -0,0 +1,27 @@
		// Copyright 2017 The Rust Project Developers. See the COPYRIGHT

Better error message for comma after base struct #45178

Better error message for comma after base struct #45178

Conversation

Badel2 commented Oct 10, 2017 • edited Loading

rust-highfive commented Oct 10, 2017

scottmcm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badel2 Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Oct 11, 2017

bors commented Oct 11, 2017

bors commented Oct 11, 2017

Mark-Simulacrum commented Oct 13, 2017

Badel2 commented Oct 10, 2017 •

edited

Loading

Badel2 Oct 11, 2017 •

edited

Loading