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

Type-changing struct update syntax #2528

Merged

Conversation

jturner314
Copy link
Contributor

@jturner314 jturner314 commented Aug 24, 2018

Extend struct update syntax (a.k.a. functional record update (FRU)) to support instances of the same struct that have different types due to generic type or lifetime parameters.

Rendered.

See also #1975 and rust-lang/rust#47741.

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the RFC. label Aug 24, 2018
@scottmcm
Copy link
Member

Ah, it needs to be the same type constructor? That makes me far less scared 🙂

The other thing this makes me ponder is that this doubles-down on the "move the other fields" interpretation. I would like to soundly support FRU on non_exhaustive structs and those with private fields at some point, where the "replace the fields on the updatee" interpretation works better.

@jturner314
Copy link
Contributor Author

I'd like Rust to support private fields in struct update syntax, too, but I'd prefer to handle that feature separately from this RFC.

This RFC is fairly limited in scope, and there aren't many alternatives if we want to support changing generic type parameters. (The primary question is the set of type inference rules.) If we are concerned about this RFC causing problems for private field support, we could delay discussion of this RFC until we discuss how to add support for private fields, or we could work on a combined RFC for both features. However, it seems like the changes in this RFC won't significantly1 affect future discussion about supporting private fields. If we had to choose between supporting private fields and supporting type changes, I'd choose to support type changes anyway:

If we don't add support for private fields to struct update syntax, then updating a struct with private fields can still be written as

let mut base = base_expr;
base.field1 = expr1;
base.field2 = expr2;
base

In contrast, there is no compact equivalent for type-changing updates without this RFC, because assignments to the fields can't change the type of base. You have to explicitly list all the fields:

let base = base_expr;
Struct {
    field1: expr1,
    field2: expr2,
    common_field1: base.common_expr1,
    common_field2: base.common_expr2,
    common_field3: base.common_expr3,
    common_field4: base.common_expr4,
}

Regarding the "always consume the base instance" approach for supporting private fields (which is a generalization of the "replace the fields on the updatee" interpretation), specifically: This approach does provide a nice, simple way to add support for private fields to current Rust, although it's a breaking change in the special case that all of the moved fields implement Copy. This approach does make enforcing some constraints more difficult when type-changing updates are allowed1 (this RFC), but it could work in combination with another means to restrict use of struct update syntax. (For example, provide an opt-in StructUpdateSyntax trait that is easy to #[derive()] and is automatically implemented by the compiler for structs where we know it can't violate any constraints (e.g. structs with only public fields and structs where the private fields control the generic type parameters).)

1 There is one way this RFC would affect support for private fields. In current Rust (and with this RFC), structs with at least one private field always have complete control over how they are constructed, and in particular, what types can be used for generic type parameters in public fields. If struct update syntax supported type changes as well as private fields without providing a means to opt-in/out, this would no longer be as straightforward:

  1. In current Rust, given struct Foo<T>(pub T, i32), Foo can directly control which types of Foo<T> can be constructed by providing constructor methods for only some types of T. If type-changing struct update syntax were allowed with private fields, the struct would have to add T to a private field (e.g. struct Foo<T>(pub T, i32, PhantomData<T>)) to enforce this behavior.

  2. Structs would lose the ability to do things like synchronizing the value of a private field with the type of a public field without constraining the type with a private field. For example, in current Rust (or with this RFC) if you do this:

    trait Trait {
        fn some_expensive_operation(arg: i32) -> i32;
    }
    
    struct Struct<T: Trait> {
        pub t: T,
        arg: i32,
        cache: i32,
    }
    
    impl<T: Trait> Struct<T> {
        fn new(t: T, arg: i32) -> Struct<T> {
            Struct {
                t,
                arg,
                cache: T::some_expensive_operation(arg),
            }
        }
    }

    the value of self.cache is always guaranteed the output of T::some_expensive_operation(self.arg), where T is the current T of the struct. If type-changing struct update syntax were allowed with private fields, then this constraint would no longer be enforced. The struct would have to constrain the T parameter with a private field, e.g. by adding a private field t_type: PhantomData<T>.

@petrochenkov
Copy link
Contributor

@scottmcm

The other thing this makes me ponder is that this doubles-down on the "move the other fields" interpretation. I would like to soundly support FRU on non_exhaustive structs and those with private fields at some point, where the "replace the fields on the updatee" interpretation works better.

The directions are opposite, but both seems useful.
We should probably just introduce some separate new syntax for the "move the whole ..expr" interpretation, especially that changing the old syntax would be pretty significant breakage.

@joshtriplett
Copy link
Member

This seems reasonable to me. 👍

@shepmaster
Copy link
Member

shepmaster commented Aug 30, 2018

separate new syntax for the move the whole ..expr"

Because bikesheds are fun...

Foo { a: 1, b: false, ..move old }

... which I see now was listed as an alternative on the linked proto RFC

Or, relying on some current behavior of ye olde identity function (a.k.a. braces)...

Foo { a: 1, b: false, ..{ old } }

@eddyb
Copy link
Member

eddyb commented Sep 7, 2018

some current behavior

It may appear weird that (expr) and {expr} are different, but only the former preserves the distinction between place and value expressions, whereas the latter wants only a value, which means that if you have a place (like a variable), it moves the value out of that.

I don't think this is something we can or ever want to change, unless we ever end up with something like C++'s T&, where you have a (reference) value behaving like a place (the deref of that reference).

@Centril Centril added A-update-syntax Struct update syntax related proposals & ideas A-typesystem Type system related proposals & ideas A-expressions Term language related proposals & ideas labels Nov 22, 2018
@pnkfelix
Copy link
Member

I like the intent of this proposal, and I really appreciate the amount of thought that has gone into the repercussions of this change, even in its relatively constrained form. (The title of this RFC makes one think it is proposing something far more general than what is actually here.)

I will admit a slight discomfort with the proposed type-inference rules. Specifically the text:

If the type of updated cannot be a subtype of the type of base without
violating any constraints, then the explicitly listed fields (field1 and
field2 in the example) are inferred independently between updated and
base.

This has an aura of back-tracking to it that gives me pause.

But I'm probably just over-reacting. It certainly seems like this could be worth prototyping.

@jplatte
Copy link
Contributor

jplatte commented Jun 18, 2020

What has to happen for this to move forward?

@burdges
Copy link

burdges commented Jun 18, 2020

As an aside, you're always doing a change X<Y> to X<Z> here, so you'll wind up doing aliases often, and sometimes wanting doc comments on them: https://users.rust-lang.org/t/type-trait-alias-docs/43901

@joshtriplett
Copy link
Member

We talked about this in the lang team meeting today. We're generally in favor of this, as long as it's limited to another struct of the same type with different generic parameters (e.g. Foo<T> to Foo<U>, not Bar to Foo).

The lang team needs to review some of the discussion of type inference details, but pending that, we'd be in favor of merging.

@joshtriplett
Copy link
Member

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 5, 2020

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

Concerns:

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 Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Aug 5, 2020
@nikomatsakis
Copy link
Contributor

@rfcbot concern review-inference-strategy

I'd like to review the inference strategy, but I'm in favor of the overall goal of the RFC.

@scottmcm
Copy link
Member

One thing I'm pondering that I'm not sure yet whether it raises to the level of a concern:

This is very much doubling down on Foo { a: 4, ..other } being Foo { a: 4, b: other.b, c: other.c }, but people regularly expect that it would instead be { let mut temp = other; temp.a = 4; temp }†.

We've talked many times about wanting to find a way to get FRU (or something like it) working with #[non_exhaustive] structs, or those that have private fields. (Particularly as a way of doing light builder-style APIs.) It seems like the latter (currently-incorrect) interpretation of FRU would be far more natural to fit into such a framework, so it's not obvious that we should do something that pushes even harder on the former interpretation. (Even though this RFC seems like a very natural extension of it.)

† or perhaps something even more nuanced to preserve the unused values, such as

{ let mut temp = other; other.a = mem::replace(&mut temp.a, 4); temp }

@jplatte
Copy link
Contributor

jplatte commented Aug 17, 2020

I think it makes sense to support both this and struct update syntax on non-exhaustive structs. That would mean that struct update syntax could no longer be just syntax sugar, but I don't see why it has to be.

However, the types of the fields in the updated instance that are not
explicitly listed (i.e. those that are moved with the `..` syntax) must be
subtypes of the corresponding fields in the base instance, and all of the
fields must be visible ([RFC 736]). In other words, the types of fields that
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah so I guess per @scottmcm's point, maybe we should roll back this rule. I guess it's worth deciding that separately.

@nikomatsakis
Copy link
Contributor

OK, so I reviewed the text on type inference. I think that what I was expecting was something like this:

  • To type-check a struct expression Foo { a, b, ..foo }, you first create a type of Foo with all inference variables.
  • You then require that the types of each field not explicitly named (i.e., whose value comes from the ..foo) is a subtype.
  • You then go and type-check the explicitly named fields (a and b) and require that their types as well are subtypes.

I realize now that this is technically a breaking change. We should write out some of the examples. In particular, it can affect the coercion rules. The RFC already cited integer fallback as one possible interaction, but another would be something like:

struct Foo<T> { t: T }

fn main() {
    let foo = Foo::<&[u32]> { t: &[22, 44] };
    let bar = Foo { t: &[44, 66], ..foo };
}

The type of bar is now inferred to Foo<&[u32; 2]>, I believe, instead of &[u32]. (Actually, that code doesn't compile today, which is kind of surprising to me, I would have expected the type of t to coerce to &[u32]... So maybe it's not a breaking change after all.)

@nikomatsakis
Copy link
Contributor

In any case, I think that I would want to update the text regarding inference to something like what I sketched above -- but @jturner314 maybe there is a reason that this logic doesn't work?

@jturner314
Copy link
Contributor Author

Let me first state that I'm not very familiar with the intricacies of how Rust's type inference works; I'm working from my limited knowledge of the type inference from my usage of the language and a little reading of the compiler docs. I think the discussion of type inference in the RFC would ideally be refined with the help of someone more knowledgeable than me.

@nikomatsakis I'm not sure I understand what you mean by the third bullet point; do you mean that the types of the explicitly listed fields in the updated instance would be required to be subtypes of the types of the corresponding fields in the base instance? I'm proposing in the RFC that type inference would try something similar to that first, but if that's not possible, to infer the types of the explicitly named fields independently of the base instance.

It's worth noting that the following statements are not equivalent. I believe statement (1) implies statement (2), but I'm not sure if that's the case. Statement (2) definitely doesn't imply statement (1).

  1. The type of updated is a subtype of the type of base.
  2. The type of each field in updated is a subtype of the type of the corresponding field in base.

Rust currently requires (1) to be true when using struct update syntax. For example, the following raises a "mismatched types" error in current Rust, even though the types of the fields in Struct<T2> are subtypes of the types of the corresponding fields in Struct<T1>.

struct Struct<T: Trait> {
    field1: T::Assoc1,
    field2: T::Assoc2,
}

trait Trait {
    type Assoc1;
    type Assoc2;
}

struct T1;

impl Trait for T1 {
    type Assoc1 = i32;
    type Assoc2 = f64;
}

struct T2;

impl Trait for T2 {
    type Assoc1 = i32;
    type Assoc2 = f64;
}

fn main() {
    let base: Struct<T1> = Struct { field1: 1, field2: 2. };
    let updated: Struct<T2> = Struct { field1: 3, ..base };
}

The proposal in the RFC uses a two-stage procedure to infer the types: the first stage tries to apply statement (1), and the fallback second stage infers the types of the explicitly named fields independently, like this:

  1. To type-check a struct expression for an 'updated' instance Foo { a, b, ..base }, create a type of Foo with all inference variables.
  2. Constrain the type of the updated instance to be a subtype of the type of base. (I believe this is equivalent to the following: constrain each type inference variable in the updated instance to be a subtype of the corresponding type variable in base.)
  3. Add any other available constraints, e.g. from the surrounding code.
  4. Check if the constraints identify the type. If so, we're done. (End of first stage.) Otherwise, continue:
  5. Reset the inference variables/constraints.
  6. For each field not explicitly named: constrain the type of the field in the updated instance to be a subtype of the type of the corresponding field in the value of base.
  7. Repeat step 3.
  8. Check if the constraints identify the type (using the i32/f64 fallback for ambiguous numeric types). If so, we're done. If not, return an error.

A backwards-compatible three-stage procedure starting with statement (2) instead of statement (1) could work like this:

  1. To type-check a struct expression for an 'updated' instance Foo { a, b, ..base }, create a type of Foo with all inference variables.
  2. For each field not explicitly named: constrain the type of the field in the updated instance to be a subtype of the type of the corresponding field in the value of base.
  3. For each explicitly named field: constrain the type of the field in the updated instance to be a subtype of the type of the corresponding field in the value of base.
  4. Add any other available constraints, e.g. from the surrounding code.
  5. Check if the constraints identify the type. If so, we're done.
  6. If the type was ambiguous in step 5:
    a. Reset the inference variables/constraints.
    b. Constrain the type of the updated instance to be a subtype of the type of base. (I believe this is equivalent to the following: constrain each type inference variable in the updated instance to be a subtype of the corresponding type variable in base.)
    c. Repeat step 4.
    d. Check if the constraints identify the type. If so, we're done.
  7. If no type could satisfy all the constraints in step 5 or step 6:
    a. Reset the inference variables/constraints.
    b. Repeat steps 2 and 4.
    b. Check if the constraints identify the type (using the i32/f64 fallback for ambiguous numeric types). If so, we're done. If not, return an error.

Note that step 6 is necessary for backwards compatibility: In my example above, consider replacing let updated: Struct<T2> = Struct { field1: 3, ..base }; with let updated = Struct { field1: 3, ..base };. With constraints only on the fields, the type of updated is ambiguous since it could be Struct<T1> or Struct<T2>. In contrast, current Rust infers updated to have type Struct<T1>.

The procedures listed above are somewhat simplified, since there may be multiple expressions of struct update syntax within a single inference context. The RFC says that in this case, "[The first stage] is evaluated simultaneously for all instances of struct update syntax within the inference context, and conflicts between applications of [the first stage] should result in compilation errors."

Fwiw, my personal favorite option for type inference is the "Combination of always independently inferring types of explicitly listed fields and disabling i32/f64 fallback" option listed in the "Rationale and alternatives" section. I think it's the easiest to understand, and I value the clarity and consistency over the verbosity of additional type annotations. I suspect that it would also be simpler to implement because it requires only a single stage.

The only reason why I didn't choose that as the proposed option in the RFC is the backwards compatibility issue (although last I checked, Rust reserves the right to make breaking changes to type inference). I wrote the RFC to narrowly provide backwards compatibility, while independently inferring types when backwards compatibility is not an issue.

hawkw pushed a commit to tokio-rs/tracing that referenced this pull request Mar 10, 2021
…1289)

This PR aims to remove a lot of initializer boilerplate code by adopting
the`struct update syntax.  If the [RFC 2528]  gets merged and
implemented, we can remove more. 😸 

[RFC 2528]: rust-lang/rfcs#2528
@jturner314
Copy link
Contributor Author

Okay, great. I've invited you as a collaborator on jturner314/rust-rfcs so that you can push to the PR branch (type-changing-struct-update-syntax), since there doesn't appear to be a way to grant access to just the one branch or to change the PR branch to a branch in your repo. (I'll remove you as a collaborator once the PR is merged.) Alternatively, you could create a new PR and link to this one.

hawkw pushed a commit to tokio-rs/tracing that referenced this pull request Mar 12, 2021
…1289)

This backports #1289 from `master`.

This PR aims to remove a lot of initializer boilerplate code by adopting
the`struct update syntax.  If the [RFC 2528]  gets merged and
implemented, we can remove more. 😸

[RFC 2528]: rust-lang/rfcs#2528
hawkw pushed a commit to tokio-rs/tracing that referenced this pull request Mar 12, 2021
…1289)

This backports #1289 from `master`.

This PR aims to remove a lot of initializer boilerplate code by adopting
the`struct update syntax.  If the [RFC 2528]  gets merged and
implemented, we can remove more. 😸

[RFC 2528]: rust-lang/rfcs#2528
@nikomatsakis
Copy link
Contributor

@rfcbot resolve review-inference-strategy

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Jun 1, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 1, 2021

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

@nikomatsakis
Copy link
Contributor

I'm sorry this has taken so ridiculously long

@nikomatsakis
Copy link
Contributor

Happy to work with someone on implementing it, though!

@pickfire
Copy link
Contributor

pickfire commented Jun 7, 2021

Will it be possible to have multiple bases later on? Since previously we can only have one type as base, now we can take a different type as base.

@nikomatsakis
Copy link
Contributor

I'm not sure what multiple base support would look like

@pickfire
Copy link
Contributor

pickfire commented Jun 8, 2021

Like let foo = Foo { a: 1, ..bar, ..baz }, but yeah it's not useful now because it needs to have the exact items but in case we do need it later it might have ambiguity.

@burdges
Copy link

burdges commented Jun 8, 2021

This RFC only discusses struct updates that change type parameters within one fixed but polymorphic type, not unrelated types with similarly named items, so this remains type controlled, not purely syntactic. Anything purely syntactic like you describe comes under structural records, which Rust removed early on, and interacts with many things, but you'll find plenty of discussion around that topic.

@burdges
Copy link

burdges commented Jun 8, 2021

I believe rust could support type changing updates with private fields via explicit visibility modifiers:

pub mod alpha {
    #[derive(Default)]
    pub struct Foo<T, U> {
        pub field1: T,
        pub(struct) field2: U,
    }
}

pub mod beta {
    /// We do not allow `MyFloat: Default` because this tempts missuse in blabla.
    pub struct MyFloat(pub f64);
    ...
    use alpha::Foo;
    /// We construct a meaningful default for this code region using pi.
    let local_default: Foo<MyFloat, i32> = Foo {
        field1: MyFloat(3.14),
        // This default with `()` exists, but `Foo<MyFloat, i32>` has no meaningful default 
        ..Foo::<(),_>::default()
    };
}

In this, we may alter our public field field1 via (type changing) update only because our visibility for field2 explicitly permits updates, or maybe this modifier should live on field1, not sure.

If you want a more fun example, then imagine silencing some fields, so that #[derive(..,Ord)] exists.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Jun 11, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 11, 2021

The final comment period, with a disposition to merge, 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.

The RFC will be merged soon.

@nikomatsakis
Copy link
Contributor

Nominating for someone to actually merge :)

@joshtriplett
Copy link
Member

On it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-expressions Term language related proposals & ideas A-typesystem Type system related proposals & ideas A-update-syntax Struct update syntax related proposals & ideas disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.