diff --git a/gopls/doc/release/v0.17.0.md b/gopls/doc/release/v0.17.0.md index 9728a317620..1a278b013cb 100644 --- a/gopls/doc/release/v0.17.0.md +++ b/gopls/doc/release/v0.17.0.md @@ -44,6 +44,9 @@ 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. +Also, extraction of a constant or variable now works at top-level, +outside of any function. + ## Pull diagnostics When initialized with the option `"pullDiagnostics": true`, gopls will advertise support for the diff --git a/gopls/internal/golang/extract.go b/gopls/internal/golang/extract.go index 27dc2d9e380..72d718c2faf 100644 --- a/gopls/internal/golang/extract.go +++ b/gopls/internal/golang/extract.go @@ -35,7 +35,8 @@ func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file } constant := info.Types[expr].Value != nil - // Create new AST node for extracted expression. + // Generate name(s) for new declaration. + baseName := cond(constant, "k", "x") var lhsNames []string switch expr := expr.(type) { case *ast.CallExpr: @@ -43,7 +44,7 @@ func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file if !ok { // conversion or single-valued call: // treat it the same as our standard extract variable case. - name, _ := generateAvailableName(expr.Pos(), path, pkg, info, "x", 0) + name, _ := freshName(info, file, expr.Pos(), baseName, 0) lhsNames = append(lhsNames, name) } else { @@ -52,14 +53,14 @@ func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file for range tup.Len() { // Generate a unique variable for each result. var name string - name, idx = generateAvailableName(expr.Pos(), path, pkg, info, "x", idx) + name, idx = freshName(info, file, expr.Pos(), baseName, idx) lhsNames = append(lhsNames, name) } } default: // TODO: stricter rules for selectorExpr. - name, _ := generateAvailableName(expr.Pos(), path, pkg, info, "x", 0) + name, _ := freshName(info, file, expr.Pos(), baseName, 0) lhsNames = append(lhsNames, name) } @@ -87,20 +88,38 @@ func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file // 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") - } - indent, err := calculateIndentation(src, tokFile, insertBeforeStmt) - if err != nil { - return nil, nil, err + var ( + insertPos token.Pos + indentation string + stmtOK bool // ok to use ":=" instead of var/const decl? + ) + if before := analysisinternal.StmtToInsertVarBefore(path); before != nil { + // Within function: compute appropriate statement indentation. + indent, err := calculateIndentation(src, tokFile, before) + if err != nil { + return nil, nil, err + } + insertPos = before.Pos() + indentation = "\n" + indent + + // Currently, we always extract a constant expression + // to a const declaration (and logic in CodeAction + // assumes that we do so); this is conservative because + // it preserves its constant-ness. + // + // In future, constant expressions used only in + // contexts where constant-ness isn't important could + // be profitably extracted to a var declaration or := + // statement, especially if the latter is the Init of + // an {If,For,Switch}Stmt. + stmtOK = !constant + } else { + // Outside any statement: insert before the current + // declaration, without indentation. + currentDecl := path[len(path)-2] + insertPos = currentDecl.Pos() + indentation = "\n" } - newLineIndent := "\n" + indent // Create statement to declare extracted var/const. // @@ -121,17 +140,19 @@ func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file // // 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}, - }, + var newNode ast.Node + if !stmtOK { + // var/const x1, ..., xn = expr + var names []*ast.Ident + for _, name := range lhsNames { + names = append(names, ast.NewIdent(name)) + } + newNode = &ast.GenDecl{ + Tok: cond(constant, token.CONST, token.VAR), + Specs: []ast.Spec{ + &ast.ValueSpec{ + Names: names, + Values: []ast.Expr{expr}, }, }, } @@ -142,7 +163,7 @@ func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file for _, name := range lhsNames { lhs = append(lhs, ast.NewIdent(name)) } - declStmt = &ast.AssignStmt{ + newNode = &ast.AssignStmt{ Tok: token.DEFINE, Lhs: lhs, Rhs: []ast.Expr{expr}, @@ -151,17 +172,17 @@ func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file // Format and indent the declaration. var buf bytes.Buffer - if err := format.Node(&buf, fset, declStmt); err != nil { + if err := format.Node(&buf, fset, newNode); err != nil { return nil, nil, err } // TODO(adonovan): not sound for `...` string literals containing newlines. - assignment := strings.ReplaceAll(buf.String(), "\n", newLineIndent) + newLineIndent + assignment := strings.ReplaceAll(buf.String(), "\n", indentation) + indentation return fset, &analysis.SuggestedFix{ TextEdits: []analysis.TextEdit{ { - Pos: insertBeforeStmt.Pos(), - End: insertBeforeStmt.Pos(), + Pos: insertPos, + End: insertPos, NewText: []byte(assignment), }, { @@ -215,40 +236,36 @@ func calculateIndentation(content []byte, tok *token.File, insertBeforeStmt ast. return string(content[lineOffset:stmtOffset]), nil } -// generateAvailableName adjusts the new function name until there are no collisions in scope. -// Possible collisions include other function and variable names. Returns the next index to check for prefix. -func generateAvailableName(pos token.Pos, path []ast.Node, pkg *types.Package, info *types.Info, prefix string, idx int) (string, int) { - scopes := CollectScopes(info, path, pos) - scopes = append(scopes, pkg.Scope()) +// freshName returns an identifier based on prefix (perhaps with a +// numeric suffix) that is not in scope at the specified position +// within the file. It returns the next numeric suffix to use. +func freshName(info *types.Info, file *ast.File, pos token.Pos, prefix string, idx int) (string, int) { + scope := info.Scopes[file].Innermost(pos) return generateName(idx, prefix, func(name string) bool { - for _, scope := range scopes { - if scope != nil && scope.Lookup(name) != nil { - return true - } - } - return false + obj, _ := scope.LookupParent(name, pos) + return obj != nil }) } -// generateNameOutsideOfRange is like generateAvailableName, but ignores names +// freshNameOutsideRange is like [freshName], but ignores names // declared between start and end for the purposes of detecting conflicts. // // This is used for function extraction, where [start, end) will be extracted // to a new scope. -func generateNameOutsideOfRange(start, end token.Pos, path []ast.Node, pkg *types.Package, info *types.Info, prefix string, idx int) (string, int) { - scopes := CollectScopes(info, path, start) - scopes = append(scopes, pkg.Scope()) +func freshNameOutsideRange(info *types.Info, file *ast.File, pos, start, end token.Pos, prefix string, idx int) (string, int) { + scope := info.Scopes[file].Innermost(pos) return generateName(idx, prefix, func(name string) bool { - for _, scope := range scopes { - if scope != nil { - if obj := scope.Lookup(name); obj != nil { - // Only report a collision if the object declaration was outside the - // extracted range. - if obj.Pos() < start || end <= obj.Pos() { - return true - } - } + // Only report a collision if the object declaration + // was outside the extracted range. + for scope != nil { + obj, declScope := scope.LookupParent(name, pos) + if obj == nil { + return false // undeclared } + if !(start <= obj.Pos() && obj.Pos() < end) { + return true // declared outside ignored range + } + scope = declScope.Parent() } return false }) @@ -640,7 +657,7 @@ func extractFunctionMethod(fset *token.FileSet, start, end token.Pos, src []byte // TODO(suzmue): generate a name that does not conflict for "newMethod". funName = "newMethod" } else { - funName, _ = generateAvailableName(start, path, pkg, info, "newFunction", 0) + funName, _ = freshName(info, file, start, "newFunction", 0) } extractedFunCall := generateFuncCall(hasNonNestedReturn, hasReturnValues, params, append(returns, getNames(retVars)...), funName, sym, receiverName) @@ -1290,7 +1307,7 @@ func generateReturnInfo(enclosing *ast.FuncType, pkg *types.Package, path []ast. var cond *ast.Ident if !hasNonNestedReturns { // Generate information for the added bool value. - name, _ := generateNameOutsideOfRange(start, end, path, pkg, info, "shouldReturn", 0) + name, _ := freshNameOutsideRange(info, file, path[0].Pos(), start, end, "shouldReturn", 0) cond = &ast.Ident{Name: name} retVars = append(retVars, &returnVariable{ name: cond, @@ -1325,7 +1342,7 @@ func generateReturnInfo(enclosing *ast.FuncType, pkg *types.Package, path []ast. } else if n, ok := varNameForType(typ); ok { bestName = n } - retName, idx := generateNameOutsideOfRange(start, end, path, pkg, info, bestName, nameIdx[bestName]) + retName, idx := freshNameOutsideRange(info, file, path[0].Pos(), start, end, bestName, nameIdx[bestName]) nameIdx[bestName] = idx z := typesinternal.ZeroExpr(file, pkg, typ) if z == nil { @@ -1540,3 +1557,11 @@ func getDecls(retVars []*returnVariable) []*ast.Field { } return decls } + +func cond[T any](cond bool, t, f T) T { + if cond { + return t + } else { + return f + } +} diff --git a/gopls/internal/golang/util.go b/gopls/internal/golang/util.go index e215cc81fc8..be5c7c0a735 100644 --- a/gopls/internal/golang/util.go +++ b/gopls/internal/golang/util.go @@ -132,6 +132,9 @@ func findFileInDeps(s metadata.Source, mp *metadata.Package, uri protocol.Docume // CollectScopes returns all scopes in an ast path, ordered as innermost scope // first. +// +// TODO(adonovan): move this to golang/completion and simplify to use +// Scopes.Innermost and LookupParent instead. func CollectScopes(info *types.Info, path []ast.Node, pos token.Pos) []*types.Scope { // scopes[i], where i 0 { //@ codeaction("1 + 2", "refactor.extract.constant", edit=constant) -+ } else if x > 0 { //@ codeaction("1 + 2", "refactor.extract.constant", edit=constant) ++ } else if k > 0 { //@ codeaction("1 + 2", "refactor.extract.constant", edit=constant) -- @variable/a.go -- @@ -10 +10 @@ + x := y + y diff --git a/gopls/internal/test/marker/testdata/codeaction/extract_variable-inexact.txt b/gopls/internal/test/marker/testdata/codeaction/extract_variable-inexact.txt index 0a9cab949a5..1781b3ce6af 100644 --- a/gopls/internal/test/marker/testdata/codeaction/extract_variable-inexact.txt +++ b/gopls/internal/test/marker/testdata/codeaction/extract_variable-inexact.txt @@ -17,8 +17,8 @@ func _(ptr *int) { -- @spaces/a.go -- @@ -4 +4,2 @@ - var _ = 1 + 2 + 3 //@codeaction("1 + 2 ", "refactor.extract.constant", edit=spaces) -+ const x = 1 + 2 -+ var _ = x+ 3 //@codeaction("1 + 2 ", "refactor.extract.constant", edit=spaces) ++ const k = 1 + 2 ++ var _ = k+ 3 //@codeaction("1 + 2 ", "refactor.extract.constant", edit=spaces) -- @funclit/a.go -- @@ -5 +5,2 @@ - var _ = func() {} //@codeaction("func() {}", "refactor.extract.variable", edit=funclit) diff --git a/gopls/internal/test/marker/testdata/codeaction/extract_variable-toplevel.txt b/gopls/internal/test/marker/testdata/codeaction/extract_variable-toplevel.txt new file mode 100644 index 00000000000..b9166c6299d --- /dev/null +++ b/gopls/internal/test/marker/testdata/codeaction/extract_variable-toplevel.txt @@ -0,0 +1,51 @@ +This test checks the behavior of the 'extract variable/constant' code action +at top level (outside any function). See issue #70665. + +-- a.go -- +package a + +const length = len("hello") + 2 //@codeaction(`len("hello")`, "refactor.extract.constant", edit=lenhello) + +var slice = append([]int{}, 1, 2, 3) //@codeaction("[]int{}", "refactor.extract.variable", edit=sliceliteral) + +type SHA256 [32]byte //@codeaction("32", "refactor.extract.constant", edit=arraylen) + +func f([2]int) {} //@codeaction("2", "refactor.extract.constant", edit=paramtypearraylen) + +-- @lenhello/a.go -- +@@ -3 +3,2 @@ +-const length = len("hello") + 2 //@codeaction(`len("hello")`, "refactor.extract.constant", edit=lenhello) ++const k = len("hello") ++const length = k + 2 //@codeaction(`len("hello")`, "refactor.extract.constant", edit=lenhello) +-- @sliceliteral/a.go -- +@@ -5 +5,2 @@ +-var slice = append([]int{}, 1, 2, 3) //@codeaction("[]int{}", "refactor.extract.variable", edit=sliceliteral) ++var x = []int{} ++var slice = append(x, 1, 2, 3) //@codeaction("[]int{}", "refactor.extract.variable", edit=sliceliteral) +-- @arraylen/a.go -- +@@ -7 +7,2 @@ +-type SHA256 [32]byte //@codeaction("32", "refactor.extract.constant", edit=arraylen) ++const k = 32 ++type SHA256 [k]byte //@codeaction("32", "refactor.extract.constant", edit=arraylen) +-- @paramtypearraylen/a.go -- +@@ -9 +9,2 @@ +-func f([2]int) {} //@codeaction("2", "refactor.extract.constant", edit=paramtypearraylen) ++const k = 2 ++func f([k]int) {} //@codeaction("2", "refactor.extract.constant", edit=paramtypearraylen) +-- b/b.go -- +package b + +// Check that package- and file-level name collisions are avoided. + +import x3 "errors" + +var x, x1, x2 any // these names are taken already +var _ = x3.New("") +var a, b int +var c = a + b //@codeaction("a + b", "refactor.extract.variable", edit=fresh) + +-- @fresh/b/b.go -- +@@ -10 +10,2 @@ +-var c = a + b //@codeaction("a + b", "refactor.extract.variable", edit=fresh) ++var x4 = a + b ++var c = x4 //@codeaction("a + b", "refactor.extract.variable", edit=fresh) diff --git a/gopls/internal/test/marker/testdata/codeaction/extract_variable.txt b/gopls/internal/test/marker/testdata/codeaction/extract_variable.txt index e6f0f893848..c14fb732978 100644 --- a/gopls/internal/test/marker/testdata/codeaction/extract_variable.txt +++ b/gopls/internal/test/marker/testdata/codeaction/extract_variable.txt @@ -15,13 +15,13 @@ func _() { -- @basic_lit1/basic_lit.go -- @@ -4 +4,2 @@ - var _ = 1 + 2 //@codeaction("1", "refactor.extract.constant", edit=basic_lit1) -+ const x = 1 -+ var _ = x + 2 //@codeaction("1", "refactor.extract.constant", edit=basic_lit1) ++ const k = 1 ++ var _ = k + 2 //@codeaction("1", "refactor.extract.constant", edit=basic_lit1) -- @basic_lit2/basic_lit.go -- @@ -5 +5,2 @@ - var _ = 3 + 4 //@codeaction("3 + 4", "refactor.extract.constant", edit=basic_lit2) -+ const x = 3 + 4 -+ var _ = x //@codeaction("3 + 4", "refactor.extract.constant", edit=basic_lit2) ++ const k = 3 + 4 ++ var _ = k //@codeaction("3 + 4", "refactor.extract.constant", edit=basic_lit2) -- func_call.go -- package extract @@ -54,7 +54,7 @@ func _() { y := ast.CompositeLit{} //@codeaction("ast.CompositeLit{}", "refactor.extract.variable", edit=scope1) } if true { - x1 := !false //@codeaction("!false", "refactor.extract.constant", edit=scope2) + x := !false //@codeaction("!false", "refactor.extract.constant", edit=scope2) } } @@ -65,6 +65,6 @@ func _() { + y := x //@codeaction("ast.CompositeLit{}", "refactor.extract.variable", edit=scope1) -- @scope2/scope.go -- @@ -11 +11,2 @@ -- x1 := !false //@codeaction("!false", "refactor.extract.constant", edit=scope2) -+ const x = !false -+ x1 := x //@codeaction("!false", "refactor.extract.constant", edit=scope2) +- x := !false //@codeaction("!false", "refactor.extract.constant", edit=scope2) ++ const k = !false ++ x := k //@codeaction("!false", "refactor.extract.constant", edit=scope2) diff --git a/gopls/internal/test/marker/testdata/codeaction/extract_variable_resolve.txt b/gopls/internal/test/marker/testdata/codeaction/extract_variable_resolve.txt index 3ab9b2df0be..2bf1803a7d8 100644 --- a/gopls/internal/test/marker/testdata/codeaction/extract_variable_resolve.txt +++ b/gopls/internal/test/marker/testdata/codeaction/extract_variable_resolve.txt @@ -26,13 +26,13 @@ func _() { -- @basic_lit1/basic_lit.go -- @@ -4 +4,2 @@ - var _ = 1 + 2 //@codeaction("1", "refactor.extract.constant", edit=basic_lit1) -+ const x = 1 -+ var _ = x + 2 //@codeaction("1", "refactor.extract.constant", edit=basic_lit1) ++ const k = 1 ++ var _ = k + 2 //@codeaction("1", "refactor.extract.constant", edit=basic_lit1) -- @basic_lit2/basic_lit.go -- @@ -5 +5,2 @@ - var _ = 3 + 4 //@codeaction("3 + 4", "refactor.extract.constant", edit=basic_lit2) -+ const x = 3 + 4 -+ var _ = x //@codeaction("3 + 4", "refactor.extract.constant", edit=basic_lit2) ++ const k = 3 + 4 ++ var _ = k //@codeaction("3 + 4", "refactor.extract.constant", edit=basic_lit2) -- func_call.go -- package extract @@ -65,7 +65,7 @@ func _() { y := ast.CompositeLit{} //@codeaction("ast.CompositeLit{}", "refactor.extract.variable", edit=scope1) } if true { - x1 := !false //@codeaction("!false", "refactor.extract.constant", edit=scope2) + x := !false //@codeaction("!false", "refactor.extract.constant", edit=scope2) } } @@ -76,6 +76,6 @@ func _() { + y := x //@codeaction("ast.CompositeLit{}", "refactor.extract.variable", edit=scope1) -- @scope2/scope.go -- @@ -11 +11,2 @@ -- x1 := !false //@codeaction("!false", "refactor.extract.constant", edit=scope2) -+ const x = !false -+ x1 := x //@codeaction("!false", "refactor.extract.constant", edit=scope2) +- x := !false //@codeaction("!false", "refactor.extract.constant", edit=scope2) ++ const k = !false ++ x := k //@codeaction("!false", "refactor.extract.constant", edit=scope2) diff --git a/internal/analysisinternal/analysis.go b/internal/analysisinternal/analysis.go index 4190f6d478d..fe67b0fa27a 100644 --- a/internal/analysisinternal/analysis.go +++ b/internal/analysisinternal/analysis.go @@ -65,8 +65,9 @@ func TypeErrorEndPos(fset *token.FileSet, src []byte, start token.Pos) token.Pos return end } -// StmtToInsertVarBefore returns the ast.Stmt before which we can safely insert a new variable. -// Some examples: +// StmtToInsertVarBefore returns the ast.Stmt before which we can +// safely insert a new var declaration, or nil if the path denotes a +// node outside any statement. // // Basic Example: // @@ -93,7 +94,7 @@ func StmtToInsertVarBefore(path []ast.Node) ast.Stmt { } } if enclosingIndex == -1 { - return nil + return nil // no enclosing statement: outside function } enclosingStmt := path[enclosingIndex] switch enclosingStmt.(type) { @@ -101,6 +102,9 @@ func StmtToInsertVarBefore(path []ast.Node) ast.Stmt { // The enclosingStmt is inside of the if declaration, // We need to check if we are in an else-if stmt and // get the base if statement. + // TODO(adonovan): for non-constants, it may be preferable + // to add the decl as the Init field of the innermost + // enclosing ast.IfStmt. return baseIfStmt(path, enclosingIndex) case *ast.CaseClause: // Get the enclosing switch stmt if the enclosingStmt is