Destructure assist #8673
Labels
A-assists
A-pattern
pattern handling related things
E-medium
fun
A technically challenging issue with high impact
S-actionable
Someone could pick this issue up and work on it right now
Comments
Veykril
added
E-medium
S-actionable
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:  * 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 <[email protected]>
|
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 |
|
That wouldn't work if 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. |
|
I think that'd still a better non-compiling result than (Also, we can put |
|
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. |
This was referenced Dec 24, 2021
8 tasks
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.
Veykril
added
fun
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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:
becomes
Similarly this should work for TupleStructs and RecordStructs:
becomes
This should also respect private fields as well as
#[non_exhaustive], putting..in that case for the fields that aren't exposed.The text was updated successfully, but these errors were encountered: