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 for tuples #78748

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

fanzier
Copy link
Contributor

@fanzier fanzier commented Nov 4, 2020

This is the first step towards implementing destructuring assignment (RFC: rust-lang/rfcs#2909, tracking issue: #71126). This PR is the first part of #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:

(a,b) = (1,2)

... becomes ...

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

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

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 4, 2020
@jyn514 jyn514 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 4, 2020
@petrochenkov petrochenkov 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 Nov 5, 2020
@varkor varkor 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 Nov 6, 2020
@petrochenkov petrochenkov 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 Nov 7, 2020
@petrochenkov
Copy link
Contributor

Thanks!
r=me after squashing commits.

@fanzier
Copy link
Contributor Author

fanzier commented Nov 7, 2020

Just squashed and force pushed. Thanks for the quick review, @petrochenkov! I don't think I have the rights to r+, so I'll wait for someone to do that.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 7, 2020

📌 Commit 3a7a997 has been approved by petrochenkov

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 7, 2020
@leonardo-m
Copy link

I've seen that about 100% of the destructuring assignments in my codebase are for tuples (usually 2-tuples, but sometimes longer). So I think it's OK to prioritize the merging of this pull, and work on the rest of the destructuring assignment cases later.

@varkor
Copy link
Member

varkor commented Nov 8, 2020

@leonardo-m: the rest of the cases are already implemented and just have to be split out from the original pull request.

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2020
Rollup of 12 pull requests

Successful merges:

 - rust-lang#77640 (Refactor IntErrorKind to avoid "underflow" terminology)
 - rust-lang#78026 (Define `fs::hard_link` to not follow symlinks.)
 - rust-lang#78114 (Recognize `private_intra_doc_links` as a lint)
 - rust-lang#78228 (Promote aarch64-unknown-linux-gnu to Tier 1)
 - rust-lang#78345 (Fix handling of item names for HIR)
 - rust-lang#78437 (BTreeMap: stop mistaking node for an orderly place)
 - rust-lang#78476 (fix some incorrect aliasing in the BTree)
 - rust-lang#78674 (inliner: Use substs_for_mir_body)
 - rust-lang#78748 (Implement destructuring assignment for tuples)
 - rust-lang#78868 (Fix tab focus on restyled switches)
 - rust-lang#78878 (Avoid overlapping cfg attributes when both macOS and aarch64)
 - rust-lang#78882 (Nicer hunk headers for rust files)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit abaa78b into rust-lang:master Nov 9, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 9, 2020
@leonardo-m
Copy link

I've finally tried this feature in my codebase, and I've seen in some situations it's not a drop-in replacement for the swap() function:

#![feature(destructuring_assignment)]
#![allow(unused_imports, unused_assignments)]

use std::mem::swap;

fn foo1() {
    let a = &mut [0_u32; 10];
    let b = &mut [0_u32; 10];
    swap(a, b); // OK
    (a, b) = (b, a); // cannot assign twice to immutable variable error
}

fn foo2(va: &mut Vec<u32>, vb: &mut Vec<u32>) {
    swap(vb, va); // OK
    (vb, va) = (va, vb); // Lifetime mismatch error
}

struct Bar {
    vb: Vec<u32>,
}

impl Bar {
    fn spam(&mut self) {
        let mut va = vec![0_u32; 10];
        swap(&mut self.vb, &mut va); // OK
        (self.vb, va) = (va, self.vb); // Error cannot move out of `self.vb` which is behind a mutable reference
    }
}

fn main() {}

(Also a possible bug report: it shows only the first error here).

@fanzier
Copy link
Contributor Author

fanzier commented Nov 10, 2020

@leonardo-m I think the line should read (*a, *b) = (*b, *a) in your first example, and then that seems to work. Similarly, the second example should be (*vb, *va) = (*va, *vb). (playground)
Then that second example gives a different error, however, and so does the third:

cannot move out of `*va` which is behind a mutable reference

This error makes sense to me because we really are moving out of it (very briefly) before reassigning something else. It seems that swap is just more powerful than assignment because of Rust's borrowing rules.

@leonardo-m
Copy link

You're very kind, thank you.

@leonardo-m
Copy link

Do you know why my original code doesn't show all three errors, but only the first?

@fanzier
Copy link
Contributor Author

fanzier commented Nov 11, 2020

Do you know why my original code doesn't show all three errors, but only the first?

@leonardo-m If I run your code in the playground, I get two errors, not one. Both of them are about mismatched lifetimes (meaning that type-checking fails). The other errors arising after adding * seem to be borrow-checking errors. I'm not an expert on this but I'd suspect that the compiler does type-checking first and if that fails already, it makes no sense to start borrow-checking. Hence the other errors aren't emitted.

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``
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``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-destructuring_assignment `#![feature(destructuring_assignment)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

None yet

9 participants