gopls/internal/golang: add "Extract constant" counterpart

If the selection is a constant expression, instead of "Extract
variable", gopls now offers "Extract constant", and generates
a constant declaration.

Regrettably, I noticed pending CL 564338 only while composing
this CL description; this CL supersedes that one. Apologies
for the redundant efforts.

+ Test, doc, relnote

Fixes golang/go#37170
Updates golang/go#63852

Change-Id: I1376662b50820936de1e156413537e0bcc2292ff
Reviewed-on: https://go-review.googlesource.com/c/tools/+/633257
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

gopls/doc/features/transformation.md

@@ -69,6 +69,7 @@ Gopls supports the following code actions:
 - `source.test` (undocumented) <!-- TODO: fix that -->
 - [`source.addTest`](#source.addTest)
 - [`gopls.doc.features`](README.md), which opens gopls' index of features in a browser
+- [`refactor.extract.constant`](#extract)
 - [`refactor.extract.function`](#extract)
 - [`refactor.extract.method`](#extract)
 - [`refactor.extract.toNewFile`](#extract.toNewFile)
@@ -358,6 +359,9 @@ newly created declaration that contains the selected code:
   ![Before extracting a var](../assets/extract-var-before.png)
   ![After extracting a var](../assets/extract-var-after.png)
 
+- **`refactor.extract.constant** does the same thing for a constant
+  expression, introducing a local const declaration.
+
 If the default name for the new declaration is already in use, gopls
 generates a fresh name.
 
@@ -380,11 +384,9 @@ number of cases where it falls short, including:
 
 The following Extract features are planned for 2024 but not yet supported:
 
-- **Extract constant** is a variant of "Extract variable" to be
-  offered when the expression is constant; see golang/go#37170.
 - **Extract parameter struct** will replace two or more parameters of a
   function by a struct type with one field per parameter; see golang/go#65552.
-  <!-- TODO(adonovan): review and land https://go.dev/cl/563235. -->
+  <!-- TODO(adonovan): review and land https://go.dev/cl/620995. -->
   <!-- Should this operation update all callers? That's more of a Change Signature. -->
 - **Extract interface for type** will create a declaration of an
   interface type with all the methods of the selected concrete type;

gopls/doc/release/v0.17.0.md

@@ -38,6 +38,12 @@ or by selecting a whole declaration or multiple declarations.
 In order to avoid ambiguity and surprise about what to extract, some kinds
 of paritial selection of a declaration cannot invoke this code action.
 
+## Extract constant
+
+When the selection is a constant expression, gopls now offers "Extract
+constant" instead of "Extract variable", and generates a `const`
+declaration instead of a local variable.
+
 ## Pull diagnostics
 
 When initialized with the option `"pullDiagnostics": true`, gopls will advertise support for the

gopls/internal/cmd/codeaction.go

@@ -51,6 +51,7 @@ Valid kinds include:
 	quickfix
 	refactor
 	refactor.extract
+	refactor.extract.constant
 	refactor.extract.function
 	refactor.extract.method
 	refactor.extract.toNewFile

gopls/internal/cmd/usage/codeaction.hlp

@@ -22,6 +22,7 @@ Valid kinds include:
 	quickfix
 	refactor
 	refactor.extract
+	refactor.extract.constant
 	refactor.extract.function
 	refactor.extract.method
 	refactor.extract.toNewFile

gopls/internal/golang/codeaction.go

@@ -236,6 +236,7 @@ var codeActionProducers = [...]codeActionProducer{
 	{kind: settings.RefactorExtractFunction, fn: refactorExtractFunction},
 	{kind: settings.RefactorExtractMethod, fn: refactorExtractMethod},
 	{kind: settings.RefactorExtractToNewFile, fn: refactorExtractToNewFile},
+	{kind: settings.RefactorExtractConstant, fn: refactorExtractVariable, needPkg: true},
 	{kind: settings.RefactorExtractVariable, fn: refactorExtractVariable, needPkg: true},
 	{kind: settings.RefactorInlineCall, fn: refactorInlineCall, needPkg: true},
 	{kind: settings.RefactorRewriteChangeQuote, fn: refactorRewriteChangeQuote},
@@ -462,11 +463,25 @@ func refactorExtractMethod(ctx context.Context, req *codeActionsRequest) error {
 	return nil
 }
 
-// refactorExtractVariable produces "Extract variable" code actions.
+// refactorExtractVariable produces "Extract variable|constant" code actions.
 // See [extractVariable] for command implementation.
 func refactorExtractVariable(ctx context.Context, req *codeActionsRequest) error {
-	if _, _, ok, _ := canExtractVariable(req.pkg.TypesInfo(), req.pgf.File, req.start, req.end); ok {
-		req.addApplyFixAction("Extract variable", fixExtractVariable, req.loc)
+	info := req.pkg.TypesInfo()
+	if expr, _, err := canExtractVariable(info, req.pgf.File, req.start, req.end); err == nil {
+		// Offer one of refactor.extract.{constant,variable}
+		// based on the constness of the expression; this is a
+		// limitation of the codeActionProducers mechanism.
+		// Beware that future evolutions of the refactorings
+		// may make them diverge to become non-complementary,
+		// for example because "if const x = ...; y {" is illegal.
+		constant := info.Types[expr].Value != nil
+		if (req.kind == settings.RefactorExtractConstant) == constant {
+			title := "Extract variable"
+			if constant {
+				title = "Extract constant"
+			}
+			req.addApplyFixAction(title, fixExtractVariable, req.loc)
+		}
 	}
 	return nil
 }

gopls/internal/golang/extract.go

@@ -26,48 +26,72 @@ import (
 	"golang.org/x/tools/internal/typesinternal"
 )
 
+// extractVariable implements the refactor.extract.{variable,constant} CodeAction command.
 func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, pkg *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
 	tokFile := fset.File(file.FileStart)
-	expr, path, ok, err := canExtractVariable(info, file, start, end)
-	if !ok {
-		return nil, nil, fmt.Errorf("extractVariable: cannot extract %s: %v", safetoken.StartPosition(fset, start), err)
+	expr, path, err := canExtractVariable(info, file, start, end)
+	if err != nil {
+		return nil, nil, fmt.Errorf("cannot extract %s: %v", safetoken.StartPosition(fset, start), err)
 	}
+	constant := info.Types[expr].Value != nil
 
 	// Create new AST node for extracted expression.
 	var lhsNames []string
 	switch expr := expr.(type) {
 	case *ast.CallExpr:
 		tup, ok := info.TypeOf(expr).(*types.Tuple)
 		if !ok {
-			// If the call expression only has one return value, we can treat it the
-			// same as our standard extract variable case.
-			lhsName, _ := generateAvailableName(expr.Pos(), path, pkg, info, "x", 0)
-			lhsNames = append(lhsNames, lhsName)
-			break
-		}
-		idx := 0
-		for i := 0; i < tup.Len(); i++ {
-			// Generate a unique variable for each return value.
-			var lhsName string
-			lhsName, idx = generateAvailableName(expr.Pos(), path, pkg, info, "x", idx)
-			lhsNames = append(lhsNames, lhsName)
+			// conversion or single-valued call:
+			// treat it the same as our standard extract variable case.
+			name, _ := generateAvailableName(expr.Pos(), path, pkg, info, "x", 0)
+			lhsNames = append(lhsNames, name)
+
+		} else {
+			// call with multiple results
+			idx := 0
+			for range tup.Len() {
+				// Generate a unique variable for each result.
+				var name string
+				name, idx = generateAvailableName(expr.Pos(), path, pkg, info, "x", idx)
+				lhsNames = append(lhsNames, name)
+			}
 		}
 
 	default:
 		// TODO: stricter rules for selectorExpr.
-		lhsName, _ := generateAvailableName(expr.Pos(), path, pkg, info, "x", 0)
-		lhsNames = append(lhsNames, lhsName)
+		name, _ := generateAvailableName(expr.Pos(), path, pkg, info, "x", 0)
+		lhsNames = append(lhsNames, name)
 	}
 
 	// TODO: There is a bug here: for a variable declared in a labeled
 	// switch/for statement it returns the for/switch statement itself
-	// which produces the below code which is a compiler error e.g.
-	// label:
-	// switch r1 := r() { ... break label ... }
+	// which produces the below code which is a compiler error. e.g.
+	//     label:
+	//         switch r1 := r() { ... break label ... }
 	// On extracting "r()" to a variable
-	// label:
-	// x := r()
-	// switch r1 := x { ... break label ... } // compiler error
+	//     label:
+	//         x := r()
+	//         switch r1 := x { ... break label ... } // compiler error
+	//
+	// TODO(golang/go#70563): Another bug: extracting the
+	// expression to the recommended place may cause it to migrate
+	// across one or more declarations that it references.
+	//
+	// Before:
+	//   if x := 1; cond {
+	//   } else if y := «x + 2»; cond {
+	//   }
+	//
+	// After:
+	//   x1 := x + 2 // error: undefined x
+	//   if x := 1; cond {
+	//   } else if y := x1; cond {
+	//   }
+	//
+	// TODO(golang/go#70665): Another bug (or limitation): this
+	// operation fails at top-level:
+	//   package p
+	//   var x = «1 + 2» // error
 	insertBeforeStmt := analysisinternal.StmtToInsertVarBefore(path)
 	if insertBeforeStmt == nil {
 		return nil, nil, fmt.Errorf("cannot find location to insert extraction")
@@ -78,16 +102,59 @@ func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file
 	}
 	newLineIndent := "\n" + indent
 
-	lhs := strings.Join(lhsNames, ", ")
-	assignStmt := &ast.AssignStmt{
-		Lhs: []ast.Expr{ast.NewIdent(lhs)},
-		Tok: token.DEFINE,
-		Rhs: []ast.Expr{expr},
+	// Create statement to declare extracted var/const.
+	//
+	// TODO(adonovan): beware the const decls are not valid short
+	// statements, so if fixing #70563 causes
+	// StmtToInsertVarBefore to evolve to permit declarations in
+	// the "pre" part of an IfStmt, like so:
+	//   Before:
+	//	if cond {
+	//      } else if «1 + 2» > 0 {
+	//      }
+	//   After:
+	//	if x := 1 + 2; cond {
+	//      } else if x > 0 {
+	//      }
+	// then it will need to become aware that this is invalid
+	// for constants.
+	//
+	// Conversely, a short var decl stmt is not valid at top level,
+	// so when we fix #70665, we'll need to use a var decl.
+	var declStmt ast.Stmt
+	if constant {
+		// const x = expr
+		declStmt = &ast.DeclStmt{
+			Decl: &ast.GenDecl{
+				Tok: token.CONST,
+				Specs: []ast.Spec{
+					&ast.ValueSpec{
+						Names:  []*ast.Ident{ast.NewIdent(lhsNames[0])}, // there can be only one
+						Values: []ast.Expr{expr},
+					},
+				},
+			},
+		}
+
+	} else {
+		// var: x1, ... xn := expr
+		var lhs []ast.Expr
+		for _, name := range lhsNames {
+			lhs = append(lhs, ast.NewIdent(name))
+		}
+		declStmt = &ast.AssignStmt{
+			Tok: token.DEFINE,
+			Lhs: lhs,
+			Rhs: []ast.Expr{expr},
+		}
 	}
+
+	// Format and indent the declaration.
 	var buf bytes.Buffer
-	if err := format.Node(&buf, fset, assignStmt); err != nil {
+	if err := format.Node(&buf, fset, declStmt); err != nil {
 		return nil, nil, err
 	}
+	// TODO(adonovan): not sound for `...` string literals containing newlines.
 	assignment := strings.ReplaceAll(buf.String(), "\n", newLineIndent) + newLineIndent
 
 	return fset, &analysis.SuggestedFix{
@@ -100,39 +167,39 @@ func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file
 			{
 				Pos:     start,
 				End:     end,
-				NewText: []byte(lhs),
+				NewText: []byte(strings.Join(lhsNames, ", ")),
 			},
 		},
 	}, nil
 }
 
 // canExtractVariable reports whether the code in the given range can be
-// extracted to a variable.
-func canExtractVariable(info *types.Info, file *ast.File, start, end token.Pos) (ast.Expr, []ast.Node, bool, error) {
+// extracted to a variable (or constant).
+func canExtractVariable(info *types.Info, file *ast.File, start, end token.Pos) (ast.Expr, []ast.Node, error) {
 	if start == end {
-		return nil, nil, false, fmt.Errorf("empty selection")
+		return nil, nil, fmt.Errorf("empty selection")
 	}
 	path, exact := astutil.PathEnclosingInterval(file, start, end)
 	if !exact {
-		return nil, nil, false, fmt.Errorf("selection is not an expression")
+		return nil, nil, fmt.Errorf("selection is not an expression")
 	}
 	if len(path) == 0 {
-		return nil, nil, false, bug.Errorf("no path enclosing interval")
+		return nil, nil, bug.Errorf("no path enclosing interval")
 	}
 	for _, n := range path {
 		if _, ok := n.(*ast.ImportSpec); ok {
-			return nil, nil, false, fmt.Errorf("cannot extract variable in an import block")
+			return nil, nil, fmt.Errorf("cannot extract variable or constant in an import block")
 		}
 	}
 	expr, ok := path[0].(ast.Expr)
 	if !ok {
-		return nil, nil, false, fmt.Errorf("selection is not an expression") // e.g. statement
+		return nil, nil, fmt.Errorf("selection is not an expression") // e.g. statement
 	}
 	if tv, ok := info.Types[expr]; !ok || !tv.IsValue() || tv.Type == nil || tv.HasOk() {
 		// e.g. type, builtin, x.(type), 2-valued m[k], or ill-typed
-		return nil, nil, false, fmt.Errorf("selection is not a single-valued expression")
+		return nil, nil, fmt.Errorf("selection is not a single-valued expression")
 	}
-	return expr, path, true, nil
+	return expr, path, nil
 }
 
 // Calculate indentation for insertion.

gopls/internal/golang/fix.go

@@ -58,7 +58,7 @@ func singleFile(fixer1 singleFileFixer) fixer {
 
 // Names of ApplyFix.Fix created directly by the CodeAction handler.
 const (
-	fixExtractVariable         = "extract_variable"
+	fixExtractVariable         = "extract_variable" // (or constant)
 	fixExtractFunction         = "extract_function"
 	fixExtractMethod           = "extract_method"
 	fixInlineCall              = "inline_call"

gopls/internal/settings/codeactionkind.go

@@ -99,6 +99,7 @@ const (
 	RefactorInlineCall protocol.CodeActionKind = "refactor.inline.call"
 
 	// refactor.extract
+	RefactorExtractConstant  protocol.CodeActionKind = "refactor.extract.constant"
 	RefactorExtractFunction  protocol.CodeActionKind = "refactor.extract.function"
 	RefactorExtractMethod    protocol.CodeActionKind = "refactor.extract.method"
 	RefactorExtractVariable  protocol.CodeActionKind = "refactor.extract.variable"

gopls/internal/settings/default.go

@@ -61,6 +61,7 @@ func DefaultOptions(overrides ...func(*Options)) *Options {
 						RefactorRewriteRemoveUnusedParam: true,
 						RefactorRewriteSplitLines:        true,
 						RefactorInlineCall:               true,
+						RefactorExtractConstant:          true,
 						RefactorExtractFunction:          true,
 						RefactorExtractMethod:            true,
 						RefactorExtractVariable:          true,

gopls/internal/test/marker/testdata/codeaction/extract_variable-if.txt

@@ -0,0 +1,41 @@
+This test checks the behavior of the 'extract variable/constant' code actions
+when the optimal place for the new declaration is within the "if" statement,
+like so:
+
+   if x := 1 + 2 or y + y ; true {
+   } else if x > 0 {
+   }
+
+A future refactor.variable implementation that does this should avoid
+using a 'const' declaration, which is not legal at that location.
+
+-- flags --
+-ignore_extra_diags
+
+-- a.go --
+package a
+
+func constant() {
+	if true {
+	} else if 1 + 2 > 0 { //@ codeaction("1 + 2", "refactor.extract.constant", edit=constant)
+	}
+}
+
+func variable(y int) {
+	if true {
+	} else if y + y > 0 { //@ codeaction("y + y", "refactor.extract.variable", edit=variable)
+	}
+}
+
+-- @constant/a.go --
+@@ -4 +4 @@
++	const x = 1 + 2
+@@ -5 +6 @@
+-	} else if 1 + 2 > 0 { //@ codeaction("1 + 2", "refactor.extract.constant", edit=constant)
++	} else if x > 0 { //@ codeaction("1 + 2", "refactor.extract.constant", edit=constant)
+-- @variable/a.go --
+@@ -10 +10 @@
++	x := y + y
+@@ -11 +12 @@
+-	} else if y + y > 0 { //@ codeaction("y + y", "refactor.extract.variable", edit=variable)
++	} else if x > 0 { //@ codeaction("y + y", "refactor.extract.variable", edit=variable)