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

Default values for struct fields #7

Merged
merged 29 commits into from
Dec 1, 2016
Merged

Default values for struct fields #7

merged 29 commits into from
Dec 1, 2016

Conversation

KodrAus
Copy link
Contributor

@KodrAus KodrAus commented Oct 6, 2016

RFC content for #1

cc: @nrc @leodasvacas

@KodrAus KodrAus changed the title Default values for struct fields [WIP] Default values for struct fields Oct 6, 2016
@KodrAus KodrAus changed the title [WIP] Default values for struct fields Default values for struct fields Oct 6, 2016
#[derive(Default)]
struct Foo {
a: &'static str,
b: bool = true

Choose a reason for hiding this comment

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

nit: "," at the end of the line

```rust
impl Default for Foo {
fn default() -> Self {
Foo {

Choose a reason for hiding this comment

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

nit: Indentation here looks a little off

Copy link
Owner

Choose a reason for hiding this comment

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

prefer spaces to tabs

```

This requires all fields in the struct be assigned a value, unless the struct implements
the Default trait.
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't rely on the Default trait, you can supply any struct instance after the .. and the new struct takes its values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't actually know that 👍 That's pretty neat

The problem with this is that it requires Foo to derive Default, which may not always
be desirable.

The existing struct construction also can't be used when a struct has private fields:
Copy link
Owner

Choose a reason for hiding this comment

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

prefer "initialisation" to "construction"


To work around these shortcomings, users can create constructor functions or more elaborate builders.
The problem with a constructor is that it needs to be kept in sync with the fields of the struct, or
require the user reassign values they want to change.
Copy link
Owner

Choose a reason for hiding this comment

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

I'd say a bigger problem with ctors is you need one for each combination of fields

Builders enable more advanced construction, but also need to kept in-line with the struct they build,
and can be overkill for large, but uncomplicated structs.

With field defaults a caller can instantiate a struct without needing builders
Copy link
Owner

Choose a reason for hiding this comment

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

From here on down is presenting the feature rather than motivating it, so I would move it to the next section

b: true,
c: 42
};
```
Copy link
Owner

Choose a reason for hiding this comment

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

I would also mention that for big structs which always get the same value, it is repetitive and inconvenient to always instantiate each field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would think that case is easily solved by a constructor function

Copy link
Owner

Choose a reason for hiding this comment

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

true, but it is still annoying boiler-plate even if it is just in a ctor function, and it is not uncommon to have multiple ctor functions


# Motivation
[motivation]: #motivation

Why are we doing this? What use cases does it support? What is the expected outcome?
Today, Rust allows you to create an instance of a struct using a literal syntax:

Copy link
Owner

Choose a reason for hiding this comment

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

structure-wise, I would give short sentences for each of the pieces of motivation here, then explain in more detail like you are doing.

I don't think you need this first example

Copy link

Choose a reason for hiding this comment

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

Is there a standard terminology for this syntax? Some languages call them "struct literals", maybe we can call them "struct initialisers", the reference uses "struct expressions".

Copy link
Owner

Choose a reason for hiding this comment

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

I usually call them struct literals, I don't think there is standard terminology.

This is the bulk of the RFC. Explain the design in enough detail for somebody familiar
with the language to understand, and for somebody familiar with the compiler to implement.
This should get into specifics and corner-cases, and include examples of how the feature is used.
The semantics of field defaults are the same as consts.
Copy link
Owner

Choose a reason for hiding this comment

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

This sentence is a bit vague - semantics could mean a lot of things. I would make this a note that we are modelling field defaults on consts and move it later.

I would start with the syntax, then explain restrictions on the value.

The semantics of field defaults are the same as consts.
The type must be supplied, and the value must be a valid compile-time constant.

Field defaults take precedence over `Default::default()`.
Copy link
Owner

Choose a reason for hiding this comment

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

Make clear that this is a feature of derive(Default)

}
}
}
```
Copy link
Owner

Choose a reason for hiding this comment

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

Probably don't need this expansion.

}
```

Supplied field values take precedence over field defaults:
Copy link
Owner

Choose a reason for hiding this comment

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

I would move this up above the derive(Default) stuff

};
```

That means if `Default` is manually implemented for a struct and default fields are
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think you need this section on Default. On the other hand, I would add some detail on function update syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain what you mean by function update syntax? You've lost me on that one :)

Copy link

@leoyvens leoyvens Oct 9, 2016

Choose a reason for hiding this comment

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

function update syntax or functional record update is the formal name for ..Foo } syntax.

Copy link
Owner

Choose a reason for hiding this comment

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

yeah sorry, I meant functional, not function.

@@ -28,9 +233,29 @@ Why should we *not* do this?
# Alternatives
[alternatives]: #alternatives

What other designs have been considered? What is the impact of not doing this?
Just stick with builders and constructor functions.
Copy link
Owner

Choose a reason for hiding this comment

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

This is basically just saying "don't do this" which is not very useful. It would be better to list tweaks to the design, and explain why our proposal is better than those alternatives

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh, this is basically a placeholder. It turns out writing about alternatives and drawbacks is kind of difficult.

Is `const` the right feature to base off?
It means field values are just as constrained as `const` is now,
but also prevents struct definitions from becomes bloated by complex logic that
maybe belongs in a builder instead.
Copy link
Owner

Choose a reason for hiding this comment

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

I would move this to alternatives (and phrased a bit differently). Also consider runtime overhead/expectations of struct literal vs function call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm investigating this one now. It would depend on the way this feature is implemented right?

Copy link
Owner

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

@KodrAus KodrAus Oct 11, 2016

Choose a reason for hiding this comment

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

Is there anywhere I can get a sense of how struct literals work now, so I've got some context of how this should be implemented?

I think the simplest implementation would be to treat struct initialisation with field defaults as sugar for the 'real' initialiser, where defaults for missing fields are spliced in.

In this case, assuming no inlining, you aren't paying for a function call. Because this is opaque to the caller, with field defaults there might be other optimisations the compiler could do to make it more efficient. I'm not sure what those could be yet though.

Copy link
Owner

Choose a reason for hiding this comment

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

So, actually, let me qualify my earlier "yes". I think it depends more on the design of the details of the feature than on the actual implementation.

If you want to investigate the details of how struct lits are compiled, you need to look at the compiler, or possibly dump the LLVM output. But really, there is nothing surprising happening atm in compilation. The only detail really FRU.

What is important, I think, is that there is no initialisation at all, it really is just a literal expression. That is, it just copies the field values into a given bit of memory.

I think the simplest implementation would be to treat struct initialisation with field defaults as sugar for the 'real' initialiser, where defaults for missing fields are spliced in.

I agree, and using const values for default values fits well with this model - there is no extra runtime overhead at all. Anything beyond const values (I think) introduces some runtime overhead. If you allow Default integration or other extensions, then there is runtime overhead.

So, I agree with your original points - "complex logic that maybe belongs in a builder instead", and I'm trying to dig into the 'why' behind this point. I think expectations around runtime overhead are part of that.

}
```

Should this type automatically derive `Default`?
Copy link
Owner

Choose a reason for hiding this comment

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

Generally, we always opt-in to non-auto-traits.

```

Should this type automatically derive `Default`?
Should we be able to construct this type as if it's a ZST, like `let foo = Foo;`?
Copy link
Owner

Choose a reason for hiding this comment

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

This is a unit struct, you can also use Foo {} for an empty struct, and the braces must match - you can't use one declaration with the other construction. That suggests you can't use Foo, but need Foo{} (also note that the two forms do have slightly different rules for naming).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense 👍

@KodrAus
Copy link
Contributor Author

KodrAus commented Oct 7, 2016

I'll work through this feedback and give it a good complete pass over this weekend. I'll shoot a ping when I'm finished.

@KodrAus
Copy link
Contributor Author

KodrAus commented Oct 14, 2016

@nrc Sorry it's taken so long to get to this point, but I'm now happy with the state of this. I've addressed the comments thus far and think it's ready for another round of feedback.

a: "Hello",
c: 42,
}
```

Choose a reason for hiding this comment

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

Would be good to add a field using default().

Today, Rust allows you to create an instance of a struct using a literal syntax.
This requires all fields in the struct be assigned a value, and can't be used
with private fields.
It can also be inconvenient for large structs that usually receive the same values.

Choose a reason for hiding this comment

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

This sentence sounds like the entire struct receive the same value, maybe reword to clarify that the fields receive the same value.


With the `..` syntax, values for missing fields can be taken from another struct.
This is convenient for using `Default` to override just a few values on initialisation:

Choose a reason for hiding this comment

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

"This is convenient for using Default" -> "For example using Default to override..."

}
```

However, this still requires an already initialised struct after the `..`.

Choose a reason for hiding this comment

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

Another drawback is that if the struct has a private field ..Default::default() isn't valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There you go. Another thing I didn't know about .. 👍

Copy link
Owner

Choose a reason for hiding this comment

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

And requires the type of every field to implement Default, which is not really common.

However, this still requires an already initialised struct after the `..`.

To work around these shortcomings, users can create constructor functions or more elaborate builders.
The problem with a constructor is that you need one for each combination of fields a caller can supply.

Choose a reason for hiding this comment

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

Or supply one constructor with mandatory values and expect the user to override public default values after construction.

let foo = Foo::new(42); // sets `foo.c`
foo.b = true; // override b


Field defaults use the same syntax as `const`s.
So the type must be supplied, and the value must be a compile-time expression, or a call to `Default::default()`.
Default field values are expected to be cheap to produce and have basic support for collection types like `Vec`.

Choose a reason for hiding this comment

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

Inference is an alternative here, you have to name the type when using Foo::default() anyways.

Copy link
Contributor Author

@KodrAus KodrAus Oct 16, 2016

Choose a reason for hiding this comment

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

I've opted for explicit typing because it's easy to follow when the types all appear in the same place. It's not a feature I'm particularly attached to though, so if you think it's not really valuable then I'll rejig it.

Copy link
Owner

Choose a reason for hiding this comment

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

I think starting with const syntax is fine. You should be clear that it is the same syntax and set of initialising expressions as consts. I would put the default option in a separate paragraph. with its justification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this is a bit different now that default is in alternatives. I've added a point that the syntax and functionality are modeled after const. Maybe it could be clearer though, since the syntax is in the grammar section, and the functionality is in interpretation.

@KodrAus
Copy link
Contributor Author

KodrAus commented Oct 16, 2016

@leodasvacas I've addressed your feedback except for the point about inferring types for default() fields. I thought we could discuss that.

@@ -6,31 +6,268 @@
# Summary
[summary]: #summary

One para explanation of the feature.
Allow structure definitions to supply default values for individual fields, and then allow those fields to be omitted from struct initialisation:
Copy link
Owner

Choose a reason for hiding this comment

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

s/structure/struct

Copy link
Owner

Choose a reason for hiding this comment

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

and elsewhere


# Motivation
[motivation]: #motivation

Why are we doing this? What use cases does it support? What is the expected outcome?
Field defaults address two issues with structure initialisation in Rust: privacy and boilerplate. This is achieved by letting callers omit fields from initialisation when a default is specified for that field. This syntax also allows allows fields to be added to structures in a backwards compatible way, by providing defaults for new fields.
Copy link
Owner

Choose a reason for hiding this comment

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

I would take out this description of the proposal and keep the motivation for a description of the problems

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This description was meant to be the summary for the motivation. I'll rework it a bit.

Why are we doing this? What use cases does it support? What is the expected outcome?
Field defaults address two issues with structure initialisation in Rust: privacy and boilerplate. This is achieved by letting callers omit fields from initialisation when a default is specified for that field. This syntax also allows allows fields to be added to structures in a backwards compatible way, by providing defaults for new fields.

Rust allows you to create an instance of a structure using a literal syntax. This requires all fields in the structure be assigned a value, and can't be used with private fields. It can also be inconvenient for large structures whose fields usually receive the same values.
Copy link
Owner

Choose a reason for hiding this comment

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

You should explain why private fields cause problems


Rust allows you to create an instance of a structure using a literal syntax. This requires all fields in the structure be assigned a value, and can't be used with private fields. It can also be inconvenient for large structures whose fields usually receive the same values.

With the `..` syntax, values for missing fields can be taken from another structure. However, this still requires an already initialised struct after the `..`. It also isn't valid if the struct has innaccessible private fields.
Copy link
Owner

Choose a reason for hiding this comment

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

I would name the syntax as functional record update here (even though it is a terrible name)

Copy link
Owner

Choose a reason for hiding this comment

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

Are you sure about the private fields thing? Maybe this statement needs some qualification (or at least explanation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be the case, according to this gist.

If there are any private fields then the functional record update borks. But that seems to go against what @leodasvacas is saying about using private fields to force callers to use it.

Choose a reason for hiding this comment

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

@KodrAus The private field trick would be a hypothetical trick if we used explicit syntax for default values. Currently it dosent work because FRU and private fields dont go along.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah gotcha :)


With the `..` syntax, values for missing fields can be taken from another structure. However, this still requires an already initialised struct after the `..`. It also isn't valid if the struct has innaccessible private fields.

To work around these shortcomings, users can create constructor functions or more elaborate builders. The problem with a constructor is that you need one for each combination of fields a caller can supply. Builders enable more advanced initialisation, but need additional boilerplate.
Copy link
Owner

Choose a reason for hiding this comment

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

I would give some examples here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a really trivial example of a constructor function and builder method


// `b` is `true`, even though `bool::default()` is `false`
let foo = Foo::default();
```
Copy link
Owner

Choose a reason for hiding this comment

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

You should note that this means derive(Default) can be used more widely because only fields with default values need to implement Default


# Drawbacks
[drawbacks]: #drawbacks

Why should we *not* do this?
Field defaults are limited to `const` expressions. This means there are values that can't be used as defaults, such any value that requires allocation, including common collections like `Vec`.
Copy link
Owner

Choose a reason for hiding this comment

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

typo: "such any"

Copy link
Owner

Choose a reason for hiding this comment

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

and I think you meant Vec::new() not just Vec


# Unresolved questions
[unresolved]: #unresolved-questions
Allowing arbitrary expressions as field defaults would make this feature more powerful. However, limiting field defaults to `const`s maintains the expectation that struct literals are cheap and deterministic. The same isn't true when arbitrary expressions that could reasonably panic or block on io are allowed.
Copy link
Owner

Choose a reason for hiding this comment

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

s/consts/constant expressions

[unresolved]: #unresolved-questions
Allowing arbitrary expressions as field defaults would make this feature more powerful. However, limiting field defaults to `const`s maintains the expectation that struct literals are cheap and deterministic. The same isn't true when arbitrary expressions that could reasonably panic or block on io are allowed.

For complex initialisation logic, builders are the preferred option because they don't need to carry this same expectation.
Copy link
Owner

Choose a reason for hiding this comment

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

s/need to//

```rust
struct Foo {
a: &'static str,
b = 42,
Copy link
Owner

Choose a reason for hiding this comment

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

Integers are not the best example for this because of the more complex type inference, maybe better to use a string or a struct.

@nrc
Copy link
Owner

nrc commented Oct 26, 2016

@KodrAus looking really good, I have another round of comments, but I think they are all polishing now. Nearly done :-)

@leoyvens
Copy link

The proposal in rust-lang/rfcs#448 seems to overlap with this, at least syntactically. ping @comex

@KodrAus
Copy link
Contributor Author

KodrAus commented Nov 2, 2016

@nrc @leodasvacas I've worked through this round of feedback. I'm not totally convinced on the constructor / builder example I have though. It adds a bit of context but is pretty trivial.

Any thoughts?

fn b(mut self, b: bool) -> Self {
self.b = b;
self
}
Copy link
Owner

Choose a reason for hiding this comment

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

The usual builder pattern has a FooBuilder struct and methods on that, rather than on Foo itself. You could keep the ctor example, remove the builder example and instead refer to a builder in the std lib.

}

impl Foo {
/// Constructor function.
Copy link
Owner

Choose a reason for hiding this comment

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

nit: I'd prefer regular comments to doc comments

@KodrAus
Copy link
Contributor Author

KodrAus commented Nov 7, 2016

@nrc Sorted those little tweaks.

@leoyvens
Copy link

This is the RFC that made FRU work only for public fields. Seems like a relevant read.

@KodrAus
Copy link
Contributor Author

KodrAus commented Nov 10, 2016

Thanks @leodasvacas. So this RFC contradicts this statement from the one you posted:

If any fields are hidden by privacy, then all forms of struct literal syntax are unavailable, including FRU.)

It does solve the mentioned drawback of backwards compatibility though. And I think it does so in a good way. You don't need to derive Default and you don't have to default fields that shouldn't have a default value. So the one declaring the struct has full control over how it can be initialised.

@nrc
Copy link
Owner

nrc commented Nov 11, 2016

I'd note that the mechanism we're proposing here is fundamentally different in that the opt-in is in the struct def, not in the user of the struct. Put another way, if you want to ensure that a field is really, really private, then just don't give it a default.

@leoyvens
Copy link

@KodrAus You are referring to the suggestion in the RFC of having a pub member that contains the private members? That is a nice workaround but most library authors don't seem to keep it in mind.


The problem with a constructor is that you need one for each combination of fields a caller can supply. To work around this, you can use builders, like [`process::Command`](https://doc.rust-lang.org/stable/std/process/struct.Command.html) in the standard library. Builders enable more advanced initialisation, but need additional boilerplate.

This RFC proposes a solution to improve `struct` literal ergonomics so they can be used for `struct`s with private fields and to reduce initialisation boilerplate for simple scenarios. This is achieved by letting callers omit fields from initialisation when a default is specified for that field. This syntax also allows allows fields to be added to `struct`s in a backwards compatible way, by providing defaults for new fields.
Copy link
Owner

Choose a reason for hiding this comment

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

nit: needs a comma after "ergonomics" and "private fields".

Typo: "allows allows"

let foo = Foo {
a: "Hello",
c: 42,
};
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be interesting for this example to give defaults for every field and then show a few different struct lits supplying different combinations of fields.


```
struct_field ::= vis ? ident ':' type_path |
vis ? ident ':' type_path '=' expr
Copy link
Owner

Choose a reason for hiding this comment

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

nit: remove space between vis and ?

vis ? ident ':' type_path '=' expr
```

The syntax is modeled after constant expressions. Field defaults are only valid for classic C `struct`s, so tuple `struct`s aren't supported.
Copy link
Owner

Choose a reason for hiding this comment

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

I would change the second sentence to "Field defaults for tuple structs are not supported."


## Interpretation

The value of a field default must be a compile-time expression. So any expression that's valid as `const` can be used as a field default. This ensures values that aren't specified by the caller are deterministic and cheap to produce. A type doesn't need to derive `Default` to be valid as a field default.
Copy link
Owner

Choose a reason for hiding this comment

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

"valid as a"

a: "Hello",
b: true,
c: 42,
};
Copy link
Owner

Choose a reason for hiding this comment

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

It's probably worth noting here that it is not quite a shorthand, because if b were private, then this wouldn't work.

@nrc
Copy link
Owner

nrc commented Nov 16, 2016

Looking good!

@KodrAus
Copy link
Contributor Author

KodrAus commented Nov 27, 2016

Sorry for a big delay once again! I decided to do a workshop this month so had to learn tokio :)

@nrc nrc merged commit e9a3b01 into nrc:master Dec 1, 2016
@nrc
Copy link
Owner

nrc commented Dec 1, 2016

Merged! I think this is looking good and is ready to submit as an RFC PR.

@KodrAus are you happy to submit it? Any questions about the process there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants