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.

Sign up for GitHub

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

Jump to bottom

Sort elaborated existential predicates in object_ty_for_trait #102947

Merged
merged 1 commit into from
Oct 13, 2022
Merged

Sort elaborated existential predicates in object_ty_for_trait #102947

merged 1 commit into from
Oct 13, 2022

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Oct 12, 2022 •
edited
Loading

r? @cjgillot

I think that #102845 caused #102933. Depending on the order that we elaborate these existential projection predicates, there's no guarantee that they'll be sorted by def id, which is what is failing the assertion in the issue.

Fixes #102933
Fixes #102973

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 12, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2022
@cjgillot
Copy link
Contributor

Could you add a comment explaining why we sort?
I (wrongly) assumed this was related to diagnostic determinism.
r=me

@compiler-errors compiler-errors force-pushed the sort-elaborated-existentials branch from e4ba3c4 to 4a8cfe9 Compare October 13, 2022 02:21
@compiler-errors
Copy link
Member Author

I left a somewhat lackluster comment (sorry), but this has been an assertion since @Mark-Simulacrum added it in #37965.

We actually probably don't need to assert that the list is totally sorted in the interner function, but just that the primary ExistentialPredicate::Trait (if there is one) comes before any other existential predicates, since the principal function uses list[0] to access it. I don't see any risk in sorting here though.

Please let me know if I should update the comment in a follow-up though, and I will happily dig deeper!

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Oct 13, 2022

📌 Commit 4a8cfe9 has been approved by cjgillot

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2022
@compiler-errors
Copy link
Member Author

The Relate implementation for poly existential predicate lists actually sorts its predicates anyways, so I doubt this would cause an issue if we relaxed this failing assertion to just check that the ExistentialPredicate::Trait comes first -- see the impl...

I kinda don't want to touch it unless someone else thinks it's a good idea, and this is definitely a follow-up item.

@Dylan-DPC Dylan-DPC mentioned this pull request Oct 13, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 13, 2022
@bors
Auto merge of rust-lang#103009 - Dylan-DPC:rollup-9c2tng6, r=Dylan-DPC
6b3ede3 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#102765 (Suggest `==` to the first expr which has `ExprKind::Assign` kind)
 - rust-lang#102854 (openbsd: don't reallocate a guard page on the stack.)
 - rust-lang#102904 (Print return-position `impl Trait` in trait verbosely if `-Zverbose`)
 - rust-lang#102947 (Sort elaborated existential predicates in `object_ty_for_trait`)
 - rust-lang#102956 (Use `full_res` instead of `expect_full_res`)
 - rust-lang#102999 (Delay `is_intrinsic` query until after we've determined the callee is a function)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 13, 2022

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Oct 13, 2022

📌 Commit 4a8cfe9 has been approved by cjgillot

It is now in the queue for this repository.

@bors bors merged commit 42991e5 into rust-lang:master Oct 13, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 13, 2022
@compiler-errors compiler-errors deleted the sort-elaborated-existentials branch November 2, 2022 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cjgillot cjgillot

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.66.0
5 participants
@compiler-errors @cjgillot @bors @rust-highfive @rustbot