[tools] gopls/internal/golang: fix extract function behavior with goto
0 views
Skip to first unread message
Madeline Kalil (Gerrit)
Jan 16, 2026, 11:44:16 AM (yesterday) Jan 16
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Madeline Kalil has uploaded the change for review![]( "Open in Gerrit")
Commit message
gopls/internal/golang: fix extract function behavior with goto
Consider an extracted block that contains at least one
free branch statement and also contains
a labeled goto statement whose label is inside
the extracted block. Currently, we incorrectly treat
this labeled goto as a free branch, resulting
in an invalid extraction.
(a free branch is a branch statement whose continuation lies
outside the extracted block -- for this case we implement
the extracted function using an additional return value
"ctrl" that specifies the control flow)
The previous logic was:
When we ast.Inspect the extracted block, we build a stack
of nodes encountered. When we encounter
a labeled branch statement, we determine if is a free
branch statement by figuring out whether the stack
contains the corresponding labeled statement. However, if the labeled statement
occurs after the branch statement within the same block statement,
we may not yet have examined it at this point, so the labeled statement
will be absent from the stack.
Instead of just looking for LabeledStmts on the stack, we should
search the entire extracted block for LabeledStmts.
Note about the extra semi colon:
Sometimes, we insert a semi colon after an extracted label.
When a label is extracted without its corresponding statement,
the AST representation is a LabeledStmt whose Stmt is an EmptyStmt.
The AST node printer adds a semicolon in this case.
(See https://github.com/golang/go/blob/532e3203492ebcac67b2f3aa2a52115f49d51997/src/go/printer/nodes.go#L1372)
While this is not always necessary, it doesn't result
in build errors so I will leave as is.
Change-Id: If458c9aba3117bafcb2dc4ebb13b78afcef514fc
Change diff
diff --git a/gopls/internal/golang/extract.go b/gopls/internal/golang/extract.go
index 2f44505..689f290 100644
--- a/gopls/internal/golang/extract.go
+++ b/gopls/internal/golang/extract.go
@@ -999,7 +999,7 @@
var branchStmts []*ast.BranchStmt
var stack []ast.Node
// Add the zero "ctrl" value to each return statement in the extracted block.
- ast.Inspect(extractedBlock, func(n ast.Node) bool {
+ astutil.PreorderStack(extractedBlock, stack, func(n ast.Node, stack []ast.Node) bool {
if n != nil {
stack = append(stack, n)
} else {
@@ -1010,14 +1010,11 @@
n.Results = append(n.Results, zeroValExpr)
case *ast.BranchStmt:
// Collect a list of branch statements in the extracted block to examine later.
- if isFreeBranchStmt(stack) {
+ if isFreeBranchStmt(stack, extractedBlock) {
branchStmts = append(branchStmts, n)
}
case *ast.FuncLit:
- // Don't descend into nested functions. When we return false
- // here, ast.Inspect does not give us a "pop" event when leaving
- // the subtree, so we need to pop here. (golang/go#73319)
- stack = stack[:len(stack)-1]
+ // Don't descend into nested functions.
return false
}
return true
@@ -1993,14 +1990,17 @@
// when we are examining the extracted block, since type information isn't
// available. We need to find the location of the label without using
// types.Info.
-func isFreeBranchStmt(stack []ast.Node) bool {
+// We treat goto and break/continue separately because while break/continue
+// must be used within a for, range, switch, or select, goto's use is
+// less restrictive within a function body.
+func isFreeBranchStmt(stack []ast.Node, extractedBlock *ast.BlockStmt) bool {
switch node := stack[len(stack)-1].(type) {
case *ast.BranchStmt:
isLabeled := node.Label != nil
switch node.Tok {
case token.GOTO:
if isLabeled {
- return !enclosingLabel(stack, node.Label.Name)
+ return !enclosingLabel(extractedBlock, node.Label.Name)
}
case token.BREAK, token.CONTINUE:
// Find innermost relevant ancestor for break/continue.
@@ -2023,10 +2023,10 @@
return true
}
-// enclosingLabel returns true if the given label is found on the stack.
-func enclosingLabel(stack []ast.Node, label string) bool {
- for _, n := range stack {
- if labelStmt, ok := n.(*ast.LabeledStmt); ok && labelStmt.Label.Name == label {
+// enclosingLabel returns true if the given label is found in the block.
+func enclosingLabel(block *ast.BlockStmt, label string) bool {
+ for _, stmt := range block.List {
+ if l, ok := stmt.(*ast.LabeledStmt); ok && l.Label != nil && l.Label.Name == label {
return true
}
}
diff --git a/gopls/internal/test/marker/testdata/codeaction/extract_control_label.txt b/gopls/internal/test/marker/testdata/codeaction/extract_control_label.txt
new file mode 100644
index 0000000..8552ac0
--- /dev/null
+++ b/gopls/internal/test/marker/testdata/codeaction/extract_control_label.txt
@@ -0,0 +1,83 @@
+This test verifies the behavior of extract function when the extracted block contains:
+- At least one free branch statement
+- A labeled branch statement and its label, with or without the corresponding
+labeled statement.
+See golang/go#77157
+
+-- flags --
+-ignore_extra_diags
+
+-- go.mod --
+module mod.test/extract
+
+go 1.18
+
+
+-- extractwithlabel.go --
+package extract
+
+//@codeaction(extractWithLabelOnly, "refactor.extract.function", edit=labeled1)
+//@codeaction(extractWithLabelAndStmt, "refactor.extract.function", edit=labeled2)
+
+func FuncGoTo(a int) {
+ for range "abc" {
+ if a > 5 { //@ loc(extractWithLabelOnly, re`(?s)if.*println.1.`), loc(extractWithLabelAndStmt, re`(?s)if.*label1:`)
+ continue
+ }
+ if a == 10 {
+ goto label1
+ }
+ label1:
+ println(1)
+ }
+}
+
+-- @labeled1/extractwithlabel.go --
+@@ -8 +8,3 @@
+- if a > 5 { //@ loc(extractWithLabelOnly, re`(?s)if.*println.1.`), loc(extractWithLabelAndStmt, re`(?s)if.*label1:`)
++ ctrl := newFunction(a)
++ switch ctrl {
++ case 1:
+@@ -11,5 +13 @@
+- if a == 10 {
+- goto label1
+- }
+- label1:
+- println(1)
+@@ -19 +16,12 @@
++func newFunction(a int) int {
++ if a > 5 { //@ loc(extractWithLabelOnly, re`(?s)if.*println.1.`), loc(extractWithLabelAndStmt, re`(?s)if.*label1:`)
++ return 1
++ }
++ if a == 10 {
++ goto label1
++ }
++label1:
++ println(1)
++ return 0
++}
++
+-- @labeled2/extractwithlabel.go --
+@@ -8 +8,3 @@
+- if a > 5 { //@ loc(extractWithLabelOnly, re`(?s)if.*println.1.`), loc(extractWithLabelAndStmt, re`(?s)if.*label1:`)
++ ctrl := newFunction(a)
++ switch ctrl {
++ case 1:
+@@ -11,4 +13 @@
+- if a == 10 {
+- goto label1
+- }
+- label1:
+@@ -19 +17,12 @@
++func newFunction(a int) int {
++ if a > 5 { //@ loc(extractWithLabelOnly, re`(?s)if.*println.1.`), loc(extractWithLabelAndStmt, re`(?s)if.*label1:`)
++ return 1
++ }
++ if a == 10 {
++ goto label1
++ }
++label1:
++ ;
++ return 0
++}
++
Change information
Files:
- M gopls/internal/golang/extract.go
- A gopls/internal/test/marker/testdata/codeaction/extract_control_label.txt
Change size: M
Delta: 2 files changed, 95 insertions(+), 12 deletions(-)
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Jan 16, 2026, 11:49:20 AM (yesterday) Jan 16
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Madeline Kalil uploaded patch set #2 to this change.
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Jan 16, 2026, 11:54:02 AM (yesterday) Jan 16
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Madeline Kalil uploaded patch set #3 to this change.
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Jan 16, 2026, 11:54:36 AM (yesterday) Jan 16
to goph...@pubsubhelper.golang.org, Alan Donovan, golang-co...@googlegroups.com
Attention needed from Alan Donovan
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Jan 16, 2026, 1:26:08 PM (yesterday) Jan 16
to Madeline Kalil, goph...@pubsubhelper.golang.org, Go LUCI, golang-co...@googlegroups.com
Attention needed from Madeline Kalil
Alan Donovan added 5 comments![]( "Open in Gerrit")
File gopls/internal/golang/extract.go
Line 1002, Patchset 3 (Latest):
astutil.PreorderStack(extractedBlock, stack, func(n ast.Node, stack []ast.Node) bool {Alan Donovan . unresolved
nil
Line 1003, Patchset 3 (Latest):
if n != nil {
stack = append(stack, n)
} else {
stack = stack[:len(stack)-1]
}Alan Donovan . unresolved
Delete (PreorderStack maintains the stack so you don't have to).
But note that, unlike Inspect, PreorderStack's stack excludes n itself, so if isFreeBranchStmt needs the current node, you'll have to pass n to it as an additional parameter and adjust the indices. (Mutating the stack at L1004 simulates the previous behavior, but it's rather subtle. It would be clearer to call `isFreeBranchStmt(append(stack, n))`.)
// enclosingLabel returns true if the given label is found in the block.
func enclosingLabel(block *ast.BlockStmt, label string) bool {
for _, stmt := range block.List {
if l, ok := stmt.(*ast.LabeledStmt); ok && l.Label != nil && l.Label.Name == label {
return true
}
}
return false
}
Alan Donovan . unresolved
Let's inline this into the GOTO case since its name no longer reflects its behavior and it's really tailor-made for isFreeBranchStmt.
Related details
Attention is currently required from:
- Madeline Kalil
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Jan 16, 2026, 2:06:50 PM (yesterday) Jan 16
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Madeline Kalil
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Madeline Kalil uploaded patch set #4 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Attention is currently required from:
- Madeline Kalil
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Jan 16, 2026, 2:07:51 PM (yesterday) Jan 16
to goph...@pubsubhelper.golang.org, Go LUCI, Alan Donovan, golang-co...@googlegroups.com
Attention needed from Alan Donovan
Madeline Kalil voted and added 5 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
5 comments
File gopls/internal/golang/extract.go
Line 1002, Patchset 3:
astutil.PreorderStack(extractedBlock, stack, func(n ast.Node, stack []ast.Node) bool {Alan Donovan . resolved
Madeline Kalilnil
Done
This is now always true.
Madeline Kalil
Acknowledged
Line 1003, Patchset 3:
if n != nil {
stack = append(stack, n)
} else {
stack = stack[:len(stack)-1]
}Alan Donovan . resolved
Delete (PreorderStack maintains the stack so you don't have to).
But note that, unlike Inspect, PreorderStack's stack excludes n itself, so if isFreeBranchStmt needs the current node, you'll have to pass n to it as an additional parameter and adjust the indices. (Mutating the stack at L1004 simulates the previous behavior, but it's rather subtle. It would be clearer to call `isFreeBranchStmt(append(stack, n))`.)
Madeline Kalil
Yep, isFreeBranchStmt does require the current node, thanks
// enclosingLabel returns true if the given label is found in the block.
func enclosingLabel(block *ast.BlockStmt, label string) bool {
for _, stmt := range block.List {
if l, ok := stmt.(*ast.LabeledStmt); ok && l.Label != nil && l.Label.Name == label {
return true
}
}
return false
}
Alan Donovan . resolved
Let's inline this into the GOTO case since its name no longer reflects its behavior and it's really tailor-made for isFreeBranchStmt.
Attention is currently required from:
- Alan Donovan
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Jan 16, 2026, 2:32:16 PM (yesterday) Jan 16
to Madeline Kalil, goph...@pubsubhelper.golang.org, Go LUCI, golang-co...@googlegroups.com
Attention needed from Madeline Kalil
Alan Donovan voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alan Donovan
| Code-Review | +2 |
2 comments
Patchset-level comments
File gopls/internal/golang/extract.go
Line 1983, Patchset 4 (Latest):
// statement at stack[len(stack)-1] cannot be found in the stack. This is usedAlan Donovan . unresolved
...the stack (for break/continue) or the extracted block (for goto).
Related details
Attention is currently required from:
- Madeline Kalil
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages