[tools] gopls/internal/analysis/modernize: add strings.Cut modernizer
Madeline Kalil (Gerrit)
Madeline Kalil has uploaded the change for review![]( "Open in Gerrit")
Commit message
gopls/internal/analysis/modernize: add strings.Cut modernizer
This modernizer offers to replace instances of strings.Index,
strings.IndexByte, bytes.Index, and bytes.IndexByte, with either
strings.Cut or bytes.Cut. It only suggests a fix when the expressions
involving the returned index and the passed string and substring params
meet certain criteria.
Fixes golang/go#71369
Change diff
diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md
index 16a5875..6fa71d6 100644
--- a/gopls/doc/analyzers.md
+++ b/gopls/doc/analyzers.md
@@ -3954,6 +3954,33 @@
Package documentation: [stringsbuilder](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize#stringbuilder)
+<a id='stringscut'></a>
+## `stringscut`: replace strings.Index etc. with strings.Cut
+
+
+This analyzer replaces instances of strings.Index, strings.IndexByte, bytes.Index, and bytes.IndexByte with strings.Cut or bytes.Cut.
+
+For example:
+
+ idx := strings.Index(s, substr)
+ if i >= 0 {
+ return s[:idx]
+ }
+
+is replaced by:
+
+ before, _, ok := strings.Cut(s, substr)
+ if ok {
+ return before
+ }
+
+This analyzer requires that all uses of idx and s match forms
+that can be resolved by replacements with the before, after, and ok variables returned by strings.Cut. It also requires that are no modifications or potential modifications to idx, s, and substr.
+
+Default: on.
+
+Package documentation: [stringscut](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize#stringscut)
+
<a id='stringscutprefix'></a>
## `stringscutprefix`: replace HasPrefix/TrimPrefix with CutPrefix
diff --git a/gopls/internal/analysis/modernize/doc.go b/gopls/internal/analysis/modernize/doc.go
index 376c537..f9c314e 100644
--- a/gopls/internal/analysis/modernize/doc.go
+++ b/gopls/internal/analysis/modernize/doc.go
@@ -244,6 +244,29 @@
with the simpler `slices.Sort(s)`, which was added in Go 1.21.
+# Analyzer stringscut
+
+stringscut: replace strings.Index etc. with strings.Cut
+
+This analyzer replaces instances of strings.Index, strings.IndexByte, bytes.Index, and bytes.IndexByte with strings.Cut or bytes.Cut.
+
+For example:
+
+ idx := strings.Index(s, substr)
+ if i >= 0 {
+ return s[:idx]
+ }
+
+is replaced by:
+
+ before, _, ok := strings.Cut(s, substr)
+ if ok {
+ return before
+ }
+
+This analyzer requires that all uses of idx and s match forms
+that can be resolved by replacements with the before, after, and ok variables returned by strings.Cut. It also requires that are no modifications or potential modifications to idx, s, and substr.
+
# Analyzer stringscutprefix
stringscutprefix: replace HasPrefix/TrimPrefix with CutPrefix
diff --git a/gopls/internal/analysis/modernize/modernize.go b/gopls/internal/analysis/modernize/modernize.go
index 08cb58f..625bd07 100644
--- a/gopls/internal/analysis/modernize/modernize.go
+++ b/gopls/internal/analysis/modernize/modernize.go
@@ -45,6 +45,7 @@
// SlicesDeleteAnalyzer, // not nil-preserving!
SlicesSortAnalyzer,
StringsCutPrefixAnalyzer,
+ StringsCutAnalyzer,
StringsSeqAnalyzer,
StringsBuilderAnalyzer,
TestingContextAnalyzer,
diff --git a/gopls/internal/analysis/modernize/modernize_test.go b/gopls/internal/analysis/modernize/modernize_test.go
index aafaab8..0be8c3d 100644
--- a/gopls/internal/analysis/modernize/modernize_test.go
+++ b/gopls/internal/analysis/modernize/modernize_test.go
@@ -70,6 +70,10 @@
RunWithSuggestedFixes(t, TestData(), modernize.StringsBuilderAnalyzer, "stringsbuilder")
}
+func TestStringsCut(t *testing.T) {
+ RunWithSuggestedFixes(t, TestData(), modernize.StringsCutAnalyzer, "stringscut")
+}
+
func TestStringsCutPrefix(t *testing.T) {
RunWithSuggestedFixes(t, TestData(), modernize.StringsCutPrefixAnalyzer,
"stringscutprefix",
diff --git a/gopls/internal/analysis/modernize/stringscut.go b/gopls/internal/analysis/modernize/stringscut.go
new file mode 100644
index 0000000..baeb280
--- /dev/null
+++ b/gopls/internal/analysis/modernize/stringscut.go
@@ -0,0 +1,453 @@
+// Copyright 2025 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package modernize
+
+import (
+ "bytes"
+ "fmt"
+ "go/ast"
+ "go/format"
+ "go/token"
+ "go/types"
+ "iter"
+ "slices"
+ "strconv"
+
+ "golang.org/x/tools/go/analysis"
+ "golang.org/x/tools/go/analysis/passes/inspect"
+ "golang.org/x/tools/go/ast/inspector"
+ "golang.org/x/tools/go/types/typeutil"
+ "golang.org/x/tools/internal/analysisinternal"
+ "golang.org/x/tools/internal/analysisinternal/generated"
+ typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex"
+ "golang.org/x/tools/internal/typesinternal"
+ "golang.org/x/tools/internal/typesinternal/typeindex"
+)
+
+var StringsCutAnalyzer = &analysis.Analyzer{
+ Name: "stringscut",
+ Doc: analysisinternal.MustExtractDoc(doc, "stringscut"),
+ Requires: []*analysis.Analyzer{
+ generated.Analyzer,
+ inspect.Analyzer,
+ typeindexanalyzer.Analyzer,
+ },
+ Run: stringscut,
+ URL: "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize#stringscut",
+}
+
+// stringscut offers a fix to replace an occurence of strings.Index, strings.IndexByte, bytes.Index,
+// or bytes.IndexByte with strings.Cut or bytes.Cut.
+// Consider some candidate for replacement idx := strings.Index(s, substr). The following must hold for a replacement to occur:
+//
+// 1. All instances of idx and s must be in one of these forms.
+// Binary expressions:
+// (a): idx < 0, 0 > idx, idx == -1, -1 == idx
+// (b): idx >= 0, 0 <= idx
+//
+// Slice expressions:
+// a: s[:idx], s[0:idx]
+// b: s[idx+len(substr):], s[len(substr) + idx:], s[idx + const], s[const + idx] (where const = len(substr))
+//
+// 2. There can be no uses of s, substr, or idx where they are
+// potentially modified (i.e. in assignments, or function calls with unknown side
+// effects).
+//
+// Then, the replacement involves the following substitutions:
+// 1. Replace "idx := strings.Index(s, substr)" with "before, after, ok := strings.Cut(s, substr)"
+// 2. Replace instances of binary expressions (a) with !ok and binary expressions (b) with ok.
+// 3. Replace slice expressions (a) with "before" and slice expressions (b) with after.
+// 4. The assignments to before, after, and ok may use the blank identifier "_" if they are unused.
+
+func stringscut(pass *analysis.Pass) (any, error) {
+ skipGenerated(pass)
+ var (
+ inspect = pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+ index = pass.ResultOf[typeindexanalyzer.Analyzer].(*typeindex.Index)
+ info = pass.TypesInfo
+
+ stringsIndex = index.Object("strings", "Index")
+ stringsIndexByte = index.Object("strings", "IndexByte")
+ bytesIndex = index.Object("bytes", "Index")
+ bytesIndexByte = index.Object("bytes", "IndexByte")
+ )
+ if !index.Used(stringsIndex, stringsIndexByte, bytesIndex, bytesIndexByte) {
+ return nil, nil
+ }
+
+ for curFile := range filesUsing(inspect, info, "go1.18") {
+ for curCall := range curFile.Preorder((*ast.CallExpr)(nil)) {
+ indexCall := curCall.Node().(*ast.CallExpr)
+ obj := typeutil.Callee(info, indexCall)
+ if obj == nil {
+ continue
+ }
+ var (
+ isStringsIndex = analysisinternal.IsFunctionNamed(obj, "strings", "Index")
+ isBytesIndex = analysisinternal.IsFunctionNamed(obj, "bytes", "Index")
+ isStringsIndexByte = analysisinternal.IsFunctionNamed(obj, "strings", "IndexByte")
+ isBytesIndexByte = analysisinternal.IsFunctionNamed(obj, "bytes", "IndexByte")
+ )
+ if !(isStringsIndex || isBytesIndex || isStringsIndexByte || isBytesIndexByte) {
+ continue
+ }
+ // Check if assigning idx with := or =, since the method to extract
+ // the *ast.Ident corresponding to idx will differ depending on
+ // which assignment is used.
+ callParent := curCall.Parent()
+ assign, okAsgn := callParent.Node().(*ast.AssignStmt) // :=
+ decl, okDecl := callParent.Parent().Node().(*ast.GenDecl) // var _ =
+ if okAsgn || okDecl {
+ var (
+ idx *ast.Ident
+ ok bool
+ )
+ if okAsgn {
+ // TODO(mkalil): Handle multiple assign statements.
+ if len(assign.Lhs) != 1 {
+ continue
+ }
+ lhs := assign.Lhs[0]
+ idx, ok = lhs.(*ast.Ident)
+ if !ok {
+ continue
+ }
+ } else if okDecl {
+ // TODO(mkalil): Handle var declarations with multiple specs.
+ if len(decl.Specs) != 1 || decl.Tok != token.VAR {
+ continue
+ }
+ idx = decl.Specs[0].(*ast.ValueSpec).Names[0]
+ }
+
+ // Have idx := strings.Index(s, substr) or var idx = strings.Index(s, substr)
+ // (or bytes.Index, etc.)
+ //
+ // Next, examine all uses of idx. If the only uses are of the
+ // forms mentioned above (i.e. idx < 0, idx >= 0, s[:idx] and s[idx +
+ // len(substr)]), then we can replace the call to strings.Index
+ // with a call to strings.Cut and use the returned ok, before,
+ // and after variables accordingly.
+
+ // Get uses of idx.
+ idxObj := info.Defs[idx]
+ idxUses := index.Uses(idxObj)
+
+ s := indexCall.Args[0]
+ substr := indexCall.Args[1]
+ lessZero, greaterZero, beforeSlice, afterSlice := parseIdxUses(pass, idxUses, idx, s, substr, indexCall.Pos())
+
+ // Either there are no uses of before, after, or ok, or some use
+ // of idx does not match our criteria - don't suggest a fix.
+ if lessZero == nil && greaterZero == nil && beforeSlice == nil && afterSlice == nil {
+ continue
+ }
+
+ // Check that there are no statements that alter the value of s
+ // or substr after the call to Index(). s and substr could be
+ // idents or string/byte literals. If they are the latter, we don't
+ // need to check their uses since they cannot be modified.
+ afterPos := indexCall.Pos() // don't consider uses before the call to strings.Index etc.
+ ids := []ast.Expr{s, substr}
+ modified := false
+ for _, id := range ids {
+ if sId, ok := id.(*ast.Ident); ok {
+ sObj := info.Uses[sId]
+ sUses := index.Uses(sObj)
+ modified = hasModifyingUses(sUses, afterPos)
+ if modified {
+ break
+ }
+ }
+ }
+ if modified {
+ continue
+ }
+
+ scope := idxObj.Parent()
+ okVarName := analysisinternal.FreshName(scope, idx.Pos(), "ok")
+ beforeVarName := analysisinternal.FreshName(scope, idx.Pos(), "before")
+ afterVarName := analysisinternal.FreshName(scope, idx.Pos(), "after")
+
+ var textEdits []analysis.TextEdit
+ changes := [][]ast.Expr{lessZero, greaterZero, beforeSlice, afterSlice}
+ newText := []string{"!" + okVarName, okVarName, beforeVarName, afterVarName}
+ for i, exprs := range changes {
+ for _, expr := range exprs {
+ textEdits = append(textEdits, analysis.TextEdit{
+ Pos: expr.Pos(),
+ End: expr.End(),
+ NewText: []byte(newText[i]),
+ })
+ }
+ }
+
+ // Format s and substr.
+ var bufS bytes.Buffer
+ err := format.Node(&bufS, pass.Fset, s)
+ if err != nil {
+ return nil, err
+ }
+ var bufSubstr bytes.Buffer
+ err = format.Node(&bufSubstr, pass.Fset, substr)
+ if err != nil {
+ return nil, err
+ }
+
+ // If there will be no uses of ok, before, or after, use the
+ // blank identifier instead.
+ if len(lessZero) == 0 && len(greaterZero) == 0 {
+ okVarName = "_"
+ }
+ if len(beforeSlice) == 0 {
+ beforeVarName = "_"
+ }
+ if len(afterSlice) == 0 {
+ afterVarName = "_"
+ }
+
+ var bytesOrStrings string
+ if isBytesIndex || isBytesIndexByte {
+ bytesOrStrings = "bytes"
+ } else if isStringsIndex || isStringsIndexByte {
+ bytesOrStrings = "strings"
+ }
+
+ // Replace the function call depending on whether we have an assignment or var declaration.
+ var pos, end token.Pos
+ if okAsgn {
+ pos, end = assign.Pos(), assign.End()
+ textEdits = append(textEdits, analysis.TextEdit{
+ Pos: pos,
+ End: end,
+ NewText: fmt.Appendf(nil, "%s, %s, %s := %s.Cut(%s, %s)", beforeVarName, afterVarName, okVarName, bytesOrStrings, bufS.String(), bufSubstr.String()),
+ })
+ } else if okDecl {
+ pos, end = decl.Pos(), decl.End()
+ textEdits = append(textEdits, analysis.TextEdit{
+ Pos: pos,
+ End: end,
+ NewText: fmt.Appendf(nil, "var %s, %s, %s = %s.Cut(%s, %s)", beforeVarName, afterVarName, okVarName, bytesOrStrings, bufS.String(), bufSubstr.String()),
+ })
+ }
+
+ var replace, with string
+ if isBytesIndex {
+ replace, with = "bytes.Index", "bytes.Cut"
+ } else if isStringsIndex {
+ replace, with = "strings.Index", "strings.Cut"
+ } else if isBytesIndexByte {
+ replace, with = "bytes.IndexByte", "bytes.Cut"
+ } else if isStringsIndexByte {
+ replace, with = "strings.IndexByte", "strings.Cut"
+ }
+ pass.Report(analysis.Diagnostic{
+ Pos: pos,
+ End: end,
+ Message: fmt.Sprintf("%s can be replaced with %s", replace, with),
+ Category: "stringscut",
+ SuggestedFixes: []analysis.SuggestedFix{{
+ Message: fmt.Sprintf("%s can be replaced with %s", replace, with),
+ TextEdits: textEdits,
+ }},
+ })
+ }
+ }
+ }
+
+ return nil, nil
+}
+
+// If all uses of idx match one of the valid formats, return a list of occurences for each format.
+// If any of the uses do not match one of the formats, return nil for all values.
+// Only examine uses after the given token.Pos.
+func parseIdxUses(pass *analysis.Pass, uses iter.Seq[inspector.Cursor], idx *ast.Ident, s, substr ast.Expr, afterPos token.Pos) (lessZero, greaterZero, beforeSlice, afterSlice []ast.Expr) {
+ for cur := range uses {
+ for c := range cur.Enclosing((*ast.BinaryExpr)(nil), (*ast.SliceExpr)(nil), (*ast.AssignStmt)(nil), (*ast.CallExpr)(nil), (*ast.GenDecl)(nil)) {
+ n := c.Node()
+ if n.Pos() <= afterPos {
+ continue
+ }
+ switch n := n.(type) {
+ case *ast.BinaryExpr:
+ check := n
+ if isLessThanZeroCheck(check) {
+ lessZero = append(lessZero, check)
+ } else if isGreaterEqualToZeroCheck(check) {
+ greaterZero = append(greaterZero, check)
+ } else {
+ // Ok if part of SliceExpr, those will be checked elsewhere
+ if _, ok := c.Parent().Node().(*ast.SliceExpr); ok {
+ continue
+ }
+ return nil, nil, nil, nil
+ }
+ case *ast.SliceExpr:
+ slice := n
+ if !equalSyntax(slice.X, s) {
+ continue
+ }
+ if isBeforeSlice(slice, idx) {
+ beforeSlice = append(beforeSlice, slice)
+ } else if isAfterSlice(pass, slice, idx, substr) {
+ afterSlice = append(afterSlice, slice)
+ } else {
+ return nil, nil, nil, nil
+ }
+ case *ast.AssignStmt:
+ // If the value of idx is reassigned, don't suggest a fix.
+ assign := n
+ for _, expr := range assign.Lhs {
+ if equalSyntax(expr, idx) {
+ return nil, nil, nil, nil
+ }
+ }
+ // If an alias to idx is created, don't suggest a fix.
+ for _, expr := range assign.Rhs {
+ if equalSyntax(expr, idx) {
+ return nil, nil, nil, nil
+ }
+ }
+ case *ast.GenDecl:
+ // If an alias to idx is created, don't suggest a fix.
+ decl := n
+ for i, spec := range decl.Specs {
+ if valueSpec, ok := spec.(*ast.ValueSpec); ok {
+ if equalSyntax(valueSpec.Values[i], idx) {
+ return nil, nil, nil, nil
+ }
+ }
+ }
+ case *ast.CallExpr:
+ // Don't suggest a fix if idx is passed as an argument to some
+ // other function (unless it's the call to strings.Index).
+ call := n
+ for _, arg := range call.Args {
+ if equalSyntax(arg, idx) {
+ return nil, nil, nil, nil
+ }
+ }
+ }
+
+ }
+
+ }
+
+ return lessZero, greaterZero, beforeSlice, afterSlice
+}
+
+// Returns true if any of the uses involve modifying the ident like in an
+// assignment statement, or potential unknown side effects, like via passing the
+// ident to a function. Any use before the "afterPos" won't be considered.
+func hasModifyingUses(uses iter.Seq[inspector.Cursor], afterPos token.Pos) bool {
+ for use := range uses {
+ for cur := range use.Enclosing((*ast.CallExpr)(nil), (*ast.AssignStmt)(nil)) {
+ if cur.Node().Pos() <= afterPos {
+ continue
+ }
+ switch n := cur.Node().(type) {
+ case *ast.CallExpr:
+ // Allow len(substr) and string() conversion, but any other
+ // CallExpr involving the ident will result in not returning a
+ // quick fix.
+ if isCall(n, "len", "string") {
+ continue
+ } else {
+ return true
+ }
+ case *ast.AssignStmt:
+ if equalSyntax(n.Lhs[0], use.Node().(*ast.Ident)) {
+ return true
+ }
+ }
+ }
+ }
+ return false
+}
+
+// Returns true if the BinaryExpr is of the form idx < 0, 0 > idx, idx == -1, or
+// -1 == idx.
+func isLessThanZeroCheck(check *ast.BinaryExpr) bool {
+ return check.Op == token.LSS && typesinternal.IsZeroExpr(check.Y) || check.Op == token.GTR && typesinternal.IsZeroExpr(check.X) || (check.Op == token.EQL && (isNegativeOne(check.X) || isNegativeOne(check.Y)))
+}
+
+// Returns true if the expr is equivalent to -1.
+func isNegativeOne(expr ast.Expr) bool {
+ switch e := expr.(type) {
+ case *ast.UnaryExpr:
+ x, ok := e.X.(*ast.BasicLit)
+ return ok && e.Op == token.SUB && x.Value == "1"
+ default:
+ return false
+ }
+}
+
+// Returns true if the BinaryExpr is of the form idx >= 0 or 0 <= idx.
+func isGreaterEqualToZeroCheck(check *ast.BinaryExpr) bool {
+ return check.Op == token.GEQ && typesinternal.IsZeroExpr(check.Y) || check.Op == token.LEQ && typesinternal.IsZeroExpr(check.X)
+}
+
+// Returns true if the SliceExpr is of the form s[:idx] or s[0:idx].
+func isBeforeSlice(slice *ast.SliceExpr, idx ast.Expr) bool {
+ return equalSyntax(slice.High, idx) && (slice.Low == nil || typesinternal.IsZeroExpr(slice.Low))
+}
+
+// Returns true if the SliceExpr is of the form s[idx+len(substr):],
+// s[len(substr) + idx:], or s[idx + const] where const is equal to len(substr).
+func isAfterSlice(pass *analysis.Pass, slice *ast.SliceExpr, idx, substr ast.Expr) bool {
+ expr, ok := slice.Low.(*ast.BinaryExpr)
+ if !ok {
+ return false
+ }
+ // Handle s[idx + const] where substr is a basic lit
+ // substr could also be a call expr like []byte("str")
+ // TODO(mkalil): This does not handle other methods used to create byte slices
+ // like byte literals, or using make/append.
+ var substrLit *ast.BasicLit
+ switch n := substr.(type) {
+ case *ast.BasicLit:
+ substrLit = n
+ case *ast.CallExpr:
+ // Check if call matches []byte
+ tv := pass.TypesInfo.Types[n.Fun]
+ if tv.IsType() && types.Identical(tv.Type, byteSliceType) {
+ // Only one arg in []byte conversion.
+ if lit, ok := n.Args[0].(*ast.BasicLit); ok {
+ substrLit = lit
+ }
+ }
+ }
+
+ if substrLit != nil {
+ substr_len := len(substrLit.Value) - 2 //BasicLit includes beginning and ending quote
+ if lit, ok := expr.X.(*ast.BasicLit); ok && equalSyntax(expr.Y, idx) {
+ num, err := strconv.Atoi(lit.Value)
+ if err != nil {
+ return false
+ }
+ return num == substr_len
+ }
+ if lit, ok := expr.Y.(*ast.BasicLit); ok && equalSyntax(expr.X, idx) {
+ num, err := strconv.Atoi(lit.Value)
+ if err != nil {
+ return false
+ }
+ return num == substr_len
+ }
+ }
+
+ return ok && slice.High == nil && expr.Op == token.ADD &&
+ (equalSyntax(expr.X, idx) && isCall(expr.Y, "len") || equalSyntax(expr.Y, idx) && isCall(expr.X, "len"))
+}
+
+// Returns true if the call expression matches the form callName(substr) for any of the names provided.
+func isCall(expr ast.Expr, callName ...string) bool {
+ call, ok := expr.(*ast.CallExpr)
+ if !ok {
+ return false
+ }
+ ident, ok := call.Fun.(*ast.Ident)
+ return ok && len(call.Args) == 1 && slices.Contains(callName, ident.Name)
+}
diff --git a/gopls/internal/analysis/modernize/testdata/src/stringscut/stringscut.go b/gopls/internal/analysis/modernize/testdata/src/stringscut/stringscut.go
new file mode 100644
index 0000000..1c1dc69
--- /dev/null
+++ b/gopls/internal/analysis/modernize/testdata/src/stringscut/stringscut.go
@@ -0,0 +1,162 @@
+package stringscut
+
+import (
+ "bytes"
+ "strings"
+)
+
+func basic(s string) bool {
+ s = "reassigned"
+ i := strings.Index(s, "=") // want "strings.Index can be replaced with strings.Cut"
+ return i >= 0
+}
+
+func basic_strings_byte(s string) bool {
+ i := strings.IndexByte(s, '+') // want "strings.IndexByte can be replaced with strings.Cut"
+ return i >= 0
+}
+
+func basic_bytes(b []byte) []byte {
+ i := bytes.Index(b, []byte("str")) // want "bytes.Index can be replaced with bytes.Cut"
+ if i >= 0 {
+ return b[:i]
+ } else {
+ return b[i+3:]
+ }
+}
+
+func basic_index_bytes(b []byte) string {
+ i := bytes.IndexByte(b, 's') // want "bytes.IndexByte can be replaced with bytes.Cut"
+ if i >= 0 {
+ return string(b[:i])
+ } else {
+ return string(b[i+1:])
+ }
+}
+
+func const_substr_len(s string) bool {
+ i := strings.Index(s, "=") // want "strings.Index can be replaced with strings.Cut"
+ r := s[i+len("="):]
+ return len(r) > 0
+}
+
+func const_for_len(s string) bool {
+ i := strings.Index(s, "=") // want "strings.Index can be replaced with strings.Cut"
+ r := s[i+1:]
+ return len(r) > 0
+}
+
+func index(s string) bool {
+ i := strings.Index(s, "=") // want "strings.Index can be replaced with strings.Cut"
+ if i < 0 {
+ return false
+ }
+ if i >= 0 {
+ return true
+ }
+ return true
+}
+
+func index_flipped(s string) bool {
+ i := strings.Index(s, "=") // want "strings.Index can be replaced with strings.Cut"
+ if 0 > i {
+ return false
+ }
+ if 0 <= i {
+ return true
+ }
+ return true
+}
+
+func invalid_index(s string) bool {
+ i := strings.Index(s, "=") // don't modernize since i is used in an "invalid" binaryexpr
+ if 0 > i {
+ return false
+ }
+ if i < 10 {
+ return true
+ }
+ return true
+}
+
+func invalid_slice(s string) string {
+ i := strings.Index(s, "=") // don't modernize since i is used in an "invalid" slice index
+ if i >= 0 {
+ return s[i+4:]
+ }
+ return ""
+}
+
+func index_and_before_after(s string) string {
+ substr := "="
+ i := strings.Index(s, substr) // want "strings.Index can be replaced with strings.Cut"
+ if i == -1 {
+ print("test")
+ }
+ if i < 0 {
+ return ""
+ } else {
+ if i >= 0 {
+ return s[:i]
+ } else {
+ return s[i+len(substr):]
+ }
+ }
+ if -1 == i {
+ return s[len(substr)+i:]
+ }
+ return "final"
+}
+
+func idx_var_init(s string) (string, string) {
+ var idx = strings.Index(s, "=") // want "strings.Index can be replaced with strings.Cut"
+ return s[0:idx], s
+}
+
+func idx_reassigned(s string) string {
+ idx := strings.Index(s, "=") // don't modernize since idx gets reassigned
+ idx = 10
+ return s[:idx]
+}
+
+func idx_printed(s string) string {
+ idx := strings.Index(s, "=") // don't modernize since idx is used
+ print(idx)
+ return s[:idx]
+}
+
+func idx_aliased(s string) string {
+ idx := strings.Index(s, "=") // don't modernize since idx gets aliased
+ i := idx
+ return s[:i]
+}
+
+func idx_aliased_var(s string) string {
+ idx := strings.Index(s, "=") // don't modernize since idx gets aliased
+ var i = idx
+ print(i)
+ return s[:idx]
+}
+
+func s_modified(s string) string {
+ idx := strings.Index(s, "=") // don't modernize since s gets modified
+ s = "newstring"
+ return s[:idx]
+}
+
+func s_modified_no_params() string {
+ s := "string"
+ idx := strings.Index(s, "=") // don't modernize since s gets modified
+ s = "newstring"
+ return s[:idx]
+}
+
+func s_in_func_call() string {
+ s := "string"
+ substr := "substr"
+ idx := strings.Index(s, substr) // don't modernize since s may get modified
+ some_func_that_may_change_s(s)
+ return s[:idx]
+}
+
+func some_func_that_may_change_s(s string) {}
diff --git a/gopls/internal/analysis/modernize/testdata/src/stringscut/stringscut.go.golden b/gopls/internal/analysis/modernize/testdata/src/stringscut/stringscut.go.golden
new file mode 100644
index 0000000..297e6b0
--- /dev/null
+++ b/gopls/internal/analysis/modernize/testdata/src/stringscut/stringscut.go.golden
@@ -0,0 +1,162 @@
+package stringscut
+
+import (
+ "bytes"
+ "strings"
+)
+
+func basic(s string) bool {
+ s = "reassigned"
+ _, _, ok := strings.Cut(s, "=") // want "strings.Index can be replaced with strings.Cut"
+ return ok
+}
+
+func basic_strings_byte(s string) bool {
+ _, _, ok := strings.Cut(s, '+') // want "strings.IndexByte can be replaced with strings.Cut"
+ return ok
+}
+
+func basic_bytes(b []byte) []byte {
+ before, after, ok := bytes.Cut(b, []byte("str")) // want "bytes.Index can be replaced with bytes.Cut"
+ if ok {
+ return before
+ } else {
+ return after
+ }
+}
+
+func basic_index_bytes(b []byte) string {
+ before, after, ok := bytes.Cut(b, 's') // want "bytes.IndexByte can be replaced with bytes.Cut"
+ if ok {
+ return string(before)
+ } else {
+ return string(after)
+ }
+}
+
+func const_substr_len(s string) bool {
+ _, after, _ := strings.Cut(s, "=") // want "strings.Index can be replaced with strings.Cut"
+ r := after
+ return len(r) > 0
+}
+
+func const_for_len(s string) bool {
+ _, after, _ := strings.Cut(s, "=") // want "strings.Index can be replaced with strings.Cut"
+ r := after
+ return len(r) > 0
+}
+
+func index(s string) bool {
+ _, _, ok := strings.Cut(s, "=") // want "strings.Index can be replaced with strings.Cut"
+ if !ok {
+ return false
+ }
+ if ok {
+ return true
+ }
+ return true
+}
+
+func index_flipped(s string) bool {
+ _, _, ok := strings.Cut(s, "=") // want "strings.Index can be replaced with strings.Cut"
+ if !ok {
+ return false
+ }
+ if ok {
+ return true
+ }
+ return true
+}
+
+func invalid_index(s string) bool {
+ i := strings.Index(s, "=") // don't modernize since i is used in an "invalid" binaryexpr
+ if 0 > i {
+ return false
+ }
+ if i < 10 {
+ return true
+ }
+ return true
+}
+
+func invalid_slice(s string) string {
+ i := strings.Index(s, "=") // don't modernize since i is used in an "invalid" slice index
+ if i >= 0 {
+ return s[i+4:]
+ }
+ return ""
+}
+
+func index_and_before_after(s string) string {
+ substr := "="
+ before, after, ok := strings.Cut(s, substr) // want "strings.Index can be replaced with strings.Cut"
+ if !ok {
+ print("test")
+ }
+ if !ok {
+ return ""
+ } else {
+ if ok {
+ return before
+ } else {
+ return after
+ }
+ }
+ if !ok {
+ return after
+ }
+ return "final"
+}
+
+func idx_var_init(s string) (string, string) {
+ var before, _, _ = strings.Cut(s, "=") // want "strings.Index can be replaced with strings.Cut"
+ return before, s
+}
+
+func idx_reassigned(s string) string {
+ idx := strings.Index(s, "=") // don't modernize since idx gets reassigned
+ idx = 10
+ return s[:idx]
+}
+
+func idx_printed(s string) string {
+ idx := strings.Index(s, "=") // don't modernize since idx is used
+ print(idx)
+ return s[:idx]
+}
+
+func idx_aliased(s string) string {
+ idx := strings.Index(s, "=") // don't modernize since idx gets aliased
+ i := idx
+ return s[:i]
+}
+
+func idx_aliased_var(s string) string {
+ idx := strings.Index(s, "=") // don't modernize since idx gets aliased
+ var i = idx
+ print(i)
+ return s[:idx]
+}
+
+func s_modified(s string) string {
+ idx := strings.Index(s, "=") // don't modernize since s gets modified
+ s = "newstring"
+ return s[:idx]
+}
+
+func s_modified_no_params() string {
+ s := "string"
+ idx := strings.Index(s, "=") // don't modernize since s gets modified
+ s = "newstring"
+ return s[:idx]
+}
+
+func s_in_func_call() string {
+ s := "string"
+ substr := "substr"
+ idx := strings.Index(s, substr) // don't modernize since s may get modified
+ some_func_that_may_change_s(s)
+ return s[:idx]
+}
+
+func some_func_that_may_change_s(s string) {}
diff --git a/gopls/internal/doc/api.json b/gopls/internal/doc/api.json
index 08b11d3..e9698d9 100644
--- a/gopls/internal/doc/api.json
+++ b/gopls/internal/doc/api.json
@@ -1683,6 +1683,12 @@
"Status": ""
},
{
+ "Name": "\"stringscut\"",
+ "Doc": "replace strings.Index etc. with strings.Cut\n\nThis analyzer replaces instances of strings.Index, strings.IndexByte, bytes.Index, and bytes.IndexByte with strings.Cut or bytes.Cut.\n\nFor example:\n\n\tidx := strings.Index(s, substr)\n\tif i \u003e= 0 {\n\t return s[:idx]\n\t}\n\nis replaced by:\n\n\tbefore, _, ok := strings.Cut(s, substr)\n\tif ok {\n\t return before\n\t}\n\nThis analyzer requires that all uses of idx and s match forms\nthat can be resolved by replacements with the before, after, and ok variables returned by strings.Cut. It also requires that are no modifications or potential modifications to idx, s, and substr.",
+ "Default": "true",
+ "Status": ""
+ },
+ {
"Name": "\"stringscutprefix\"",
"Doc": "replace HasPrefix/TrimPrefix with CutPrefix\n\nThe stringscutprefix analyzer simplifies a common pattern where code first\nchecks for a prefix with `strings.HasPrefix` and then removes it with\n`strings.TrimPrefix`. It replaces this two-step process with a single call\nto `strings.CutPrefix`, introduced in Go 1.20. The analyzer also handles\nthe equivalent functions in the `bytes` package.\n\nFor example, this input:\n\n\tif strings.HasPrefix(s, prefix) {\n\t use(strings.TrimPrefix(s, prefix))\n\t}\n\nis fixed to:\n\n\tif after, ok := strings.CutPrefix(s, prefix); ok {\n\t use(after)\n\t}\n\nThe analyzer also offers fixes to use CutSuffix in a similar way.\nThis input:\n\n\tif strings.HasSuffix(s, suffix) {\n\t use(strings.TrimSuffix(s, suffix))\n\t}\n\nis fixed to:\n\n\tif before, ok := strings.CutSuffix(s, suffix); ok {\n\t use(before)\n\t}",
"Default": "true",
@@ -3537,6 +3543,12 @@
"Default": true
},
{
+ "Name": "stringscut",
+ "Doc": "replace strings.Index etc. with strings.Cut\n\nThis analyzer replaces instances of strings.Index, strings.IndexByte, bytes.Index, and bytes.IndexByte with strings.Cut or bytes.Cut.\n\nFor example:\n\n\tidx := strings.Index(s, substr)\n\tif i \u003e= 0 {\n\t return s[:idx]\n\t}\n\nis replaced by:\n\n\tbefore, _, ok := strings.Cut(s, substr)\n\tif ok {\n\t return before\n\t}\n\nThis analyzer requires that all uses of idx and s match forms\nthat can be resolved by replacements with the before, after, and ok variables returned by strings.Cut. It also requires that are no modifications or potential modifications to idx, s, and substr.",
+ "URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize#stringscut",
+ "Default": true
+ },
+ {
"Name": "stringscutprefix",
"Doc": "replace HasPrefix/TrimPrefix with CutPrefix\n\nThe stringscutprefix analyzer simplifies a common pattern where code first\nchecks for a prefix with `strings.HasPrefix` and then removes it with\n`strings.TrimPrefix`. It replaces this two-step process with a single call\nto `strings.CutPrefix`, introduced in Go 1.20. The analyzer also handles\nthe equivalent functions in the `bytes` package.\n\nFor example, this input:\n\n\tif strings.HasPrefix(s, prefix) {\n\t use(strings.TrimPrefix(s, prefix))\n\t}\n\nis fixed to:\n\n\tif after, ok := strings.CutPrefix(s, prefix); ok {\n\t use(after)\n\t}\n\nThe analyzer also offers fixes to use CutSuffix in a similar way.\nThis input:\n\n\tif strings.HasSuffix(s, suffix) {\n\t use(strings.TrimSuffix(s, suffix))\n\t}\n\nis fixed to:\n\n\tif before, ok := strings.CutSuffix(s, suffix); ok {\n\t use(before)\n\t}",
"URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize#stringscutprefix",
diff --git a/gopls/internal/settings/analysis.go b/gopls/internal/settings/analysis.go
index 2bf700b..ab4d9d3 100644
--- a/gopls/internal/settings/analysis.go
+++ b/gopls/internal/settings/analysis.go
@@ -261,6 +261,7 @@
{analyzer: modernize.SlicesDeleteAnalyzer, severity: protocol.SeverityHint, nonDefault: true}, // not nil-preserving
{analyzer: modernize.SlicesSortAnalyzer, severity: protocol.SeverityHint},
{analyzer: modernize.StringsBuilderAnalyzer, severity: protocol.SeverityHint},
+ {analyzer: modernize.StringsCutAnalyzer, severity: protocol.SeverityHint},
{analyzer: modernize.StringsCutPrefixAnalyzer, severity: protocol.SeverityHint},
{analyzer: modernize.StringsSeqAnalyzer, severity: protocol.SeverityHint},
{analyzer: modernize.TestingContextAnalyzer, severity: protocol.SeverityHint},
Change information
- M gopls/doc/analyzers.md
- M gopls/internal/analysis/modernize/doc.go
- M gopls/internal/analysis/modernize/modernize.go
- M gopls/internal/analysis/modernize/modernize_test.go
- A gopls/internal/analysis/modernize/stringscut.go
- A gopls/internal/analysis/modernize/testdata/src/stringscut/stringscut.go
- A gopls/internal/analysis/modernize/testdata/src/stringscut/stringscut.go.golden
- M gopls/internal/doc/api.json
- M gopls/internal/settings/analysis.go
Related details
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)
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
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)
Alan Donovan added 32 comments![]( "Open in Gerrit")
Thanks for tackling this problem. Phew there's a lot of code here! I think we can reduce it somewhat and make the flow of concepts clearer by tweaking the boundaries between components a little.
# Analyzer stringscutThe modernize package has moved; you'll need to git mv gopls/internal/analysis/modernize/foo to go/analysis/passes/modernize/foo.
This analyzer replaces instances of strings.Index, strings.IndexByte, bytes.Index, and bytes.IndexByte with strings.Cut or bytes.Cut.How about:
This analyzer replaces certain patterns of use of [strings.Index] and string slicing by [strings.Cut], added in go1.18.
...code...
It also handles variants using [strings.IndexByte] instead of Index, or the bytes package instead of strings.
Fixes are offered only in cases in which there are no potential modifications of the i, s, or substr expressions between their definition and use.
if i >= 0 {idx
(but better to use i throughout)
// stringscut offers a fix to replace an occurence of strings.Index, strings.IndexByte, bytes.Index,occurrence
// b: s[idx+len(substr):], s[len(substr) + idx:], s[idx + const], s[const + idx] (where const = len(substr))k
(since const is a keyword)
// Then, the replacement involves the following substitutions:
// 1. Replace "idx := strings.Index(s, substr)" with "before, after, ok := strings.Cut(s, substr)"
// 2. Replace instances of binary expressions (a) with !ok and binary expressions (b) with ok.
// 3. Replace slice expressions (a) with "before" and slice expressions (b) with after.An annotated example would illustrate this whole comment more clearly. Present the basic transformation from
```
i := strings.Index(s, substr)
if i >= 0 {
use(s[:i], s[i:])
}
```
to:
```
before, after, ok := strings.Cut(s, substr)
if !ok {
use(s[:idx], s[0:idx]
}
```and then explain the principles of the variants, for example:
```
The condition must establish that i > -1. Variants include ...etc...
If the condition is negated (e.g. establishes `i < 0`), we use `if ok` instead.
```
This explanation suggests that there should be a function in the implementation that establishes whether `i > -1`, its negation, or neither. That argues for unifying isLessThanZeroCheck and isGreaterEqualToZeroCheck into a single function that returns -1, +1 or zero.
delete blank (so comment becomes a doc comment)
for curFile := range filesUsing(inspect, info, "go1.18") {Let's perform the version check at the last moment. Since the version filter is not expected to be very selective, it's more efficient to make a single pass over the whole package than one per file.
// Check file version.
file := enclosingFile(curCall)
if !fileUses(info, file, "go1.18") {
continue // new(expr) not available in this file
}
isStringsIndex = analysisinternal.IsFunctionNamed(obj, "strings", "Index")
isBytesIndex = analysisinternal.IsFunctionNamed(obj, "bytes", "Index")
isStringsIndexByte = analysisinternal.IsFunctionNamed(obj, "strings", "IndexByte")
isBytesIndexByte = analysisinternal.IsFunctionNamed(obj, "bytes", "IndexByte")
)
if !(isStringsIndex || isBytesIndex || isStringsIndexByte || isBytesIndexByte) {
continue
}Combine:
```
if !analysisinternal.IsFunctionNamed(obj, "strings", "Index", "IndexByte") &&
!analysisinternal.IsFunctionNamed(obj, "bytes", "Index", "IndexByte") {
```
callParent := curCall.Parent()You might find this code from CL 706635 is a more direct way to get the variable obj to which cur is assigned:
```
switch ek, idx := cur.ParentEdge(); ek {
case edge.ValueSpec_Values:
curName := cur.Parent().ChildAt(edge.ValueSpec_Names, idx)
if id, ok := curName.Node().(*ast.Ident); ok {
obj = info.Defs[id]
}
case edge.AssignStmt_Rhs:
curLhs := cur.Parent().ChildAt(edge.AssignStmt_Lhs, idx)
if id, ok := curLhs.Node().(*ast.Ident); ok {
obj = info.ObjectOf(id)
}
}
```
assign, okAsgn := callParent.Node().(*ast.AssignStmt) // :=:= or =
if okAsgn || okDecl {The body of this if statement is very long, without clear separation of steps. I don't mind long blocks of code (or long functions) so long as the sequence is obvious, but distinct tasks are interleaved here: the recognition of the various forms of `x = f()`, `x := f()`, `var x = f()` assignment and extraction of the LHS symbol; processing of the uses of x; checking for lvalue uses; editing the condition; and editing the assignment.
It would be great if we could separate these tasks more clearly. I notice that we are reformatting the s and substr expressions at L193. What we've learned from other modernizers is that the less we reformat, the better, since it loses comments and vertical space. If we can instead make more surgical edits, it's more likely to produce a cleaner result. For example, rather than rewriting
```
var i = string.Index(s, sub)
--------------------------------------------
var before, after, ok = string.Cut (s, sub)
```
we should aim instead for these edits:
```
var i = string.Index(s, sub)
----------------- -----
var before, after, ok = string.Cut (s, sub)
```
This means we only need to record the positions of the LHS identifier, and the SelectorExpr.Sel from the CallExpr.Fun, and then we can forget about the differences between var and := assignments, leading to a cleaner separation of steps.
idxObj := info.Defs[idx]idxVar := info.Defs[idx].(*types.Var)
// If all uses of idx match one of the valid formats, return a list of occurences for each format.What are the valid formats? Some orientation comments would help here.
func parseIdxUses(pass *analysis.Pass, uses iter.Seq[inspector.Cursor], idx *ast.Ident, s, substr ast.Expr, afterPos token.Pos) (lessZero, greaterZero, beforeSlice, afterSlice []ast.Expr) {"parse" isn't quite right here. (It may be in plain English, but in this domain it always refers to building a tree from a stream of tokens.) `check` would be better.
But we'll need to make sure all the names of stringscut helper functions are specific enough that they don't pollute the modernize package's namespace. The ideal names will hopefully emerge as we iterate on the code organization.
func parseIdxUses(pass *analysis.Pass, uses iter.Seq[inspector.Cursor], idx *ast.Ident, s, substr ast.Expr, afterPos token.Pos) (lessZero, greaterZero, beforeSlice, afterSlice []ast.Expr) {This should be *types.Var for a local variable; we shouldn't care about the syntax of idx any more. (The caller should pass idxObj aka idxVar.)
All of the uses of equalSyntax on idx should be using resolved symbols, i.e. checking whether the specific symbol is used, not just another symbol whose name happens to be spelled the same way.
for c := range cur.Enclosing((*ast.BinaryExpr)(nil), (*ast.SliceExpr)(nil), (*ast.AssignStmt)(nil), (*ast.CallExpr)(nil), (*ast.GenDecl)(nil)) {(I haven't studied this function yet.)
case *ast.CallExpr:
// Allow len(substr) and string() conversion, but any other
// CallExpr involving the ident will result in not returning a
// quick fix.
if isCall(n, "len", "string") {
continue
} else {
return true
}Passing a value to a function cannot modify it, only things referred to by that value. So for an int or a string (but not `[]byte`), we can ignore calls.
case *ast.AssignStmt:Assignments are not the only uses of variables; `&v` operations should also be assumed to (ultimately) mutate a variable. See the uses check in stringsbuilder for an example.
// Returns true if the BinaryExpr is of the form idx < 0, 0 > idx, idx == -1, orThe function doesn't actually check the "idx" operand, but it should. Do add test cases before you fix the bugs.
return check.Op == token.LSS && typesinternal.IsZeroExpr(check.Y) || check.Op == token.GTR && typesinternal.IsZeroExpr(check.X) || (check.Op == token.EQL && (isNegativeOne(check.X) || isNegativeOne(check.Y)))newline
// Returns true if the expr is equivalent to -1.Doc comments should be sentences with the declared name as their subject:
// isNegativeOne reports whether expr denotes -1.
// It may report false negatives.
return check.Op == token.GEQ && typesinternal.IsZeroExpr(check.Y) || check.Op == token.LEQ && typesinternal.IsZeroExpr(check.X)It would be more general to ask the type checker whether this expression denotes the constant zero, rather than whether it is spelled "0". We have a helper for that: isZeroIntLiteral.
(BTW, that function's name is a lie since it works on non-literal constants too. Let's rename it to isZeroIntConst.)
func isAfterSlice(pass *analysis.Pass, slice *ast.SliceExpr, idx, substr ast.Expr) bool {*types.Var
func isAfterSlice(pass *analysis.Pass, slice *ast.SliceExpr, idx, substr ast.Expr) bool {info *types.Info
return falseWe should also reject s[low:high:max] if max is present.
// Handle s[idx + const] where substr is a basic lit
// substr could also be a call expr like []byte("str")
// TODO(mkalil): This does not handle other methods used to create byte slices
// like byte literals, or using make/append.
var substrLit *ast.BasicLit
switch n := substr.(type) {
case *ast.BasicLit:
substrLit = n
case *ast.CallExpr:
// Check if call matches []byte
tv := pass.TypesInfo.Types[n.Fun]
if tv.IsType() && types.Identical(tv.Type, byteSliceType) {
// Only one arg in []byte conversion.
if lit, ok := n.Args[0].(*ast.BasicLit); ok {
substrLit = lit
}
}
}This concept seems like a useful function abstraction:
```
// stringConst returns the value of the constant string denoted by e, if any,
// and whether it is wrapped in a []byte("...") conversion.
func stringConst(info *types.Info, e ast.Expr) (k constant.Value, byteconv bool)
```
The implementation shouldn't use info.Types and needn't care about BasicLit.
substr_len := len(substrLit.Value) - 2 //BasicLit includes beginning and ending quoteThis isn't the correct way to compute the length of the string denoted by a string literal. Use strconv.Unquote.
However, we should be asking the type checker for the constant value of the string so that we don't need to rely on knowledge of how it was denoted (e.g. `"/"` versus `os.PathSeparator`). See L421.
func isCall(expr ast.Expr, callName ...string) bool {This name is too general for a function this specific at package scope. It should have a more specific name, or, better, be declared locally at L440 as isLenCall.
ident, ok := call.Fun.(*ast.Ident)
return ok && len(call.Args) == 1 && slices.Contains(callName, ident.Name)```
return typeutil.Callee(info, call) == builtinLen &&
is[*ast.Ident](call.Args[0]) &&
info.Uses(call.Args[0].(*ast.Ident)) == idx
```
and then you don't need equalSyntax.
Related details
- Madeline Kalil
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)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
var StringsCutAnalyzer = &analysis.Analyzer{Unexport? Since it should go through proposal review.
Related details
- Madeline Kalil
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)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Madeline Kalil
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)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Madeline Kalil
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)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Madeline Kalil
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)
Madeline Kalil voted and added 30 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
30 comments
Thank you very much for the review Alan, and sorry this CL is so big!
Unexport? Since it should go through proposal review.
Done
The modernize package has moved; you'll need to git mv gopls/internal/analysis/modernize/foo to go/analysis/passes/modernize/foo.
Done
This analyzer replaces instances of strings.Index, strings.IndexByte, bytes.Index, and bytes.IndexByte with strings.Cut or bytes.Cut.How about:
This analyzer replaces certain patterns of use of [strings.Index] and string slicing by [strings.Cut], added in go1.18.
...code...
It also handles variants using [strings.IndexByte] instead of Index, or the bytes package instead of strings.
Fixes are offered only in cases in which there are no potential modifications of the i, s, or substr expressions between their definition and use.
Done
idx
(but better to use i throughout)
Done
sort lines (move to L47)
Done
// stringscut offers a fix to replace an occurence of strings.Index, strings.IndexByte, bytes.Index,Madeline Kaliloccurrence
Done
// b: s[idx+len(substr):], s[len(substr) + idx:], s[idx + const], s[const + idx] (where const = len(substr))k
(since const is a keyword)
Done
// Then, the replacement involves the following substitutions:
// 1. Replace "idx := strings.Index(s, substr)" with "before, after, ok := strings.Cut(s, substr)"
// 2. Replace instances of binary expressions (a) with !ok and binary expressions (b) with ok.
// 3. Replace slice expressions (a) with "before" and slice expressions (b) with after.An annotated example would illustrate this whole comment more clearly. Present the basic transformation from
```
i := strings.Index(s, substr)
if i >= 0 {
use(s[:i], s[i:])
}
```
to:
```
before, after, ok := strings.Cut(s, substr)
if !ok {
use(s[:idx], s[0:idx]
}
```and then explain the principles of the variants, for example:
```
The condition must establish that i > -1. Variants include ...etc...
If the condition is negated (e.g. establishes `i < 0`), we use `if ok` instead.
```This explanation suggests that there should be a function in the implementation that establishes whether `i > -1`, its negation, or neither. That argues for unifying isLessThanZeroCheck and isGreaterEqualToZeroCheck into a single function that returns -1, +1 or zero.
Done
delete blank (so comment becomes a doc comment)
Done
for curFile := range filesUsing(inspect, info, "go1.18") {Let's perform the version check at the last moment. Since the version filter is not expected to be very selective, it's more efficient to make a single pass over the whole package than one per file.
// Check file version.
file := enclosingFile(curCall)
if !fileUses(info, file, "go1.18") {
continue // new(expr) not available in this file
}
Done
var (
isStringsIndex = analysisinternal.IsFunctionNamed(obj, "strings", "Index")
isBytesIndex = analysisinternal.IsFunctionNamed(obj, "bytes", "Index")
isStringsIndexByte = analysisinternal.IsFunctionNamed(obj, "strings", "IndexByte")
isBytesIndexByte = analysisinternal.IsFunctionNamed(obj, "bytes", "IndexByte")
)
if !(isStringsIndex || isBytesIndex || isStringsIndexByte || isBytesIndexByte) {
continue
}Combine:
```
if !analysisinternal.IsFunctionNamed(obj, "strings", "Index", "IndexByte") &&
!analysisinternal.IsFunctionNamed(obj, "bytes", "Index", "IndexByte") {
```
I need the individual cases to form the analyzer message like "strings.Index can be replaced with strings.Cut".
You might find this code from CL 706635 is a more direct way to get the variable obj to which cur is assigned:
```
switch ek, idx := cur.ParentEdge(); ek {
case edge.ValueSpec_Values:
curName := cur.Parent().ChildAt(edge.ValueSpec_Names, idx)
if id, ok := curName.Node().(*ast.Ident); ok {
obj = info.Defs[id]
}
case edge.AssignStmt_Rhs:
curLhs := cur.Parent().ChildAt(edge.AssignStmt_Lhs, idx)
if id, ok := curLhs.Node().(*ast.Ident); ok {
obj = info.ObjectOf(id)
}
}
```
Done
assign, okAsgn := callParent.Node().(*ast.AssignStmt) // :=Madeline Kalil:= or =
Done
The body of this if statement is very long, without clear separation of steps. I don't mind long blocks of code (or long functions) so long as the sequence is obvious, but distinct tasks are interleaved here: the recognition of the various forms of `x = f()`, `x := f()`, `var x = f()` assignment and extraction of the LHS symbol; processing of the uses of x; checking for lvalue uses; editing the condition; and editing the assignment.
It would be great if we could separate these tasks more clearly. I notice that we are reformatting the s and substr expressions at L193. What we've learned from other modernizers is that the less we reformat, the better, since it loses comments and vertical space. If we can instead make more surgical edits, it's more likely to produce a cleaner result. For example, rather than rewriting
```
var i = string.Index(s, sub)
--------------------------------------------
var before, after, ok = string.Cut (s, sub)
```
we should aim instead for these edits:
```
var i = string.Index(s, sub)
----------------- -----
var before, after, ok = string.Cut (s, sub)
```This means we only need to record the positions of the LHS identifier, and the SelectorExpr.Sel from the CallExpr.Fun, and then we can forget about the differences between var and := assignments, leading to a cleaner separation of steps.
Done
idxVar := info.Defs[idx].(*types.Var)
Done
// If all uses of idx match one of the valid formats, return a list of occurences for each format.What are the valid formats? Some orientation comments would help here.
Done
func parseIdxUses(pass *analysis.Pass, uses iter.Seq[inspector.Cursor], idx *ast.Ident, s, substr ast.Expr, afterPos token.Pos) (lessZero, greaterZero, beforeSlice, afterSlice []ast.Expr) {This should be *types.Var for a local variable; we shouldn't care about the syntax of idx any more. (The caller should pass idxObj aka idxVar.)
All of the uses of equalSyntax on idx should be using resolved symbols, i.e. checking whether the specific symbol is used, not just another symbol whose name happens to be spelled the same way.
Thanks, fixed all uses of EqualSyntax. I now have two helpers with different arguments that both use type information to get the idents' objects and see if they are the same
case *ast.CallExpr:
// Allow len(substr) and string() conversion, but any other
// CallExpr involving the ident will result in not returning a
// quick fix.
if isCall(n, "len", "string") {
continue
} else {
return true
}Passing a value to a function cannot modify it, only things referred to by that value. So for an int or a string (but not `[]byte`), we can ignore calls.
I changed this function a bit based on the example of inspecting Uses in stringsbuilder
Assignments are not the only uses of variables; `&v` operations should also be assumed to (ultimately) mutate a variable. See the uses check in stringsbuilder for an example.
Done
// Returns true if the BinaryExpr is of the form idx < 0, 0 > idx, idx == -1, orThe function doesn't actually check the "idx" operand, but it should. Do add test cases before you fix the bugs.
I think at this point we can assume that one of the operands is "idx", since we're only checking expressions that involve uses of idx.
return check.Op == token.LSS && typesinternal.IsZeroExpr(check.Y) || check.Op == token.GTR && typesinternal.IsZeroExpr(check.X) || (check.Op == token.EQL && (isNegativeOne(check.X) || isNegativeOne(check.Y)))Madeline Kalilnewline
Done
Doc comments should be sentences with the declared name as their subject:
// isNegativeOne reports whether expr denotes -1.
// It may report false negatives.
Done
return check.Op == token.GEQ && typesinternal.IsZeroExpr(check.Y) || check.Op == token.LEQ && typesinternal.IsZeroExpr(check.X)It would be more general to ask the type checker whether this expression denotes the constant zero, rather than whether it is spelled "0". We have a helper for that: isZeroIntLiteral.
(BTW, that function's name is a lie since it works on non-literal constants too. Let's rename it to isZeroIntConst.)
Done
func isAfterSlice(pass *analysis.Pass, slice *ast.SliceExpr, idx, substr ast.Expr) bool {Madeline Kalil*types.Var
Done
func isAfterSlice(pass *analysis.Pass, slice *ast.SliceExpr, idx, substr ast.Expr) bool {Madeline Kalilinfo *types.Info
Done
We should also reject s[low:high:max] if max is present.
Done
// Handle s[idx + const] where substr is a basic lit
// substr could also be a call expr like []byte("str")
// TODO(mkalil): This does not handle other methods used to create byte slices
// like byte literals, or using make/append.
var substrLit *ast.BasicLit
switch n := substr.(type) {
case *ast.BasicLit:
substrLit = n
case *ast.CallExpr:
// Check if call matches []byte
tv := pass.TypesInfo.Types[n.Fun]
if tv.IsType() && types.Identical(tv.Type, byteSliceType) {
// Only one arg in []byte conversion.
if lit, ok := n.Args[0].(*ast.BasicLit); ok {
substrLit = lit
}
}
}This concept seems like a useful function abstraction:
```
// stringConst returns the value of the constant string denoted by e, if any,
// and whether it is wrapped in a []byte("...") conversion.
func stringConst(info *types.Info, e ast.Expr) (k constant.Value, byteconv bool)
```
The implementation shouldn't use info.Types and needn't care about BasicLit.
A bit confused about this, will ask you in person
This name is too general for a function this specific at package scope. It should have a more specific name, or, better, be declared locally at L440 as isLenCall.
Done
ident, ok := call.Fun.(*ast.Ident)
return ok && len(call.Args) == 1 && slices.Contains(callName, ident.Name)```
return typeutil.Callee(info, call) == builtinLen &&
is[*ast.Ident](call.Args[0]) &&
info.Uses(call.Args[0].(*ast.Ident)) == idx
```
and then you don't need equalSyntax.
- Alan Donovan
- Mateusz Poliwczak
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)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- Alan Donovan
- Madeline Kalil
- Mateusz Poliwczak
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)
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
- Alan Donovan
- Mateusz Poliwczak
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ingo Oeser (Gerrit)
Ingo Oeser added 5 comments![]( "Open in Gerrit")
In a few places strings.Cut is overkill and strings.Contains will be clearer code.
Every time none of the two result slices are needed, strings.Cut usage can be simplified to strings.Contains to have clearer code.
Please either exclude those cases or replace them with strings.Contains.
}This use case is better replaced by strings.Contains instead of strings.Cut
}Also here strings.Contains(s, "+") would be the better replacement.
}Also here: strings.Contains is the clearer replacement.
return truestrings.Contains is better here as well.
Related details
- Alan Donovan
- Madeline Kalil
- Mateusz Poliwczak
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)
Alan Donovan added 28 comments![]( "Open in Gerrit")
delete blank
nextcall:
for curCall := range inspect.Root().Preorder((*ast.CallExpr)(nil)) {
// Check file version.
file := astutil.EnclosingFile(curCall)
if !fileUses(info, file, "go1.18") {
continue // strings.Index not available in this file
}
indexCall := curCall.Node().(*ast.CallExpr)
obj := typeutil.Callee(info, indexCall)
if obj == nil {
continue
}
var (
isStringsIndex = typesinternal.IsFunctionNamed(obj, "strings", "Index")
isBytesIndex = typesinternal.IsFunctionNamed(obj, "bytes", "Index")
isStringsIndexByte = typesinternal.IsFunctionNamed(obj, "strings", "IndexByte")
isBytesIndexByte = typesinternal.IsFunctionNamed(obj, "bytes", "IndexByte")
)
if !(isStringsIndex || isBytesIndex || isStringsIndexByte || isBytesIndexByte) {
continue
}Rather than looking at every call in the package and testing (four times) whether it is one of the four callees we care about, it would be much more efficient to use the index to report just the handful of calls that we want:
```
for _, obj := range []types.Object{
index.Object("strings", "Index"),
index.Object("strings", "IndexByte"),
index.Object("bytes", "Index"),
index.Object("bytes", "IndexByte"),
} {
// (obj may be nil)
nextcall:
for curCall := range index.Calls(obj) {
```
var idxObj types.ObjectIt's confusing that we have `index` (a type index), strings.Index (a function in the target code), `idx` (a ParentEdge index), and `i` (a local variable in the target code). It would help to be very consistent about the differences between the last three. I propose we use the prefix i (e.g. iIdent, iVar) for the local variable, and `i` in all our code examples.
I think this block can be simplified to:
```
var iIdent *ast.Ident // defining identifier of i var
switch ek, idx := curCall.ParentEdge(); ek {
case edge.ValueSpec_Values:
// Have: var i = strings.Index(...)
curName := curCall.Parent().ChildAt(edge.ValueSpec_Names, idx)
iIdent = curName.Node().(*ast.Ident)
case edge.AssignStmt_Rhs:
// Have: i := strings.Index(...)
// (Must be i's definition.)
curLhs := curCall.Parent().ChildAt(edge.AssignStmt_Lhs, idx)
iIdent, _ = curLhs.Node().(*ast.Ident) // may be nil
}
if iIdent == nil {
continue
}
iVar, _ := info.Defs[iIdent].(*types.Var)
if iVar == nil {
continue
}
// Inv: iIdent is i's definition.
```
Note that this asserts that i is a var, and also rejects non-defining uses of i as in `var i int; i = strings.Index(...)`, which I think is important for correctness; see comment at L178.
idxObj = info.ObjectOf(id)idxVar, _ = info.Defs[id].(*types.Var)
We know it must be var. And we don't want to continue if
selExpr, ok := indexCall.Fun.(*ast.SelectorExpr)ast.Unparen(...).(T)
Always call Unparen before any kind of type switch to remove irrelevant parens.
But see next comment.
if !ok {
continue
}It is actually possible to call strings.Index without a selector, if the strings package is dot imported. In that case, Fun is only an `*ast.Ident` here. Fortunately we have the `typesinternal.UsedIdent(indexCall.Fun)` helper which will take care of these details and return the ident you care about.
idxVar, ok := info.Defs[idx].(*types.Var)Isn't this the same as idxObj? We already did the Defs lookup.
afterPos := indexCall.Pos() // don't consider uses before the call to strings.Index etc.Strictly, we can't ignore uses before the call. Consider
```
var index int
f(&index)
index = strings.Index(s, ...)
g()
if index >= 0 {
use(s[:index])
}
```
In this example, f could escape a pointer to index (e.g. by storing it in a global) and then the call to g() could mutate index through that pointer. So any l-value reference to index other than its strings.Index assignment is potentially problematic.
I think the simplest thing to do is to handle only cases where index is declared and assigned to strings.Index at the same time, for example:
```
var index = strings.Index(...)
```
or:
```
index := strings.Index(...)
```
but not:
```
var index int
index, zero := strings.Index(...), 0
```
And the simplest way to detect that is for the logic at L129-143 to assert that the identifier is a def and skip otherwise:
```
switch ek, i := curCall.ParentEdge(); ek {
case edge.ValueSpec_Values:
...
indexVar, _ = info.Defs[id].(*types.Var)
}
if indexVar == nil {
continue
}
```
Then you don't need to worry about afterPos here, you can simply check all uses.
if sId, ok := id.(*ast.Ident); ok {This is just the easy case. What if s or substr are not mere identifiers, but, say, expressions of the form `ptr.field[i]`? Then the problem of proving that the value of the expression is the same each time it is evaluated becomes much more challenging. We need to either (a) reject the candidate if !ok, or (b) analyze s and substr to see whether they are referentially transparent, and if not, analyze all code between declaration and use and see if there are statements or expressions with potential side effects. Obviously (a) is much easier than (b), and seems like a good first step, but it could reject a significant fraction of candidates.
Pos: indexCall.Pos(),Rather than report the whole call, let's just flag the strings.Index portion, i.e. call.Fun.
Message: fmt.Sprintf("%s can be replaced with %s", replace, with),Let's aim for "strings.Index call can be simplified using Cut"
There's no need to repeat the package name.
Message: fmt.Sprintf("%s can be replaced with %s", replace, with),This should be a verb phrase describing the fix, e.g. "Simplify %s call using %s".
for c := range cur.Enclosing((*ast.BinaryExpr)(nil), (*ast.SliceExpr)(nil), (*ast.AssignStmt)(nil), (*ast.CallExpr)(nil), (*ast.GenDecl)(nil)) {This will report all enclosing nodes of the specified types. For ancestors above the parent, we have no reason to believe that any of their operands is a use of the `index` var. Perhaps you meant simply to call Parent and then type switch?
case *ast.AssignStmt:
// If the value of idx is reassigned, don't suggest a fix.
assign := n
for _, expr := range assign.Lhs {
if resolvesToSameObj(info, idx, expr) {
return nil, nil, nil, nil
}
}
// If an alias to idx is created, don't suggest a fix.
for _, expr := range assign.Rhs {
if resolvesToSameObj(info, idx, expr) {
return nil, nil, nil, nil
}
}
case *ast.GenDecl:
// If an alias to idx is created, don't suggest a fix.
decl := n
for i, spec := range decl.Specs {
if valueSpec, ok := spec.(*ast.ValueSpec); ok {
if resolvesToSameObj(info, idx, valueSpec.Values[i]) {
return nil, nil, nil, nil
}
}
}
case *ast.CallExpr:
// Don't suggest a fix if idx is passed as an argument to some
// other function (unless it's the call to strings.Index).
call := n
for _, arg := range call.Args {
if resolvesToSameObj(info, idx, arg) {
return nil, nil, nil, nil
}
}
}
I wonder why it is necessary to check for assignments here. If we're trying to establish that all uses of the index var appear in a few special forms such as `index < 0`, then shouldn't _any_ use that doesn't match cause this operation to fail?
// checkValue reports whether the check establishes that idx < 0 or idx > -1Helper functions tailored for stringscut should use names that make sense in the wider context of the modernize package. (Go has no file-local func declarations, even though that would sometimes be useful.)
// inv: a check passed to checkValue should have idx as one of its operands.has
func checkValue(info *types.Info, check *ast.BinaryExpr) int {There might be a nice general abstraction here over expressions that returns the non-constant operand (here assumed to be idx) along with its sign (-1, 0, +1), or (nil, 0) if the sign cannot be deduced. For another day.
check.Op == token.GTR && isZeroIntConst(info, check.X) || (check.Op == token.EQL &&linebreak
if check.Op == token.GEQ && isZeroIntConst(info, check.Y) || check.Op == token.LEQ && isZeroIntConst(info, check.X) ||linebreak
check.Op == token.EQL && isZeroIntConst(info, check.X) || isZeroIntConst(info, check.Y) && check.Op == token.EQL {linebreak
check.Op == token.EQL && isZeroIntConst(info, check.X) || isZeroIntConst(info, check.Y) && check.Op == token.EQL {flip the operands (...Op... && ...Y...) for consistency.
switch e := expr.(type) {return isIntConst(info, e, -1)
Rename to isMinusOneIntConst to follow the pattern. Alternatively declare it locally within checkValue.
return slice.Low == nil || isZeroIntConst(info, slice.Low)Don't forget to check that the slice.Max operand (`s[::max]`) is also absent.
for curFile := range filesUsing(inspect, info, "go1.18") {Madeline KalilLet's perform the version check at the last moment. Since the version filter is not expected to be very selective, it's more efficient to make a single pass over the whole package than one per file.
// Check file version.
file := enclosingFile(curCall)
if !fileUses(info, file, "go1.18") {
continue // new(expr) not available in this file
}
Done
Am I looking at the latest draft? This check is still done eagerly.
var (
isStringsIndex = analysisinternal.IsFunctionNamed(obj, "strings", "Index")
isBytesIndex = analysisinternal.IsFunctionNamed(obj, "bytes", "Index")
isStringsIndexByte = analysisinternal.IsFunctionNamed(obj, "strings", "IndexByte")
isBytesIndexByte = analysisinternal.IsFunctionNamed(obj, "bytes", "IndexByte")
)
if !(isStringsIndex || isBytesIndex || isStringsIndexByte || isBytesIndexByte) {
continue
}Madeline KalilCombine:
```
if !analysisinternal.IsFunctionNamed(obj, "strings", "Index", "IndexByte") &&
!analysisinternal.IsFunctionNamed(obj, "bytes", "Index", "IndexByte") {
```
I need the individual cases to form the analyzer message like "strings.Index can be replaced with strings.Cut".
Right, but you don't need one line for each case of the cross product of {strings, bytes} x {Index, IndexByte}. The 10 lines of code that define (replace, with) can be eliminated by computing the string directly from the value of obj in a single expression; see L249.
if okAsgn || okDecl {Madeline KalilThe body of this if statement is very long, without clear separation of steps. I don't mind long blocks of code (or long functions) so long as the sequence is obvious, but distinct tasks are interleaved here: the recognition of the various forms of `x = f()`, `x := f()`, `var x = f()` assignment and extraction of the LHS symbol; processing of the uses of x; checking for lvalue uses; editing the condition; and editing the assignment.
It would be great if we could separate these tasks more clearly. I notice that we are reformatting the s and substr expressions at L193. What we've learned from other modernizers is that the less we reformat, the better, since it loses comments and vertical space. If we can instead make more surgical edits, it's more likely to produce a cleaner result. For example, rather than rewriting
```
var i = string.Index(s, sub)
--------------------------------------------
var before, after, ok = string.Cut (s, sub)
```
we should aim instead for these edits:
```
var i = string.Index(s, sub)
----------------- -----
var before, after, ok = string.Cut (s, sub)
```This means we only need to record the positions of the LHS identifier, and the SelectorExpr.Sel from the CallExpr.Fun, and then we can forget about the differences between var and := assignments, leading to a cleaner separation of steps.
Done
Please use the aligned before/after notation in my comment above to document each set of TextEdits.
Message: fmt.Sprintf("%s can be replaced with %s", replace, with),```
Sprintf("%s can be replaced with %s.Cut",
obj.Pkg().Name(), obj.Name(),
obj.Pkg().Name())
```
then delete LL236-245.
Related details
- Madeline Kalil
- Mateusz Poliwczak
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)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Madeline Kalil
- Mateusz Poliwczak
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)
Madeline Kalil voted and added 18 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
18 comments
Madeline Kalildelete blank
Sorry, I think I accidentally resolved the other comment to do this.
nextcall:
for curCall := range inspect.Root().Preorder((*ast.CallExpr)(nil)) {
// Check file version.
file := astutil.EnclosingFile(curCall)
if !fileUses(info, file, "go1.18") {
continue // strings.Index not available in this file
}
indexCall := curCall.Node().(*ast.CallExpr)
obj := typeutil.Callee(info, indexCall)
if obj == nil {
continue
}
var (
isStringsIndex = typesinternal.IsFunctionNamed(obj, "strings", "Index")
isBytesIndex = typesinternal.IsFunctionNamed(obj, "bytes", "Index")
isStringsIndexByte = typesinternal.IsFunctionNamed(obj, "strings", "IndexByte")
isBytesIndexByte = typesinternal.IsFunctionNamed(obj, "bytes", "IndexByte")
)
if !(isStringsIndex || isBytesIndex || isStringsIndexByte || isBytesIndexByte) {
continue
}Rather than looking at every call in the package and testing (four times) whether it is one of the four callees we care about, it would be much more efficient to use the index to report just the handful of calls that we want:
```
for _, obj := range []types.Object{
index.Object("strings", "Index"),
index.Object("strings", "IndexByte"),
index.Object("bytes", "Index"),
index.Object("bytes", "IndexByte"),
} {
// (obj may be nil)
nextcall:
for curCall := range index.Calls(obj) {
```
Done
ast.Unparen(...).(T)
Always call Unparen before any kind of type switch to remove irrelevant parens.
But see next comment.
Acknowledged
It is actually possible to call strings.Index without a selector, if the strings package is dot imported. In that case, Fun is only an `*ast.Ident` here. Fortunately we have the `typesinternal.UsedIdent(indexCall.Fun)` helper which will take care of these details and return the ident you care about.
Done
Isn't this the same as idxObj? We already did the Defs lookup.
Oops yes, thank you.
afterPos := indexCall.Pos() // don't consider uses before the call to strings.Index etc.Strictly, we can't ignore uses before the call. Consider
```
var index int
f(&index)
index = strings.Index(s, ...)
g()
if index >= 0 {
use(s[:index])
}
```
In this example, f could escape a pointer to index (e.g. by storing it in a global) and then the call to g() could mutate index through that pointer. So any l-value reference to index other than its strings.Index assignment is potentially problematic.I think the simplest thing to do is to handle only cases where index is declared and assigned to strings.Index at the same time, for example:
```
var index = strings.Index(...)
```
or:
```
index := strings.Index(...)
```
but not:
```
var index int
index, zero := strings.Index(...), 0
```
And the simplest way to detect that is for the logic at L129-143 to assert that the identifier is a def and skip otherwise:
```
switch ek, i := curCall.ParentEdge(); ek {
case edge.ValueSpec_Values:
...
indexVar, _ = info.Defs[id].(*types.Var)
}
if indexVar == nil {
continue
}
```Then you don't need to worry about afterPos here, you can simply check all uses.
Thanks, I didn't think of this case. There's logic in checkIdxUses that does not allow using any pointers of index like &index. I updated this function so that it doesn't only examine uses after "afterPos" and added a test case.
var textEdits []analysis.TextEditMadeline Kaliledits
Done
Rather than report the whole call, let's just flag the strings.Index portion, i.e. call.Fun.
Done
Message: fmt.Sprintf("%s can be replaced with %s", replace, with),Let's aim for "strings.Index call can be simplified using Cut"
There's no need to repeat the package name.
Done
Message: fmt.Sprintf("%s can be replaced with %s", replace, with),This should be a verb phrase describing the fix, e.g. "Simplify %s call using %s".
Done
// checkValue reports whether the check establishes that idx < 0 or idx > -1Helper functions tailored for stringscut should use names that make sense in the wider context of the modernize package. (Go has no file-local func declarations, even though that would sometimes be useful.)
How about checkIdxComparison to match checkIdxUses?
// inv: a check passed to checkValue should have idx as one of its operands.Madeline Kalilhas
Done
check.Op == token.GTR && isZeroIntConst(info, check.X) || (check.Op == token.EQL &&Madeline Kalillinebreak
Done
if check.Op == token.GEQ && isZeroIntConst(info, check.Y) || check.Op == token.LEQ && isZeroIntConst(info, check.X) ||Madeline Kalillinebreak
Done
check.Op == token.EQL && isZeroIntConst(info, check.X) || isZeroIntConst(info, check.Y) && check.Op == token.EQL {Madeline Kalillinebreak
Done
check.Op == token.EQL && isZeroIntConst(info, check.X) || isZeroIntConst(info, check.Y) && check.Op == token.EQL {flip the operands (...Op... && ...Y...) for consistency.
Done
return isIntConst(info, e, -1)
Rename to isMinusOneIntConst to follow the pattern. Alternatively declare it locally within checkValue.
I decided to just inline the call to isIntLiteral(info, e, -1)
return slice.Low == nil || isZeroIntConst(info, slice.Low)Don't forget to check that the slice.Max operand (`s[::max]`) is also absent.
Thanks, I check this inside checkIdxUses before the call to isBeforeSlice
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |