-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Provide suggestion for missing >
in a type parameter list
#94495
Conversation
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
I am somewhat concerned about the ratio of misleading to useful "add ... though I'm not sure how much additional effort it would take to improve this to e.g. not suggest adding For example:
|
Those two cases in particular could/should be handled, one by explicitly accepting |
@JohnCSimon I'm gonna revert the labels back here because I'm waiting on further review 😅 I feel like the fallout is acceptable as is for merging, but can understand if the reviewers disagree. |
When encountering an inproperly terminated type parameter list, provide a suggestion to close it after the last non-constraint type parameter that was successfully parsed. Fix rust-lang#94058.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's hard to say anything about the "very wrong syntax" cases. But, I think this is worth it from the new tests, which more likely represent what a user would normally run into.
One nit, but r=me with/without.
arg.span().shrink_to_hi(), | ||
"you might have meant to end the type parameters here", | ||
">".to_string(), | ||
Applicability::MaybeIncorrect, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if MaybeIncorrect
is the right choice here. A lot of the suggestions in tests don't result in valid code (namely when we later have a closing >
. But 🤷♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's the best of the available Applicability
s we have available today :)
9cc0897
to
157c67b
Compare
Added one more check for |
@bors r+ |
📌 Commit 157c67b has been approved by |
⌛ Testing commit 157c67b with merge 6a755d790d12f0ba5a1234f613f147f19725be76... |
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
Looks like a timeout error?
@bors retry |
☀️ Test successful - checks-actions |
Finished benchmarking commit (ab0c2e1): comparison url. Summary: This benchmark run did not return any relevant results. 7 results were found to be statistically significant but too small to be relevant. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
When encountering an inproperly terminated type parameter list, provide
a suggestion to close it after the last non-constraint type parameter
that was successfully parsed.
Fix #94058.