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

Wrong error message trying to initialise an empty struct #3192

Closed
bblum opened this issue Aug 13, 2012 · 10 comments
Closed

Wrong error message trying to initialise an empty struct #3192

bblum opened this issue Aug 13, 2012 · 10 comments
Labels
A-frontend Area: Compiler frontend (errors, parsing and HIR)

Comments

@bblum
Copy link
Contributor

bblum commented Aug 13, 2012

Updated bug report

Compiling this (erroneous) program:

struct mutex;
fn main() {
    let x = mutex { }; // this should be `let x = mutex;`
    println(fmt!("%?", x));
}

yields:

/tmp/foo.rs:3:18: 3:19 error: expected `;` but found `{`
/tmp/foo.rs:3     let x = mutex { };

We currently emit a nice error message if you incorrectly write the struct mutex definition as:

struct mutex { }

that tells you that you need to write the Unit-like struct as struct mutex;; we should do the same for the struct initialization expression.

Original bug report

struct mutex { }
fn main() {
    let x = mutex { };
}

gives:

struct.rs:3:18: 3:19 error: expected `;` but found `{`
struct.rs:3     let x = mutex { };

but if you comment out the let x line, it gives:

struct.rs:1:0: 1:16 error: a class must have at least one field
struct.rs:1 struct mutex { }

Personally, I think we should be allowed to have empty structs, but either way, the latter error message should have higher priority(?) than the former.

@catamorphism
Copy link
Contributor

Parsing happens before typechecking, and the "at least one field" error is a type error. So this won't change.

We discussed it a while ago and agreed to disallow empty structs, but if you want to reopen the discussion, feel free to file a separate RFC bug.

@bblum
Copy link
Contributor Author

bblum commented Aug 13, 2012

@catamorphism Maybe the parser could allow mutex { }, so that it doesn't fail until typechecking?

@catamorphism catamorphism reopened this Aug 13, 2012
@catamorphism
Copy link
Contributor

Sorry, I was reading too fast and didn't understand at first that the parser is essentially checking for the same thing that the typechecker is.

That seems reasonable to me.

@bblum
Copy link
Contributor Author

bblum commented Aug 22, 2012

I attempted to fix this. Something like (libsyntax/parse/parse.rs):

1007                 // This might be a struct literal.
1008                 if self.looking_at_record_literal() {
1009 +------ 27 lines: It's a struct literal.---------------------------------------------
1036                 } else if self.look_ahead(1) == token::RBRACE {   // (Code I added starts here)
1037                     // It's an empty struct literal.
1038                     // I'd handle this condition in looking_at_record, but
1039                     // empty records are ambiguous with empty blocks.
1040                     self.bump();
1041                     ex = expr_struct(pth, ~[], none);
1042                     return self.mk_pexpr(lo, hi, ex);   // (ends here)
1043                 }

But it seems like this makes the grammar ambiguous with empty-bodied 'do' statements -- do unkillable { } failed to parse.

I wonder if this will stop being ambiguous once type names have to be camel case?

@nikomatsakis
Copy link
Contributor

Type names will never have to be camel case---that's intended to be a convention. I think that's a known ambiguity that was supposed to be resolved by do (and for) favoring one interpretation. @pcwalton can confirm (I know we did discuss it). We could always require parentheses in do/for to remove the ambiguity, but that would mean:

do spawn() { ... }

Which, admittedly, is not so bad.

@bblum
Copy link
Contributor Author

bblum commented Aug 22, 2012

it occurred to me that the grammar might not have to be ambiguous after all, and it was just my fix that was wrong. we have something like:

e := { expr* }
   | do path[(expr*)] [|ident*|] expr
   | path { (ident:expr)* }

and do path { } got confused with path { }. i think to resolve it, either parsing do should use lookahead in the same way as my code above, or my code above should somehow have lower priority. the latter seems better, but i'm not sure how it would be written.

@pcwalton
Copy link
Contributor

I think this bug is just asking for a better parser error message when you try to create an empty structure. That doesn't seem backwards incompatible to me. Renominating.

@graydon
Copy link
Contributor

graydon commented Jun 13, 2013

just a bug, removing milestone/nomination.

@pnkfelix
Copy link
Member

pnkfelix commented Aug 8, 2013

Visiting for triage email from 2013-07-29.

I think I can revise the parser in a disciplined manner to put in a better error message (as pcwalton suggested). I spent some time Monday hacking on that, I'll have a draft up for people to look at soon.

@pnkfelix
Copy link
Member

reopening as reminder that I need to go back and fix up PR #8467

@pnkfelix pnkfelix reopened this Aug 20, 2013
@ghost ghost assigned pnkfelix Aug 20, 2013
pnkfelix added a commit to pnkfelix/rust that referenced this issue Oct 9, 2013
alexcrichton added a commit to alexcrichton/rust that referenced this issue Feb 3, 2014
RalfJung pushed a commit to RalfJung/rust that referenced this issue Feb 17, 2024
celinval pushed a commit to celinval/rust-dev that referenced this issue Jun 4, 2024
Our `Cargo.lock` uses https://crates.io/crates/libc/0.2.154, which has
been yanked. Do a `cargo update` to use a different version.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend Area: Compiler frontend (errors, parsing and HIR)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants