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

Implementation Issue for RFC 2528: type-changing struct update syntax #86555 #86618

Closed
nikomatsakis opened this issue Jun 25, 2021 · 12 comments
Closed
Assignees
Labels
A-typesystem Area: The type system E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. F-type-changing-struct-update `#![feature(type_changing_struct_update)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jun 25, 2021

This issue tracks the implementation of rust-lang/rfcs#2528, "type changing struct update syntax" (general tracking issue: #86555).

@nikomatsakis
Copy link
Contributor Author

@rustbot assign @kolharsam

They expressed an interest in this!

@rustbot rustbot self-assigned this Jun 26, 2021
@kolharsam
Copy link

@rustbot claim

@rustbot rustbot assigned kolharsam and unassigned rustbot Jun 26, 2021
@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Jun 26, 2021

Here are some mentoring notes.

PR 0: Create the feature gate

We need a feature gate to control when users are accessing this feature. Instructions for adding a feature gate are here:

https://rustc-dev-guide.rust-lang.org/feature-gates.html#adding-a-feature-gate

Adding a feature gate can and should be its own PR!

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Jun 26, 2021

PR 1: Write up some tests

A good next step is to open a PR creating a directory in src/tests/ui for this feature and adding the various tests. It's ok if they get errors today! Just document the current behavior. But I've always found it's actually much easier to write tests BEFORE you start the code then after!

@nikomatsakis nikomatsakis added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 26, 2021
@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Jun 26, 2021

PR 2: Update the type checker

First off, Foo { ... } expressions are type checked by this function:

https://github.com/rust-lang/rust/blob/50cc3de2ac37969080e5afb3cada6977eba20533/compiler/rustc_typeck/src/check/expr.rs#L1114-L1122

We create this type adt_ty that represents the resulting type of the expression. It will embed inference variables for each of the generics on the struct (e.g., Foo<?0>), unless they were specified (e.g., the user wrote Foo::<u32>):

https://github.com/rust-lang/rust/blob/50cc3de2ac37969080e5afb3cada6977eba20533/compiler/rustc_typeck/src/check/expr.rs#L1124-L1125

The following code checks each of the fields provided by the user, ensuring their names match and so forth. This code doesn't have to change for the most part:

https://github.com/rust-lang/rust/blob/50cc3de2ac37969080e5afb3cada6977eba20533/compiler/rustc_typeck/src/check/expr.rs#L1140-L1148

One thing we might want to do though is to return or otherwise get access to the list of 'remaining fields' (those whose types were not specified by the user):

https://github.com/rust-lang/rust/blob/50cc3de2ac37969080e5afb3cada6977eba20533/compiler/rustc_typeck/src/check/expr.rs#L1211-L1216

After checking the types of the fields, we check if there is a "base expression" (Foo { ..foo }):

https://github.com/rust-lang/rust/blob/50cc3de2ac37969080e5afb3cada6977eba20533/compiler/rustc_typeck/src/check/expr.rs#L1149-L1150

If so, we check that it has the type adt_ty. This is the code that has to change, because it is where you get an error if the types don't line up!

https://github.com/rust-lang/rust/blob/50cc3de2ac37969080e5afb3cada6977eba20533/compiler/rustc_typeck/src/check/expr.rs#L1154-L1155

What we want to be know is that:

  • For each field field that comes from the base expression
  • It has some type source_field_ty in that base expression
  • That field also has some type target_field_ty in the struct we are creating
  • The type source_field_ty from the base expression must be a subtype of target_field_ty

Right now, we don't have any explicit "field-by-field" code, beacuse when we check that the type of the base expression is equal to adt_ty, we check that the types of all the fields in the base expression are compatible. But that is too strong.

However, we do have code that computes the various source_field_ty types:

https://github.com/rust-lang/rust/blob/50cc3de2ac37969080e5afb3cada6977eba20533/compiler/rustc_typeck/src/check/expr.rs#L1157-L1167

FRU here stands for "functional record update", btw. Anyway, these types today are stored into the TypeckResults struct and are not used further by this phase of type-checking. Instead, they are used by the MIR construction code as we desugar this code into MIR. No changes to MIR should be necessary for this work, though.

Because this is not backwards compatible, we have to make our changes conditional on the feature gate being enabled. If it is not enabled, the code should do the same as it does today. But it is IS enabled, we should remove this check and instead enforce that:

  • The base type is an ADT with the same def-id as the one we are creating.
  • Go through each of the fields in the ADT:
    • If the field is NOT in the "remaining fields" set, you can ignore it, as it was given a value by the user.
    • Otherwise, get the type from fru_fields (this is source_field_ty in my explanation above)
    • Compute the type in the target ADT (we do this in the existing code here, so we could store and remember those types)
    • Check that they are a subtype

The final check ("that they are a subtype") is done by calling infcx.at(..).sup(..), you should be able to find some examples of code that does that.

@kolharsam
Copy link

Here are some mentoring notes.

PR 0: Create the feature gate

We need a feature gate to control when users are accessing this feature. Instructions for adding a feature gate are here:

https://rustc-dev-guide.rust-lang.org/feature-gates.html#adding-a-feature-gate

Adding a feature gate can and should be its own PR!

@nikomatsakis Have made a respective PR for the same here: #86646

@kolharsam
Copy link

kolharsam commented Jun 27, 2021

PR 1: Write up some tests

A good next step is to open a PR creating a directory in src/tests/ui for this feature and adding the various tests. It's ok if they get errors today! Just document the current behavior. But I've always found it's actually much easier to write tests BEFORE you start the code then after!

@nikomatsakis I've made a PR with a failing test here: #86660.
Currently, it just document's the error that might be seen while using the suggested updates to the syntax in within the RFC.

Please let me know if there are any other test cases that need to be added because I wasn't really sure about what else could've been added as a test.

@camelid camelid added the A-typesystem Area: The type system label Sep 9, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 22, 2021
…ackh726

add feature flag for `type_changing_struct_update`

This implements the PR0 part of the mentoring notes within rust-lang#86618.

overrides the previous inactive rust-lang#86646 pr.

r? `@nikomatsakis`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 22, 2021
…ackh726

add feature flag for `type_changing_struct_update`

This implements the PR0 part of the mentoring notes within rust-lang#86618.

overrides the previous inactive rust-lang#86646 pr.

r? ``@nikomatsakis``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 23, 2021
…ackh726

add feature flag for `type_changing_struct_update`

This implements the PR0 part of the mentoring notes within rust-lang#86618.

overrides the previous inactive rust-lang#86646 pr.

r? ```@nikomatsakis```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 9, 2021
implement rfc-2528 type_changing-struct-update

This PR implement rfc2528-type_changing-struct-update.
The main change process is as follows:
1. Move the processing part of `base_expr` into `check_expr_struct_fields` to avoid returning `remaining_fields` (a relatively complex hash table)
2. Before performing the type consistency check(`check_expr_has_type_or_error`), if the `type_changing_struct_update` feature is set, enter a different processing flow, otherwise keep the original flow
3. In the case of the same structure definition, check each field in `remaining_fields`. If the field in `base_expr` is not the suptype of the field in `adt_ty`, an error(`FeildMisMatch`) will be reported.

The MIR part does not need to be changed, because only the items contained in `remaining_fields` will be extracted from `base_expr` when MIR is generated. This means that fields with different types in `base_expr` will not be used
Updates rust-lang#86618
cc `@nikomatsakis`
@Systemcluster
Copy link

The current implementation seems to interfere with with type inference:

#![feature(type_changing_struct_update)]

#[derive(Default)]
struct A {
    x: i32
}

fn main() {
    let a = A{
        ..Default::default()
    };
}
error[E0308]: mismatched types
  --> src/main.rs:10:11
   |
10 |         ..Default::default()
   |           ^^^^^^^^^^^^^^^^^^ expected struct `A`, found inferred type
   |
   = note: expected struct `A`
                found type `_`

Playground

@nikomatsakis
Copy link
Contributor Author

I don't believe we've implemented this feature yet =) there are mentoring notes here. I know that someone was looking into it, maybe it was @kolharsam? (If I failed to get back to you, @kolharsam, sorry, are you still interested?)

@kolharsam
Copy link

@nikomatsakis Yes, I was looking into it, and I'm still interested in continuing the work.

@Kobzol
Copy link
Contributor

Kobzol commented Feb 2, 2022

The feature has actually been implemented in #90035, if I'm not mistaken. It already seems to work: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=a84582797c0337611b94e3c5afaeba91.
@Systemcluster could you please create a separate issue with your code snippet? It looks like a bug in the implementation of the feature.

@Mark-Simulacrum
Copy link
Member

We think this is implemented now, so closing this issue. #86555 is still tracking the feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-typesystem Area: The type system E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. F-type-changing-struct-update `#![feature(type_changing_struct_update)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants