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

Allow specialized const trait impls. #95292

Merged
merged 6 commits into from
Nov 11, 2022

Conversation

BGR360
Copy link
Contributor

@BGR360 BGR360 commented Mar 25, 2022

Fixes #95186.
Fixes #95187.

I've done my best to create a comprehensive test suite for the interaction between min_specialization and const_trait_impls. I wouldn't be surprised if there are interesting cases I haven't tested, please let me know.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 25, 2022
@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 25, 2022
@BGR360
Copy link
Contributor Author

BGR360 commented Mar 25, 2022

@rustbot label +A-specialization +A-traits +F-specialization +F-const_trait_impls

EDIT: whoops i thought you could do that for PRs

@workingjubilee workingjubilee added A-trait-system Area: Trait system A-specialization Area: Trait impl specialization F-specialization `#![feature(specialization)]` F-const_trait_impl `#![feature(const_trait_impl)]` labels Mar 25, 2022
@lcnr
Copy link
Contributor

lcnr commented Mar 25, 2022

marking this as blocked this on a general refactoring of const trait impls, see https://rust-lang.zulipchat.com/#narrow/stream/144729-wg-traits/topic/Review.3A.20specialized.20const.20trait.20impls/near/276581945

Not sure whether it would be sensible to close this issue for now or only merge the tests with their current behavior and a FIXME.

@lcnr lcnr 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 25, 2022
@bors
Copy link
Contributor

bors commented Jun 28, 2022

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

@fee1-dead fee1-dead self-assigned this Sep 10, 2022
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

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

Could you please rebase? I believe this should be unblocked now. Thank you for working on this.

@fee1-dead fee1-dead added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Sep 10, 2022
@BGR360
Copy link
Contributor Author

BGR360 commented Sep 11, 2022

At long last 🎉

I'll take a stab at it later today. I imagine it will be more involved than just a rebase, right? IIRC there was some sort of major refactor taking place over the last few months. I sort of expected that I would have to re-implement the fix from scratch.

EDIT: oh, after looking at the code on master, looks like PredicateKinds still have a constness. I thought I remember someone saying that that was gonna be reworked. But I'm not complaining!

@BGR360
Copy link
Contributor Author

BGR360 commented Sep 11, 2022

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 11, 2022
@lcnr lcnr 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 Sep 12, 2022
@BGR360
Copy link
Contributor Author

BGR360 commented Sep 14, 2022

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 14, 2022
Copy link
Contributor Author

@BGR360 BGR360 left a comment

Choose a reason for hiding this comment

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

@rustbot ready

@lcnr I finally got around to addressing the last bit of feedback you gave here. Please give a look 🙏

Comment on lines +17 to +20
// bgr360: I was only able to exercise the code path that raises the
// "missing ~const qualifier" error by making this base impl non-const, even
// though that doesn't really make sense to do. As seen below, if the base impl
// is made const, rustc fails earlier with an overlapping impl failure.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts?

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 6, 2022
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

some last nits, then r=me

@BGR360
Copy link
Contributor Author

BGR360 commented Nov 10, 2022

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Nov 10, 2022

@BGR360: 🔑 Insufficient privileges: Not in reviewers

@BGR360
Copy link
Contributor Author

BGR360 commented Nov 10, 2022

@lcnr I don't have permission for bors

@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented Nov 11, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 11, 2022

📌 Commit 94f67e6 has been approved by lcnr

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 Nov 11, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 11, 2022
…earth

Rollup of 8 pull requests

Successful merges:

 - rust-lang#95292 (Allow specialized const trait impls.)
 - rust-lang#100386 (Make `Sized` coinductive, again)
 - rust-lang#102215 (Implement the `+whole-archive` modifier for `wasm-ld`)
 - rust-lang#103468 (Fix unused lint and parser caring about spaces to won't produce invalid code)
 - rust-lang#103531 (Suggest calling the instance method of the same name when method not found)
 - rust-lang#103960 (piece of diagnostic migrate)
 - rust-lang#104051 (update Miri)
 - rust-lang#104129 (rustdoc: use javascript to layout notable traits popups)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Manishearth added a commit to Manishearth/rust that referenced this pull request Nov 11, 2022
Allow specialized const trait impls.

Fixes rust-lang#95186.
Fixes rust-lang#95187.

I've done my best to create a comprehensive test suite for the interaction between `min_specialization` and `const_trait_impls`. I wouldn't be surprised if there are interesting cases I haven't tested, please let me know.
@bors bors merged commit cd30ccf into rust-lang:master Nov 11, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 11, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…earth

Rollup of 8 pull requests

Successful merges:

 - rust-lang#95292 (Allow specialized const trait impls.)
 - rust-lang#100386 (Make `Sized` coinductive, again)
 - rust-lang#102215 (Implement the `+whole-archive` modifier for `wasm-ld`)
 - rust-lang#103468 (Fix unused lint and parser caring about spaces to won't produce invalid code)
 - rust-lang#103531 (Suggest calling the instance method of the same name when method not found)
 - rust-lang#103960 (piece of diagnostic migrate)
 - rust-lang#104051 (update Miri)
 - rust-lang#104129 (rustdoc: use javascript to layout notable traits popups)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-specialization Area: Trait impl specialization A-trait-system Area: Trait system F-const_trait_impl `#![feature(const_trait_impl)]` F-specialization `#![feature(specialization)]` 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
10 participants