7

constified implementations of `Default` by fee1-dead · Pull Request #86808 · rus...

 3 years ago
source link: https://github.com/rust-lang/rust/pull/86808
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Copy link

Member

fee1-dead commented on Jul 2

edited

No description provided.

Copy link

Collaborator

rust-highfive commented on Jul 2

r? @scottmcm

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

Copy link

Contributor

CDirkx commented on Jul 2

edited

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.

This comment has been hidden.

This comment has been hidden.

Copy link

Member

Author

fee1-dead commented on Jul 16

edited

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

This comment has been hidden.

This comment has been hidden.

Copy link

Member

Mark-Simulacrum commented on Jul 29

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?

Copy link

Member

Author

fee1-dead commented on Jul 29

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

Copy link

Contributor

oli-obk commented 29 days ago

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.

Copy link

Member

Author

fee1-dead commented 25 days ago

edited

Ping @rust-lang/libs-api

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

Copy link

Member

dtolnay left a comment

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.

Copy link

Member

Author

fee1-dead commented 24 days ago

edited

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 #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.

Copy link

Contributor

bors commented 24 days ago

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

Copy link

Member

yaahc commented 24 days ago

edited
* 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.

Copy link

Member

Author

fee1-dead commented 24 days ago

edited

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

Copy link

Contributor

oli-obk commented 11 days ago

@bors r+

Copy link

Contributor

bors commented 11 days ago

pushpin Commit c5983c8 has been approved by oli-obk

Copy link

Contributor

bors commented 11 days ago

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

Copy link

Contributor

oli-obk commented 10 days ago

@bors r+

Copy link

Contributor

bors commented 10 days ago

pushpin Commit 6bd2ecb has been approved by oli-obk

Copy link

Contributor

bors commented 10 days ago

hourglass Testing commit 6bd2ecb with merge 1805ef4...

Copy link

Contributor

bors commented 10 days ago

boom Test timed out

Copy link

Collaborator

rust-log-analyzer commented 10 days ago

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

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

Copy link

Contributor

oli-obk commented 10 days ago

@bors retry

Copy link

Contributor

bors commented 10 days ago

hourglass Testing commit 6bd2ecb with merge adf1688...

Copy link

Contributor

bors commented 10 days ago

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK