feat: Allow “Extract variable” to be invoked on field names in record expressions. - #23213
Merged
Conversation
rustbot
added
the
S-waiting-on-review
A4-Tacks
reviewed
Aug 23, 2026
| // expression. | ||
| let kind = node.kind(); | ||
| ast::Expr::can_cast(kind) || kind == SyntaxKind::RECORD_EXPR_FIELD | ||
| }) |
Member
There was a problem hiding this comment.
Why add this? I noticed without this, dont_extract_in_pattern would still pass through
Try not to break
Contributor
Author
There was a problem hiding this comment.
On revisiting this, there were several problems.
- The
dont_extract_in_patterntest did not actually test this code path, because the test had a selection and this is the empty-selection code path. I’ve fixed that by adding another test, and also split out those tests to a separate commit to clarify that they are testing existing behavior. - I don’t actually need to rewrite this as much as I did. I’ve now kept the existing structure, and only changed
ast::Expr::castinto the broader condition needed to operate on field names. - The
skip(1)was entirely my mistake in understanding the existing code, and has been removed.
Collaborator
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
kpreid
force-pushed
the
extract
branch
3 times, most recently
from
August 25, 2026 22:33
63fa313 to
68abcda
Compare
A4-Tacks
reviewed
Aug 25, 2026
| .is_some_and(|p| p.kind() == SyntaxKind::RECORD_EXPR_FIELD)) | ||
| })?; | ||
|
|
||
| node.ancestors().find_map(valid_target_expr(ctx))?.syntax().clone() |
Member
There was a problem hiding this comment.
Don't be so complicated, just:
let expr = ancestors_at_offset(ctx.source_file().syntax(), ctx.offset())
- .next()
- .and_then(ast::Expr::cast)?;
+ .find(|it| !ast::NameRef::can_cast(it.kind()))
+ .and_then(either::Either::<ast::Expr, ast::RecordExprField>::cast)?;
expr.syntax().ancestors().find_map(valid_target_expr(ctx))?.syntax().clone()… expressions.
This change allows a “mistake” I make very often to succeed: putting
the cursor on “foo” in `Struct { foo: bar() }` when I want to
extract `let foo = bar();`.
It is also groundwork for being able to extract multiple field
expressions at once.
A4-Tacks
approved these changes
Aug 26, 2026
| // field, or a record field’s name. This prevents the assist from appearing when | ||
| // it is unlikely to be relevant, such as when the cursor is in a pattern. | ||
| // (If we did not want to restrict it this way, we could just apply | ||
| // `valid_target_expr()` to all ancestors.) |
Member
There was a problem hiding this comment.
Redundant comments, we have testing
Contributor
Author
There was a problem hiding this comment.
This comment is not for specifying the desired functionality, but for someone reading the code (like me) to understand what the goal of this piece of the code is. Not having this part explained was a significant obstacle; I had to dig into git-blame to understand what the code was supposed to achieve.
rustbot
removed
the
S-waiting-on-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change allows a “mistake” I make very often to succeed: putting the cursor on “foo” in
Struct { foo: bar() }when I want to extractlet foo = bar();.It is also preparation for being able to extract multiple field expressions at once (#21863), because it generalizes the analysis of the selection to be able to work on nodes that are not themselves expressions.
Notes
The added
take_while()and the testsdont_extract_in_pattern_*are an attempt to preserve the behavior added in #18982, but that PR did not add any tests or clearly specify what effect it was intended to have, so I may have gotten it wrong.This is only my second contribution to assists, so all feedback is welcome.