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

RFC: The ConstDefault trait #2204

Closed
wants to merge 8 commits into from

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Nov 3, 2017

Rendered.

Add a notion of a constant default as in:

pub trait ConstDefault {
    const DEFAULT: Self;
}

@durka
Copy link
Contributor

durka commented Nov 3, 2017

Seems like a great idea! My only qualm is that if traits ever get const methods, that might be a slightly better syntax. But I don't want to delay based on a hypothetical.


This blanket `impl` will now conflict with those `Default` `impl`s already in
the standard library. Therefore, those `impl`s are removed. This incurrs no
breaking change.

Choose a reason for hiding this comment

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

What about cases where the ConstDefault impl is more restrictive than the existing Default impl? For example, tuples are Default if the elements are, but this blanket impl only covers tuples with ConstDefault elements.

Copy link
Contributor Author

@Centril Centril Nov 3, 2017

Choose a reason for hiding this comment

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

Is this what you had in mind?

pub trait Default { fn default() -> Self; }

pub trait ConstDefault { const DEFAULT: Self; }

impl<T: ConstDefault> Default for T {
    fn default() -> Self { <T as ConstDefault>::DEFAULT }
}

impl<T0: ConstDefault, T1: ConstDefault> ConstDefault for (T0, T1) {
    const DEFAULT: Self = (T0::DEFAULT, T1::DEFAULT);
}

impl<T0: Default, T1: Default> Default for (T0, T1) {
    fn default() -> Self { (T0::default(), T1::default()) }
}

fn main() {}

which gives:

error[E0119]: conflicting implementations of trait `Default` for type `(_, _)`:

Well... that's unfortunate, and a nice catch on your part - I don't know how to solve this other than to remove the blanket impl since the following is not a thing (and would be possibly unsound?):

impl<T0: Default + !ConstDefault, T1: Default  + !ConstDefault> Default
for (T0, T1) {
    fn default() -> Self { (T0::default(), T1::default()) }
}

Thoughts on how to proceed?

Copy link
Contributor

@durka durka Nov 3, 2017

Choose a reason for hiding this comment

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

I think the blanket impl has to be removed :( at least until there's a way to say "when these two impls overlap, choose this one".

One mitigation would be to have #[derive(ConstDefault)] generate impls of both ConstDefault and Default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@durka Oh why didn't I think of that solution ;) The mitigating idea seems like a wonderful fix for now.

Copy link

@hanna-kruppe hanna-kruppe Nov 3, 2017

Choose a reason for hiding this comment

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

Yeah I don't see a way to salvage the blanket impl either, I just didn't want to claim that unilaterally :)

Having derive(ConstDefault) generate an implementation for Default is appealing but there's not currently any precedent (each std derive generates one impl, for the trait it specifies -- custom derives can deviate from this). This is despite the fact that such a feature would be equally appealing for several pairs of existing traits (Clone/Copy, PartialEq/Eq, PartialOrd/Ord), and it being even easier to justify for those existing traits:

  • those pairs of traits are more closely related (supertraits) to each other than Default and ConstDefault are, and
  • documentation even states that the two traits must be behaviorally equivalent (except for PartialEq/Eq, where it's enforced by the shape of the Eq trait), which I don't see an analogue of in this RFC.

If you want to go this route, I'd like to see some justification for this irregularity.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Special casing #[derive(ConstDefault, Default)] is not really useful since it would be a logic error on the part of the user given T: ConstDefault to provide Default for T which is not defined as T::DEFAULT.

Isn't that... why it would be useful? So you don't have to do the rote impl Default for Thing { fn default() -> Thing { Thing::DEFAULT } } after you derive ConstDefault.

Otherwise all sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, let's assume we have this setup:

enum A { A1, A2 }
enum B { B1, B2 }
impl ConstDefault for A { const DEFAULT: Self = A::A1; }
impl ConstDefault for B { const DEFAULT: Self = B::B1; }
impl Default for A { fn default() -> Self { A::A1 } }
impl Default for B { fn default() -> Self { B::B1 } }

#[derive(ConstDefault, Default)]
struct P(A, B);

The following is generated:

impl ConstDefault for P { const DEFAULT: Self = P(A::DEFAULT, B::DEFAULT); }
impl Default for P { fn default() -> Self { P(A::default(), B::default()) } }

The values for these coincide - and they must, because it would be a logic error otherwise, and this holds inductively.

Copy link
Contributor

@durka durka Nov 9, 2017

Choose a reason for hiding this comment

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

Yes, I agree. So generating impl Default for P { fn default() -> P { P::DEFAULT } } would be a pure optimization, same as the Copy/Clone thing to generate impl Clone for P { fn clone(&self) -> P { *self } } instead of impl Clone for P { fn clone(&self) -> P { P(self.0.clone(), self.1.clone()) } }.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it wouldn't be an optimization at all, depending on how consts are expanded in the compiler. Either way it's an implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a const () -> T where T is some type- i.e: a true constant, the compiler can of course elect to evaluate that once, save the result and reuse it everywhere else. Therefore it can be an optimization in theory, at least for compile times. So I guess I'll include a note about special casing since it's a useful place to talk about ConstDefault != Default being a logic error.

@clarfonthey
Copy link
Contributor

A const fn version would be a much nicer syntax because it wouldn't require Self: Clone.

@durka
Copy link
Contributor

durka commented Nov 7, 2017

@clarcharr what? Clone isn't mentioned anywhere...

@clarfonthey
Copy link
Contributor

@durka I forgot the difference between static and const for a second; this still works with const.

@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Nov 14, 2017
@mikeyhew
Copy link

@durka

Seems like a great idea! My only qualm is that if traits ever get const methods, that might be a slightly better syntax. But I don't want to delay based on a hypothetical.

It may make sense to use const instead of const fn whenever the function has no arguments. Otherwise, what is const for?

@clarfonthey
Copy link
Contributor

@mikeyhew that is a good point that should probably be brought up in the Rust API guidelines!

@Centril
Copy link
Contributor Author

Centril commented Dec 4, 2017

Updated according to recent discussion.

@Centril
Copy link
Contributor Author

Centril commented Dec 6, 2017

I'm letting this RFC continue concurrently with #2237 . If that RFC is accepted (my preference) - I will close this one as resolved since you then have T: const Default bounds.

@sfackler
Copy link
Member

sfackler commented May 2, 2018

The libs team discussed this today, and agree that some kind of const impl/bound approach is the better way to go here. There's a bunch of other trait-based functionality I'd like to be able to use in a const context, but it doesn't seem great to have ConstAdd, ConstAddAssign, ConstBitOr, ...!

@rfcbot fcp close

@rfcbot
Copy link
Collaborator

rfcbot commented May 2, 2018

Team member @sfackler has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-close This RFC is in PFCP or FCP with a disposition to close it. labels May 2, 2018
@Centril
Copy link
Contributor Author

Centril commented May 2, 2018

@sfackler I agree; Something like #2237 is the better way to do this. You can just close this outright if you wish :)

@rfcbot
Copy link
Collaborator

rfcbot commented May 8, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels May 8, 2018
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels May 18, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented May 18, 2018

The final comment period, with a disposition to close, as per the review above, is now complete.

By the power vested in me by Rust, I hereby close this RFC.

@rfcbot rfcbot added closed This FCP has been closed (as opposed to postponed) and removed disposition-close This RFC is in PFCP or FCP with a disposition to close it. labels May 18, 2018
@rfcbot rfcbot closed this May 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed This FCP has been closed (as opposed to postponed) finished-final-comment-period The final comment period is finished for this RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants