Suggest using an appropriate keyword for struct and enum

[TaKO8Ki]

closes #75770

[rust-highfive]

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

[nagisa]

Seems like this shouldn't happen, the suggested replacement has 0 chance to work at all.

[TaKO8Ki]

I fixed it.

[nagisa]

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),
}

[estebank]

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".

[estebank]

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.

[nagisa]

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?

[nagisa]

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",

[nagisa]

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

[nagisa]

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.

[nagisa]

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

[nagisa]

I'm going to pass this on to @estebank for a further review, since they have a better grasp over the overall direction of diagnostics in Rust.

[nagisa]

r? @estebank

[estebank]

Let's make sure we have tests to avoid regressing on knock down errors (let's have a struct and enum with multiple variants and fields to see that we only emit a single error for them).

[TaKO8Ki]

@estebank
Ok! I am going to improve it.

[apiraino]

Switching review labels until PR is ready again for review, thanks!

@rustbot author

[bors]

☔ The latest upstream changes (presumably #96528) made this pull request unmergeable. Please resolve the merge conflicts.