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

Destructure assist #8673

Open
1 of 3 tasks
Veykril opened this issue Apr 26, 2021 · 4 comments
Open
1 of 3 tasks

Destructure assist #8673

Veykril opened this issue Apr 26, 2021 · 4 comments
Labels
A-assists E-medium S-actionable Someone could pick this issue up and work on it right now

Comments

@Veykril
Copy link
Member

Veykril commented Apr 26, 2021

It would be nice to have an assist for destructuring a binding into its "subbindings" as in, when you have a name to a tuple value have it destructure into names for each componenet of the tuple, same for structure fields etc. See IntelliJ's implementation here https://github.com/intellij-rust/intellij-rust/blob/master/src/main/kotlin/org/rust/ide/intentions/DestructureIntention.kt

Some examples for how it should work:

let foo$0 = (1, 2, 3);
let bar = foo.0;
let _ = foo.into();

becomes

let (_0, _1, _2) = (1, 2, 3);
let bar = _0;
let _ = (_0, _1, _2).into();

Similarly this should work for TupleStructs and RecordStructs:

struct Foo { bar: i32 }
let foo$0 = Foo { bar: 1 };
let bar = foo.bar;
let _ = foo.into();

becomes

struct Foo { bar: i32 }
let Foo { bar } = Foo { bar: 1 };
let bar = bar;
let _ = (Foo { bar }).into();

This should also respect private fields as well as #[non_exhaustive], putting .. in that case for the fields that aren't exposed.

@Veykril Veykril added E-medium S-actionable Someone could pick this issue up and work on it right now A-assists labels Apr 26, 2021
bors bot added a commit that referenced this issue Aug 19, 2021
9855: feature: Destructure Tuple Assist r=Veykril a=Booksbaum

Part of #8673. This PR only handles tuples, not TupleStruct and RecordStruct.

Code Assist to destructure a tuple into its items:
![Destructure_Tuple_Assist](https://user-images.githubusercontent.com/15612932/129020107-775d7c94-dca7-4d1f-a0a2-cd63cabf4132.gif)



* Should work in nearly all pattern positions, like let assignment, function parameters, match arms, for loops, and nested variables (`if let Some($0t) = Some((1,2))`)  
  -> everywhere `IdentPat` is allowed
  * Exception: If there's a sub-pattern (``@`):`
    ```rust
    if let t @ (1..=3, 1..=3) = ... {}
    //     ^
    ```
    -> `t` must be a `Name`; `TuplePat` (`(_0, _1)`) isn't allowed
    * inside subpattern is ok:
      ```rust
      let t @ (a, _) = ((1,2), 3);
      //       ^
      ```
      ->
      ```rust
      let t @ ((_0, _1), _) = ((1,2), 3);
      ```
* Assist triggers only at tuple declaration, not tuple usage.  
  (might be useful especially when it creates a sub-pattern (after ``@`)` and only changes the usage under cursor -- but not part of this PR).

### References
References can be destructured:
```rust
let t = &(1,2);
//  ^
let v = t.0;
```
->
```rust
let (_0, _1) = &(1,2);
let v = _0;
```
BUT: `t.0` and `_0` have different types (`i32` vs. `&i32`) -> `v` has now a different type.

I think that's acceptable: I think the destructure assist is mostly used in simple, immediate scopes and not huge existing code.

Additional Notes:
* `ref` has same behaviour (-> `ref` is kept for items)
  ```rust
  let ref t = (1,2);
  //      ^
  ```
  ->
  ```rust
  let (ref _0, ref _1) = (1,2);
  ```
* Rust IntelliJ Plugin: doesn't trigger with `&` or `ref` at all 

### mutable
```rust
let mut t = (1,2);
//      ^
```
->
```rust
let (mut _0, mut _1) = (1,2);
```
and
```rust
let t = &mut (1,2);
//  ^
```
->
```rust
let (_0, _1) = &mut (1,2);
```
Again: with reference (`&mut`), `t.0` and `_0` have different types (`i32` vs `&mut i32`).  
And there's an additional issue with `&mut` and assignment:
```rust
let t = &mut (1,2);
//  ^
t.0 = 9;
```
->
```rust
let (_0, _1) = &mut (1,2);
_0 = 9;
//   ^
//   mismatched types
//   expected `&mut {integer}`, found integer
//   consider dereferencing here to assign to the mutable borrowed piece of memory
```
But I think that's quite a niche use case, so I don't catch that (`*_0 = 9;`)

Additional Notes:
* Rust IntelliJ Plugin: removes the `mut` (`let mut t = ...` -> `let (_0, _1) = ...`), doesn't trigger with `&mut`

### Binding after ``@``
Destructure tuple in sub-pattern is implemented:
```rust
let t = (1,2);
//  ^
let v = t.0;
let f = t.into();
```
->
```rust
let t @ (_0, _1) = (1,2);
let v = _0;
let f = t.into();
```
BUT: Bindings after ``@`` aren't currently in stable and require `#![feature(bindings_after_at)]` (though should be generally [available quite soon](rust-lang/rust#85305 (comment)) (with `1.56.0`)).  
But I don't know how to check for an enabled feature -> Destructure tuple in sub-pattern [isn't enabled](https://github.com/Booksbaum/rust-analyzer/blob/a4ee6c7954f910da3ca74fc0e25edda9077ad184/crates/ide_assists/src/handlers/destructure_tuple_binding.rs#L32) yet.

* When Destructure in sub-pattern is enabled there are two assists:
  * `Destructure tuple in place`:
    ```rust
    let t = (1,2);
    //  ^
    ```
    ->
    ```rust
    let (_0, _1) = (1,2);
    let v = _0;
    let f = /*t*/.into();
    ```
  * `Destructure tuple in sub-pattern`:
    ```rust
    let t = (1,2);
    //  ^
    let v = t.0;
    let f = t.into();
    ```
    ->
    ```rust
    let t @ (_0, _1) = (1,2);
    let v = _0;
    let f = t.into();
    ```
* When Destructure in sub-pattern is disabled, only the first one is available and just named `Destructure tuple`

<br/>
<br/>

### Caveats
* Unlike in #8673 or IntelliJ rust plugin, I'm not leaving the previous tuple name at function calls.  
  **Reasoning**: It's not too unlikely the tuple variable shadows another variable. Destructuring the tuple while leaving the function call untouched, results in still a valid function call -- but now with another variable:
  ```rust
  let t = (8,9);
  let t = (1,2);
  //  ^
  t.into()
  ```
  => Destructure Tuple
  ```rust
  let t = (8,9);
  let (_0, _1) = (1,2);
  t.into()
  ```
  `t.into()` is still valid -- using the first tuple.  
  Instead I comment out the tuple usage, which results in invalid code -> must be handled by user:
  ```rust
  /*t*/.into()
  ```
  * (though that might be a biased decision: For testing I just declared a lot of `t`s and quite ofen in lines next to each other...)
  * Issue: there are some cases that results in still valid code:
    * macro that accept the tuple as well as no arguments:
      ```rust
      macro_rules! m {
          () => { "foo" };
          ($e:expr) => { $e; "foo" };
      }
      let t = (1,2);
      m!(t);
      m!(/*t*/);
      ```
      -> both calls are valid ([test](https://github.com/Booksbaum/rust-analyzer/blob/a4ee6c7954f910da3ca74fc0e25edda9077ad184/crates/ide_assists/src/handlers/destructure_tuple_binding.rs#L1474))  
    * Probably with tuple as return value. Changing the return value most likely results in an error -- but in another place; not where the tuple usage was. 

  -> not sure that's the best way....  
  Additional the tuple name surrounded by comment is more difficult to edit than just the name.
* Code Assists don't support snippet placeholder, and rust analyzer just the first `$0` -> unfortunately no editing of generated tuple item variables. Cursor (`$0`) is placed on first generated item.

<br/>
<br/>

### Issues
* Tuple index usage in macro calls aren't converted:
  ```rust
  let t = (1,2);
  //  ^
  let v = t.0;
  println!("{}", t.0);
  ```
  ->
  ```rust
  let (_0, _1) = (1,2);
  let v = _0;
  println!("{}", /*t*/.0);
  ```
  ([tests](https://github.com/Booksbaum/rust-analyzer/blob/a4ee6c7954f910da3ca74fc0e25edda9077ad184/crates/ide_assists/src/handlers/destructure_tuple_binding.rs#L1294))
  * Issue is:  
    [name.syntax()](https://github.com/Booksbaum/rust-analyzer/blob/a4ee6c7954f910da3ca74fc0e25edda9077ad184/crates/ide_assists/src/handlers/destructure_tuple_binding.rs#L242-L244) in each [usage](https://github.com/Booksbaum/rust-analyzer/blob/a4ee6c7954f910da3ca74fc0e25edda9077ad184/crates/ide_assists/src/handlers/destructure_tuple_binding.rs#L108-L113) of a tuple is syntax & text_range in its file.  
    EXCEPT when tuple usage is in a macro call (`m!(t.0)`), the macro is expanded and syntax (and range) is based on that expanded macro, not in actual file.  
    That leads to several things:
    * I cannot differentiate between calling the macro with the tuple or with tuple item:
      ```rust
      macro_rules! m {
          ($t:expr, $i:expr) => { $t.0   $i };
      }
      let t = (1,2);
      m!(t, t.0);
      ```
      -> both `t` usages are resolved as tuple index usage
    * Range of resolved tuple index usage is in expanded macro, not in actual file  
     -> don't know where to replace index usage

    -> tuple items passed into a macro are ignored, and only the tuple name itself is handled (uncommented)
* I'm not checking if the generated names conflict with already existing variables.
  ```rust
  let _0 = 42;            // >-|
  let t = (1,2);          //   |
  let v = _0;             // <-|
  //  ^ 42
  ```
  => deconstruct tuple
  ```rust
  let _0 = 42;
  let (_0, _1) = (1,2);     // >-|
  let v = _0;               // <-|
  //  ^ now 1
  ```
  * I tried to get the scope at tuple declaration and its usages. And then iterate all names with [`process_all_names`](https://github.com/rust-analyzer/rust-analyzer/blob/145b51f9daf5371f1754c09eb2e3a77e0a24a0dc/crates/hir/src/semantics.rs#L935). But that doesn't find all local names for declarations (`let t = (1,2)`) (for usages it does)
  * This isn't unique to this Code Assist, but happen in others too (like `extract into variable` or `extract into function`). But here a name conflict is more likely (when destructuring multiple tuples, for examples nested ones (`let t = ((1,2),3)` -> `let (_0, _1) = ...` -> `let ((_0, _1), _1) = ...` -> error))
  * IntelliJ rust plugin does handle this (-> name is `_00`)

Co-authored-by: BooksBaum <15612932 [email protected]>
@flodiebold
Copy link
Member

flodiebold commented Aug 23, 2021

Hm, why do we turn

let foo$0 = (1, 2, 3);
let bar = foo.0;
let _ = foo.into();

into

let (_0, _1, _2) = (1, 2, 3);
let bar = _0;
let _ = /*foo*/.into();

and not

let (_0, _1, _2) = (1, 2, 3);
let bar = _0;
let _ = (_0, _1, _2).into();

?

Edit: Actually that seems to be what IntelliJ does from a look at their code, but the PR #9855 only seems to argue against leaving foo there 🤔

@Veykril
Copy link
Member Author

Veykril commented Aug 23, 2021

That wouldn't work if into takes &self though would it? Since we can't create a ref to the tuple without moving everything into it. So for the following:

let foo$0 = (NonCopy, NonCopy, NonCopy);
let bar = foo.0;
let _ = foo.take_by_ref();
let _ = foo.take_by_ref();

we would have to generate this(or even without emitting a ref expression as it doesnt change the problem)

let (_0, _1, _2) = (1, 2, 3);
let bar = _0;
let _ = (&(_0, _1, _2)).take_by_ref();
let _ = (&(_0, _1, _2)).take_by_ref();

which won't compile.

We can definitely be smart about a few cases for this though like in the specific example of yours.

@flodiebold
Copy link
Member

flodiebold commented Aug 23, 2021

I think that'd still a better non-compiling result than /*foo*/. No need to be smart about it, IMO.

(Also, we can put (_0, _1, _2) there, without the &; (_0, _1, _2).take_by_ref() will always resolve to the same method as foo.take_by_ref(). It's different if it was foo = &(1, 2, 3) of course.)

@Veykril
Copy link
Member Author

Veykril commented Aug 23, 2021

Hm ye actually reconstructing instead of commenting out does seem more appealing and still prevents us from introducing logic errors by mistake that we would get if we didn't replace the usage at all.

bors added a commit that referenced this issue Feb 27, 2024
feature: Add `destructure_struct_binding`

Adds an assist for destructuring a struct in a binding (#8673). I saw that #13997 has been abandoned for a while, so I thought I'd give it a go.

## Example

```rust
let foo = Foo { bar: 1, baz: 2 };
let bar2 = foo.bar;
let baz2 = foo.baz;
let foo2 = foo;

let fizz = Fizz(1, 2);
let buzz = fizz.0;
```
becomes
```rust
let Foo { bar, baz } = Foo { bar: 1, baz: 2 };
let bar2 = bar;
let baz2 = baz;
let foo2 = todo!();

let Fizz(_0, _1) = Fizz(1, 2);
let buzz = _0;
```

More examples in the tests.

## What is included?

- [x] Destructure record, tuple, and unit struct bindings
- [x] Edit field usages
- [x] Non-exhaustive structs in foreign crates and private fields get hidden behind `..`
- [x] Nested bindings
- [x] Carry over `mut` and `ref mut` in nested bindings to fields, i.e. `let Foo { ref mut bar } = ...` becomes `let Foo { bar: Bar { baz: ref mut baz } } = ...`
- [x] Attempt to resolve collisions with other names in the scope
- [x] If the binding is to a reference, field usages are dereferenced if required
- [x] Use shorthand notation if possible

## Known limitations

- `let foo = Foo { bar: 1 }; foo;` currently results in `let Foo { bar } = Foo { bar: 1 }; todo!();` instead of reassembling the struct. This requires user intervention.
- Unused fields are not currently omitted. I thought that this is more ergonomic, as there already is a quick fix action for adding `: _` to unused field patterns.
bors added a commit that referenced this issue Feb 29, 2024
feature: Add `destructure_struct_binding`

Adds an assist for destructuring a struct in a binding (#8673). I saw that #13997 has been abandoned for a while, so I thought I'd give it a go.

## Example

```rust
let foo = Foo { bar: 1, baz: 2 };
let bar2 = foo.bar;
let baz2 = foo.baz;
let foo2 = foo;

let fizz = Fizz(1, 2);
let buzz = fizz.0;
```
becomes
```rust
let Foo { bar, baz } = Foo { bar: 1, baz: 2 };
let bar2 = bar;
let baz2 = baz;
let foo2 = todo!();

let Fizz(_0, _1) = Fizz(1, 2);
let buzz = _0;
```

More examples in the tests.

## What is included?

- [x] Destructure record, tuple, and unit struct bindings
- [x] Edit field usages
- [x] Non-exhaustive structs in foreign crates and private fields get hidden behind `..`
- [x] Nested bindings
- [x] Carry over `mut` and `ref mut` in nested bindings to fields, i.e. `let Foo { ref mut bar } = ...` becomes `let Foo { bar: Bar { baz: ref mut baz } } = ...`
- [x] Attempt to resolve collisions with other names in the scope
- [x] If the binding is to a reference, field usages are dereferenced if required
- [x] Use shorthand notation if possible

## Known limitations

- `let foo = Foo { bar: 1 }; foo;` currently results in `let Foo { bar } = Foo { bar: 1 }; todo!();` instead of reassembling the struct. This requires user intervention.
- Unused fields are not currently omitted. I thought that this is more ergonomic, as there already is a quick fix action for adding `: _` to unused field patterns.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-assists E-medium S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

No branches or pull requests

2 participants