Steps to reproduce

go.mod

module main

go 1.24.5 //1.22+

main.go

package main

func main() {
    for i, v := range []string{"a", "b", "c"} {
        i := i
        v := v
        println(i, v)
    }
}

Run 

modernize -fix ./...

Only the first redeclaration is removed. The second remains:

for i, v := range []string{"a", "b", "c"} {
    v := v
    println(i, v)
}

And whats even worse, if you run the tool again, it removes the remaining redeclaration, leading to a different result on the second run. This is unexpected and unacceptable for a static analysis tool - it should be idempotent and consistent across runs.

This issue also affected the gopls source itself, which is how I discovered it. See this change: https://go-review.googlesource.com/c/tools/+/665215/1/gopls/internal/golang/references.go

There is a test case that expects this behavior, but I believe it is flawed. The correct behavior should be to remove both redundant redeclarations in a single pass.

Comment From: gopherbot

Change https://go.dev/cl/692615 mentions this issue: gopls/internal/analysis/modernize/forvar: provide fix for second loop var

Comment From: skewb1k

Opened CL with a working but maybe not the best solution for this. I'm open to iterating on it if the problem and the proposed behavior make sense.

Comment From: gabyhelp

Related Issues

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

Comment From: adonovan

Only the first redeclaration is removed. The second remains

This is not a bug, since the suggested fix was sound and improved the code, but a request for a missing feature, namely, that the loopVarRedecl function in the forVar modernizer works a little harder to recognize the k, v pattern.

And whats even worse, if you run the tool again, it removes the remaining redeclaration, leading to a different result on the second run. This is unexpected and unacceptable for a static analysis tool - it should be idempotent and consistent across runs.

This is to be expected, and is an inevitable consequence of the design of the analysis framework: any suggested fix may itself be a trigger of new analysis diagnostics from the same or (more often) a different analyzer. When it is the same analyzer (as in this case) we can usually make the analyzer a little smarter, but in general, it is not possible to reanalyze the fixed code because it may import new packages whose type information is not available. The best we can do is keep applying fixes until the process converges. Each individual fix should be progress, though.

Comment From: skewb1k

This is not a bug, since the suggested fix was sound and improved the code, but a request for a missing feature, namely, that the loopVarRedecl function in the forVar modernizer works a little harder to recognize the k, v pattern.

I can't imagine a case where it's intended behavior for a static analyzer—designed to fix code—to leave obvious fixes unresolved. If the tool can apply the fix on a second run immediately after the first, with no user intervention in between, then it clearly had all the necessary information during the first run as well. Even if the fix couldn't be done "smartly" in one pass, the tool should rerun itself in a loop until all diagnostics are resolved. After all, that's what the user will be forced to do manually anyway—so why leave room for error or forgetfulness?

but in general, it is not possible to reanalyze the fixed code because it may import new packages whose type information is not available.

In that case, the tool definitely shouldn't exit with status code 0, because the resulting code doesn't compile and the user has no way to commit it safely in that state.

Of course, this is just my opinion, and I understand that implementing this properly in a complex analyzer may be difficult. But I still think this kind of behavior is a bug - not an optional "nice-to-have."

Comment From: adonovan

If the tool can apply the fix on a second run immediately after the first, with no user intervention in between, then it clearly had all the necessary information during the first run as well.

This is unfortunately not the case. Consider a set of analyzers deployed in a batch pipeline, where each package is analyzed, in dependencies-first order, as a separate unit of work in a distributed system, like a distributed build. (This is how Google's large scale analysis pipeline works, FWIW.) Now consider the package import graph A -> B -> C. The analysis of A will get source for A, and type information and analysis facts for B, but not for C. Now imagine that one of the fixes applied to A inserts the snippet of code import C. (This is exactly what happens with fixes from //go:fix inline.) There is simply no way to re-analyze package A with this fix applied, because it doesn't have access to type information for C.

Even if the fix couldn't be done "smartly" in one pass, the tool should rerun itself in a loop until all diagnostics are resolved. After all, that's what the user will be forced to do manually anyway—so why leave room for error or forgetfulness?

Again, not possible for the reason above.

In that case, the tool definitely shouldn't exit with status code 0, because the resulting code doesn't compile and the user has no way to commit it safely in that state.

Again, it is not possible to type-check the fixed code and ascertain whether it would compile.

Of course, this is just my opinion, and I understand that implementing this properly in a complex analyzer may be difficult. But I still think this kind of behavior is a bug - not an optional "nice-to-have."

It would be nice to have; but it is architecturally incompatible with a distributed build model.

Comment From: adonovan

It would be nice to have; but it is architecturally incompatible with a distributed build model.

(Of course, that doesn't mean the local multichecker-based command couldn't re-execute the entire workflow repeatedly until a fixed point is reached.)

Comment From: skewb1k

Thanks for the explanation. Reality, as always, turns out to be more complicated than one would hope. Whether it's a bug report or a feature request doesn’t matter much in the end if resolving it brings useful functionality. I've updated the fix; could you review it/assign reviewers/trigger the bot? (It’s still a bit unclear to me when the bot assigns reviewers automatically, and whether the author should choose someone, or if maintainers just descend from the heavens)

Comment From: adonovan

Reality, as always, turns out to be more complicated than one would hope.

Indeed.

I've updated the fix; could you review it/assign reviewers/trigger the bot? (It’s still a bit unclear to me when the bot assigns reviewers automatically, and whether the author should choose someone, or if maintainers just descend from the heavens)

I misread your "not the best solution" comment earlier as meaning you were still hacking on it, but I'll take a look now. Thanks for the fix.