feature: Destructure Tuple Assist #9855

Booksbaum · 2021-08-11T11:38:09Z

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

Code Assist to destructure a tuple into its items:

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 (@):
```
if let t @ (1..=3, 1..=3) = ... {}
//     ^
```
  -> t must be a Name; TuplePat ((_0, _1)) isn't allowed
  - inside subpattern is ok:
```
let t @ (a, _) = ((1,2), 3);
//       ^
```
    ->
```
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:

let t = &(1,2);
//  ^
let v = t.0;

->

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)

let ref t = (1,2);
//      ^

->

let (ref _0, ref _1) = (1,2);

Rust IntelliJ Plugin: doesn't trigger with & or ref at all

mutable

let mut t = (1,2);
//      ^

->

let (mut _0, mut _1) = (1,2);

and

let t = &mut (1,2);
//  ^

->

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:

let t = &mut (1,2);
//  ^
t.0 = 9;

->

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:

let t = (1,2);
//  ^
let v = t.0;
let f = t.into();

->

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 (with 1.56.0)).
But I don't know how to check for an enabled feature -> Destructure tuple in sub-pattern isn't enabled yet.

When Destructure in sub-pattern is enabled there are two assists:

Destructure tuple in place:

let t = (1,2);
//  ^

->

let (_0, _1) = (1,2);
let v = _0;
let f = /*t*/.into();

Destructure tuple in sub-pattern:

let t = (1,2);
//  ^
let v = t.0;
let f = t.into();

->

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

Caveats

Unlike in Destructure assist #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:
```
let t = (8,9);
let t = (1,2);
//  ^
t.into()
```
=> Destructure Tuple
```
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:
```
/*t*/.into()
```
- (though that might be a biased decision: For testing I just declared a lot of ts 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:
```
macro_rules! m {
    () => { "foo" };
    ($e:expr) => { $e; "foo" };
}
let t = (1,2);
m!(t);
m!(/*t*/);
```
    -> both calls are valid (test)
  - 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.

Issues

Tuple index usage in macro calls aren't converted:
```
let t = (1,2);
//  ^
let v = t.0;
println!("{}", t.0);
```
->
```
let (_0, _1) = (1,2);
let v = _0;
println!("{}", /*t*/.0);
```
(tests)
- Issue is:
  name.syntax() in each usage 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:
```
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.
```
let _0 = 42;            // >-|
let t = (1,2);          //   |
let v = _0;             // <-|
//  ^ 42
```
=> deconstruct tuple
```
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. 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)

Veykril · 2021-08-11T12:13:58Z

Awesome, I've been wanting this for a long time 🎉

Addressing some of the points you have raised in the PR description for now:

References can be destructured:
let t = &(1,2);
//  ^
let v = t.0;
->
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.

This seems fine to me for now, but I don't see a reason why we shouldn't insert a deref for these cases in the end.
Same thing for the mutable ref counterpart as well as them in place position.

BUT: Bindings after @ aren't currently in stable and require #![feature(bindings_after_at)] (though should be generally available quite soon (with 1.56.0)).
But I don't know how to check for an enabled feature -> Destructure tuple in sub-pattern isn't enabled yet.

I don't think we have any feature checking mechanisms and since we currently only target latest stable rust it should be fine to just have this unconditionally, especially given 1.56 being there pretty soon.

Unlike in Destructure assist #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:

Oh I dig this, I didn't even think of that problem being a possibility. Tracking down possible logic errors introduced by this could be pretty annoying so forcing a compile error of some sort seems reasonable for these cases. OTOH given we have bindings_after_at soon, maybe it would make sense to just not sure the in place destructure assist in these cases.

Macros causing problems here is to be expected, I don't think there is much we can do in most cases here for the time being so its fine to not have those working for now, but we should have tests showing this incorrect behaviour for them so they won't be forgotten.

I'm not checking if the generated names conflict with already existing variables.

As you've noticed, we don't really handle this in any assist currently, definitely not somethign that needs to be tackled in this PR as it's something that can be done for assist in general instead at some later point.

Booksbaum · 2021-08-11T19:07:36Z

BUT: t.0 and _0 have different types (i32 vs. &i32) -> v has now a different type.

This seems fine to me for now, but I don't see a reason why we shouldn't insert a deref for these cases in the end.
Same thing for the mutable ref counterpart as well as them in place position.

that results in some ugly code:

struct S;
let t = &(S, 2);
//  ^

// cannot use just `t.0` because: `cannot move` 
let v = &t.0;

->

struct S;
let (_0, _1) = &(S, 2);

let v = &*_0;

which of course can be recognized and removed....

Another issue: precedence of deref operator

struct S;
impl S {
  fn r(&self) -> Self { S }
}

let t = &(S,2);
//  ^
let s = t.0.ref();

->

let (_0, _1) = &(S,2);
let s = *_0.ref();

Method call has higher precedence than deref -> compiler error:

type S cannot be dereferenced

-> must be recognized too and put in parentheses ((*_0).ref()), or better don't emit *.
There are probably even more situations when deref is or isn't required.

At least how I use such refactoring assists:
I prefer to not have special treatment and just keep the ref: When a function still works with ref everything is fine. And if it doesn't I have to think how to adjust to the refactoring (which is already the case with everything but tuple index usage (-> gets uncomment)).
And I certainly prefer that over valid, but strange looking &* code.

-> I think in the end it's about how the assist is used: Refactoring in an existing, large code area and probably lots of places a tuple is used. Or during writing the code and thinking "hey, it would be easier/cleaner to use the tuple items directly". And therefore just very little code and invalid stuff is ok.

Considering uncommenting tuples (without index usage) is already done and results in invalid code, I think it's reasonable to allow invalid code here too. The question is then just: where does that error emerge (how close to the tuple usage location)

OTOH given we have bindings_after_at soon, maybe it would make sense to just not sure the in place destructure assist in these cases.

I think especially while rewriting code destructure in place is still useful. Though it might be sensible to mention in the assist description (in VS Code) when there are usages other than its indices.

One easy thing to favour destructure in sub-pattern over in place might be moving (and displaying) the in sub-pattern assist before the in place assist. (Currently it's the other way around -- for no other reason than: I simply implemented the general case in place assist first, and sub-pattern afterwards)

Veykril · 2021-08-12T13:39:37Z

that results in some ugly code:
[...]
which of course can be recognized and removed....

-> must be recognized too and put in parentheses ((*_0).ref()), or better don't emit *.
There are probably even more situations when deref is or isn't required.

All of these are fine to recognize and special case so that they are emitted as better/simpler but working code. A lot of assists have special cases for some things so that they are emitted more idiomatically/correctly.

I personally expect an assist when it is able to generate correct code to do so. I find it odd(and a bug even in most cases) if an assist generates non compiling code even though it would be able to generate code that compiles and works as intended.
When I use an assist I want it to refactor things and not require me to fix up a lot of changed places, if I have to fix up a lot of parts manually then I don't see the reason in using the assist in the first place.

Basically my stance is, if the assist can generate code that compiles and does logically the correct thing it should do so, if it can generate compiling code that may introduce a logic error/semantic differences it would be better to bail out, that is either not apply or somehow force a compilation error at the problematic code side(as done here with the commenting).

I think especially while rewriting code destructure in place is still useful. Though it might be sensible to mention in the assist description (in VS Code) when there are usages other than its indices.

One easy thing to favour destructure in sub-pattern over in place might be moving (and displaying) the in sub-pattern assist before the in place assist. (Currently it's the other way around -- for no other reason than: I simply implemented the general case in place assist first, and sub-pattern afterwards)

Fair point, I agree that we should at least swap the order in that case since destructure in sub-pattern is what one would usually want once its stable and certainly mention this in the docs.

Booksbaum · 2021-08-13T17:24:21Z

All of these are fine to recognize and special case so that they are emitted as better/simpler but working code. A lot of assists have special cases for some things so that they are emitted more idiomatically/correctly.

[...]

Ok, I try to catch at least some of the special cases.

But this can get quite complex quite quickly, for example because of auto-ref/deref:

trait T {
    fn do_stuff(self);
}
#[derive(Clone, Copy)]
struct S;
impl T for S {
    fn do_stuff(self) { println!("-"); }
}
impl T for &S {
    fn do_stuff(self) { println!("&"); }
}

let t = &(S,2);
t.0.do_stuff();   // output: -

let (_0, _1) = &(S,2);
_0.do_stuff();    // output: &
(*_0).do_stuff(); // output: -

But if there's only one trait impl (either one), we want to use _0 instead of *_0.
Ideally these two methods should of course do the same -- but unfortunately don't have to.

Is there some way to lookup what method would be called if t.0 would be changed to &t.0? (I just noticed: I can't even lookup the currently used function if its a trait implementation (like in example above)?)
Currently (in local edit) I'm always emitting deref, because otherwise I don't know if the same or another method is called.

Ah well ... that are further improvements. First I'm looking at parentheses and refs-derefs that cancel each other out (`&*`)

Veykril · 2021-08-14T12:24:14Z

Ye its fine not to catch all corner cases for now, that should be gradually improved on afterwards.

Booksbaum · 2021-08-17T16:46:21Z

deref handling for usages of ref tuples added.

(

let t = &(1,2);

->

let (_0, _1) = &(1,2);

(-> t.0 is i32, but _0 is & i32)

)

Currently handled cases:

&* -> cancel each outer out:
```
let v = &t.0;
```
->
```
let v = _0;   // == `let v = &*_0`
```
- removes parentheses:
```
(&t.0)
&(t.0)
```
  ->
```
_0
_0
```
  - Note: further parens aren't removed: ((&t.0)) -> (_0)
Parentheses based on Expression precedence and deref based if something can be auto-derefed:
- (*_0): (deref and parens)
  - self_param in method calls (t.0.my_method(...))
    - Special case: when original t.0 is a value (no ref) and my_method(&self): t.0 gets auto-refed -> no need for _0 to deref.
      - Note: That's only when t.0 isn't ref and &self (-> t.0 gets auto-refed). In all other cases there might be a trait impl -- which I don't inspect (see comment above)
      - Note: Only when my_method isn't a trait implementation: ctx.sema.resolve_method_call doesn't resolve trait impl.
  - ? operator (t.0?)
- _0: (no deref necessary)
  - field access (t.0.my_field)
    - Note: another tuple field isn't supported: t.0.1: 0.1 is parsed as float (-> even first index usage isn't converted at all: /*t*/.0.1)
  - array index (t.0[1])
- otherwise: *_0 (deref, but no parens) (let v = *_0)
  - Additional "side effect": assignment to mut ref is now handled too (*_0 = ...).

Edit: rebase master & force push because of conflicts. Only the last commit ("Add deref (*) handling for usages of ref tuples") is new.

Only tuple name is handled (uncommented), not tuple index

Note: 2nd Assist description is moved down: generated doc tests extracts now all tests (previously only the first one). But it uses the first `Assist` name -- which is the wrong one for the 2nd test. And 2nd assist is currently disabled -> would fail anyway.

Destructure in sub-pattern before Destructure in place to favor the first one

crates/ide_assists/src/handlers/destructure_tuple_binding.rs

Veykril · 2021-08-18T15:13:58Z

Looks got to me overall with 2 small nits.

I would say we keep the bindings_after_at version disabled for now, until that feature hits stable at which point we can change it to show both when applicable as discussed.

Regarding the method resolution problems with traits, I'm not sure myself, I'd say we can keep that out of this initial PR and look at that in a follow up.

And great work on the tests by the way! Thanks a lot for the work on this!

Add node about uncommenting tuple in macro call

Veykril · 2021-08-19T15:18:52Z

Thanks!
bors r

bors · 2021-08-19T15:25:17Z

Build succeeded:

lnicola · 2021-08-19T15:33:16Z

changelog feature (first contribution) add "Destructure tuple" assist:

Veykril self-assigned this Aug 11, 2021

Booksbaum added 10 commits August 17, 2021 19:22

Add Destructure Tuple

478b298

Handle mut & ref

3e40519

Add destructure in sub-pattern (after @)

450a9c5

Handle tuple in macro call

0d2490f

Only tuple name is handled (uncommented), not tuple index

Fix: different assist ids in doc and code

45ef57b

Cleanup according to style-requirements in tests

b441aa2

Add generated doctest

d0cf283

Switch order of assists

384fae7

Destructure in sub-pattern before Destructure in place to favor the first one

Add deref (*) handling for usages of ref tuples

b1ebb82

Booksbaum force-pushed the destructure_tuple branch from 17881a9 to b1ebb82 Compare August 17, 2021 17:26

Veykril reviewed Aug 18, 2021

View reviewed changes

crates/ide_assists/src/handlers/destructure_tuple_binding.rs Outdated Show resolved Hide resolved

Veykril reviewed Aug 18, 2021

View reviewed changes

crates/ide_assists/src/handlers/destructure_tuple_binding.rs Outdated Show resolved Hide resolved

Booksbaum added 2 commits August 19, 2021 14:09

Remove match_ast! macro call

2c27adc

Add node about uncommenting tuple in macro call

Remove match_ast usage

8a9feed

Veykril mentioned this pull request Aug 19, 2021

Destructure assist #8673

Open

3 tasks

bors bot merged commit 59aa091 into rust-lang:master Aug 19, 2021

Booksbaum deleted the destructure_tuple branch August 19, 2021 16:14

IFcoltransG mentioned this pull request Jan 21, 2023

feature: Add 'destructure struct binding' assist #13997

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Destructure Tuple Assist #9855

feature: Destructure Tuple Assist #9855

Booksbaum commented Aug 11, 2021

Veykril commented Aug 11, 2021

Booksbaum commented Aug 11, 2021

Veykril commented Aug 12, 2021

Booksbaum commented Aug 13, 2021

Veykril commented Aug 14, 2021

Booksbaum commented Aug 17, 2021 •

edited

Loading

Veykril commented Aug 18, 2021

Veykril commented Aug 19, 2021

bors bot commented Aug 19, 2021

lnicola commented Aug 19, 2021

feature: Destructure Tuple Assist #9855

feature: Destructure Tuple Assist #9855

Conversation

Booksbaum commented Aug 11, 2021

References

mutable

Binding after @

Caveats

Issues

Veykril commented Aug 11, 2021

Booksbaum commented Aug 11, 2021

Veykril commented Aug 12, 2021

Booksbaum commented Aug 13, 2021

Veykril commented Aug 14, 2021

Booksbaum commented Aug 17, 2021 • edited Loading

Veykril commented Aug 18, 2021

Veykril commented Aug 19, 2021

bors bot commented Aug 19, 2021

lnicola commented Aug 19, 2021

Binding after `@`

Booksbaum commented Aug 17, 2021 •

edited

Loading