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

feature: Add destructure_struct_binding #16638

Merged
merged 4 commits into from
Feb 29, 2024

Conversation

Lindronics
Copy link
Contributor

@Lindronics Lindronics commented Feb 22, 2024

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

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

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?

  • Destructure record, tuple, and unit struct bindings
  • Edit field usages
  • Non-exhaustive structs in foreign crates and private fields get hidden behind ..
  • Nested bindings
  • 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 } } = ...
  • Attempt to resolve collisions with other names in the scope
  • If the binding is to a reference, field usages are dereferenced if required
  • 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.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 22, 2024
@Lindronics Lindronics force-pushed the destructure-struct-binding branch 3 times, most recently from e556829 to 059b35f Compare February 23, 2024 17:15
@Lindronics Lindronics marked this pull request as ready for review February 23, 2024 17:26
Comment on lines +65 to +84
fn is_auto_ref(ctx: &AssistContext<'_>, call_expr: &MethodCallExpr) -> bool {
fn impl_(ctx: &AssistContext<'_>, call_expr: &MethodCallExpr) -> Option<bool> {
let rec = call_expr.receiver()?;
let rec_ty = ctx.sema.type_of_expr(&rec)?.original();
// input must be actual value
if rec_ty.is_reference() {
return Some(false);
}

// doesn't resolve trait impl
let f = ctx.sema.resolve_method_call(call_expr)?;
let self_param = f.self_param(ctx.db())?;
// self must be ref
match self_param.access(ctx.db()) {
hir::Access::Shared | hir::Access::Exclusive => Some(true),
hir::Access::Owned => Some(false),
}
}
impl_(ctx, call_expr).unwrap_or(false)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to use sema.expr_adjustments(&rec) and inspect the adjustments for whether an autoref happened or not. That should simplify this I believe

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(also just noticed you didn't write that part, but if you want you could look into that nevertheless)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I just moved it for now. I wouldn't mind looking at it in a follow-up PR!

@Veykril
Copy link
Member

Veykril commented Feb 27, 2024

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Feb 27, 2024

📌 Commit 059b35f has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 27, 2024

⌛ Testing commit 059b35f with merge b0bf646...

bors added a commit that referenced this pull request 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
Copy link
Contributor

bors commented Feb 27, 2024

💔 Test failed - checks-actions

@Veykril
Copy link
Member

Veykril commented Feb 27, 2024

Needs a rebase

@Lindronics Lindronics force-pushed the destructure-struct-binding branch from 059b35f to 653fc75 Compare February 27, 2024 16:07
@Lindronics
Copy link
Contributor Author

Needs a rebase

Done, thanks for reviewing!

Separate into create and apply edit

Rename usages

Hacky name map

Add more tests

Handle non-exhaustive

Add some more TODOs

Private fields

Use todo

Nesting

Improve rest token generation

Cleanup

Doc -> regular comment

Support mut
@Lindronics Lindronics force-pushed the destructure-struct-binding branch from 653fc75 to dc7b502 Compare February 29, 2024 13:21
@Veykril
Copy link
Member

Veykril commented Feb 29, 2024

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Feb 29, 2024

📌 Commit dc7b502 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 29, 2024

⌛ Testing commit dc7b502 with merge 71eb540...

@bors
Copy link
Contributor

bors commented Feb 29, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 71eb540 to master...

@bors bors merged commit 71eb540 into rust-lang:master Feb 29, 2024
11 checks passed
bors added a commit that referenced this pull request Mar 5, 2024
fix: Don't destructure struct with no public fields

Unfortunately I missed this case in #16638.

If a struct only has private members, the assist should not be applicable. Though valid syntax exists (`Foo { .. }`), it isn't particularly useful. Since this case applies to a lot of common types (`Arc`, `Vec`, ...), it probably makes the most sense to hide the action.

As a side effect, this also disables the action for unit structs, where it also isn't particularly useful. I'd be open to changing it though if you think it makes more sense to keep it.

This also fixes the `make::record_pat_field_list` function to produce valid syntax if the field list is empty, as it is used in other places too.

## Current behaviour
```rust
// In crate `other_crate`
pub struct Foo { bar: i32 }

// In current crate
fn do_something(foo: other_crate::Foo) {}

// Becomes
fn do_something(other_crate::Foo { , .. }: other_crate::Foo) {}
```

## Fixed behaviour
Assist should not be applicable in this case anymore.
@lnicola lnicola mentioned this pull request Dec 16, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants