Improve diagnostics when parsing angle args

[b-naber]

#79266 introduced parsing of generic arguments in associated type constraints, this however resulted in possibly very confusing error messages in cases in which closing angle brackets were missing such as in Vec<(u32, _, _) = vec![], which outputs an incorrectly parsed equality constraint error, as noted by @cynecx.

This PR tries to provide better error messages in such cases.

r? @petrochenkov

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[oli-obk]

I love the change in diagnostics. I do have some reserves about the implementation though. I wonder if it's tackling it from the wrong side. I'm not sure if this would get us the same result, but it could be worth a try:

At the error reporting site of

rust/compiler/rustc_parse/src/parser/path.rs

Line 589 in cfed918

"only path types can be used in associated type constraints",

we could add a structured suggestion to add a >. While this would be wrong in various cases, a sufficiently explanatory diagnostic message on the suggestion could probably make that clear?

We could also try to dedupliate/condense the current PR if you prefer. I'll let @rust-lang/wg-diagnostics weigh in, before you have to put in work that others may end up disagreeing with.

[b-naber]

@oli-obk Thanks for looking at the PR. I understand your concerns, I'm also not really satisfied with the implementation that handles the case involving the error you mentioned. I think handling this the way you proposed is probably the better solution. I can remove that part and implement what you suggested. The case in which a syntactically correct equality constraint is parsed (e.g. as in let v : Vec<T = vec![]), is, I think at least a little better (acceptably?) handled. This should also occur frequently enough that it warrants being handled specifically. Thoughts?

[oli-obk]

The case in which a syntactically correct equality constraint is parsed (e.g. as in let v : Vec<T = vec![]), is, I think at least a little better (acceptably?) handled. This should also occur frequently enough that it warrants being handled specifically. Thoughts?

I'm not sure how to detect let v: Vec<T = vec![]; at

rust/compiler/rustc_parse/src/parser/path.rs

Line 589 in cfed918

"only path types can be used in associated type constraints",

without also detecting

rust/src/test/ui/generic-associated-types/parse/trait-path-types.stderr

Lines 1 to 5 in cfed918

	error: only path types can be used in associated type constraints
	--> $DIR/trait-path-types.rs:9:29
	|
	LL | fn f<'a>(arg : Box<dyn X< [u8; 1] = u32>>) {}
	| ^^^^^^^

in which the suggestion would be wrong. I consider it totally ok for having a bad suggestion in the latter, as long as the suggestion comment makes it clear that it is not a guaranteed correct suggestion. Is this what you were also saying?

[oli-obk]

r? @oli-obk

[b-naber]

No, that's not what I meant. You're right, we cannot catch a 'missing closing angle args' error inside parse_angle_args when we parse a valid equality constraint such as T = vec![]. What I had in mind was to implement what you suggested, i.e. to add the 'missing closing angle args' error suggestion in

rust/compiler/rustc_parse/src/parser/path.rs

Line 589 in cfed918

"only path types can be used in associated type constraints",

and remove this part

https://github.com/rust-lang/rust/blob/d66096b8356b132eced7f118a84f5eba3e23f016/compiler/rustc_parse/src/parser/diagnostics.rs#L1990

But we keep the part in which we handle correctly parsed equality constraints as the last element in args, which is done here:

https://github.com/rust-lang/rust/blob/d66096b8356b132eced7f118a84f5eba3e23f016/compiler/rustc_parse/src/parser/diagnostics.rs#L1957

Or do you have a problem with this part in parse_path_segment in general?

https://github.com/rust-lang/rust/blob/d66096b8356b132eced7f118a84f5eba3e23f016/compiler/rustc_parse/src/parser/path.rs#L224-L227

[petrochenkov]

A much simpler alternative - petrochenkov@a759e56.
If the type in get_ident_from_generic_arg doesn't look like Ident<Args>, then just treat it as a type argument and treat the following = or : as an unexpected token.

[b-naber]

@petrochenkov I was hoping someone from the diagnostics team would weigh in on this. To me these 'expected token' errors are probably good enough in case there is actually a missing closing angle bracket, but I think they might be confusing if we actually have incorrect generic argument types in constraints. E.g. in fn f1<'a>(arg : Box<dyn X<X::Y = u32>>) {} an 'expected token' error would be a bad diagnostic imo. The other cases that are currently caught in get_ident_from_generic_arg are probably a lot less likely to occur in practice, so that a 'missing angle bracket' error is likely the reason for these errors occuring. So in those cases, i.e. all errors that are detected inside get_ident_from_generic_arg except the 'multiple path segment' error, your errors should probably be sufficient. How should we proceed with this?

[oli-obk]

I do like the simplicity of @petrochenkov 's solution, but wearing my wg-diagnostics hat I would also like to see an additional message at the

expected [..] found =

site. Now since all of these errors are about a "found =", maybe we can add some additional note right there instead of trying patch it in at the various sites like it was happening before?

[b-naber]

@oli-obk Adding a suggestion to a 'found =' error in @petrochenkov 's solution would have not worked cleanly. Instead I tried to implement what you suggested initially, i.e. giving a suggestion for a 'possibly missing closing angle bracket' whenever there's an 'only types' error when parsing equality constraints.

[oli-obk]

Oh, so it looks to me like we can do both your current PR and @petrochenkov 's change. Are you ok with this @petrochenkov ?

[petrochenkov]

@oli-obk
Yes, combining the PR as it exists now with my change should be ok.

[petrochenkov]

Could you assign this to me for a final look once it's otherwise ready?
(Or just ping, but I'm more likely to miss it.)

[oli-obk]

@b-naber please pull in the commit from petrochenkov mentioned in #80065 (comment) and adjust all tests. I will then reassign to petrochenkov for a last review

[b-naber]

I don't see how these two commits are mergeable. In @petrochenkov 's solution we return Ok(Some(AngleBracketedArg::Arg(arg))) in 'parse_angle_arg', control then passes to some upper level call which finds an unexpected token. If we don't output any error in get_ident_from_generic_arg, but intercept the resulting Err(gen_arg) in parse_angle_arg as is done in my last commit, I'm not sure what would be a good error to use here for outputting the suggestion (or rather why not just keep the errors that 'get_ident_from_generic_arg` already created?). Catching the 'unexpected token error' and then modifying it for a 'possibly missing closing angle bracket' seems to not make any sense to me at all.

[oli-obk]

Catching the 'unexpected token error' and then modifying it for a 'possibly missing closing angle bracket' seems to not make any sense to me at all.

Hmm... while I see that it is a first to special case a generic "expected, found" error where a = was encountered, but one of the expected tokens is >, I think it could be a reasonable middle ground between parser simplicity and helpful diagnostics

[b-naber]

@estebank Thanks for the review. Addressed your comments.

[b-naber]

@petrochenkov Thanks for your review. Addressed your comments.

[petrochenkov]

r=me after rebase.

[b-naber]

@petrochenkov I get pretty bad conflicts in const-generics tests:

error[E0658]: default values for const generic parameters are experimental
  --> $DIR/default_trait_param.rs:1:28
   |
LL | trait Foo<const KIND: bool = true> {}
<<<<<<< HEAD
   |                            ^^^^^^
   |
   = note: see issue #44580 <https://github.com/rust-lang/rust/issues/44580> for more information
   = help: add `#![feature(const_generics_defaults)]` to the crate attributes to enable
=======
   |                       ---- ^ expected one of 7 possible tokens
   |                       |
   |                       maybe try to close unmatched angle bracket
>>>>>>> d228142f35f (add and update tests)

error: aborting due to previous error

For more information about this error, try `rustc --explain E0658`.

Should this error at all? It seems that on master this only errors because the feature is experimental. This seems very wrong, we shouldn't output a parsing error here, should we?

[oli-obk]

in stderr files, you can just ignore the conflicts and re-run ./x.py test src/test/ui --bless, it will fix the files even if they have conflicts. There is no need to manually resolve conflicts in .stderr files

[petrochenkov]

@b-naber
After rebase and --bless all the weirdness disappear and trait Foo<const KIND: bool = true> does not report maybe try to close unmatched angle bracket, so I don't see any issues here.

[b-naber]

@petrochenkov rebased.

[oli-obk]

@bors r=petrochenkov

[bors]

📌 Commit 3ba6cf1 has been approved by petrochenkov

[bors]

⌛ Testing commit 3ba6cf1 with merge 1986b58...

[bors]

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 1986b58 to master...