implement rfc-2528 type_changing-struct-update #90035

SparrowLii · 2021-10-19T03:12:46Z

This PR implement rfc2528-type_changing-struct-update.
The main change process is as follows:

Move the processing part of base_expr into check_expr_struct_fields to avoid returning remaining_fields (a relatively complex hash table)
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
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 #86618
cc @nikomatsakis

rust-highfive · 2021-10-19T03:12:48Z

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

jackh726 · 2021-10-21T19:22:34Z

#89730 is smaller, just the feature gate - let's land that first, then rebase on that

SparrowLii · 2021-10-22T00:54:17Z

OK, that sounds good

bors · 2021-10-23T09:55:58Z

☔ The latest upstream changes (presumably #90188) made this pull request unmergeable. Please resolve the merge conflicts.

jackh726 · 2021-10-23T13:55:54Z

@SparrowLii can you rebase on master now that #89730 has been merged?

I'm going to try to do a proper review here this weekend.

SparrowLii · 2021-10-23T19:54:12Z

That is encouraging! But I don't have a test environment at hand right now. I will resolve the conflict this Monday, as soon as possible

SparrowLii · 2021-10-25T03:42:05Z

The rebasing and modification of tests are done. I think it is ready to be reviewed.

jackh726

Overall pretty straightforward, a few nits. Can you squash the commits (other than spliting out that one commit)?

jackh726 · 2021-10-25T15:24:37Z

compiler/rustc_feature/src/active.rs

@@ -686,7 +686,7 @@ declare_features! (

    /// Allows creation of instances of a struct by moving fields that have
    /// not changed from prior instances of the same struct (RFC #2528)
-    (incomplete, type_changing_struct_update, "1.58.0", Some(86555), None),
+    (active, type_changing_struct_update, "1.58.0", Some(86555), None),


I think this is still incomplete

I say this because, even though it "works", I'd like a few more tests for better coverage before switching this to "active".

OK, it is incomplete now

src/doc/unstable-book/src/language-features/type-changing-struct-update.md

jackh726 · 2021-10-26T18:35:42Z

src/test/ui/rfcs/rfc-2528-type_changing-struct-update/lifetime-update.rs

+    let m1: Machine<State1> = Machine {
+        state: State1,
+        lt_str: &s,
+                //~^ ERROR `s` does not live long enough [E0597]


So...this is behavior on nightly today. The intention is in fact to allow changing lifetime parameters, right? Probably, the error should get moved to line 20 (creating m1 should be fine, we should infer '_ as lifetime, but trying to create m2 without updating the lifetime on lt is a problem.

I don't think this needs to be changed in this PR, but it does need a FIXME (and probably either an issue filed or a mention in the (implementation?) tracking issue)

That's right. I added FIXME in lifetime-update.rs

jackh726 · 2021-10-26T18:37:04Z

src/test/ui/rfcs/rfc-2528-type_changing-struct-update/lifetime-update.stderr

+LL |     let m2: Machine<'static, State1> = Machine {
+   |             ------------------------ type annotation requires that `s` is borrowed for `'static`


Yeah, at least the information is around; just the span needs to change I think

I added FIXME in lifetime-update.rs

jackh726 · 2021-10-26T18:39:44Z

src/test/ui/rfcs/rfc-2528-type_changing-struct-update/feature-gate.rs

@@ -1,3 +1,5 @@
+// gate-test-type_changing_struct_update


Just noting that the FIXME probably isn't correct. But, perhaps this should give a suggest to add the feature if on nightly.

I correct the location of FIXME. But if we want to add a feature trigger, we can no longer use the original check_expr_has_type_or_error function, because we need to know which field caused a type error. This may require us to merge the two feature branches. I will submit a new commit to try to implement it.

compiler/rustc_typeck/src/check/expr.rs

jackh726 · 2021-10-26T18:52:24Z

compiler/rustc_typeck/src/check/expr.rs

+                                    span: base_expr.span,
+                                });
+                            };
+                            variant


So, why use variant here but non_enum_variant() below? These should be the same thing, but just a bit weird. (Probably an opportunity for a bit of cleanup)

Yep they both use variant now. I didn’t find other code that need to be cleaned up, please let me know if there are any

jackh726 · 2021-10-26T18:54:15Z

src/test/ui/rfcs/rfc-2528-type_changing-struct-update/type-generic-update.stderr

+error[E0308]: mismatched types
+  --> $DIR/type-generic-update.rs:31:11
+   |
+LL |         ..m1
+   |           ^^ field type mismatch: Machine.state
+   |
+   = note: expected type `i32`
+              found type `f64`


This is...fine...for now, but this needs better diagnostics. Should be a followup issue filed or noted on tracking issue.

I added a FIXME to indicate that the diagnostic information needs to be enhanced

jackh726 · 2021-10-26T18:56:41Z

compiler/rustc_typeck/src/check/expr.rs

+            // the fields with the base_expr. This could cause us to hit errors later
+            // when certain fields are assumed to exist that in fact do not.
+            if !error_happened {
+                let fru_tys = if self.tcx.features().type_changing_struct_update {


I think having these be two separate branches is fine for now, but I think it'd be nice to eventually merge them as much as possible. Can you add a FIXME?

Yes, have add a FIXME here. In order to add a feature trigger, I think it is necessary to merge them. I will submit a new commit to try to implement it.

compiler/rustc_typeck/src/check/expr.rs

put the test dir in test/ui/rfcs

SparrowLii · 2021-10-29T03:38:39Z

Overall pretty straightforward, a few nits. Can you squash the commits (other than spliting out that one commit)?

OK. Have made a squash.

SparrowLii · 2021-10-29T03:43:40Z

As stated by FIXME in lifetime-update.rs, spans generated by type errors caused by lifetime should be corrected. We may need to raise this point in the tracking issue.

jackh726

Last commit feels a bit messy to me; not sure how best to clean it up though.

Left a couple commits.

jackh726 · 2021-11-04T17:56:23Z

compiler/rustc_typeck/src/check/expr.rs

+            if ty.is_never() {
+                assert!(
+                    !self.typeck_results.borrow().adjustments().contains_key(base_expr.hir_id),
+                    "expression with never type wound up being adjusted"
+                );
+                let adj_ty = self.next_ty_var(TypeVariableOrigin {
+                    kind: TypeVariableOriginKind::AdjustmentType,
+                    span: base_expr.span,
+                });
+                self.apply_adjustments(
+                    base_expr,
+                    vec![Adjustment { kind: Adjust::NeverToAny, target: adj_ty }],
+                );
+                ty = adj_ty;
+            }


Hmm, what is this needed for? If this is needed, can you add a test for it?

jackh726 · 2021-11-04T17:59:40Z

compiler/rustc_typeck/src/check/expr.rs

+            let cause = self.misc(base_expr.span);
+            let mut fru_tys = None;
+            let mut err = None;
+            let is_struct;


Why define this here?

jackh726 · 2021-11-04T18:01:50Z

compiler/rustc_typeck/src/check/expr.rs

-            };
-            self.typeck_results.borrow_mut().fru_field_types_mut().insert(expr_id, fru_tys);
+                err = self.demand_suptype_diag(base_expr.span, expected_ty, ty);
+                is_struct = false;


Why not just emit the error here?

Or, better yet, any way we can just return early and do away with the nesting?

SparrowLii · 2021-11-05T01:31:26Z

Thanks for reviewing. I manually copied most of the implementation of check_expr_has_type_or_error for adding the feature error, which would make the code messy. That is unwise. So I went back to the previous submission and found a new way. It should be clear enough now and can be reviewed.

jackh726 · 2021-11-09T13:07:40Z

I think this can definitely be cleaned up a bit in the future, and there is a FIXME for that, so @bors r+

bors · 2021-11-09T13:07:42Z

📌 Commit 926892d has been approved by jackh726

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#89561 (Type inference for inline consts) - rust-lang#90035 (implement rfc-2528 type_changing-struct-update) - rust-lang#90613 (Allow to run a specific rustdoc-js* test) - rust-lang#90683 (Make `compiler-docs` only control the default instead of being a hard off-switch) - rust-lang#90685 (x.py: remove fixme by deleting code) - rust-lang#90701 (Record more artifact sizes during self-profiling.) - rust-lang#90723 (Better document `Box` and `alloc::alloc::box_free` connection) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned jackh726 Oct 19, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2021

This comment has been minimized.

Sign in to view

jackh726 mentioned this pull request Oct 21, 2021

add feature flag for type_changing_struct_update #89730

Merged

jackh726 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2021

jackh726 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 23, 2021

This comment has been minimized.

Sign in to view

SparrowLii force-pushed the rfc2528 branch from a2c8667 to 506389e Compare October 25, 2021 01:45

This comment has been minimized.

Sign in to view

SparrowLii force-pushed the rfc2528 branch from e03cf2f to 6599d25 Compare October 25, 2021 02:59

jackh726 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 25, 2021

jackh726 requested changes Oct 26, 2021

View reviewed changes

jackh726 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 26, 2021

implement type-changing-struct-update

7bde18a

put the test dir in test/ui/rfcs

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 28, 2021

move the processing part of base_expr into check_expr_struct_fields

f3679bc

SparrowLii force-pushed the rfc2528 branch from 6599d25 to f3679bc Compare October 29, 2021 03:17

Add feature trigger and correct is_struct check

1ab2616

jackh726 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 29, 2021

SparrowLii requested a review from jackh726 November 4, 2021 03:37

jackh726 requested changes Nov 4, 2021

View reviewed changes

Add feature trigger and enable is_struct check

926892d

SparrowLii requested a review from jackh726 November 8, 2021 09:31

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 9, 2021

matthiaskrgr mentioned this pull request Nov 9, 2021

Rollup of 7 pull requests #90734

Merged

bors merged commit 610b4e5 into rust-lang:master Nov 9, 2021

rustbot added this to the 1.58.0 milestone Nov 9, 2021

crlf0710 added the F-type-changing-struct-update `#![feature(type_changing_struct_update)]` label Nov 15, 2021

crlf0710 mentioned this pull request Nov 15, 2021

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

Open

5 tasks

compiler-errors mentioned this pull request Dec 4, 2021

struct update syntax causes ICE #91502

Closed

nbdd0121 mentioned this pull request Dec 17, 2021

rustc 1.59.0-nightly (c5ecc1570 2021-12-15): panic when trait bound not satisfied #92010

Closed

Kobzol mentioned this pull request Feb 2, 2022

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

Closed

		LL \| let m2: Machine<'static, State1> = Machine {
		\| ------------------------ type annotation requires that `s` is borrowed for `'static`

implement rfc-2528 type_changing-struct-update #90035

implement rfc-2528 type_changing-struct-update #90035

Conversation

SparrowLii commented Oct 19, 2021 • edited Loading

rust-highfive commented Oct 19, 2021

This comment has been minimized.

jackh726 commented Oct 21, 2021

SparrowLii commented Oct 22, 2021

bors commented Oct 23, 2021

jackh726 commented Oct 23, 2021

SparrowLii commented Oct 23, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SparrowLii commented Oct 25, 2021

jackh726 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparrowLii Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparrowLii Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparrowLii commented Oct 29, 2021 • edited Loading

SparrowLii commented Oct 29, 2021

jackh726 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparrowLii commented Nov 5, 2021 • edited Loading

jackh726 commented Nov 9, 2021

bors commented Nov 9, 2021

SparrowLii commented Oct 19, 2021 •

edited

Loading

SparrowLii Oct 29, 2021 •

edited

Loading

SparrowLii Oct 29, 2021 •

edited

Loading

SparrowLii commented Oct 29, 2021 •

edited

Loading

SparrowLii commented Nov 5, 2021 •

edited

Loading