-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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] Default field values #3681
base: master
Are you sure you want to change the base?
Conversation
866b2c7
to
43fea8c
Compare
43fea8c
to
16f8bfd
Compare
16f8bfd
to
d81136d
Compare
text/0000-default-field-values.md
Outdated
} | ||
``` | ||
|
||
These can then be used in the following way with the Functional Record Update syntax, with no value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should call this FRU, since there's no base object.
My preference would be to say this RFC doesn't actually touch FRU at all, just expands struct field expressions (and the derives and such)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's useful if the RFC draws parallel to the FRU syntax whenever possible, to avoid needing to re-explain things like how struct initializer expressions are type checked (especially bc going into the detail of how it actually works means this RFC is gonna get it wrong). But it's worthwhile calling this something different than FRU.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree in the reference section, C-E, but not in the summary.
text/0000-default-field-values.md
Outdated
Because this RFC gives a way to have default field values, you can now simply | ||
invert the pattern expression and initialize a `Config` like so (15): | ||
|
||
```rust | ||
let config = Config { width, height, .. }; | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this RFC actually makes this possible.
It would need an additional feature like allowing
#[non_exhaustive(but_future_fields_will_have_defaults)]
pub struct Config {
pub width: u16,
pub height: u16,
}
because #[non_exhaustive]
is a license to add private fields without defaults today.
And there are types today like
#[non_exhaustive]
pub struct Foo;
that we really don't want to make creatable by Foo { .. }
in other crates, since the reason they have that attribute today is to prevent such construction.
I think that's fine, though: this RFC is useful without such an addition, and thus it can be left as future work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to see if I get you right, you saying that this RFC should disallow #[non_exhaustive]
and default fields on the same struct, and leave any interaction as an open question? I'm down with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that defaults would still be allowed, but the type would remain unconstructable from outside the defining crate. That way you can still use things like a default on PhantomData
on internal, non_exhaustive
types.
Given that
#[non_exhaustive]
pub struct Foo;
is commonly used to write a type that cannot be constructed outside the crate (replacing the old pub struct Foo(());
way of writing this), I don't think we can ever say that any non_exhaustive
types are constructible outside the defining crate without the defining crate opting-in to that somehow. Telling everyone they have to change back to having a private field is a non-starter, in my opinion.
But I'd also be fine with saying that you just can't mix them yet, and make decisions about it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I think it would be useful to preserve the property that Struct { field: value, .. }
as an expression (the proposed syntax) is equivalent to Struct { field: value, ..Default::default() }
, and as such, these examples would only work if these structs derived or manually implemented Default
.
That should cover the API concerns, although it would make Default
become a lang item, which I am personally fine with but I am not everyone, so, that would be a downside to this approach.
If it doesn't interact with Default
at all, I agree that it shouldn't allow this, since it does break APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think requiring Default
is unnecessarily limiting, since it would prevent using the nice new syntax with structs where some fields have defaults and others intentionally do not, e.g. if syn::Index
(a tuple struct field name) used the new syntax it could be:
pub struct Index {
pub index: u32,
pub span: Span = Span::call_site(),
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get pretty far with linting here.
A lint can check if the body of your Default::default
impl is const fn
-compatible, and suggest changing it to have inline defaults if so.
One thing I like about this feature is that it means that the vast majority of (braced-struct) Default
s move to being derive
able -- even Vec
's Default
could, for example! -- and thus anything that's not would start to really stand out, and starting being a candidate for IDE hints and such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rereading this I believe that the part of the feature that has a bad interaction is allowing the construction of types that have private default field values. Disallowing that in general, or when the type is also annotated with non_exhaustive
would eliminate the issue for every cross-crate case, right? The only potential issue would be intra-crate, and that's already out of scope of non_exhaustive
, IIUC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On reading the RFC, I was honestly a little confused on why interaction with non_exhaustive would be a problem.
My understanding is that non_exhaustive means you can't construct the struct outside of the crate. The ability to have defaults shouldn't have any impact on that. And prohibiting use of defaults on a non_exhaustive struct seems unnecessarily restrictive, since that could still be useful within the crate.
Now, there may be value in a non_exhaustive_but_future_fields_have_defaults functionality, but I think that should be a separate attribute, or add an argument to the existing non_exhaustive attribute, not usurp and change the meaning of the current non_exhaustive attribute.
In particular, the syntax `Foo { .. }` mirrors the identical and already | ||
existing pattern syntax. This makes the addition of `Foo { .. }` at worst | ||
low-cost and potentially cost-free. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One drawback that comes to mind is that it'll mean that a pattern Foo { .. }
can match more things than just the expression Foo { .. }
, because the pattern matches any value of the unmentioned fields, but the expression sets them to a particular value.
That means that, with the unstable inline_const_pat
, the arm const { Foo { .. } } =>
matches less than the arm Foo { .. } =>
(assuming a type like struct Foo { a: i32 = 1 }
).
I think I'm probably fine with that, but figured I'd mention it regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is an excellent call-out of a non-obvious interaction I hadn't accounted for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it would be worth adding syntax alternatives like Foo { ... }
or Foo { ..some_keyword }
to the "Rationale and alternatives" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that is already the case?
x if x == Foo { a, b }
matches less than Foo { a, b }
assuming a
and b
variables are in scope.
Although f you define a
and b
as constants then Foo { a, b }
will match the exact value, which is.... interesting.
I don't think this is a problem, it is expected that patterns behave differently from expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In particular, the syntax
Foo { .. }
mirrors the identical and already
existing pattern syntax.
Indeed, and that's a downside -- it mirrors the syntax but has different semantics. The text here makes it sound like that's a good thing; I think that should be reworded (and not just called out under "Downsides").
} | ||
``` | ||
|
||
```rust |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth also comparing against derive macros like typed-builder, which at least in my experience are a very common solution to this problem.
facilitates specification of field defaults; or it can directly use the default | ||
values provided in the type definition. | ||
|
||
### `structopt` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe replace with clap? since clap v3, basically all structopt stuff was integrated into clap and structopt is de-facto deprecated. https://docs.rs/structopt/0.3.26/structopt/index.html#maintenance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is a holdover from the original RFC written a number of years ago when structopt
was in common usage.
``` | ||
|
||
# Rationale and alternatives | ||
[rationale-and-alternatives]: #rationale-and-alternatives |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see a section on differences from FRU (especially the interaction with privacy and #[non_exhaustive]
) including why it is the way it is and the reason for diverging from it in this RFC. And possibly a future possibility about how they can be made more consistent in the future.
I'm generally a big fan of this RFC, but undoing past mistakes by adding more features without fixing the features we have leads to an uneven and complex language surface. So let's try to make sure we can do both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it is reasonable to change this RFC to explicitly follow RFC-0736, and disallow the construction of types with private fields, if that will make this RFC less controversial. Particularly when considering the interaction with #[derive(Default)]
, the feature is useful on its own without expanding the field privacy rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not saying it's wrong, but I want to understand and consider the rationale for RFC-0736. I dislike it, but think there was probably a good reason for it, even if I can't remember what it was at the moment. That reason may or may not apply here.
edit: I would be happy to help with this btw, it's just too late for me to do right now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with FRU is all about it not having the desugaring that lots of people expect.
There would be no problem with FRU and privacy if it only allowed you to modify public fields on a type, but the problem is that what it does is secretly duplicate private fields on a type, which is blatantly wrong for literally-but-not-semantically Copy
types like the pointer in a Vec
. https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/struct.20update.20syntax.20with.20non.20exhaustive.20fields/near/438944351
I think the way forward here is to fix FRU (sketch in https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/struct.20update.20syntax.20with.20non.20exhaustive.20fields/near/438946306) so that it can obviously work fine with non_exhaustive
(and private fields), rather than try to work in an exception here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sketch uses a completely new syntax, so we wouldn't be fixing FRU so much as deprecating it and replacing it with something else.
It seems better to design this one to work the way we want from the beginning? Especially since people will have to opt in and might rely on restrictions we implement today (even if we document that those restrictions will be lifted in the future.. people don't usually read the docs on language features so much as try them out in the playground).
what happens in this case: struct Weird {
a: u8 = 1,
b: u8,
}
impl Default for Weird {
fn default() -> Self {
Self {
a: 2,
b: 3,
}
}
} what does |
|
@lolbinarycat under the proposal that you're replying to, it would be using the I do not think that allowing |
Something I haven't seen addressed: does the following code work? #[derive(Default)]
struct Foobar<T> {
foo: [T; 10] = Default::default(),
bar: Option<T> = Default::default(),
} Because if so, that paragraph from the RFC doesn't really hold:
|
Perhaps we're trying to fill out a table that looks something like this:
In each square are the answer to questions such as:
Footnotes
|
@PoignardAzur because of the way the expansion would look, you would get the following error on the
forcing you to write
This is predicated, of course, on |
Actually, thinking about it, you would probably get an error even if Default isn't derived, right? Otherwise Which means every default value must be valid with the type's default bounds, which means a field with a default value cannot possibly introduce new Default bounds, which means ignoring it when generating |
Erm, yes, obviously we should do that @lolbinarycat :). I like it.
I tend to agree that we should settle on Option 2 and probably Option 3, and that these can be stabilized one at a time and after the rest of the RFC, if desired. Do you have reservations about this @estebank? I am mindful of scope creep but my feeling is that the interaction is quite important and within the scope of the RFC. The remaining question is what to call the opt-in, which is less important than settling on the semantics we're going for. If the lang team meets about this RFC we can accelerate the bikeshed painting, or we can leave it as an open question in the RFC. |
On I think it's fine if they're not forced to be the same so long as the easy ways to do things are all consistent. For example, so long as it's easier than getting So realistically, what does it need to get things inconsistent? It needs all of
I don't think that banning (2) is acceptable (cc @tmandry because of their post above), because we don't have perfect derive. I really don't want to have the outcome here be that you're not allowed to write something like the following: struct Foo<T> { a: u32 = 4, b: Option<T> = None }
impl<T> Default for Foo<T> { fn default() -> Self { Self { .. } } because that seems entirely reasonable and useful. So what if we target (3) instead? A lint for "hey, in the I don't think we need to ban (With the lint described, this is already substantially easier to get right than |
Maybe that's what
It specifically suggests the expansion of... #[derive(Default)]
struct Foo<T> {
bar: Option<T> = None,
} ...to: struct Foo<T> {
bar: Option<T>,
}
impl<T> Default for Foo<T> {
fn default() -> Foo<T> {
Foo {
bar: None,
}
}
} (That would of course have some SemVer-relevant effects worth considering.) |
It's really non-obvious to me whether that's even possible. At the very least it's got to be quite nuanced in cases where there are multiple type parameters with potential trait bounds between them, such that you can still get to a type parameter without ever actually mentioning it in that type or that initialization expression. And it's hard to say whether a |
The solution for that could be to add opt-out annotations to other traits as well. You could add a |
This is probably something you should built team consensus on, because it impacts multiple RFCs. For example, we were talking about having a macro generate your It would be nice to have a canonical "If your type has these type parameters and these fields, it should generate these bounds" guideline. |
@PoignardAzur The two things that work well are the "bound all the type parameters" for semver that all the built-in ones use, and the "bound all the field types" approach of so-called perfect derive. Trying to use tokens to figure out trait usage will always be sketchy at best. |
I was assuming we could make If we can't do that and make manual derives an error with ~no cost, I agree we should use a lint and tailor it as much as possible to make sure no one ever creates divergence accidentally. (I like Scott's suggestion to lint on already-defaulted fields set in the Default impl.) With that kind of checking it won't be much different from the fact that you can do side-effecting things in a |
What about the case all fields have a default value, would it be reasonable to allow deriving Default in that case, even if we don't allow it if only some of the fields have defaults. |
@tmccombs do you mean allowing |
I'd like to fight back a bit on the "inferring bounds from tokens is intractable" claim, because I do think it's worth considering. You mentioned associated types in another post. Here's an example of a "tricky" struct using them: #[derive(Default)]
struct MyStruct<T, U>
where
T: SomeTrait<U>,
{
t: T,
u: T::Assoc,
}
trait SomeTrait<T> {
type Assoc;
}
impl<T, U> SomeTrait<U> for T {
type Assoc = U;
} On the one hand, yes, the impl<T: ::core::default::Default, U: ::core::default::Default> ::core::default::Default
for MyStruct<T, U>
where
T: SomeTrait<U>,
T::Assoc: ::core::default::Default,
{
#[inline]
fn default() -> MyStruct<T, U> {
MyStruct {
t: ::core::default::Default::default(),
u: ::core::default::Default::default(),
}
}
} The macro already needs to bind on So the question isn't "Should derive macros generate one bound per argument or should they consider tokens in fields", because they already do the latter. Rather, it's "How complex should the token parsing be?". Now, granted, existing derive macros do this parsing on top of generating one bound per argument, and I'm suggesting replacing the one-bound-per-argument rule entirely. That does add potential for brittleness. But I'd argue the difference isn't quite as cut and dry as you think. It's worth examining the possibility further. Note that this is also relevant to #3683. If you write: #[derive(Default)]
enum MyEnum<T, U> {
#[default]
Foo {
field: Option<T>,
}
Bar(U),
} You'd like your derive to assume |
@SOF3 no I mean deriving Default would be allowed if: fields.all(|field| field.default_value.is_some()) || (fields.all(|field| field.type.implements(Default)) && fields.all(|field| field.default_value.is_none())) So if all fields have defaults, the Default impl would use all the default values. If no fields have default values, the implementation of |
That's just doing both the bound-the-parameter and the bound-the-field cases I mentioned. Bound the whole field certainly works, though I'm surprised it bothers doing that. I guess it wanted more "well it just doesn't exist" cases and fewer "it fails to compile" cases. (I find that odd, because a field of type
Wouldn't you want |
I agree we can probably come up with a rule that works based on parsing the struct. The real concern is that it can introduce semver hazards to be too clever about when a derive is implemented. The expansion you posted is interesting; it demonstrates that we already do this to some extent. That lends some credibility to the idea that it will "probably be fine" to do this in practice, but it still makes me nervous. I would rather not increasingly require library maintainers to rely on tooling to catch subtle instances of semver breakage like this. As a sketch, I think if we had some combination of
for adding bounds, and a |
My personal problem with assigning default values at struct-definition site is that it conflates data with behavior. There is no reason why a struct couldn't have multiple sets of defaults. In my opinion, when exploring alternatives to this RFC, we should strongly consider ones that detach the concept of a "set of defaults" from the struct definition itself. In my opinion the place for defining a set of defaults should be in an For those who would bring up Functions already provide a decent way to define a set of defaults, and they can be used with the struct update syntax ( One of the main problems is not being able to "delegate" the defaults for fields you don't want to explicitly define. impl MyStruct {
const fn my_defaults() -> Self {
Self {
field1: Default::default(),
field2: Default::default(),
field3: Default::default(),
field4: Default::default(),
field5: Default::default(),
field6: Default::default(),
field7: Default::default(),
field8: Default::default(),
// I only care about explicitly defining this
field9: 42,
field10: Default::default(),
field11: Default::default(),
field12: Default::default(),
// And this
field13: "Explicitly set",
field14: Default::default(),
field15: Default::default(),
field16: Default::default(),
field17: Default::default(),
field18: Default::default(),
field19: Default::default(),
}
}
} If there was a way to delegate fields you're not interested in explicitly setting to a trait method, that would solve a large chunk of the motivation of this RFC. impl MyStruct {
const fn my_defaults() -> Self {
Self {
// I only care about explicitly defining this
field9: 42,
// And this
field13: "Explicitly set",
// Pseudo syntax for delegating defaults
for .. use Default::default
}
}
} I understand that part of the motivation of this RFC is usability in const contexts, which would make my proposal above depend on const trait impls (Like being able to constly impl There are other possible alternatives for defining default-sets for a type, without having to tie the defaults to the definition of said type. I strongly suggest exploring such alternatives. |
@crumblingstatue but your proposal only addresses one of the reasons listed in the motivation section of this RFC. It doesn't solve the problem of some fields being required and others being optional (and thus the need for builder patterns). It doesn't provide information for third party derive macros. |
Recapping a relevant discussion on discord, Also, it seems the RFC fails to address what is so bad about builders. |
The RFC does specify the motivation: Builders require additional boilerplate. |
I think in general, defaults mean "the single set of values people would expect this type to have if they didn't think about it too much". It's an easy Schelling point. If your type has multiple sets of possible defaults, it probably shouldn't have default fields or implement the Default trait. |
Well, that is the thing. That there is no language concept for the default field values of a struct. |
If built-in syntax is confusing, then perhaps it could be a (language built-in) macro instead. impl MyStruct {
const fn my_defaults() -> Self {
Self {
// I only care about explicitly defining this
field9: 42,
// And this
field13: "Explicitly set",
init_rest_with!(Default::default()),
}
}
} |
Allow
struct
definitions to provide default values for individual fields andthereby allowing those to be omitted from initializers. When deriving
Default
,the provided values will then be used. For example:
Rendered