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

Minimally constify Add #133237

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented Nov 20, 2024

  • This PR removes the requirement for impl const to have a const stability attribute. cc @RalfJung I believe you mentioned that it would make much more sense to require const_traits to have const stability instead. I agree with that sentiment but I don't think that is required for a small scale experimentation like this PR. Add const stability tracking for traits project-const-traits#16 should definitely be prioritized in the future, but removing the impl check should be good for now as all callers need const_trait_impl enabled for any const impl to work.
  • This PR is intentionally minimal as constifying other traits can become more complicated (PartialEq, for example, would run into requiring implementing it for str as that is used in matches, which runs into the implementation for slice equality which uses specialization)

Per the reasons above, anyone who is interested in making traits const in the standard library are strongly encouraged to reach out to us on the Zulip channel before proceeding with the work.

cc @rust-lang/project-const-traits

I believe there is prior approval from libs that we can experiment, so

r? project-const-traits

@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-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 20, 2024
@RalfJung
Copy link
Member

RalfJung commented Nov 20, 2024

Agreed regarding const stability attributes. We currently do require them for regular impl so before we stabilize anything here we need to clean this up, but for now const_trait_impl blocks everything so we don't need per-trait tracking.

Per the reasons above, anyone who is interested in making traits const in the standard library are strongly encouraged to reach out to us on the Zulip channel before proceeding with the work.

I also feel like we should not repeat what we did last time and have a big "const trait everything" spree until we are confident that the system works and we can stabilize the first parts. We need to do experimentation to gain that confidence, of course, but IMO last time we went too far in using this unstable feature throughout the standard library before it was ready.

Copy link
Member

Choose a reason for hiding this comment

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

Could this test also call twice from const context?

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

r=me after test tweak

@compiler-errors compiler-errors 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 Nov 21, 2024
@fee1-dead
Copy link
Member Author

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Nov 21, 2024

📌 Commit 514ef18 has been approved by compiler-errors

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 21, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 21, 2024
…iler-errors

Minimally constify `Add`

* This PR removes the requirement for `impl const` to have a const stability attribute. cc `@RalfJung` I believe you mentioned that it would make much more sense to require `const_trait`s to have const stability instead. I agree with that sentiment but I don't think that is _required_ for a small scale experimentation like this PR. rust-lang/project-const-traits#16 should definitely be prioritized in the future, but removing the impl check should be good for now as all callers need `const_trait_impl` enabled for any const impl to work.
* This PR is intentionally minimal as constifying other traits can become more complicated (`PartialEq`, for example, would run into requiring implementing it for `str` as that is used in matches, which runs into the implementation for slice equality which uses specialization)

Per the reasons above, anyone who is interested in making traits `const` in the standard library are **strongly encouraged** to reach out to us on the [Zulip channel](https://rust-lang.zulipchat.com/#narrow/channel/419616-t-compiler.2Fproject-const-traits) before proceeding with the work.

cc `@rust-lang/project-const-traits`

I believe there is prior approval from libs that we can experiment, so

r? project-const-traits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants