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

Tracking issue for std::default::default() #73014

Closed
1 of 4 tasks
ilya-bobyr opened this issue Jun 5, 2020 · 47 comments · Fixed by #113469
Closed
1 of 4 tasks

Tracking issue for std::default::default() #73014

ilya-bobyr opened this issue Jun 5, 2020 · 47 comments · Fixed by #113469
Labels
B-unstable Feature: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ilya-bobyr
Copy link
Contributor

ilya-bobyr commented Jun 5, 2020

The feature gate for the issue is #![feature(default_free_fn)].

Addition of a free default() function to the default module.

When constructing a value using Default::default() the word "default" is repeated twice.

See #73001 for additional details, including alternative solutions.

Steps

Unresolved Questions

  • Should the language allow direct import of trait methods, this feature becomes redundant. This addition is likely to be stabilized only when we decide that direct import of trait methods is will not be supported.

Implementation history

@ilya-bobyr ilya-bobyr added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jun 5, 2020
@dtolnay dtolnay changed the title Tracking Issue for free default() function forwarding to Default::default() Tracking issue for std::default::default() Jun 5, 2020
@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 5, 2020
@ilya-bobyr
Copy link
Contributor Author

@rustbot modify labels to +F-default_free_fn

@bluss
Copy link
Member

bluss commented Jun 5, 2020

You call out Default::default() in the description here - but why aren't T::default() or <$any_type>::default() good alternatives? They work today, if the Default trait is in scope, and it is in the prelude.

@ilya-bobyr
Copy link
Contributor Author

T::default() is convenient when the compiler can not infer the type. But when it can, T because redundant. Here is a discussion where this was mentioned as well: #73001

@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2020

FWIW I think it is often still good style to say T::default() to communicate to the human reader the type being constructed away, which can be declared very far away in the code (even in a different file).

@ilya-bobyr
Copy link
Contributor Author

ilya-bobyr commented Jun 6, 2020

FWIW I think it is often still good style to say T::default() to communicate to the human reader the type being constructed away, which can be declared very far away in the code (even in a different file).

I do no think this change takes away that and in certain cases you have to say T::default(). The change allows you to say default() in cases where you are currently saying Default::default().

@birkenfeld
Copy link
Contributor

You call out Default::default() in the description here - but why aren't T::default() or <$any_type>::default() good alternatives? They work today, if the Default trait is in scope, and it is in the prelude.

Default::default() happens a lot in struct displays, with StructType { ..., StructType::default() } being as redundant as StructType { ..., Default::default() }.

@bestouff
Copy link

bestouff commented Jun 17, 2020

A bit late to the party, but why not just a use Default::default() in the prelude instead ?
That's less code to inline for the optimizer.

@ilya-bobyr
Copy link
Contributor Author

A bit late to the party, but why not just a use Default::default() in the prelude instead ?
That's less code to inline for the optimizer.

I am not sure I understand exactly what are you suggesting. But if the motivation described in the issues summary is not enough, please, read the discussion in #73001

@KodrAus KodrAus added B-unstable Feature: Implemented in the nightly compiler and unstable. Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. labels Jul 29, 2020
@robinmoussu
Copy link

Is there a reason why this new free function isn't added in the prelude when the feature default_free_fn is used?

@jhpratt
Copy link
Member

jhpratt commented Nov 18, 2020

@robinmoussu Any additions to the prelude need to go through their own RFC. The bar is quite high, to say the least.

@quebin31
Copy link

quebin31 commented Mar 6, 2021

What's the status of this feature? I've found that The Book should be updated (Appendix C - Derivable Traits) but not really sure how to proceed.

@BurntSushi
Copy link
Member

BurntSushi commented May 21, 2021

One thing that I think is worth calling out is that this is putting a free default function inside the std::default module. So in order to avoid the stutter complaint of using Default::default(), it sounds like you'd have to add a use std::default::default; somewhere in order to use default. So that stutter is still there in some sense and you have to write an import for it. Granted, this gets rid of the stutter at the call site.

I also want to bring up the T::default() alternative. In particular, while it of course is not a replacement to avoid the stutter in all cases, I do think it is generally better to use T::default() in lieu of either a free default function or Default::default. So to me, this on its own reduces the incidence of having to write a stuttering Default::default call, and thus also reduces the importance of adding things to std that allow one to avoid the stutter.

As a more general point, to be honest, I'm not a huge fan of dumping aliases to things into std. I think it invites questions like, "Wait, what's the difference between the free default function and Default::default?" And it kind of just becomes another thing that folks need to learn, and another thing where some code will undoubtedly be using Default::default and other code will just be using the free function, which are expressed as two different things in std's API.

More generally, do we have other aliases like this in std already? I know we have things like std::u64::MAX and u64::MAX, but we've deprecated the former. I wonder if it makes more sense to talk about "let's add aliases to std" as a more general policy question.

In #73001, @dtolnay wrote:

When there are a lot (sometimes 100+) of these in a file it's reasonable to want to import Default::default, and the suggestions of ZWriteData { key, ..ZWriteData::default() } or ZWriteData { key, ..Default::default() } or ZWriteData { key, ..<_>::default() } are not better, whether due to repetition or an intimidating amount of punctuation.

To be honest, this seems infrequent enough where it might be reasonable to just define an internal helper function.

@bestouff
Copy link

Isn't the plan to have std::default::default in the prelude ?

@BurntSushi
Copy link
Member

I'm not aware of that plan. Could you please link to it?

@bestouff
Copy link

You're right my bad. It was just a reminiscence of some random reddit post.

@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/libs-api meeting. The general sentiment was:

  • We'd like to make sure that default() can work even in const contexts. Ping @rust-lang/wg-const-eval; what's the status of being able to say, for a free function like default(), that it's const if and only if there's a const impl of Default? We'd like to know if that'd be possible, even if it's nightly-only. And if it is, we'd like to use that for default().
  • Assuming that's possible, those of us in the meeting were generally in favor of having default(), and adding it to the prelude.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 1, 2021

it's possible and already works (not yet in bootstrap libcore though): https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=abd79539fa8900dd7f9e9d657f5f3e9a

But we are not yet confident in whether we can safely (in the stability sense) stabilize a function that uses this feature without also stabilizing something of that feature at the same time.

@jhpratt
Copy link
Member

jhpratt commented Sep 1, 2021

Are we working on the assumption that something like use core::default::Default::default; won't be available in the future? Or is it more that we're okay with the potential for a little duplication down the road?

@joshtriplett
Copy link
Member

@jhpratt If one day it becomes possible to use a trait method, then I think we'd still want to export the free function default(), we'd just turn it into a pub use. So I don't see any reason we should wait for that.

@oli-obk That looks great. It'd be helpful to know if we could stabilize a function using const_fn_trait_bound and const_trait_impl without first stabilizing those two features themselves, but even if that's not the case, the confirmation that this can work is good enough for me to think we should stabilize this.

@rust-lang/libs-api I'm starting an FCP to get consensus on providing the free function default(), and exporting it from the prelude. (This consensus does not mean we have to stabilize it immediately; we can work with const-eval to figure out if it's possible to export something based on const_fn_trait_bound and const_trait_impl, or if we need to wait for those features. For now, I'd just like to confirm consensus for the general plan of providing the function and the prelude re-export.)

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Sep 2, 2021

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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 Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 2, 2021
@mockersf
Copy link
Contributor

mockersf commented Jun 19, 2023

This came up with Bevy uses of a free default() function: bevyengine/bevy#8879
The lint UnconditionalRecursion: "function cannot return without recursing" is not raised by using ..default()

Does not raise a warning:

#![feature(default_free_fn)]
use std::default::default;

struct SomeStruct;
impl Default for SomeStruct {
    fn default() -> Self {
        Self { ..default() }
    }
}

Raises a warning:

struct SomeStruct;
impl Default for SomeStruct {
    fn default() -> Self {
        Self {
            ..Default::default()
        }
    }
}

This is #102825

@m-ou-se m-ou-se added the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Jun 21, 2023
@jakoschiko
Copy link
Contributor

There is one thing that I noticed:

When I initialize a struct Foo I often start with Foo { ..Foo::default() } and then jump to the definition of Foo::default (using rust-analyzer) to see what the defaults are and to decide which fields I want to change. That's possible with Foo::default() and Default::default(), but that's not possible with the free default function (e.g. the one from bevy, it just jumps to definition inside the bevy crate). That's very unfortunate because from a visual perspective I would prefer ..default().

@gilescope
Copy link
Contributor

gilescope commented Jun 24, 2023 via email

@Amanieu
Copy link
Member

Amanieu commented Jun 27, 2023

We discussed this in the libs-api meeting today. We would prefer to see a language feature to import the Default::default method directly. The language team seems to be open to this, but someone needs to write up a language proposal and drive the design of this feature.

Since we are unlikely to stabilize the default function as-is, we would like to remove it from the standard library.

@rfcbot fcp close

@rfcbot
Copy link

rfcbot commented Jun 27, 2023

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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 Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Jun 27, 2023
@ilya-bobyr
Copy link
Contributor Author

We discussed this in the libs-api meeting today. We would prefer to see a language feature to import the Default::default method directly. The language team seems to be open to this, but someone needs to write up a language proposal and drive the design of this feature.

Since we are unlikely to stabilize the default function as-is, we would like to remove it from the standard library.

I am not familiar with the removal process, but if it is just another issue with, essentially, a revert of the original change, I can do that.
Though, if someone happens to have a link to an example, I would appreciate it.

Was the consensus to remove it now, or after support for importing trait methods is added to the language?

If someone wants to mentor me in writing a language proposal, I could probably do that as well.
As I have not participated in the language processes in the past, it is not very likely I would manage it by myself.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jun 27, 2023
@rfcbot
Copy link

rfcbot commented Jun 27, 2023

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

@Amanieu Amanieu removed the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Jul 4, 2023
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 7, 2023
@rfcbot
Copy link

rfcbot commented Jul 7, 2023

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

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jul 7, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 9, 2023
…anieu

Remove `default_free_fn` feature

Closes rust-lang#73014
r? `@Amanieu`
@bors bors closed this as completed in ec479ba Jul 9, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jul 13, 2023
@dscottboggs
Copy link

So, the feature has been removed in favor of a language change, but without actually formally proposing the language change? Disappointing.

@minghu6
Copy link

minghu6 commented Aug 18, 2023

We discussed this in the libs-api meeting today. We would prefer to see a language feature to import the Default::default method directly. The language team seems to be open to this, but someone needs to write up a language proposal and drive the design of this feature.

Since we are unlikely to stabilize the default function as-is, we would like to remove it from the standard library.

@rfcbot fcp close

TL;DR

@gamertike
Copy link

maybe we can have a language feature (alias) specifically for when creating structs with StructName {} to support .. which expands to ..Default::default.

Example:

MyStruct {
  a: 1,
  b: 2,
  ..,
}

This makes sense because the language already has this syntax is places such as pattern matching

@LunarLambda
Copy link

has there been any followup RFC/tracking issue for an eventual use Trait::method; syntax?

@crumblingstatue
Copy link
Contributor

has there been any followup RFC/tracking issue for an eventual use Trait::method; syntax?

For what it's worth, rust-lang/rfcs#1995 is an open issue on this, maybe we could have some discussion there to move things forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Feature: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.