Skip to content

Improve unconstrained impl diagnostic (fixes #107295) #126026

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

Conversation

LlewVallis
Copy link

Fixes #107295

Problem

When a type/lifetime/const generic param is unconstrained (as defined by the rules in this RFC) the current diagnostic isn't very helpful - it just tells you the param wasn't constrained. It isn't obvious (without having read the RFC) what can be done to make the param constrained.

Solution

We add some additional notes to the existing diagnostic:

  • In all cases, this PR adds an extra note which tells the user under what circumstances a generic is constrained. This message is slightly different depending on whether this is an impl trait or an inherent impl. The message slightly oversimplifies things to be more understandable, but the edge cases it misses are covered by the special messages below:
    • If the LHS was the thing being implemented (i.e. circular), then we use an extra message to explain that circularity isn't allowed
    • Otherwise, if the unconstrained param was mentioned in the RHS of an equality w/ a projection/associated-type then we add a message saying that this must mean the LHS of the equality has an unconstrained param

I added a few UI tests which probably explain these messages better than I have here :)

After looking at the RFC I linked, AFAICT there are no more edge cases where the dot points above could be incorrect, but I'm not an expert, so please correct me if there are.


Also as a side note - the enforce_impl_params_are_constrained function is getting a bit long, lmk if you want me to split it up.

@rustbot
Copy link
Collaborator

rustbot commented Jun 5, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 5, 2024
@LlewVallis LlewVallis force-pushed the llew-better-unconstrained-impl-diagnostic branch from 7394b00 to 437083a Compare June 5, 2024 11:53
@rust-log-analyzer

This comment has been minimized.

@LlewVallis LlewVallis force-pushed the llew-better-unconstrained-impl-diagnostic branch from 437083a to e0f14e4 Compare June 5, 2024 12:58
@compiler-errors
Copy link
Member

r? types

@rustbot rustbot added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Jun 5, 2024
@rustbot rustbot assigned jackh726 and unassigned wesleywiser Jun 5, 2024
Comment on lines 217 to 219
is_impl_trait: bool,
is_bound_to_projection: bool,
is_bound_to_circular_projection: bool,
Copy link
Member

Choose a reason for hiding this comment

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

I haven't read the rest of your impl, this is just drive-by commentary.

Could please turn these bools into custom enums? And merge these three flags as much as possible unless they are already mutually exclusive?

Copy link
Author

Choose a reason for hiding this comment

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

Yep 👍 is_impl_trait is orthogonal to the other two booleans, but is_bound_to_projection and is_bound_to_circular_projection can be merged

@LlewVallis LlewVallis force-pushed the llew-better-unconstrained-impl-diagnostic branch from e0f14e4 to ba52ede Compare June 6, 2024 11:36
Comment on lines +103 to +105
let mentioned_params = cgp::parameters_for(tcx, projection.term().skip_binder(), true);
let is_clause_circular =
Some(projection.required_poly_trait_ref(tcx).skip_binder()) == impl_trait_ref;
Copy link
Author

Choose a reason for hiding this comment

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

Almost forgot, but wanted to add a quick note/question regarding skip_binder here... from what I understand, binders correspond to things like for<'a>, and skip_binder is generally a bad idea since it just unwraps the binder wo/ updating references to the quantifier.

My Q: is this specific usage safe? Or is some else more appropriate? (My understanding of this is a bit hazy 🙂)

@bors
Copy link
Collaborator

bors commented Jun 29, 2024

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

@Dylan-DPC Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 19, 2024
@Dylan-DPC
Copy link
Member

@LlewVallis if you can resolve the conflicts, we can push this forward for a review. Thanks

@LlewVallis
Copy link
Author

Hey 👋 I'm short on time lately, so might leave this for someone else to pick up

@LlewVallis LlewVallis closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unconstrained type parameter makes no suggestions as to how to fix
9 participants