rust-analyzer icon indicating copy to clipboard operation
rust-analyzer copied to clipboard

Destructure assist

Open Veykril opened this issue 4 years ago • 4 comments

  • [x] Tuple destructuring (implemented in #9855)
  • [ ] RecordStruct destructuring
  • [ ] TupleStruct destructuring

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 avatar Apr 26 '21 22:04 Veykril

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 :thinking:

flodiebold avatar Aug 23 '21 13:08 flodiebold

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.

Veykril avatar Aug 23 '21 14:08 Veykril

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.)

flodiebold avatar Aug 23 '21 14:08 flodiebold

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.

Veykril avatar Aug 23 '21 14:08 Veykril

@Veykril between https://github.com/rust-lang/rust-analyzer/pull/9855 and https://github.com/rust-lang/rust-analyzer/pull/16638, is there anything left to do here?

lnicola avatar Dec 16 '24 19:12 lnicola

Having a quick test with the example snippets we can still improve this I feel like. Right now we seem to have two assists, one for tuples one for structs, both of which behave slightly differently. The tuple one comments out code where it would need to reassemble the destructured binding, where as the struct one places a todo!(). We should fix that, for one I would assume these assists to share code / be just a single one, not two separate ones. Additionally we should really be re-assembling the struct/tuple where needed instead of commenting out the code or placing a todo!()

Veykril avatar Dec 17 '24 08:12 Veykril