x/tools/go/analysis/passes/modernize: stringscut reports false positives

[ldez]

Version

golang.org/x/tools v0.39.0

What did you do?

I run the stringscut analyzer (though golangci-lint) on the code snippets above.

• https://go.dev/play/p/nVYQAyLOMYH
• https://go.dev/play/p/VOSeIIPKQXu

What did you see happen?

$ golangci-lint run --enable-only modernize
foo.go:29:10: stringscut: strings.IndexByte can be simplified using strings.Cut (modernize)
        if i := strings.IndexByte(s, '='); i > 0 {
                ^
1 issues:
* modernize: 1

$ golangci-lint run --enable-only modernize      
bar.go:21:10: stringscut: strings.Index can be simplified using strings.Cut (modernize)
        if i := strings.Index(a, b); i > 0 {
                ^
1 issues:
* modernize: 1

What did you expect to see?

As you can see in the code snippet, the analyzer suggests something that is not equivalent to the original functions.

## Cut
 abc true
a bc true
abc  true
## Original
=abc  false
a bc true
abc  true
## Modified
 abc true
a bc true
abc  true

## Original

b
## Modified

b
b

[gabyhelp]

Related Issues

• tools/gopls/internal/analysis/modernize: stringscutprefix offers invalid fix #73547 (closed)
• x/tools/gopls/internal/analysis/modernize: use strings.Cut #71369 (closed)
• x/tools/gopls/internal/analysis/modernize: apply modernizations for TrimPrefix to TrimSuffix too #75205 (closed)
• x/tools/gopls: modernize could suggest using strings.Cut* instead of strings.Split* #74494

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[adonovan]

Thanks for the very clear bug report.

@madelinekalil The problem seems to be that checkIdxComparison returns an affirmative result for both i < 0 and i < -1 (or any other negative constant), wheres the transformation is safe only if the condition is exactly i < 0 (or its negation).

I think the solution is to use the ZeroInequality helper I sketched a few weeks back; see https://go.dev/cl/726485. But it needs some tests.

[gopherbot]

Change https://go.dev/cl/726485 mentions this issue: go/analysis/passes/modernize: sketch of stringscut fix