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

Implement destructuring assignment #71156

Closed
wants to merge 28 commits into from

Conversation

fanzier
Copy link
Contributor

@fanzier fanzier commented Apr 14, 2020

Implement destructuring assignment, as suggested in rust-lang/rfcs#372. The accompanying RFC is rust-lang/rfcs#2909.

Note: This PR has been split up into #78748, #78836, #79016, which have been merged.

Quick summary: This allows destructuring the LHS of an assignment if it's a (possibly nested) tuple, tuple struct, or slice.
It is implemented via a desugaring (AST -> HIR lowering) as follows:

(a,b) = (1,2)

... becomes ...

{
  let (lhs0,lhs1) = (1,2);
  a = lhs0;
  b = lhs1;
}

Parser changes:

  • In order to support _ in the LHS, it allows parsing _ as an expression (a change to the parser). It is still disallowed in all other places.
  • Struct expressions are not required to have a base expression, i.e. Struct { a: 1, .. } becomes legal (in order to act like a struct pattern).

Tracking issue: #71126.

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2020
@varkor varkor marked this pull request as draft April 14, 2020 23:14
@Centril Centril self-assigned this Apr 14, 2020
@rust-highfive

This comment has been minimized.

@@ -16,4 +16,7 @@ fn main() {
assert_eq!((a, b), (2, 2));
(b, ..) = (5, 6, 7);
assert_eq!(b, 5);

// Check the empty tuple.
() = ();
Copy link
Member

Choose a reason for hiding this comment

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

This is quite amusing.

Copy link
Member

Choose a reason for hiding this comment

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

since the assignment expression returns () this means the following is now valid Rust:

()=()=()=()=()=()=()=()=()=()=()=()=()=()=()=()=()=()=()=()=()=()=()=()=();

Copy link
Member

Choose a reason for hiding this comment

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

A beautiful addition to the weird-exprs test.

Copy link
Member

Choose a reason for hiding this comment

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

Add it to the weird-exprs.rs test :)

@@ -1225,7 +1226,9 @@ pub fn noop_visit_expr<T: MutVisitor>(Expr { kind, id, span, attrs }: &mut Expr,
ExprKind::Struct(path, fields, expr) => {
vis.visit_path(path);
fields.flat_map_in_place(|field| vis.flat_map_field(field));
visit_opt(expr, |expr| vis.visit_expr(expr));
if let StructRest::Base(expr) = expr {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to write this as a match with all the cases expanded?

Copy link
Member

Choose a reason for hiding this comment

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

This case is only ever going to apply to one variant, so I don't think there's danger of missing anything like this.

@bors
Copy link
Contributor

bors commented May 2, 2020

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

@bors
Copy link
Contributor

bors commented May 3, 2020

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

@rust-highfive
Copy link
Collaborator

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
========================== Starting Command Output ===========================
[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/5b0453d1-e90d-4699-89d8-8e332f64d40b.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/71156/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
---
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git config --get-all http.proxy
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/71156/merge:refs/remotes/pull/71156/merge
---
 ---> f7353ccad5b1
Step 6/7 : ENV RUN_CHECK_WITH_PARALLEL_QUERIES 1
 ---> Using cache
 ---> ed38efbaa060
Step 7/7 : ENV SCRIPT python3 ../x.py test src/tools/expand-yaml-anchors &&            python3 ../x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu &&            python3 ../x.py build --stage 0 src/tools/build-manifest &&            python3 ../x.py test --stage 0 src/tools/compiletest &&            python3 ../x.py test src/tools/tidy &&            /scripts/validate-toolstate.sh
 ---> c5008ef7ae8e
Successfully built c5008ef7ae8e
Successfully tagged rust-ci:latest
Built container sha256:c5008ef7ae8e94d7ef502e3cef26e61208e14ebdb36913f3a8bb86291bd6430b
---
    Checking rustc_feature v0.0.0 (/checkout/src/librustc_feature)
    Checking fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
    Checking rustc_ast_pretty v0.0.0 (/checkout/src/librustc_ast_pretty)
    Checking rustc_hir v0.0.0 (/checkout/src/librustc_hir)
    Checking rustc_query_system v0.0.0 (/checkout/src/librustc_query_system)
    Checking rustc_attr v0.0.0 (/checkout/src/librustc_attr)
    Checking rustc_parse v0.0.0 (/checkout/src/librustc_parse)
    Checking rustc_hir_pretty v0.0.0 (/checkout/src/librustc_hir_pretty)
    Checking rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
---
  local time: Sun May  3 10:56:20 UTC 2020
  network time: Sun, 03 May 2020 10:56:20 GMT
== end clock drift check ==

##[error]Bash exited with code '1'.
##[section]Finishing: Run build
##[section]Starting: Checkout rust-lang/rust@refs/pull/71156/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
Author       : Microsoft
Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
==============================================================================
Cleaning any cached credential from repository: rust-lang/rust (GitHub)
##[section]Finishing: Checkout rust-lang/rust@refs/pull/71156/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (3763) (python)
##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@Muirrum
Copy link
Member

Muirrum commented Jun 13, 2020

@rustbot modify labels to +S-waiting-on-author -S-waiting-on-review

@rustbot rustbot 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 Jun 13, 2020
@leonardo-m
Copy link

I hope to see this ergonomy improvement in Rust

@Muirrum
Copy link
Member

Muirrum commented Aug 6, 2020

@fanzier Any updates on this PR?

@kennytm
Copy link
Member

kennytm commented Aug 7, 2020

@Muirrum the RFC rust-lang/rfcs#2909 have to be merged first before this PR can be progressed.

@leonardo-m
Copy link

leonardo-m commented Aug 11, 2020

The most basic use case:
(a, b) = (b, a);

To replace:
swap(&mut a, &mut b);

Probably unrelated: with const generics I sometimes wish to do something like this:

const (A, B): (u32, u32) = (10, 20);

@fanzier fanzier marked this pull request as draft November 4, 2020 16:07
@fanzier
Copy link
Contributor Author

fanzier commented Nov 4, 2020

I tried to rebase this branch first but there was a conflict with the PR #77283, which doesn't work anymore with the desugaring in this PR. As a consequence, the test src/test/ui/issues/issue-77218.rs fails. Since I don't understand @estebank's PR well, I'm not sure how to fix this. The problem is the check in compiler/rustc_typeck/src/check/expr.rs in the function check_expr_assign, which doesn't fire anymore.
@varkor any ideas?

Edit: To be clear, this is just a regression in the diagnostics: the help message is missing.

@fanzier
Copy link
Contributor Author

fanzier commented Nov 4, 2020

As suggested by @petrochenkov, I've opened a new PR just for tuple destructuring as a first step: #78748.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 9, 2020
…enkov

Implement destructuring assignment for tuples

This is the first step towards implementing destructuring assignment (RFC: rust-lang/rfcs#2909, tracking issue: rust-lang#71126). This PR is the first part of rust-lang#71156, which was split up to allow for easier review.

Quick summary: This change allows destructuring the LHS of an assignment if it's a (possibly nested) tuple.
It is implemented via a desugaring (AST -> HIR lowering) as follows:
```rust
(a,b) = (1,2)
```
... becomes ...
```rust
{
  let (lhs0,lhs1) = (1,2);
  a = lhs0;
  b = lhs1;
}
```

Thanks to `@varkor` who helped with the implementation, particularly around default binding modes.

r? `@petrochenkov`
@bors
Copy link
Contributor

bors commented Nov 9, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@arora-aman
Copy link
Member

arora-aman commented Nov 11, 2020

Can someone confirm I'm understanding this correctly?

The following will be converted

let tup = (1, 2)
let (_, x) = tup

to

let tup = (1, 2)
let (lhs1, lhs2) = tup
let _ = lhs1;
let x = lhs2;

@programmerjake
Copy link
Member

Can someone confirm I'm understanding this correctly?

yes, except it would work like this:
The following will be converted

let tup = (1, 2);
(_, x) = tup;

to

let tup = (1, 2);
let (_, lhs2) = tup;
x = lhs2;

m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 12, 2020
…ing, r=petrochenkov

Implement destructuring assignment for structs and slices

This is the second step towards implementing destructuring assignment (RFC: rust-lang/rfcs#2909, tracking issue: rust-lang#71126). This PR is the second part of rust-lang#71156, which was split up to allow for easier review.

Note that the first PR (rust-lang#78748) is not merged yet, so it is included as the first commit in this one. I thought this would allow the review to start earlier because I have some time this weekend to respond to reviews. If `@petrochenkov` prefers to wait until the first PR is merged, I totally understand, of course.

This PR implements destructuring assignment for (tuple) structs and slices. In order to do this, the following *parser change* was necessary: struct expressions are not required to have a base expression, i.e. `Struct { a: 1, .. }` becomes legal (in order to act like a struct pattern).

Unfortunately, this PR slightly regresses the diagnostics implemented in rust-lang#77283. However, it is only a missing help message in `src/test/ui/issues/issue-77218.rs`. Other instances of this diagnostic are not affected. Since I don't exactly understand how this help message works and how to fix it yet, I was hoping it's OK to regress this temporarily and fix it in a follow-up PR.

Thanks to `@varkor` who helped with the implementation, particularly around the struct rest changes.

r? `@petrochenkov`
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 12, 2020
…ing, r=petrochenkov

Implement destructuring assignment for structs and slices

This is the second step towards implementing destructuring assignment (RFC: rust-lang/rfcs#2909, tracking issue: rust-lang#71126). This PR is the second part of rust-lang#71156, which was split up to allow for easier review.

Note that the first PR (rust-lang#78748) is not merged yet, so it is included as the first commit in this one. I thought this would allow the review to start earlier because I have some time this weekend to respond to reviews. If ``@petrochenkov`` prefers to wait until the first PR is merged, I totally understand, of course.

This PR implements destructuring assignment for (tuple) structs and slices. In order to do this, the following *parser change* was necessary: struct expressions are not required to have a base expression, i.e. `Struct { a: 1, .. }` becomes legal (in order to act like a struct pattern).

Unfortunately, this PR slightly regresses the diagnostics implemented in rust-lang#77283. However, it is only a missing help message in `src/test/ui/issues/issue-77218.rs`. Other instances of this diagnostic are not affected. Since I don't exactly understand how this help message works and how to fix it yet, I was hoping it's OK to regress this temporarily and fix it in a follow-up PR.

Thanks to ``@varkor`` who helped with the implementation, particularly around the struct rest changes.

r? ``@petrochenkov``
@fanzier
Copy link
Contributor Author

fanzier commented Nov 14, 2020

As all three parts of this PR (#78748, #78836, #79016) have been merged (or at least approved), I'll close this PR.

A huge thank you to @petrochenkov for reviewing them so quickly and thoroughly, catching a few bugs in the process!

@fanzier fanzier closed this Nov 14, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 15, 2020
test: add `()=()=()=...` to weird-exprs.rs

Idea from rust-lang#71156 (comment) 😄

Builds on nightly since rust-lang#78748 has been merged.
@varkor varkor added the F-destructuring_assignment `#![feature(destructuring_assignment)]` label Nov 19, 2020
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 20, 2020
…ing, r=petrochenkov

Implement destructuring assignment for structs and slices

This is the second step towards implementing destructuring assignment (RFC: rust-lang/rfcs#2909, tracking issue: rust-lang#71126). This PR is the second part of rust-lang#71156, which was split up to allow for easier review.

Note that the first PR (rust-lang#78748) is not merged yet, so it is included as the first commit in this one. I thought this would allow the review to start earlier because I have some time this weekend to respond to reviews. If ``@petrochenkov`` prefers to wait until the first PR is merged, I totally understand, of course.

This PR implements destructuring assignment for (tuple) structs and slices. In order to do this, the following *parser change* was necessary: struct expressions are not required to have a base expression, i.e. `Struct { a: 1, .. }` becomes legal (in order to act like a struct pattern).

Unfortunately, this PR slightly regresses the diagnostics implemented in rust-lang#77283. However, it is only a missing help message in `src/test/ui/issues/issue-77218.rs`. Other instances of this diagnostic are not affected. Since I don't exactly understand how this help message works and how to fix it yet, I was hoping it's OK to regress this temporarily and fix it in a follow-up PR.

Thanks to ``@varkor`` who helped with the implementation, particularly around the struct rest changes.

r? ``@petrochenkov``
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 20, 2020
…etrochenkov

Make `_` an expression, to discard values in destructuring assignments

This is the third and final step towards implementing destructuring assignment (RFC: rust-lang/rfcs#2909, tracking issue: rust-lang#71126). This PR is the third and final part of rust-lang#71156, which was split up to allow for easier review.

With this PR, an underscore `_` is parsed as an expression but is allowed *only* on the left-hand side of a destructuring assignment. There it simply discards a value, similarly to the wildcard `_` in patterns. For instance,
```rust
(a, _) = (1, 2)
```
will simply assign 1 to `a` and discard the 2. Note that for consistency,
```
_ = foo
```
is also allowed and equivalent to just `foo`.

Thanks to ````@varkor```` who helped with the implementation, particularly around pre-expansion gating.

r? ````@petrochenkov````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-patterns Relating to patterns and pattern matching B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. F-destructuring_assignment `#![feature(destructuring_assignment)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.