gopls/internal/golang: refactor.extract.variable: allow all exprs

This CL changes "Extract variable" to allow its use on all
legitimate expressions. Previously it had a short allowlist
of obviously valid forms based on syntax alone, which excluded
anything that might be a type (not a term). It also used to
trigger spuriously on (e.g.) an ExprStmt that calls a nullary
function, or a two-valued comma,ok expression.
Now, it consults the type checker for the definitive answer.

Also, it uses PathEnclosingInterval's "exact" flag so that
extraneous whitespace is permitted.

+ test

Fixes golang/go#70561

Change-Id: I45471ca99e11b04db02671fbd1702dec2547b258
Reviewed-on: https://go-review.googlesource.com/c/tools/+/631777
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

Author: adonovan

gopls/internal/golang/codeaction.go

@@ -236,7 +236,7 @@ var codeActionProducers = [...]codeActionProducer{
 	{kind: settings.RefactorExtractFunction, fn: refactorExtractFunction},
 	{kind: settings.RefactorExtractMethod, fn: refactorExtractMethod},
 	{kind: settings.RefactorExtractToNewFile, fn: refactorExtractToNewFile},
-	{kind: settings.RefactorExtractVariable, fn: refactorExtractVariable},
+	{kind: settings.RefactorExtractVariable, fn: refactorExtractVariable, needPkg: true},
 	{kind: settings.RefactorInlineCall, fn: refactorInlineCall, needPkg: true},
 	{kind: settings.RefactorRewriteChangeQuote, fn: refactorRewriteChangeQuote},
 	{kind: settings.RefactorRewriteFillStruct, fn: refactorRewriteFillStruct, needPkg: true},
@@ -465,7 +465,7 @@ func refactorExtractMethod(ctx context.Context, req *codeActionsRequest) error {
 // refactorExtractVariable produces "Extract variable" code actions.
 // See [extractVariable] for command implementation.
 func refactorExtractVariable(ctx context.Context, req *codeActionsRequest) error {
-	if _, _, ok, _ := canExtractVariable(req.start, req.end, req.pgf.File); ok {
+	if _, _, ok, _ := canExtractVariable(req.pkg.TypesInfo(), req.pgf.File, req.start, req.end); ok {
 		req.addApplyFixAction("Extract variable", fixExtractVariable, req.loc)
 	}
 	return nil

gopls/internal/golang/extract.go

@@ -28,19 +28,14 @@ import (
 
 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(start, end, file)
+	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)
 	}
 
-	// Create new AST node for extracted code.
+	// Create new AST node for extracted expression.
 	var lhsNames []string
 	switch expr := expr.(type) {
-	// TODO: stricter rules for selectorExpr.
-	case *ast.BasicLit, *ast.CompositeLit, *ast.IndexExpr, *ast.SliceExpr,
-		*ast.UnaryExpr, *ast.BinaryExpr, *ast.SelectorExpr:
-		lhsName, _ := generateAvailableName(expr.Pos(), path, pkg, info, "x", 0)
-		lhsNames = append(lhsNames, lhsName)
 	case *ast.CallExpr:
 		tup, ok := info.TypeOf(expr).(*types.Tuple)
 		if !ok {
@@ -57,8 +52,11 @@ func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file
 			lhsName, idx = generateAvailableName(expr.Pos(), path, pkg, info, "x", idx)
 			lhsNames = append(lhsNames, lhsName)
 		}
+
 	default:
-		return nil, nil, fmt.Errorf("cannot extract %T", expr)
+		// TODO: stricter rules for selectorExpr.
+		lhsName, _ := generateAvailableName(expr.Pos(), path, pkg, info, "x", 0)
+		lhsNames = append(lhsNames, lhsName)
 	}
 
 	// TODO: There is a bug here: for a variable declared in a labeled
@@ -110,33 +108,31 @@ func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file
 
 // canExtractVariable reports whether the code in the given range can be
 // extracted to a variable.
-func canExtractVariable(start, end token.Pos, file *ast.File) (ast.Expr, []ast.Node, bool, error) {
+func canExtractVariable(info *types.Info, file *ast.File, start, end token.Pos) (ast.Expr, []ast.Node, bool, error) {
 	if start == end {
-		return nil, nil, false, fmt.Errorf("start and end are equal")
+		return nil, nil, false, fmt.Errorf("empty selection")
+	}
+	path, exact := astutil.PathEnclosingInterval(file, start, end)
+	if !exact {
+		return nil, nil, false, fmt.Errorf("selection is not an expression")
 	}
-	path, _ := astutil.PathEnclosingInterval(file, start, end)
 	if len(path) == 0 {
-		return nil, nil, false, fmt.Errorf("no path enclosing interval")
+		return nil, nil, false, 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")
 		}
 	}
-	node := path[0]
-	if start != node.Pos() || end != node.End() {
-		return nil, nil, false, fmt.Errorf("range does not map to an AST node")
-	}
-	expr, ok := node.(ast.Expr)
+	expr, ok := path[0].(ast.Expr)
 	if !ok {
-		return nil, nil, false, fmt.Errorf("node is not an expression")
+		return nil, nil, false, fmt.Errorf("selection is not an expression") // e.g. statement
 	}
-	switch expr.(type) {
-	case *ast.BasicLit, *ast.CompositeLit, *ast.IndexExpr, *ast.CallExpr,
-		*ast.SliceExpr, *ast.UnaryExpr, *ast.BinaryExpr, *ast.SelectorExpr:
-		return expr, path, true, nil
+	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, false, fmt.Errorf("cannot extract an %T to a variable", expr)
+	return expr, path, true, nil
 }
 
 // Calculate indentation for insertion.

gopls/internal/test/integration/misc/codeactions_test.go

@@ -0,0 +1,36 @@
+This test checks that extract variable permits:
+- extraneous whitespace in the selection
+- function literals
+- pointer dereference expressions
+- parenthesized expressions
+
+-- a.go --
+package a
+
+func _(ptr *int) {
+	var _ = 1 + 2 + 3 //@codeaction("1 + 2 ", "refactor.extract.variable", edit=spaces)
+	var _ = func() {} //@codeaction("func() {}", "refactor.extract.variable", edit=funclit)
+	var _ = *ptr      //@codeaction("*ptr", "refactor.extract.variable", edit=ptr)
+	var _ = (ptr)     //@codeaction("(ptr)", "refactor.extract.variable", edit=paren)
+}
+
+-- @spaces/a.go --
+@@ -4 +4,2 @@
+-	var _ = 1 + 2 + 3 //@codeaction("1 + 2 ", "refactor.extract.variable", edit=spaces)
++	x := 1 + 2
++	var _ = x+ 3 //@codeaction("1 + 2 ", "refactor.extract.variable", edit=spaces)
+-- @funclit/a.go --
+@@ -5 +5,2 @@
+-	var _ = func() {} //@codeaction("func() {}", "refactor.extract.variable", edit=funclit)
++	x := func() {}
++	var _ = x //@codeaction("func() {}", "refactor.extract.variable", edit=funclit)
+-- @ptr/a.go --
+@@ -6 +6,2 @@
+-	var _ = *ptr      //@codeaction("*ptr", "refactor.extract.variable", edit=ptr)
++	x := *ptr
++	var _ = x      //@codeaction("*ptr", "refactor.extract.variable", edit=ptr)
+-- @paren/a.go --
+@@ -7 +7,2 @@
+-	var _ = (ptr)     //@codeaction("(ptr)", "refactor.extract.variable", edit=paren)
++	x := (ptr)
++	var _ = x     //@codeaction("(ptr)", "refactor.extract.variable", edit=paren)