rustc: fix ICE regarding struct variants in enums

[yorkie]

Hi, this is inspired by the output of previous rustc output:

note: the compiler hit an unexpected failure path. this is a bug.
note: we would appreciate a bug report: http://doc.rust-lang.org/complement-bugreport.html
note: run with `RUST_BACKTRACE=1` for a backtrace
task 'rustc' failed at 'assertion failed: fields.len() > 0', /Users/rustbuild/src/rust-buildbot/slave/nightly-mac/build/src/librustc/middle/ty.rs:3657

This unexpected failure will scare off someone IMO, right, then such that a better error print would be better :p

[yorkie]

and someone wrote this:

enum {
  Foo {}
}

really maybe a usual case :(

[aochagavia]

Did you mean "definition" instead of "defination"?

[aochagavia]

By the way... I think it would be more informative for the reader if you followed the style of the error a couple of lines below (enum_variants: all fields in struct must have a name). What about enum_variants: struct variant must have at least one field?

EDIT: ignore this comment and read what huonw wrote below

[aochagavia]

Additionaly, I think you will need to use cx.sess.span_err(ast_variant.span, "error message") instead of cx.sess.fatal.

EDIT: ignore this comment and read what huonw wrote below

[huonw]

I recommend cx.sess.span_fatal(ast_variant.span, "struct variants must have at least one field");.

I think it would be invalid to continue compilation at this point, so one must use fatal rather than err (which just prints an error message, and causes compilation to stop at some future point, but not immediately). Also, the enum_variants: ... warning in the error message below is only appropriate there, since that one is a bug (the parser shouldn't let through any UnnamedFields in a struct variant), while this is a user-facing message, and users don't care about the compiler-internals function which emits the error message.

[aochagavia]

For anyone reviewing this, an example of the ICE is available in the playpen

@yorkie A better title for the pull request would be "Fix ICE regarding struct variants in enums". ICE means internal compiler error.

[huonw]

Also, this should have a test, add a file to src/test/compile-fail, e.g. struct-variant-no-fields.rs:

// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

enum Foo {
    Bar { } //~ ERROR struct variant must have at least one field
}

fn main() {}

See https://github.com/rust-lang/rust/wiki/Note-testsuite#language-and-compiler-tests for more details.

[huonw]

The travis build failed in make tidy because this line has trailing whitespace, can you remove the whitespace?

[aochagavia]

@huonw don't forget the #![feature(struct_variant)]... In the playpen it doesn't compile without it.

[yorkie]

thanks guys so much, @aochagavia and @huonw.
plz review my new commit :p

[yorkie]

and plus, i have changed the title of my pr/commit to: rustc: fix ICE regarding struct variants in enums

[sfackler]

There's a tidy issue: https://travis-ci.org/rust-lang/rust/jobs/30350054

/home/travis/build/rust-lang/rust/src/librustc/middle/ty.rs:3728: line longer than 100 chars

[yorkie]

done :p

[alexcrichton]

The error above has "struct variants must have" whereas this error says "struct variant must have"

[yorkie]

Hi, @alexcrichton, i have changed this in ty.rs, now LGTY?

[yorkie]

hi, the travis is green :)

[luqmana]

@yorkie I don't think this should be done in librustc but rather libsyntax. What's the point of accepting it in the parser if we're just gonna error out later. The relevant bit in libsyntax would be parse_enum_def in libsyntax/parse/parser.rs. I think the check would make more sense there.

[yorkie]

i dont 100% agreed on this, because this error breaks here, do we really need to move this patch to the parsing phase? i need more from u

[alexcrichton]

Closing due to inactivity, but feel free to reopen with comments addressed!