Skip to content

Commit 8843590

Browse files
xzbdmwadonovan
authored andcommitted
gopls/internal/codeaction: replace all occurrences of expression (refactor.extract.variable.all)
This change introduces a new refactoring code action "Extract n occurrences of expression". The original "Extract Variable" is treated as a limited version of "Extract n occurrences of expression", they share the same underlying implementation. The difference is that "extract_all" utilizes a slice of structural equal expression by searching through the entire function body, while the limited version simply append the expression under selection to that slice. Also: - change default variable name from "x" to "newVar", default const name from "k" to "newConst", users will almost always perform a rename after extraction, so this change make it more discoverable, especially when there are multiple matches. - add marker test extract_expressions.txt and extract_expressions_resolve.txt. Updates golang/go#70085 Fixes golang/go#70563 Change-Id: I767b82be8a60d39c7aff087197c65d435b138826 GitHub-Last-Rev: b66a18d GitHub-Pull-Request: #539 Reviewed-on: https://go-review.googlesource.com/c/tools/+/624035 Reviewed-by: Alan Donovan <[email protected]> Reviewed-by: Robert Findley <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
1 parent fcb4185 commit 8843590

21 files changed

+1068
-209
lines changed
47.7 KB
Loading
53.3 KB
Loading

gopls/doc/features/transformation.md

+10-3
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ Gopls supports the following code actions:
7676
- [`refactor.extract.method`](#extract)
7777
- [`refactor.extract.toNewFile`](#extract.toNewFile)
7878
- [`refactor.extract.variable`](#extract)
79+
- [`refactor.extract.variable-all`](#extract)
7980
- [`refactor.inline.call`](#refactor.inline.call)
8081
- [`refactor.rewrite.changeQuote`](#refactor.rewrite.changeQuote)
8182
- [`refactor.rewrite.fillStruct`](#refactor.rewrite.fillStruct)
@@ -364,14 +365,22 @@ newly created declaration that contains the selected code:
364365
will be a method of the same receiver type.
365366

366367
- **`refactor.extract.variable`** replaces an expression by a reference to a new
367-
local variable named `x` initialized by the expression:
368+
local variable named `newVar` initialized by the expression:
368369

369370
![Before extracting a var](../assets/extract-var-before.png)
370371
![After extracting a var](../assets/extract-var-after.png)
371372

372373
- **`refactor.extract.constant** does the same thing for a constant
373374
expression, introducing a local const declaration.
375+
- **`refactor.extract.variable-all`** replaces all occurrences of the selected expression
376+
within the function with a reference to a new local variable named `newVar`.
377+
This extracts the expression once and reuses it wherever it appears in the function.
374378

379+
![Before extracting all occurrences of EXPR](../assets/extract-var-all-before.png)
380+
![After extracting all occurrences of EXPR](../assets/extract-var-all-after.png)
381+
382+
- **`refactor.extract.constant-all** does the same thing for a constant
383+
expression, introducing a local const declaration.
375384
If the default name for the new declaration is already in use, gopls
376385
generates a fresh name.
377386

@@ -387,10 +396,8 @@ number of cases where it falls short, including:
387396

388397
- https://github.com/golang/go/issues/66289
389398
- https://github.com/golang/go/issues/65944
390-
- https://github.com/golang/go/issues/64821
391399
- https://github.com/golang/go/issues/63394
392400
- https://github.com/golang/go/issues/61496
393-
- https://github.com/golang/go/issues/50851
394401

395402
The following Extract features are planned for 2024 but not yet supported:
396403

gopls/doc/release/v0.18.0.md

+6-1
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,15 @@ func (C[T]) Pop() (T, bool) { ... }
2828
var _ Stack[int] = C[int]{}
2929
```
3030

31+
## Extract all occurrences of the same expression under selection
32+
33+
When you have multiple instances of the same expression in a function,
34+
you can use this code action to extract it into a variable.
35+
All occurrences of the expression will be replaced with a reference to the new variable.
36+
3137
## Improvements to "Definition"
3238

3339
The Definition query now supports additional locations:
3440

3541
- When invoked on a return statement, it reports the location
3642
of the function's result variables.
37-

gopls/internal/golang/codeaction.go

+34-2
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,8 @@ var codeActionProducers = [...]codeActionProducer{
238238
{kind: settings.RefactorExtractToNewFile, fn: refactorExtractToNewFile},
239239
{kind: settings.RefactorExtractConstant, fn: refactorExtractVariable, needPkg: true},
240240
{kind: settings.RefactorExtractVariable, fn: refactorExtractVariable, needPkg: true},
241+
{kind: settings.RefactorExtractConstantAll, fn: refactorExtractVariableAll, needPkg: true},
242+
{kind: settings.RefactorExtractVariableAll, fn: refactorExtractVariableAll, needPkg: true},
241243
{kind: settings.RefactorInlineCall, fn: refactorInlineCall, needPkg: true},
242244
{kind: settings.RefactorRewriteChangeQuote, fn: refactorRewriteChangeQuote},
243245
{kind: settings.RefactorRewriteFillStruct, fn: refactorRewriteFillStruct, needPkg: true},
@@ -467,14 +469,15 @@ func refactorExtractMethod(ctx context.Context, req *codeActionsRequest) error {
467469
// See [extractVariable] for command implementation.
468470
func refactorExtractVariable(ctx context.Context, req *codeActionsRequest) error {
469471
info := req.pkg.TypesInfo()
470-
if expr, _, err := canExtractVariable(info, req.pgf.File, req.start, req.end); err == nil {
472+
if exprs, err := canExtractVariable(info, req.pgf.File, req.start, req.end, false); err == nil {
471473
// Offer one of refactor.extract.{constant,variable}
472474
// based on the constness of the expression; this is a
473475
// limitation of the codeActionProducers mechanism.
474476
// Beware that future evolutions of the refactorings
475477
// may make them diverge to become non-complementary,
476478
// for example because "if const x = ...; y {" is illegal.
477-
constant := info.Types[expr].Value != nil
479+
// Same as [refactorExtractVariableAll].
480+
constant := info.Types[exprs[0]].Value != nil
478481
if (req.kind == settings.RefactorExtractConstant) == constant {
479482
title := "Extract variable"
480483
if constant {
@@ -486,6 +489,35 @@ func refactorExtractVariable(ctx context.Context, req *codeActionsRequest) error
486489
return nil
487490
}
488491

492+
// refactorExtractVariableAll produces "Extract N occurrences of EXPR" code action.
493+
// See [extractAllOccursOfExpr] for command implementation.
494+
func refactorExtractVariableAll(ctx context.Context, req *codeActionsRequest) error {
495+
info := req.pkg.TypesInfo()
496+
// Don't suggest if only one expr is found,
497+
// otherwise it will duplicate with [refactorExtractVariable]
498+
if exprs, err := canExtractVariable(info, req.pgf.File, req.start, req.end, true); err == nil && len(exprs) > 1 {
499+
start, end, err := req.pgf.NodeOffsets(exprs[0])
500+
if err != nil {
501+
return err
502+
}
503+
desc := string(req.pgf.Src[start:end])
504+
if len(desc) >= 40 || strings.Contains(desc, "\n") {
505+
desc = astutil.NodeDescription(exprs[0])
506+
}
507+
constant := info.Types[exprs[0]].Value != nil
508+
if (req.kind == settings.RefactorExtractConstantAll) == constant {
509+
var title string
510+
if constant {
511+
title = fmt.Sprintf("Extract %d occurrences of const expression: %s", len(exprs), desc)
512+
} else {
513+
title = fmt.Sprintf("Extract %d occurrences of %s", len(exprs), desc)
514+
}
515+
req.addApplyFixAction(title, fixExtractVariableAll, req.loc)
516+
}
517+
}
518+
return nil
519+
}
520+
489521
// refactorExtractToNewFile produces "Extract declarations to new file" code actions.
490522
// See [server.commandHandler.ExtractToNewFile] for command implementation.
491523
func refactorExtractToNewFile(ctx context.Context, req *codeActionsRequest) error {

0 commit comments

Comments
 (0)