x/tools/gopls: modernizer for Go 1.19 atomic types (atomic.Int32, ...)

[aktau]

Go 1.19 introduced new atomic types (wrappers). Allowing one to replace:

var x int32
for range 100 {
  go atomic.AddInt32(&x, 1)
}

With:

var x atomic.Int32
for range 100 {
  go x.Add(1)
}

On the whole, this is safer, as the atomic types don't allow non-atomic access, which is a source of bugs. Note the +checkatomic annotation of checklocks.

These new-style atomics also don't suffer from misalignment problems, see dominikh/go-tools#171.

Not all usages are (easily) migratable, a non-exhaustive list of the top of my head:

• Initialized values (e.g.: x := int32(42) ), though this could be done with an ugly closure, it may not be worth it.
• Multi-package usages (exposed atomics, if they exist for some reason)
• ...(probably a lot more)

cc @adonovan

[gabyhelp]

Related Issues

• x/tools/gopls: modernizer for sync.OnceValues #76822
• proposal: sync/atomic: deprecate AddXxx, CompareAndSwapXxx, LoadXxx, StoreXxx, SwapXxx #55302 (closed)
• runtime: convert atomic values to runtime/internal/atomic types #53821
• x/tools/go/pointer: sound models of `sync/atomic` `Pointer[T]` and `Value` #56918 (closed)
• x/tools/gopls/internal/analysis/modernize: add modernizer for WaitGroup.Go #73059 (closed)
• x/tools/go/analysis/passes/modernize: replace unsafe pointer arithmetic with unsafe.Add(ptr, n) #76648

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

[adonovan]

This seems reasonable, tractable, and worth doing.

Any use of x as an lvalue (e.g. &x, x = ...) would have to be analyzed as one of a few supported patterns (Load, Store, Swap, etc), and all other uses would conservatively suppress the fix.

Initialized values (e.g.: x := int32(42) ), though this could be done with an ugly closure, it may not be worth it.

I agree that initializers are a deal breaker. In some easy cases they could be replaced by a call to Store, but the analysis seems tricky in general. I don't see how a closure helps because you would still need to assign the Int32, which entails copying a noCopy field, which would trigger a copylocks diagnostic.

[gopherbot]

Change https://go.dev/cl/740140 mentions this issue: go/analysis/passes/modernize: atomic: use new atomic types

[gopherbot]

Change https://go.dev/cl/740141 mentions this issue: x/tools: sketch of atomic modernizer output

[aktau]

... I don't see how a closure helps because you would still need to assign the Int32, which entails copying a noCopy field, which would trigger a copylocks diagnostic.

After reflecting upon it, I think you're right about that. A closure to enable using this as an expression won't work without triggering copylocks.

On that front, it may be worthwhile to think about enhancing copylocks so that copies on default-initialized types are allowed as long as they are not shared. Similar to how gVisor checklocks doesn't warn for not using mutexes around protected fields if they have just been constructed and haven't been (able to be) passed into another goroutine yet.

But, that's another discussion.