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

constified implementations of Default #86808

Merged
merged 2 commits into from
Aug 18, 2021

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented Jul 2, 2021

No description provided.

@rust-highfive
Copy link
Collaborator

r? @scottmcm

(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 Jul 2, 2021
library/alloc/src/string.rs Outdated Show resolved Hide resolved
@CDirkx
Copy link
Contributor

CDirkx commented Jul 2, 2021

What would be the policy for changing to const impls in the standard library? Is that something we want to do very conservatively, only to test the implementation for the RFC, or more liberally (or even everywhere where possible). I know of quite some const code that would become a lot more readable when able to use const impl Eq etc., but given that the RFC hasn't been accepted yet I don't know how much the library team wants to commit to this.

library/alloc/src/lib.rs Outdated Show resolved Hide resolved
@fee1-dead

This comment has been minimized.

@jonas-schievink jonas-schievink added the F-const_trait_impl `#![feature(const_trait_impl)]` label Jul 3, 2021
@rust-log-analyzer

This comment has been minimized.

@fee1-dead
Copy link
Member Author

fee1-dead commented Jul 16, 2021

Update: in this zulip thread we felt comfortable marking the feature not incomplete and start experimenting (non generic impls) in libstd.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

Update: in this zulip thread we felt comfortable marking the feature not incomplete and start experimenting (non generic impls) in libstd.

Have the incomplete / not yet ready for use parts of the feature gate been split out yet? I'm not sure we should be using it until that has been done: we've run into ongoing trouble with specialization, for example, precisely because of the actually-not-ok bits in min_specialization. I'd prefer to avoid doing so with const trait if there are known limitations.

Regardless, I think a decision from T-libs-api on what the policy for const-unstable trait impls should look like would be merited. I'm a little surprised this is possible since historically impls weren't possible to feature gate - is the story different in the const case for some reason?

@fee1-dead
Copy link
Member Author

Update: in this zulip thread we felt comfortable marking the feature not incomplete and start experimenting (non generic impls) in libstd.

Have the incomplete / not yet ready for use parts of the feature gate been split out yet? I'm not sure we should be using it until that has been done: we've run into ongoing trouble with specialization, for example, precisely because of the actually-not-ok bits in min_specialization. I'd prefer to avoid doing so with const trait if there are known limitations.

I don't think we currently have incomplete / not yet ready for use parts. There are actually aspects of the RFC that are yet to be implemented, but I don't think this PR can expose not-ok stuff to users.

I'm a little surprised this is possible since historically impls weren't possible to feature gate - is the story different in the const case for some reason?

AFAIK the stability should be inherited and maybe this is only true for const stability. But I did add a test for this: https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2632-const-trait-impl/staged-api.rs

@oli-obk
Copy link
Contributor

oli-obk commented Jul 29, 2021

is the story different in the const case for some reason?

Yes it is. const stability is a separate system that does not participate in trait resolution, so you don't get type-system unsoundness where a trait is both implemented and not. Instead you just get an error if you call a non-const impl from a const context.

@fee1-dead
Copy link
Member Author

fee1-dead commented Aug 2, 2021

Ping @rust-lang/libs-api

@rustbot label T-libs-api -S-waiting-on-review S-waiting-on-team

@rustbot rustbot added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 2, 2021
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I think a decision from T-libs-api on what the policy for const-unstable trait impls should look like would be merited.

I don't think we have any objection to starting to land const impls as long as they are properly gated, i.e. stable code cannot be broken by ongoing churn in the const impl feature, all the way through its stabilization or eventual removal. I can't judge that from the PR but if the compiler team asserts it's properly gated, then it's ready.

As for which impls to make const, I think the bar is going to be the same as for const inherent methods: it has to be reasonably usable in const or have an extremely strong justification otherwise. For example we regularly reject const on an inherent method of a type that is impossible to construct in const, even if that method's implementation is otherwise compatible with const.

@fee1-dead
Copy link
Member Author

fee1-dead commented Aug 3, 2021

I think:

  • I am sure that non-generic const impls will be good; (gated behind #[rustc_const_unstable] of course :) )
  • a bit less sure about generic const fns (should be better after Try filtering out non-const impls when we expect const impls #87375)
  • not so sure about implementing traits with associated types
  • Don't think we should constify impls with trait bounds now, because of potential regressions (see this) I need to add a few ui tests tracking this.

Update: Upon digging further, I think it is actually fine to add impls that match any point above. The only case where I am unsure is traits where associated types have bounds.

@bors
Copy link
Contributor

bors commented Aug 3, 2021

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

@yaahc
Copy link
Member

yaahc commented Aug 3, 2021

* I am sure that non-generic const impls will be good;

When you say "non-generic" in this case you mean traits that don't have generic parameters like Default vs From<T>, and not impls that don't have generic parameters like impl Default for Foo vs impl<T> Default for Vec<T> correct? Just trying to gauge if you're indicating that these Default impls should be good to go or not.

@fee1-dead
Copy link
Member Author

fee1-dead commented Aug 4, 2021

Sorry, to be clear, I meant that const impls without trait bounds other than Sized (and also : ?Sized bound) will be good, so yes, I think these Default implementations are good to go.

@rustbot label -S-waiting-on-team S-waiting-on-review

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 17, 2021
The libs-api team agrees to allow const_trait_impl to appear in the
standard library as long as stable code cannot be broken (they are
properly gated) this means if the compiler teams thinks it's okay, then
it's okay.

My priority on constifying would be:

	1. Non-generic impls (e.g. Default) or generic impls with no
	   bounds
	2. Generic functions with bounds (that use const impls)
	3. Generic impls with bounds
	4. Impls for traits with associated types

For people opening constification PRs: please cc me and/or oli-obk.
@oli-obk
Copy link
Contributor

oli-obk commented Aug 17, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Aug 17, 2021

📌 Commit 6bd2ecb has been approved by oli-obk

@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 Aug 17, 2021
@bors
Copy link
Contributor

bors commented Aug 17, 2021

⌛ Testing commit 6bd2ecb with merge 1805ef4dd4df410428fb1cd37a47f88781bc469c...

@bors
Copy link
Contributor

bors commented Aug 17, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 17, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@oli-obk
Copy link
Contributor

oli-obk commented Aug 17, 2021

@bors retry

@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 Aug 17, 2021
@bors
Copy link
Contributor

bors commented Aug 17, 2021

⌛ Testing commit 6bd2ecb with merge adf1688...

@bors
Copy link
Contributor

bors commented Aug 18, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing adf1688 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 18, 2021
@bors bors merged commit adf1688 into rust-lang:master Aug 18, 2021
@fee1-dead fee1-dead deleted the constify-1 branch August 29, 2021 13:45
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 30, 2021
Constify ?-operator for Result and Option

Try to make `?`-operator usable in `const fn` with `Result` and `Option`, see rust-lang#74935 . Note that the try-operator itself was constified in rust-lang#87237.

TODO
* [x] Add tests for const T -> T conversions
* [x] cleanup commits
* [x] Remove `#![allow(incomplete_features)]`
* [?] Await decision in rust-lang#86808 - I'm not sure
* [x] Await support for parsing `~const` in bootstrapping compiler
* [x] Tracking issue(s)? - rust-lang#88674
@cuviper cuviper added this to the 1.56.0 milestone Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_trait_impl `#![feature(const_trait_impl)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.