x/tools/gopls: yet more modernizer bugs

[adonovan]

Found after running modernize on std + cmd:

• don't apply maps transformations to maps package itself or its in-package tests.
• don't import slices (or almost any other package) from runtime or its dependencies.
• remove unused imports such as "sort"
• for i := 0; i < 1e3; i++ -> for range 1e3 requires an int conversion or change of literal.
• for i := 0; i < n; i++ -> for range n is invalid if the loop body contains i++ (I thought we already checked this?). Example: path/filepath.scanChunk (Fixed by https://go.dev/cl/650815 )
• some comments go missing (e.g. in minmax).
• var i int; for i = 0; i < n; i++ -> for range n removes uses of i that might make local var i unreferenced (example)
• some comments go missing (e.g. in minmax).

Refactoring is hard. :( We may need to be selective about which categories we apply without review until all these bugs are fixed. Perhaps we also need a flag to select categories too.

Mistakes that break the build are of low severity, because the mistake is impossible to miss and the remedy is obvious. Loss of commentary is of medium severity as it may go undetected due to reviewer fatigue in a large CL. The really severe problems are latent behavior changes such as the "loop body contains i++" case above (now fixed).

[gabyhelp]

Related Issues

• x/tools/gopls/internal/analysis: add "modernizer" analyzers #70815

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

[gopherbot]

Change https://go.dev/cl/650815 mentions this issue: gopls/internal/analysis/modernize: fix rangeint bug

[findleyr]

Putting this in the v0.18.0 milestone until we make a decision. It seems premature to expose a tool that blanket-applies these modernizers.

[gopherbot]

Change https://go.dev/cl/651095 mentions this issue: [gopls-release-branch.0.18] gopls/internal/analysis/modernize: fix rangeint bug

[erikdubbelboer]

Here is another buggy transformation it did on my code:

-       start := strings.Index(origin, "//")
-       if start < 0 {
-               start = 0
-       } else {
-               start += 2
-       }
+       start := max(strings.Index(origin, "//"), 0)

[adonovan]

Many thanks for the bug report. It appears that minmax failed to check that IfStmt.Else is nil in its matcher for pattern 2. Fix pending.

[gopherbot]

Change https://go.dev/cl/651375 mentions this issue: gopls/internal/analysis/modernize: fix minmax bug

[nemith]

I added #71878 but it seems to be the same bug that @erikdubbelboer reported (I checked the OP here and not the comments first. Sorry)

[adonovan]

I added #71878 but it seems to be the same bug that @erikdubbelboer reported (I checked the OP here and not the comments first. Sorry)

Don't apologize, filing a separate issue is much appreciated. Let's make that issue the canonical one for this bug.

[gopherbot]

Change https://go.dev/cl/651237 mentions this issue: [gopls-release-branch.0.18] gopls/internal/analysis/modernize: fix minmax bug

[seehuhn]

Here is a buggy transformation by "modernize":

 func Test(a []float64) float64 {
-       x := 0.0
-       if x < a[0] {
-               x = a[0]
-       } else if x > a[1] {
-               x = a[1]
-       }
+       x := max(0.0, a[0])
        return x
 }

Before the change, Test([]float64{-1.0, -0.5}) returns -0.5, after applying "modernize" the same call returns 0.

[adonovan]

Here is a buggy transformation by "modernize":

Thanks; this is the same bug as above (and thus #71878), which is now fixed at head. We'll release the patch today.

[dethi]

First, thanks a lot for the tool! I'm slowly applying on our codebase and it works super well.

I found one additional bug in the transformation of i := 0; for i = 0; i < n; i++ -> for i = range n if i is used outside of the for loop body. See the minimal reproduction: https://go.dev/play/p/9tlB2oilyJn

[adonovan]

First, thanks a lot for the tool! I'm slowly applying on our codebase and it works super well.

I found one additional bug in the transformation of i := 0; for i = 0; i < n; i++ -> for i = range n if i is used outside of the for loop body. See the minimal reproduction: https://go.dev/play/p/9tlB2oilyJn

Good point, thanks for reporting it. I've split this out as #71952.

Glad it (mostly) works for you.

[adonovan]

First, thanks a lot for the tool! I'm slowly applying on our codebase

Reminder: use this command to batch-apply fixes:

$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix -test ./...

(Unfortunately the vital -fix flag was missing from the original release notes.)

[dethi]

Yes, that's what I'm using. I'm doing it package by package, to get smaller diff and avoid merge conflict in our fast changing monorepo.

My complete process is actually, for each packages:

• 1st change: replace interface{} -> any, using gofmt -r 'interface{} -> any' -w. Mostly because this is an always safe/uninteresting change. Isolating it make it easier to review the later changes
• 2nd change: modernize -fix -test=false
• 3rd change: modernize -fix -test=true, splitting out the test in a separate change gives us more confidence

[adonovan]

Yes, that's what I'm using. I'm doing it package by package, to get smaller diff and avoid merge conflict in our fast changing monorepo.

My complete process is actually, for each packages:

• 1st change: replace interface{} -> any, using gofmt -r 'interface{} -> any' -w. Mostly because this is an always safe/uninteresting change. Isolating it make it easier to review the later changes
• 2nd change: modernize -fix -test=false
• 3rd change: modernize -fix -test=true, splitting out the test in a separate change gives us more confidence

That's a good idea. Others have pointed out the any fixes are so numerous that they need to be done as one large but low-risk change; perhaps we should have better flag support for selecting categories of diagnostics. Separating the tests is a nice touch. Do let us know if there are things we could do to make this workflow easier.