Skip to content
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

Dedup elaborated predicates with const generic parameter in AutoTrait #108397

Closed
wants to merge 1 commit into from

Conversation

megakorre
Copy link
Contributor

@megakorre megakorre commented Feb 23, 2023

Fixes #107715

Explanation

The param_env passed in to evaluate_predicates in the added test case has the constant N substituted for the value of the referencing constant 1. But the call to elaborate_predicates produces non normalized obligations (without the constant being substituted).

This means we will call SelectionContext.select with a param_env with multiple Predicates that will relate and cause a ambiguity. And eventually run in to a panic!.

Other normalization workarounds already happens in the AutoTraitFinder
in the add_user_pred method to work around lifetime differences.
But that method is called before elaborate_predicates which is what ends up adding the non normalized predicate.

This PR adds extra de-duplication with normalization that ends up removing the redundant predicate that causes the error here.

The first place I tried to put this de-duplication was in the Elaborator (this).
But the normalize utility I'm calling is not available from rustc_infer and the comments in AutoTraitFinder seems to suggest that the need to normalize the predicates like this is unique to it's use.

Existing Regression

running cargo-rustc-bisect finds that the test example was working before d49e7e7 (PR: #103279).
The removal of the code here makes the test pass. Not because the predicates then gets normalized the same. The param_env will still have extra predicates in it. But without the extra eval in the relate_consts code SelectionContext.select will only pick on off the 2 bounds and there will not be a ambiguity.
Making sure there are not multiple duplicates in the param_env seems more in line with the rest of AutoTraitFinder.

generic_const_exprs

With generic_const_exprs turned on the example works before and after this PR. The predicates get normalized to leaving the const un-evaluated in all cases.
So I left a comment that this de-duping can probably be removed when that feature is stable. this code has a similar comment.

Questions:

  • I could add a extra test case that generates docs for the same test case but with generic_const_exprs enabled. That functionality was not broken before this so maybe its not needed? 🤷🏻 But the functionality of this is different in that case

@rustbot
Copy link
Collaborator

rustbot commented Feb 23, 2023

r? @nagisa

(rustbot has picked a reviewer for you, use r? to override)

@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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 23, 2023
@megakorre megakorre force-pushed the issue_107715 branch 2 times, most recently from 4510389 to ee75156 Compare February 23, 2023 14:28
@megakorre megakorre marked this pull request as ready for review February 23, 2023 20:49
@megakorre megakorre changed the title WIP: auto_trait: dedup elaborated predicates with normalization Dedup elaborated predicates with const generic parameter in AutoTrait Feb 23, 2023
@bors
Copy link
Contributor

bors commented Feb 25, 2023

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

@nagisa
Copy link
Member

nagisa commented Feb 26, 2023

r? @lcnr

@rustbot rustbot assigned lcnr and unassigned nagisa Feb 26, 2023
@lcnr
Copy link
Contributor

lcnr commented Feb 27, 2023

r? @BoxyUwU

@rustbot rustbot assigned BoxyUwU and unassigned lcnr Feb 27, 2023
@apiraino
Copy link
Contributor

I'll play with the ball, too

r? compiler

@rustbot rustbot assigned oli-obk and unassigned BoxyUwU Mar 29, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Mar 29, 2023

@rustbot blocked on #108503

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2023
@fmease
Copy link
Member

fmease commented May 1, 2023

@megakorre Could you update the snippet issue: #107715 in the PR description to Fixes #107715 so GitHub can prominently link to this PR from the issue (I almost missed that there is a fix) and autoclose it when this PR gets merged? That would be great, thanks!

@Dylan-DPC
Copy link
Member

Closing this as the issue this is referencing is already closed/fixed.

@Dylan-DPC Dylan-DPC closed this Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
10 participants