Skip to content

Suggest using an appropriate keyword for struct and enum #94996

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Suggest using an appropriate keyword for struct and enum #94996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eff6598
suggest using a appropriate keyword for `struct` and `enum`
TaKO8Ki Mar 16, 2022
f50da2e
fix derive-bad.stderr
TaKO8Ki Mar 16, 2022
f49601d
not suggest if `span` originates in a derive-macro's expansion
TaKO8Ki Mar 20, 2022
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
110 changes: 89 additions & 21 deletions compiler/rustc_parse/src/parser/item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,11 +273,13 @@ impl<'a> Parser<'a> {
// TYPE ITEM
self.parse_type_alias(def())?
} else if self.eat_keyword(kw::Enum) {
let start_span = self.prev_token.span;
// ENUM ITEM
self.parse_item_enum()?
self.parse_item_enum(start_span)?
} else if self.eat_keyword(kw::Struct) {
let start_span = self.prev_token.span;
// STRUCT ITEM
self.parse_item_struct()?
self.parse_item_struct(start_span)?
} else if self.is_kw_followed_by_ident(kw::Union) {
// UNION ITEM
self.bump(); // `union`
Expand Down Expand Up @@ -1199,13 +1201,14 @@ impl<'a> Parser<'a> {
}

/// Parses an enum declaration.
fn parse_item_enum(&mut self) -> PResult<'a, ItemInfo> {
fn parse_item_enum(&mut self, start_span: Span) -> PResult<'a, ItemInfo> {
Copy link
Member

@nagisa nagisa Mar 20, 2022

Choose a reason for hiding this comment

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

start_span seems a little misnomer here, given that the span begins at the enum or struct keyword, rather than covering the entire item (i.e. including the qualifiers such as pub). Maybe something like keyword_span would work better?

let id = self.parse_ident()?;
let mut generics = self.parse_generics()?;
generics.where_clause = self.parse_where_clause()?;

let (variants, _) =
self.parse_delim_comma_seq(token::Brace, |p| p.parse_enum_variant()).map_err(|e| {
let (variants, _) = self
.parse_delim_comma_seq(token::Brace, |p| p.parse_enum_variant(start_span))
.map_err(|e| {
self.recover_stmt();
e
})?;
Expand All @@ -1214,7 +1217,7 @@ impl<'a> Parser<'a> {
Ok((id, ItemKind::Enum(enum_definition, generics)))
}

fn parse_enum_variant(&mut self) -> PResult<'a, Option<Variant>> {
fn parse_enum_variant(&mut self, start_span: Span) -> PResult<'a, Option<Variant>> {
let variant_attrs = self.parse_outer_attributes()?;
self.collect_tokens_trailing_token(
variant_attrs,
Expand All @@ -1226,11 +1229,36 @@ impl<'a> Parser<'a> {
if !this.recover_nested_adt_item(kw::Enum)? {
return Ok((None, TrailingToken::None));
}
let enum_field_start_span = this.token.span;
let ident = this.parse_field_ident("enum", vlo)?;
if this.token.kind == token::Colon && !start_span.in_derive_expansion() {
let snapshot = this.clone();
this.bump();
match this.parse_ty() {
Ok(_) => {
let mut err = this.struct_span_err(
enum_field_start_span.to(this.prev_token.span),
"the enum cannot have a struct field declaration",
Copy link
Member

@nagisa nagisa Mar 20, 2022

Choose a reason for hiding this comment

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

Diagnostics in parser largely follow a “expected ...” style of messages. The suggested wording would likely be more consistent with the rest of the diagnostics coming from the parser.

Suggested change
"the enum cannot have a struct field declaration",
"expected an `enum` variant, found a field",

);
err.span_suggestion_verbose(
start_span,
"consider using `struct` instead of `enum`",
Copy link
Member

@nagisa nagisa Mar 20, 2022

Choose a reason for hiding this comment

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

Hm, an union could also make sense here, wouldn't it? While it is comparatively much rarer, looking to define an enum like this may very well be thinking of an union in the first place.

Suggested change
"consider using `struct` instead of `enum`",
"consider defining a `struct` or an `union` instead of an `enum`",

Not entirely sure how or if span_suggestion can provide multiple alternative replacements, though.

Copy link
Member

@nagisa nagisa Mar 20, 2022

Choose a reason for hiding this comment

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

It might very well be that this suggestion is largely superfluous with the improved error message.

"struct".to_string(),
Applicability::MaybeIncorrect,
);
return Err(err);
}
Err(e) => {
e.cancel();
*this = snapshot;
}
};
}

let struct_def = if this.check(&token::OpenDelim(token::Brace)) {
// Parse a struct variant.
let (fields, recovered) = this.parse_record_struct_body("struct", false)?;
let (fields, recovered) =
this.parse_record_struct_body("struct", false, None)?;
VariantData::Struct(fields, recovered)
} else if this.check(&token::OpenDelim(token::Paren)) {
VariantData::Tuple(this.parse_tuple_struct_body()?, DUMMY_NODE_ID)
Expand Down Expand Up @@ -1258,7 +1286,7 @@ impl<'a> Parser<'a> {
}

/// Parses `struct Foo { ... }`.
fn parse_item_struct(&mut self) -> PResult<'a, ItemInfo> {
fn parse_item_struct(&mut self, start_span: Span) -> PResult<'a, ItemInfo> {
let class_name = self.parse_ident()?;

let mut generics = self.parse_generics()?;
Expand All @@ -1284,17 +1312,23 @@ impl<'a> Parser<'a> {
VariantData::Unit(DUMMY_NODE_ID)
} else {
// If we see: `struct Foo<T> where T: Copy { ... }`
let (fields, recovered) =
self.parse_record_struct_body("struct", generics.where_clause.has_where_token)?;
let (fields, recovered) = self.parse_record_struct_body(
"struct",
generics.where_clause.has_where_token,
Some(start_span),
)?;
VariantData::Struct(fields, recovered)
}
// No `where` so: `struct Foo<T>;`
} else if self.eat(&token::Semi) {
VariantData::Unit(DUMMY_NODE_ID)
// Record-style struct definition
} else if self.token == token::OpenDelim(token::Brace) {
let (fields, recovered) =
self.parse_record_struct_body("struct", generics.where_clause.has_where_token)?;
let (fields, recovered) = self.parse_record_struct_body(
"struct",
generics.where_clause.has_where_token,
Some(start_span),
)?;
VariantData::Struct(fields, recovered)
// Tuple-style struct definition with optional where-clause.
} else if self.token == token::OpenDelim(token::Paren) {
Expand Down Expand Up @@ -1323,12 +1357,18 @@ impl<'a> Parser<'a> {

let vdata = if self.token.is_keyword(kw::Where) {
generics.where_clause = self.parse_where_clause()?;
let (fields, recovered) =
self.parse_record_struct_body("union", generics.where_clause.has_where_token)?;
let (fields, recovered) = self.parse_record_struct_body(
"union",
generics.where_clause.has_where_token,
None,
Copy link
Member

@nagisa nagisa Mar 20, 2022

Choose a reason for hiding this comment

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

If we're handling structs, we should also handle unions.

)?;
VariantData::Struct(fields, recovered)
} else if self.token == token::OpenDelim(token::Brace) {
let (fields, recovered) =
self.parse_record_struct_body("union", generics.where_clause.has_where_token)?;
let (fields, recovered) = self.parse_record_struct_body(
"union",
generics.where_clause.has_where_token,
None,
)?;
VariantData::Struct(fields, recovered)
} else {
let token_str = super::token_descr(&self.token);
Expand All @@ -1345,12 +1385,13 @@ impl<'a> Parser<'a> {
&mut self,
adt_ty: &str,
parsed_where: bool,
start_span: Option<Span>,
) -> PResult<'a, (Vec<FieldDef>, /* recovered */ bool)> {
let mut fields = Vec::new();
let mut recovered = false;
if self.eat(&token::OpenDelim(token::Brace)) {
while self.token != token::CloseDelim(token::Brace) {
let field = self.parse_field_def(adt_ty).map_err(|e| {
let field = self.parse_field_def(adt_ty, start_span).map_err(|e| {
self.consume_block(token::Brace, ConsumeClosingDelim::No);
recovered = true;
e
Expand Down Expand Up @@ -1413,12 +1454,15 @@ impl<'a> Parser<'a> {
}

/// Parses an element of a struct declaration.
fn parse_field_def(&mut self, adt_ty: &str) -> PResult<'a, FieldDef> {
fn parse_field_def(&mut self, adt_ty: &str, start_span: Option<Span>) -> PResult<'a, FieldDef> {
let attrs = self.parse_outer_attributes()?;
self.collect_tokens_trailing_token(attrs, ForceCollect::No, |this, attrs| {
let lo = this.token.span;
let vis = this.parse_visibility(FollowedByType::No)?;
Ok((this.parse_single_struct_field(adt_ty, lo, vis, attrs)?, TrailingToken::None))
Ok((
this.parse_single_struct_field(adt_ty, lo, vis, attrs, start_span)?,
TrailingToken::None,
))
})
}

Expand All @@ -1429,9 +1473,10 @@ impl<'a> Parser<'a> {
lo: Span,
vis: Visibility,
attrs: Vec<Attribute>,
start_span: Option<Span>,
) -> PResult<'a, FieldDef> {
let mut seen_comma: bool = false;
let a_var = self.parse_name_and_ty(adt_ty, lo, vis, attrs)?;
let a_var = self.parse_name_and_ty(adt_ty, lo, vis, attrs, start_span)?;
if self.token == token::Comma {
seen_comma = true;
}
Expand Down Expand Up @@ -1555,9 +1600,32 @@ impl<'a> Parser<'a> {
lo: Span,
vis: Visibility,
attrs: Vec<Attribute>,
start_span: Option<Span>,
) -> PResult<'a, FieldDef> {
let prev_token_kind = self.prev_token.kind.clone();
let name = self.parse_field_ident(adt_ty, lo)?;
self.expect_field_ty_separator()?;
if let Err(mut error) = self.expect_field_ty_separator() {
if (matches!(vis.kind, VisibilityKind::Inherited)
&& (prev_token_kind == token::Comma
|| prev_token_kind == token::OpenDelim(token::Brace)))
|| !matches!(vis.kind, VisibilityKind::Inherited)
{
let snapshot = self.clone();
if let Err(err) = self.parse_ty() {
if let Some(span) = start_span && !span.in_derive_expansion() {
error.span_suggestion_verbose(
span,
"consider using `enum` instead of `struct`",
"enum".to_string(),
Applicability::MaybeIncorrect,
);
}
err.cancel();
*self = snapshot;
}
}
return Err(error);
}
let ty = self.parse_ty()?;
if self.token.kind == token::Colon && self.look_ahead(1, |tok| tok.kind != token::Colon) {
self.struct_span_err(self.token.span, "found single colon in a struct field type path")
Expand Down
13 changes: 13 additions & 0 deletions src/test/ui/suggestions/suggest-using-appropriate-keyword-for-struct-and-enum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
pub struct NotStruct {
Variant
} //~ ERROR expected `:`, found `}`

pub struct NotStruct {
field String //~ ERROR expected `:`, found `String`
}

pub enum NotEnum {
field: u8 //~ ERROR the enum cannot have a struct field declaration
}

fn main() {}
32 changes: 32 additions & 0 deletions src/test/ui/suggestions/suggest-using-appropriate-keyword-for-struct-and-enum.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
error: expected `:`, found `}`
--> $DIR/suggest-using-appropriate-keyword-for-struct-and-enum.rs:3:1
|
LL | Variant
| - expected `:`
LL | }
| ^ unexpected token
|
help: consider using `enum` instead of `struct`
|
LL | pub enum NotStruct {
| ~~~~
Comment on lines +9 to +12
Copy link
Member

@nagisa nagisa Mar 20, 2022

Choose a reason for hiding this comment

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

I'm not entirely sure if we should be as trigger happy as proposed here about suggesting a different ADT type. I feel like it is much more likely that an ADT body will contain mistakes compared to user having specified a wrong kind of ADT.

Here it feels like the user was much more likely to just have forgetten to specify a type : _ for whatever reason. Though I could still see us making a suggestion as proposed here if there was at least one variant with data. That is in cases like these:

pub struct NotStruct {
    Variant,
    Variant2(u8),
}

Copy link
Contributor

@estebank estebank Mar 22, 2022

Choose a reason for hiding this comment

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

I agree with nagisa's comment. I think that for a case as ambiguous as these we might be better served by pointing at all the relevant places and let the user figure what they wanted. That could be done for cases like this one by adding a label to the struct keyword saying "while parsing this struct".

Copy link
Contributor

@estebank estebank Aug 25, 2022

Choose a reason for hiding this comment

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

The comment above is still valid. I would prefer to simplify the logic and "just" add a span_label pointing at the item type being parsed. The likelihood of false positives is too high as it is.


error: expected `:`, found `String`
--> $DIR/suggest-using-appropriate-keyword-for-struct-and-enum.rs:6:11
|
LL | field String
| ^^^^^^ expected `:`

error: the enum cannot have a struct field declaration
--> $DIR/suggest-using-appropriate-keyword-for-struct-and-enum.rs:10:5
|
LL | field: u8
| ^^^^^^^^^
|
help: consider using `struct` instead of `enum`
|
LL | pub struct NotEnum {
| ~~~~~~

error: aborting due to 3 previous errors