Should `Foo::new() -> Foo` just be `impl Default for Foo`?

Is there any reason to write:

impl Foo {
  pub fn new() -> Foo { ... }
}

instead of

impl Default for Foo { ... } 

i.e. we have a new() field which takes no arguments and returns a Foo

2 Likes

I would say that it is idiomatic to also provide a new method when you implement the Default trait.

8 Likes

My philosophy is to implement Default, because it allows that type to then be used in code that only cares if a type is Default (e.g., embedding inside of a Mutex/RwLock and being able to use Mutex::default()). If whatever is being done in Default can be done as a const fn, that's when I tend to add a new() implementation.

Many people do prefer being able to type Foo::new() because it's shorter than Foo::default(). So far my crates don't have enough users to have generated complaints about my opinion on this subject :slight_smile:

2 Likes

The API guidelines also comment on this at the bottom of item C-COMMON-TRAITS.

6 Likes

Additionally, it's a smart policy to have Default impls call new, not vice versa. This allows you to write new as a const fn.

8 Likes

I got curious about the origins of this, since many things in Rust have had vibrant discussions around them.It's been bugging me the last few days ever since this was pointed out -- I am striving to write well-designed crates, and this API guideline went against my own philosophy.

So, I went and tried to find when this change was added to the guidelines. I found this issue and this is the PR. Sadly, both don't contain any discussions about this particular paragraph being added.

The issue links three comments made about new vs default, and all three seem to express sentiments that Default can replace new() if there's no reason for the two to exist. Yet, the text that was added to the guidelines reflects a strong preference to having both new() and default().

My two arguments against having a new() that calls through to Default are:

  1. From a consumer perspective, there is no guarantee by Default that it must be the same as new(), so I must ensure I document that calling new() is the equivalent of calling Default::default(). Consumers must consult the documentation or code to determine if new() and Default are identical.
  2. I must add another unit test, and potentially a trait implementations. Given the documentation we added in point 1 states that new() is the same as calling Default, a good unit test would assert that the outputs of both functions are the same. However, that can't be done if the types don't already implement PartialEq. Does the convenience of new() justify extra derive and an extra unit test?

I believe these are very minor arguments, but these are the reasons I formed the opinions on API design I currently hold.

I'm curious, what do other people feel on the subject?

1 Like

The const fn issue will hopefully be solved in the future with the const_trait_impl feature.

I don't like duplicated functionality, would prefer to only have one function.

2 Likes

A (0-ary) new() that isn't equivalent to Default::default() is highly non-idiomatic. This is not a good reason to not provide new().

If the types don't support a notion of equality, then how can you even say that the two functions return "equivalent" values? It seems to me like there's nothing to test (nor document nor ensure) in this case. However, this should be an exceptional case – most types should implement PartialEq/Eq as well, by these very Guidelines.

This is a valid point.

I ran into a better situation today. I had an existing type that followed my current pattern -- it implemented Default but did not provide a new() function.

Today, I decided I wanted to add a constructor that took the single "always required" type for this struct. The type implements default, so for my struct, a Default implementation still makes sense. But, it's such an important property that I want it to be set upon construction and not afterwards. If I had an existing new() function, I would add this type to that function causing a breaking API change.

Because of my decision to not implement new() since it would have duplicated Default's functionality, I saved myself a SemVer version bump when I added this new feature today.

What claims will the documentation make about the function "new"? Regardless of how the documentation is phrased, it will make claims about its functionality and those claims can be tested. Whether someone chooses to test a function that is a simple pass-through is another story, but those functions certainly are things that can be tested.

Regarding PartialEq -- I've encountered many situations where I can't implement a standard trait because a type I'm consuming from another crate didn't implement that trait. While we can agree that most types should implement these traits, it doesn't help the situation when someone disagrees with implementing it for a type or it cannot be implemented for other constraints.

Not as far as I know. I have shifted in this direction since discovering that rustc_hash::FxHashMap has default() but not new().

When your type has both an "empty" initial value and a slightly more intelligent initial value, the typical pattern is to implement Default then provide an appropriately named constructor.

For example, say you were implementing an interpreter which might provide the user with a bunch of builtin functions. The way I would implement it is like this:

#[derive(Default)]
struct Interpreter {
  operations: HashMap<String, Function>,
}

impl Interpreter {
  pub fn with_builtins() -> Self {
    let mut interpreter = Interpreter::default();
    interpreter.operations.insert("add", ...);
    ...

    interpreter
  }
}

That way your Interpreter::new() constructor can still defer to Default (or vice versa), while the the "smart" constructor is just adding to the API without changing existing functionality in a semver-breaking way.

4 Likes

The constructor in my example is the primary method of constructing the type. Per the API guidelines:

The name new should generally be used for the primary method of instantiating a type. Sometimes it takes no arguments, as in the examples above. Sometimes it does take arguments, like Box::new which is passed the value to place in the Box .

Since it's what I want people to use to construct a new instance of this type (unless they choose Default), the appropriate name is new. Prior to this new design today, I took "generally" to mean that Default could be considered a primary means of instantiating a type. After all, the API guidelines recommend all types that can be initialized in a default state should implement Default.

To put it another way -- if I were designing the type today, the function name would be new(). When I'm refactoring, I first try to strive for the ideal API before I concern myself with backwards compatibility. To me, the ideal API has this function named new based on my understanding of the API guidelines and my own expectations based on consuming crates in the ecosystem.

I personally haven't read a reason that justifies introducing duplicate code into my repositories.

1 Like

If that's the case, then new would indeed be an appropriate name.

The API guidelines are just guidelines to help steer the ecosystem towards common idioms. It's best to follow them when you can, but you also need to know when you should deviate.

For example,on one type it might make sense to have this...

#[derive(Default)]
struct Foo { required: Required }

impl Foo {
  fn new() -> Self { Foo::default() }
}

#[derive(Default)]
struct Required { ... }

... or maybe this...

struct Foo { required: Required }

impl Foo {
  fn new_with_required(required: Required) -> Self { Foo { required } }
}

// could also be derived
impl Default for Foo {
  fn default() -> Self { Foo::new_with_required(Required::default()) }
}

... or for another type it might make sense to write this instead:

struct Config { sections: HashMap<String, Section> } 

impl Config {
  /// Construct a new `Config` object which can be modified by the user. 
  /// This might also be called `Config::empty()`.
  fn new() -> Config { Config { sections: HashMap::new() } }

  fn load(path: &Path) -> Result<Config, Error> { ... }
}

impl Default for Config {
  fn default() -> Self {
    // construct a Config that most people will want to use out of the box
    let mut sections = HashMap::new();
    sections.insert("first".to_string(), ...);
    sections.insert("second".to_string(), ...);
    ...
    Config { sections }
  }
}

All are equally valid given the context in which they are used.

Most types that have a Default impl will also have a new() constructor though because the two actually serve subtly different purposes... A new() constructor is typically the primary way to create your type and has the shorter name, while the Default constructor is more useful in generic contexts or when you know it will be stored in a type that wants to derive Default.

All of these examples provided simply reiterate the current API guidelines. I understand that these are guidelines, and I understand the guidelines as-written. I take issue with this phrase in them:

Note that it is common and expected for types to implement both Default and an empty new constructor

The bolded phrase is what I am saying is a bad API guideline, and I've tried to provide reasons I believe it's bad. This phrase encourages code duplication -- purposefully implementing two methods that do the exact same thing. The phrase "and expected" shows up in both sections where this practice is mentioned. I would take no issue if the guideline omitted "and expected."

Why are the guidelines encouraging code duplication? What problem are the guidelines solving by telling users they're expected to provide both functions? Looking at those reasons now in 2022, Is code duplication the best approach to solving the problem?

You would generally implement one by calling the other method.

3 Likes

I'd spell it fn with_required(...), like with_capacity in the std library. Reads nicely as Foo::with_required(...).

2 Likes

That doesn't go to answer the original code smell: why are we encouraging users to implement two functions that serve the exact same purpose? Even if the methods aren't actually implemented using copies of the code, it's still duplicate code.

What problem is being solved by having two functions with the same purpose and different names? Is this API guideline recommending two functions with identical purposes the best way to solve it when looking at Rust today?

I am more concerned with the duplication of API than with having to write more code in the implementation (although both are annoying).

I think part of the reason for this recommendation might be that inherent methods are better visible in the docs than trait methods. Perhaps that's something that could be addressed in rustdoc somehow?

1 Like

Well, there are many places where the same functionality can be accessed both via a dedicated method and via a trait. It's useful because implementing traits lets your type be used in generic functions that require that trait.

I mean, for an extreme example consider the number of ways you can convert an &str into a String. There's String::new(s), String::from(s), s.to_string(), s.to_owned(), s.into(), and probably some I forget. Besides String::new, all of these exist so the &str type can be used in various contexts that require a type to implement some particular trait.

2 Likes

That's a good argument for why the trait method is useful, but not an argument for why the inherent method is useful if it's already implemented via the trait.

2 Likes