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

Comma after struct expansion is a syntax error #41834

Closed
Xion opened this issue May 8, 2017 · 6 comments
Closed

Comma after struct expansion is a syntax error #41834

Xion opened this issue May 8, 2017 · 6 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. WG-diagnostics Working group: Diagnostics

Comments

@Xion
Copy link

Xion commented May 8, 2017

On rustc 1.17:

struct Foo {
    one: i32,
    two: i32,
    three: i32,
}

impl Default for Foo {
    fn default() -> Self {
        Foo { one : 1, two: 2, three: 3 }
    }
}

fn main() {
    // this works
    let foo = Foo {
        one: 11,
        ..Foo::default()
    };
    // this is a syntax error
    let foo = Foo {
        one: 111,
        ..Foo::default(),
    };
}

(playground link: https://is.gd/2wvenA)

This feels very inconsistent, as Rust has no problems with extra commas anywhere else. It is also an extremely "natural" mistake to make, so even if it's rightly signaled & easily corrected, it is still quite annoying.

@F001
Copy link
Contributor

F001 commented May 9, 2017

I agree this is annoying and need to be fixed.
@petrochenkov , I guess this minor issue doesn't require RFC process, right?
This issue seems related to https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/parser.rs#L2369 . I'll submit a fix soon.

@qnighy
Copy link
Contributor

qnighy commented May 9, 2017

Rust has no problems with extra commas anywhere else.

Just for remark: the ... in extern "C" { fn foo(x: &str, ...); } doesn't allow a trailing comma either (in current Rust stable/nightly). I like the trailing comma for struct update syntax though!

@nikomatsakis
Copy link
Contributor

So we discussed this some in the @rust-lang/lang meeting. I would say that opinions were mixed, but in general I think that we are inclined (for now) to leave this as is.

The arguments basically were as follows. First, everyone agreed that we actively do not want to allow ..Foo anywhere but at the end of a struct expression.

Given that, the argument in favor of allowing commas is essentially that people might expect to put it there because they always end lines with a comma (habit, essentially).

The argument against is that a trailing comma's main purpose is to avoid messy diffs and simplify macro authors lives, and @petrochenkov made a convincing case that neither applies here. Moreover, a trailing comma suggests that you can move things around in the list, and that you might put something afterwards, neither of which are true here. So including the comma is somewhat misleading.

Therefore, I think we're inclined to leave the grammar as is, and prohibit trailing commas after a ..Foo syntax in a struct.

@Mark-Simulacrum Mark-Simulacrum added A-parser Area: The parsing of Rust source code to an AST A-diagnostics Area: Messages for errors, warnings, and lints labels Jun 23, 2017
@Mark-Simulacrum
Copy link
Member

Leaving this open to track improving the diagnostic here.

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 26, 2017
@estebank
Copy link
Contributor

estebank commented Oct 2, 2017

In order to avoid multiple errors after finding this comma, as it happens now, the comma has to be specifically accepted and a diagnostic generated for this case in particular, along the lines of

error: can't use a comma after struct expansion
  --> src/main.rs:22:25
   |
22 |         ..Foo::default(),
   |                         ^ help: remove this comma

The method libsyntax/parse/parser.rs:Parser::parse_struct_expr needs to be modified to accept expressions that end in a comma, but generate and emit a diagnostic error. You might need to modify the call to self.parse_expr and you can look at self.parse_field to see what might be the difference in handling. Adding an err.span_suggestion_short(comma_span, "remove this comma", "".to_owned()) to the generated error is enough to add the help line as shown above.

@estebank estebank added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. hacktoberfest WG-diagnostics Working group: Diagnostics labels Oct 2, 2017
kennytm added a commit to kennytm/rust that referenced this issue Oct 13, 2017
…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.
@Phlosioneer
Copy link
Contributor

Triage: I think this should be closed now, because #45178 has now been merged into master as of 3 days ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. WG-diagnostics Working group: Diagnostics
Projects
None yet
Development

No branches or pull requests

8 participants