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

fix: sort and deduplicate auto traits in trait object types #12793

Merged
merged 1 commit into from
Aug 31, 2022

Conversation

lowr
Copy link
Contributor

@lowr lowr commented Jul 18, 2022

Fixes #12739

Chalk solver doesn't sort and deduplicate auto traits in trait object types, so we need to handle them ourselves in the lowering phase, just like rustc and chalk-integration do.

Quoting from the Chalk book:

Note that -- for this purpose -- ordering of bounds is significant. That means that if you create a dyn Foo + Send and a dyn Send + Foo, chalk would consider them distinct types. The assumption is that bounds are ordered in some canonical fashion somewhere else.

Also, trait object types with more than one non-auto traits were previously allowed, but are now disallowed with this patch.

@lowr lowr changed the title fix: sort and deduplicate auto traits in trait objects fix: sort and deduplicate auto traits in trait object types Jul 18, 2022
@Veykril Veykril requested a review from flodiebold August 2, 2022 14:03
@bors
Copy link
Contributor

bors commented Aug 18, 2022

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

@lowr lowr force-pushed the fix/12739 branch 2 times, most recently from b86387e to a626774 Compare August 18, 2022 10:35
return None;
}

auto_traits.sort_by_key(|b| b.trait_id().unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't sort_unstable_by_key() be preferred?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds right, we don't need stability here. Thanks for pointing out!

@Veykril
Copy link
Member

Veykril commented Aug 31, 2022

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Aug 31, 2022

📌 Commit 7ecead2 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 31, 2022

⌛ Testing commit 7ecead2 with merge 56d8886...

@bors
Copy link
Contributor

bors commented Aug 31, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 56d8886 to master...

@bors bors merged commit 56d8886 into rust-lang:master Aug 31, 2022
bors added a commit that referenced this pull request Sep 5, 2022
fix: sort all bounds on trait object types

Fixes #13181

#12793 allowed different ordering of trait bounds in trait object types but failed to account for the ordering of projection bounds. I opted for sorting all the bounds at once rather than splitting them into `SmallVec`s so it's easier to do the same thing for other bounds when we have them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error reported over different Send and Sync ordering
4 participants