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

Free default() forwarding to Default::default() #73001

Merged
merged 2 commits into from
Jun 8, 2020

Conversation

ilya-bobyr
Copy link
Contributor

@ilya-bobyr ilya-bobyr commented Jun 4, 2020

It feels redundant to have to say Default::default() every time I need a new value of a type that has a Default instance.
Especially so, compared to Haskell, where the same functionality is called def.
Providing a free default() function that forwards to Default::default() could improve the situation.
The trait is still there, so if someone wants to be explicit and to say Default::default() - it still works, but if imported as std::default::default;, then the free function reduces typing and visual noise.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 4, 2020
@Mark-Simulacrum
Copy link
Member

r? @dtolnay as this is an unstable addition to standard library surface so should get libs team attention.


That said, speaking personally, I don't think this is a good move. I have usually found that uses of Default::default() are usually a poor choice in the long run -- I prefer T::default() in generic contexts or SomeType::default() where possible.

@ilya-bobyr
Copy link
Contributor Author

ilya-bobyr commented Jun 4, 2020

When you need to be explicit about the type - I totally agree, you want T::default().
But in certain contexts type can be inferred. This is where, currently, one needs to say Default::default(). And this is what the Default documentation currently suggests as well.

For example, when changing a value of some of the fields in the struct:

let options = SomeOptions { foo: 42, ..Default::default() };

I do not think it adds a lot of clarity if you repeat the type names twice:

let options = SomeOptions { foo: 42, ..SomeOptions::default() };

@petrochenkov
Copy link
Contributor

petrochenkov commented Jun 4, 2020

I'd rather make use Default::default; (and any use Trait::trait_item;) work at language level.
It should be a pretty minor change from both compiler and language point of view because it's mostly equivalent to already supported use Enum::Variant;.

(As for SomeOptions { foo: 42, ..Default::default() } specifically, it's probably better solved at language level as well with SomeOptions { foo: 42, .. }.)

EDIT: There are existing issues for both of these features somewhere in this or the rfcs repo, but I can't find them right now.

@Mark-Simulacrum
Copy link
Member

Yeah, I would prefer a language solution too I think :)

I will also note that I think I've used the .. syntax in structs maybe twice over the last ~3+ years of Rust development, so it doesn't immediately jump out at me as sufficient justification for a new library function; but maybe its usage is more common in some areas of Rust code I am unfamiliar with.

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 don't have high confidence that the language change will happen, as much as it would be better, so I am on board with landing this unstable.

We probably wouldn't stabilize until use Default::default; is ruled out though.

src/libcore/default.rs Outdated Show resolved Hide resolved
src/libcore/default.rs Outdated Show resolved Hide resolved
src/libcore/default.rs Outdated Show resolved Hide resolved
src/libcore/default.rs Outdated Show resolved Hide resolved
src/libcore/default.rs Outdated Show resolved Hide resolved
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.

Thanks! I like this example code better.

src/libcore/default.rs Outdated Show resolved Hide resolved
src/libcore/default.rs Outdated Show resolved Hide resolved
@ilya-bobyr
Copy link
Contributor Author

Thanks for the review!

Spent some time looking for existing proposals on extending the language to allow direct import of trait methods but did not find anything. Thought, language change would be a much, much larger time investment, also requiring more expertise. Not sure if I would be able to pull it off.

@ilya-bobyr ilya-bobyr force-pushed the master branch 2 times, most recently from 229c95a to 67f9a39 Compare June 5, 2020 07:28
@bluss
Copy link
Member

bluss commented Jun 5, 2020

Since there is no RFC I will put in a couple of questions here.

We have the following existing alternatives:

  1. T::default() if the type is known or partially known. Note that we can use path-like syntax PathLike::default() and type syntax <Type>::default().
  2. <_>::default() as a special case of the type syntax when the type is to be inferred. This works the same way as the free function in this PR.
  3. Default::default.

The existing syntaxes have the following benefit. Using an explicit type makes the code easy to read, when the explicit type is the "source" of the type, this becomes the best option. Even when the type would be redundant, it can, as a matter of taste, still be good code style. Instead of let x: T = Default::default(), prefer let x = T::default() instead.

The <Type>::default() syntax allows partial inference, for example like <HashMap<_, _>>::default() (pick a hashmap with the default for the hasher parameter).

The <_>::default() syntax has the benefit that it allows full type inference, and that it works as long as the Default trait is in scope. So it has power equal to the free function being added here.

The drawback would be if you don't think this is good-looking or easy to understand code.

  1. A fourth alternative is that the user defines its own equivalent default() function.

I wrote this so that we know the alternatives, so that they can be specifically addressed w.r.t stabilization of this.

#[unstable(feature = "default_free_fn", issue = "73014")]
#[inline]
pub fn default<T: Default>() -> T {
Default::default()
Copy link
Member

Choose a reason for hiding this comment

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

This is a nitpick I'd mention in any PR, but I guess it's extra strange now - here, why don't we prefer using the style T::default()?

Copy link
Member

Choose a reason for hiding this comment

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

I would like to stick with Default::default(). The documentation of the function describes it as being equivalent to Default::default() and this reinforces that, even if T::default() would also be equivalent. It would be less clear for the documentation to describe the function as being equivalent to T::default() because T::default() has type T while Default::default() has a type that is filled in by inference.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm here because avoiding Default::default is a pet peeve of mine, which in some way I must partly share with ilya-bobyr then. Since we say equivalent, any equivalent code would be fine, and I think we should prefer the style that we would like to see in Rust codebases around the ecosystem, hence T::default().

If the docs say Default::default, maybe that could change now or in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default::default() was the first thing I thought about, as this is what I was trying to replace. If @dtolnay likes Default::default() more here, I would just leave it.

@dtolnay
Copy link
Member

dtolnay commented Jun 5, 2020

@bluss In response to #73001 (comment): I don't disagree with anything in there but I don't see this as being a replacement for let x = T::default(); i.e. let x: T = default(); is not better. Where this comes up in my codebase is when instantiating Thrift-generated data structures which look a lot like the example code in the PR.

let zw = ZWriteData { key, ..default() };

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.

src/libcore/default.rs Outdated Show resolved Hide resolved
When creating default values a trait method needs to be called with an
explicit trait name.  `Default::default()` seems redundant.  A free
function on the other hand, when imported directly, seems to be a better
API, as it is just `default()`.  When implementing the trait, a method
is still required.
@dtolnay
Copy link
Member

dtolnay commented Jun 7, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jun 7, 2020

📌 Commit ebb8722 has been approved by dtolnay

@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 Jun 7, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 7, 2020
Free `default()` forwarding to `Default::default()`

It feels a bit redundant to have to say `Default::default()` every time I need a new value of a type that has a `Default` instance.
Especially so, compared to Haskell, where the same functionality is called `def`.
Providing a free `default()` function that forwards to `Default::default()` seems to improve the situation.
The trait is still there, so if someone wants to be explicit and to say `Default::default()` - it still works, but if imported as `std::default::default;`, then the free function reduces typing and visual noise.
@RalfJung
Copy link
Member

RalfJung commented Jun 8, 2020

@bors rollup

@leonardo-m
Copy link

An usage example of default could be added to Vec::resize_with.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 8, 2020
Rollup of 10 pull requests

Successful merges:

 - rust-lang#72026 (Update annotate-snippets-rs to 0.8.0)
 - rust-lang#72583 (impl AsRef<[T]> for vec::IntoIter<T>)
 - rust-lang#72615 (Fix documentation example for gcov profiling)
 - rust-lang#72761 (Added the documentation for the 'use' keyword)
 - rust-lang#72799 (Add `-Z span-debug` to allow for easier debugging of proc macros)
 - rust-lang#72811 (Liballoc impl)
 - rust-lang#72963 (Cstring `from_raw` and `into_raw` safety precisions)
 - rust-lang#73001 (Free `default()` forwarding to `Default::default()`)
 - rust-lang#73075 (Add comments to `Resolve::get_module`)
 - rust-lang#73092 (Clean up E0646)

Failed merges:

r? @ghost
@bors bors merged commit 244465d into rust-lang:master Jun 8, 2020
@ilya-bobyr
Copy link
Contributor Author

An usage example of default could be added to Vec::resize_with.

Agreed. But maybe when the feature is stabilized?

As David Tolnay (dtolnay) said: "I would prefer not to feature an unstable API so heavily in the doc of an extremely widely used trait."
And it totally makes sense: one can used unstable features only when using nightly build or when compiling rustc yourself.

@dtolnay
Copy link
Member

dtolnay commented Jun 8, 2020

Yeah that should be a checklist item in the tracking issue. It can be done in the stabilization PR or a separate PR just beforehand.

@ilya-bobyr
Copy link
Contributor Author

I've added an item to the tracking issue.

@jyn514
Copy link
Member

jyn514 commented Jul 7, 2022

EDIT: There are existing issues for both of these features somewhere in this or the rfcs repo, but I can't find them right now.

rust-lang/rfcs#1995

@hkBst
Copy link
Contributor

hkBst commented Jul 1, 2023

Since there is no RFC I will put in a couple of questions here.

We have the following existing alternatives:

1. `T::default()` if the type is known or partially known. Note that we can use path-like syntax `PathLike::default()` and type syntax `<Type>::default()`.

2. `<_>::default()` as a special case of the type syntax when the type is to be inferred. This works the same way as the free function in this PR.

3. `Default::default`.

The existing syntaxes have the following benefit. Using an explicit type makes the code easy to read, when the explicit type is the "source" of the type, this becomes the best option. Even when the type would be redundant, it can, as a matter of taste, still be good code style. Instead of let x: T = Default::default(), prefer let x = T::default() instead.

The <Type>::default() syntax allows partial inference, for example like <HashMap<_, _>>::default() (pick a hashmap with the default for the hasher parameter).

The <_>::default() syntax has the benefit that it allows full type inference, and that it works as long as the Default trait is in scope. So it has power equal to the free function being added here.

The drawback would be if you don't think this is good-looking or easy to understand code.

4. A fourth alternative is that the user defines its own equivalent `default()` function.

I wrote this so that we know the alternatives, so that they can be specifically addressed w.r.t stabilization of this.

  1. You can use Default as D; locally, to be able to use D::default().

ad 4. this user-defined alternative could be called d, or def, or whatever is to the user's taste:

#[derive(Default)]
struct Struct {
    member1: u128,
    member2: u128,
}

fn d<T:Default>() -> T {
    T::default()
}

fn main() {
    use Default as D;
    Struct { ..Default::default() };
    Struct { ..D::default() };
    Struct { ..d() };
}

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.