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