feature: Destructure Tuple Assist

[Booksbaum]

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]

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]

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]

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]

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]

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

[Booksbaum]

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.

[Veykril]

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!

[Veykril]

Thanks!
bors r+

[bors]

Build succeeded:

• Rust (macos-latest)
• Rust (ubuntu-latest)
• Rust (windows-latest)
• TypeScript (ubuntu-latest)
• TypeScript (windows-latest)

[lnicola]

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