go/analysis/passes/modernize: rangeint: handle usages of loop label

Currently, when a label is used, the rangeint modernizer fails to
properly detect when i is used after a loop. This is because we only
search for i usage within the for loop's parent subtree. When a label is
used, the label becomes the loop's parent, and i can only be found in
the loop's loop's parent subtree.

To fix this, this change makes the rangeint modernizer look for i usage
in the first ancestor of a for loop that is not a label.

Fixes golang/go#76470

Change-Id: I79395efba26ad689514b7841d343932448ea9318
Reviewed-on: https://go-review.googlesource.com/c/tools/+/724960
Auto-Submit: Nicholas Husin <[email protected]>
Reviewed-by: Nicholas Husin <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Josh Rickmar <[email protected]>

Author: nicholashusin

go/analysis/passes/modernize/rangeint.go

@@ -161,7 +161,22 @@ func rangeint(pass *analysis.Pass) (any, error) {
 						// don't offer a fix, as a range loop
 						// leaves i with a different final value (limit-1).
 						if init.Tok == token.ASSIGN {
-							for curId := range curLoop.Parent().Preorder((*ast.Ident)(nil)) {
+							// Find the nearest ancestor that is not a label.
+							// Otherwise, checking for i usage outside of a for
+							// loop might not function properly further below.
+							// This is because the i usage might be a child of
+							// the loop's parent's parent, for example:
+							//     var i int
+							// Loop:
+							//     for i = 0; i < 10; i++ { break loop }
+							//     // i is in the sibling of the label, not the loop
+							//     fmt.Println(i)
+							//
+							ancestor := curLoop.Parent()
+							for is[*ast.LabeledStmt](ancestor.Node()) {
+								ancestor = ancestor.Parent()
+							}
+							for curId := range ancestor.Preorder((*ast.Ident)(nil)) {
 								id := curId.Node().(*ast.Ident)
 								if info.Uses[id] == v {
 									// Is i used after loop?

go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go

@@ -271,3 +271,19 @@ func issue74687() {
 		println(i)
 	}
 }
+
+func issue76470() {
+	var i, j int
+UsedAfter:
+	for i = 0; i < 10; i++ { // nope: i is accessed after the loop
+		break UsedAfter
+	}
+	if i == 9 {
+		panic("Modernizer changes behavior")
+	}
+
+NotUsedAfter:
+	for j = 0; j < 10; j++ { // want "for loop can be modernized using range over int"
+		break NotUsedAfter
+	}
+}

go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go.golden

@@ -271,3 +271,18 @@ func issue74687() {
 	}
 }
 
+func issue76470() {
+	var i, j int
+UsedAfter:
+	for i = 0; i < 10; i++ { // nope: i is accessed after the loop
+		break UsedAfter
+	}
+	if i == 9 {
+		panic("Modernizer changes behavior")
+	}
+
+NotUsedAfter:
+	for j = range(10) { // want "for loop can be modernized using range over int"
+		break NotUsedAfter
+	}
+}