go/analysis/passes/modernize: fix stringscut false positive for unguarded afterSlice

After i = strings.Index(s, ":"), with a single-byte separator,
it is common to use s[i+1:], regardless of the sign of i,
as a safe way to express "the part after the colon, or the
whole string if missing".

However, the modernizer transformation to:

    before, after, _ = strings.Cut(s, ":")
    ...
    use(after)

is not sound when i < 0 case because Cut returns after == ""
in this case.

The solution is to apply the transformation only when the s[i+1:]
expression is dominated by a check that i is non-negative.

Fixes golang/go#77737

Change-Id: Iaf0238e1fbcaf70ba3b4afe571474cfbb467b3e4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/747900
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Madeline Kalil <mkalil@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>

Author: francisco3ferraz

go/analysis/passes/modernize/stringscut.go

@@ -183,7 +183,7 @@ func stringscut(pass *analysis.Pass) (any, error) {
 			// len(substr)]), then we can replace the call to Index()
 			// with a call to Cut() and use the returned ok, before,
 			// and after variables accordingly.
-			negative, nonnegative, beforeSlice, afterSlice := checkIdxUses(pass.TypesInfo, index.Uses(iObj), s, substr)
+			negative, nonnegative, beforeSlice, afterSlice := checkIdxUses(pass.TypesInfo, index.Uses(iObj), s, substr, iObj)
 
 			// Either there are no uses of before, after, or ok, or some use
 			// of i does not match our criteria - don't suggest a fix.
@@ -374,14 +374,31 @@ func indexArgValid(info *types.Info, index *typeindex.Index, expr ast.Expr, afte
 // 2. nonnegative - a condition equivalent to i >= 0
 // 3. beforeSlice - a slice of `s` that matches either s[:i], s[0:i]
 // 4. afterSlice - a slice of `s` that matches one of: s[i+len(substr):], s[len(substr) + i:], s[i + const], s[k + i] (where k = len(substr))
-func checkIdxUses(info *types.Info, uses iter.Seq[inspector.Cursor], s, substr ast.Expr) (negative, nonnegative, beforeSlice, afterSlice []ast.Expr) {
+//
+// Additionally, all beforeSlice and afterSlice uses must be dominated by a
+// nonnegative guard on i (i.e., inside the body of an if whose condition
+// checks i >= 0, or in the else of a negative check, or after an
+// early-return negative check). This ensures that the rewrite from
+// s[i+len(sep):] to "after" preserves semantics, since when i == -1,
+// s[i+len(sep):] may yield a valid substring (e.g. s[0:] for single-byte
+// separators), but "after" would be "".
+//
+// When len(substr)==1, it's safe to use s[i+1:] even when i < 0.
+// Otherwise, each replacement of s[i+1:] must be guarded by a check
+// that i is nonnegative.
+func checkIdxUses(info *types.Info, uses iter.Seq[inspector.Cursor], s, substr ast.Expr, iObj types.Object) (negative, nonnegative, beforeSlice, afterSlice []ast.Expr) {
+	requireGuard := true
+	if l := constSubstrLen(info, substr); l != -1 && l != 1 {
+		requireGuard = false
+	}
+
 	use := func(cur inspector.Cursor) bool {
 		ek := cur.ParentEdgeKind()
 		n := cur.Parent().Node()
 		switch ek {
 		case edge.BinaryExpr_X, edge.BinaryExpr_Y:
 			check := n.(*ast.BinaryExpr)
-			switch checkIdxComparison(info, check) {
+			switch checkIdxComparison(info, check, iObj) {
 			case -1:
 				negative = append(negative, check)
 				return true
@@ -397,10 +414,10 @@ func checkIdxUses(info *types.Info, uses iter.Seq[inspector.Cursor], s, substr a
 			if slice, ok := cur.Parent().Parent().Node().(*ast.SliceExpr); ok &&
 				sameObject(info, s, slice.X) &&
 				slice.Max == nil {
-				if isBeforeSlice(info, ek, slice) {
+				if isBeforeSlice(info, ek, slice) && (!requireGuard || isSliceIndexGuarded(info, cur, iObj)) {
 					beforeSlice = append(beforeSlice, slice)
 					return true
-				} else if isAfterSlice(info, ek, slice, substr) {
+				} else if isAfterSlice(info, ek, slice, substr) && (!requireGuard || isSliceIndexGuarded(info, cur, iObj)) {
 					afterSlice = append(afterSlice, slice)
 					return true
 				}
@@ -410,10 +427,10 @@ func checkIdxUses(info *types.Info, uses iter.Seq[inspector.Cursor], s, substr a
 			// Check that the thing being sliced is s and that the slice doesn't
 			// have a max index.
 			if sameObject(info, s, slice.X) && slice.Max == nil {
-				if isBeforeSlice(info, ek, slice) {
+				if isBeforeSlice(info, ek, slice) && (!requireGuard || isSliceIndexGuarded(info, cur, iObj)) {
 					beforeSlice = append(beforeSlice, slice)
 					return true
-				} else if isAfterSlice(info, ek, slice, substr) {
+				} else if isAfterSlice(info, ek, slice, substr) && (!requireGuard || isSliceIndexGuarded(info, cur, iObj)) {
 					afterSlice = append(afterSlice, slice)
 					return true
 				}
@@ -465,8 +482,15 @@ func hasModifyingUses(info *types.Info, uses iter.Seq[inspector.Cursor], afterPo
 // Since strings.Index returns exactly -1 if the substring is not found, we
 // don't need to handle expressions like i <= -3.
 // We return 0 if the expression does not match any of these options.
-// We assume that a check passed to checkIdxComparison has i as one of its operands.
-func checkIdxComparison(info *types.Info, check *ast.BinaryExpr) int {
+func checkIdxComparison(info *types.Info, check *ast.BinaryExpr, iObj types.Object) int {
+	isI := func(e ast.Expr) bool {
+		id, ok := e.(*ast.Ident)
+		return ok && info.Uses[id] == iObj
+	}
+	if !isI(check.X) && !isI(check.Y) {
+		return 0
+	}
+
 	// Ensure that the constant (if any) is on the right.
 	x, op, y := check.X, check.Op, check.Y
 	if info.Types[x].Value != nil {
@@ -515,44 +539,49 @@ func isBeforeSlice(info *types.Info, ek edge.Kind, slice *ast.SliceExpr) bool {
 	return ek == edge.SliceExpr_High && (slice.Low == nil || isZeroIntConst(info, slice.Low))
 }
 
-// isAfterSlice reports whether the SliceExpr is of the form s[i+len(substr):],
-// or s[i + k:] where k is a const is equal to len(substr).
-func isAfterSlice(info *types.Info, ek edge.Kind, slice *ast.SliceExpr, substr ast.Expr) bool {
-	lowExpr, ok := slice.Low.(*ast.BinaryExpr)
-	if !ok || slice.High != nil {
-		return false
-	}
-	// Returns true if the expression is a call to len(substr).
-	isLenCall := func(expr ast.Expr) bool {
-		call, ok := expr.(*ast.CallExpr)
-		if !ok || len(call.Args) != 1 {
-			return false
-		}
-		return sameObject(info, substr, call.Args[0]) && typeutil.Callee(info, call) == builtinLen
-	}
-
+// constSubstrLen returns the constant length of substr, or -1 if unknown.
+func constSubstrLen(info *types.Info, substr ast.Expr) int {
 	// Handle len([]byte(substr))
-	if is[*ast.CallExpr](substr) {
-		call := substr.(*ast.CallExpr)
+	if call, ok := substr.(*ast.CallExpr); ok {
 		tv := info.Types[call.Fun]
 		if tv.IsType() && types.Identical(tv.Type, byteSliceType) {
 			// Only one arg in []byte conversion.
 			substr = call.Args[0]
 		}
 	}
-	substrLen := -1
 	substrVal := info.Types[substr].Value
 	if substrVal != nil {
 		switch substrVal.Kind() {
 		case constant.String:
-			substrLen = len(constant.StringVal(substrVal))
+			return len(constant.StringVal(substrVal))
 		case constant.Int:
 			// constant.Value is a byte literal, e.g. bytes.IndexByte(_, 'a')
 			// or a numeric byte literal, e.g. bytes.IndexByte(_, 65)
-			substrLen = 1
+			// ([]byte(rune) is not legal.)
+			return 1
+		}
+	}
+	return -1
+}
+
+// isAfterSlice reports whether the SliceExpr is of the form s[i+len(substr):],
+// or s[i + k:] where k is a const is equal to len(substr).
+func isAfterSlice(info *types.Info, ek edge.Kind, slice *ast.SliceExpr, substr ast.Expr) bool {
+	lowExpr, ok := slice.Low.(*ast.BinaryExpr)
+	if !ok || slice.High != nil {
+		return false
+	}
+	// Returns true if the expression is a call to len(substr).
+	isLenCall := func(expr ast.Expr) bool {
+		call, ok := expr.(*ast.CallExpr)
+		if !ok || len(call.Args) != 1 {
+			return false
 		}
+		return sameObject(info, substr, call.Args[0]) && typeutil.Callee(info, call) == builtinLen
 	}
 
+	substrLen := constSubstrLen(info, substr)
+
 	switch ek {
 	case edge.BinaryExpr_X:
 		kVal := info.Types[lowExpr.Y].Value
@@ -578,6 +607,75 @@ func isAfterSlice(info *types.Info, ek edge.Kind, slice *ast.SliceExpr, substr a
 	return false
 }
 
+// isSliceIndexGuarded reports whether a use of the index variable i (at the given cursor)
+// inside a slice expression is dominated by a nonnegative guard.
+// A use is considered guarded if any of the following are true:
+//   - It is inside the Body of an IfStmt whose condition is a nonnegative check on i.
+//   - It is inside the Else of an IfStmt whose condition is a negative check on i.
+//   - It is preceded (in the same block) by an IfStmt whose condition is a
+//     negative check on i with a terminating body (e.g., early return).
+//
+// Conversely, a use is immediately rejected if:
+//   - It is inside the Body of an IfStmt whose condition is a negative check on i.
+//   - It is inside the Else of an IfStmt whose condition is a nonnegative check on i.
+//
+// We have already checked (see [hasModifyingUses]) that there are no
+// intervening uses (incl. via aliases) of i that might alter its value.
+func isSliceIndexGuarded(info *types.Info, cur inspector.Cursor, iObj types.Object) bool {
+	for anc := range cur.Enclosing() {
+		switch anc.ParentEdgeKind() {
+		case edge.IfStmt_Body, edge.IfStmt_Else:
+			ifStmt := anc.Parent().Node().(*ast.IfStmt)
+			check := condChecksIdx(info, ifStmt.Cond, iObj)
+			if anc.ParentEdgeKind() == edge.IfStmt_Else {
+				check = -check
+			}
+			if check > 0 {
+				return true // inside nonnegative-guarded block (i >= 0 here)
+			}
+			if check < 0 {
+				return false // inside negative-guarded block (i < 0 here)
+			}
+		case edge.BlockStmt_List:
+			// Check preceding siblings for early-return negative checks.
+			for sib, ok := anc.PrevSibling(); ok; sib, ok = sib.PrevSibling() {
+				ifStmt, ok := sib.Node().(*ast.IfStmt)
+				if ok && condChecksIdx(info, ifStmt.Cond, iObj) < 0 && bodyTerminates(ifStmt.Body) {
+					return true // preceded by early-return negative check
+				}
+			}
+		case edge.FuncDecl_Body, edge.FuncLit_Body:
+			return false // stop at function boundary
+		}
+	}
+	return false
+}
+
+// condChecksIdx reports whether cond is a BinaryExpr that checks
+// the index variable iObj for negativity or non-negativity.
+// Returns -1 for negative (e.g. i < 0), +1 for nonnegative (e.g. i >= 0), 0 otherwise.
+func condChecksIdx(info *types.Info, cond ast.Expr, iObj types.Object) int {
+	binExpr, ok := cond.(*ast.BinaryExpr)
+	if !ok {
+		return 0
+	}
+	return checkIdxComparison(info, binExpr, iObj)
+}
+
+// bodyTerminates reports whether the given block statement unconditionally
+// terminates execution (via return, break, continue, or goto).
+func bodyTerminates(block *ast.BlockStmt) bool {
+	if len(block.List) == 0 {
+		return false
+	}
+	last := block.List[len(block.List)-1]
+	switch last.(type) {
+	case *ast.ReturnStmt, *ast.BranchStmt:
+		return true // return, break, continue, goto
+	}
+	return false
+}
+
 // sameObject reports whether we know that the expressions resolve to the same object.
 func sameObject(info *types.Info, expr1, expr2 ast.Expr) bool {
 	if ident1, ok := expr1.(*ast.Ident); ok {

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

@@ -8,7 +8,9 @@ import (
 func basic(s string) bool {
 	s = "reassigned"
 	i := strings.Index(s, "=") // want "strings.Index can be simplified using strings.Cut"
-	print(s[:i])
+	if i >= 0 {
+		print(s[:i])
+	}
 	return i >= 0
 }
 
@@ -50,7 +52,9 @@ func skip_var_decl(s string) bool {
 
 func basic_substr_arg(s string, substr string) bool {
 	i := strings.Index(s, substr) // want "strings.Index can be simplified using strings.Cut"
-	print(s[i+len(substr):])
+	if i >= 0 {
+		print(s[i+len(substr):])
+	}
 	return i >= 0
 }
 
@@ -62,20 +66,26 @@ func wrong_len_arg(s string, substr string) bool {
 
 func basic_strings_byte(s string) bool {
 	i := strings.IndexByte(s, '+') // want "strings.IndexByte can be simplified using strings.Cut"
-	print(s[:i])
+	if i >= 0 {
+		print(s[:i])
+	}
 	return i >= 0
 }
 
 func basic_strings_byte_int(s string) bool {
 	i := strings.IndexByte(s, 55) // want "strings.IndexByte can be simplified using strings.Cut"
-	print(s[:i])
+	if i >= 0 {
+		print(s[:i])
+	}
 	return i >= 0
 }
 
 func basic_strings_byte_var(s string) bool {
 	b := byte('b')
 	i := strings.IndexByte(s, b) // want "strings.IndexByte can be simplified using strings.Cut"
-	print(s[:i])
+	if i >= 0 {
+		print(s[:i])
+	}
 	return i >= 0
 }
 
@@ -89,7 +99,7 @@ func basic_bytes(b []byte) []byte {
 }
 
 func basic_index_bytes(b []byte) string {
-	i := bytes.IndexByte(b, 's') // want "bytes.IndexByte can be simplified using bytes.Cut"
+	i := bytes.IndexByte(b, 's') // don't modernize: b[i+1:] in else is not guarded
 	if i >= 0 {
 		return string(b[:i])
 	} else {
@@ -99,14 +109,20 @@ func basic_index_bytes(b []byte) string {
 
 func const_substr_len(s string) bool {
 	i := strings.Index(s, "=") // want "strings.Index can be simplified using strings.Cut"
-	r := s[i+len("="):]
-	return len(r) > 0
+	if i >= 0 {
+		r := s[i+len("="):]
+		return len(r) > 0
+	}
+	return false
 }
 
 func const_for_len(s string) bool {
 	i := strings.Index(s, "=") // want "strings.Index can be simplified using strings.Cut"
-	r := s[i+1:]
-	return len(r) > 0
+	if i >= 0 {
+		r := s[i+1:]
+		return len(r) > 0
+	}
+	return false
 }
 
 func index(s string) bool {
@@ -154,7 +170,7 @@ func invalid_slice(s string) string {
 
 func index_and_before_after(s string) string {
 	substr := "="
-	i := strings.Index(s, substr) // want "strings.Index can be simplified using strings.Cut"
+	i := strings.Index(s, substr) // don't modernize: s[i+len(substr):] and s[len(substr)+i:] not nonneg-guarded
 	if i == -1 {
 		print("test")
 	}
@@ -174,7 +190,7 @@ func index_and_before_after(s string) string {
 }
 
 func idx_var_init(s string) (string, string) {
-	var idx = strings.Index(s, "=") // want "strings.Index can be simplified using strings.Cut"
+	var idx = strings.Index(s, "=") // don't modernize: s[0:idx] is not guarded
 	return s[0:idx], s
 }
 
@@ -219,7 +235,7 @@ func s_modified_no_params() string {
 func s_in_func_call() string {
 	s := "string"
 	substr := "substr"
-	idx := strings.Index(s, substr) // want "strings.Index can be simplified using strings.Cut"
+	idx := strings.Index(s, substr) // don't modernize: s[:idx] is not guarded
 	function(s)
 	return s[:idx]
 }
@@ -306,3 +322,51 @@ func reference_str(s *string) {}
 func reference_int(i *int) {}
 
 func blank() {}
+
+// Regression test for unguarded slice uses (https://go.dev/issue/77737).
+// The s[colon+1:] usage outside the "if" relies on -1+1=0 to return the full
+// string when the separator is absent. Rewriting to "after" would return "".
+func unguarded_after_slice(s string) (int, string) {
+	colon := strings.Index(s, ":")
+	if colon != -1 {
+		print(s[:colon])
+	}
+	return colon, s[colon+1:] // don't modernize: s[colon+1:] not guarded
+}
+
+// Same as above but with the guard using i < 0.
+func unguarded_after_slice_negcheck(s string) string {
+	i := strings.Index(s, ":")
+	if i < 0 {
+		print("not found")
+	}
+	return s[i+1:] // don't modernize: s[i+1:] not guarded
+}
+
+// Safe: both slice uses are inside the nonneg guard.
+func guarded_both_slices(s string) (string, string) {
+	i := strings.Index(s, ":") // want "strings.Index can be simplified using strings.Cut"
+	if i >= 0 {
+		return s[:i], s[i+1:]
+	}
+	return "", s
+}
+
+// Safe: slice uses after early-return negative check.
+func guarded_early_return(s string) (string, string) {
+	i := strings.Index(s, ":") // want "strings.Index can be simplified using strings.Cut"
+	if i < 0 {
+		return "", s
+	}
+	return s[:i], s[i+1:]
+}
+
+// Safe: slice uses in else of negative check.
+func guarded_neg_else(s string) (string, string) {
+	i := strings.Index(s, ":") // want "strings.Index can be simplified using strings.Cut"
+	if i == -1 {
+		return "", s
+	} else {
+		return s[:i], s[i+1:]
+	}
+}