code review 7097048: cmd/vet: detect misuse of atomic.Add*

cmd/vet: detect misuse of atomic.Add*

Re-assigning the return value of an atomic operation to the same variable being operated is a common mistake:

x = atomic.AddUint64(&x, 1)

Add this check to go vet.

Fixes issue 4065.

[dvyukov, 2013-01-12 11:24:32]

https://codereview.appspot.com/7097048/diff/2001/src/cmd/vet/main.go
File src/cmd/vet/main.go (right):

https://codereview.appspot.com/7097048/diff/2001/src/cmd/vet/main.go#newcode210
src/cmd/vet/main.go:210: // walkCall walks a call expression.
comment is wrong

[divoxx, 2013-01-12 11:24:40]

Hello dvyukov@google.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[dvyukov, 2013-01-12 11:28:08]

On 2013/01/12 11:24:40, divoxx wrote:
> Hello mailto:dvyukov@google.com (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go/

I am not a good reviewer for this code. I never worked with AST.

[remyoudompheng, 2013-01-12 11:34:24]

https://codereview.appspot.com/7097048/diff/6001/src/cmd/vet/atomic.go
File src/cmd/vet/atomic.go (right):

https://codereview.appspot.com/7097048/diff/6001/src/cmd/vet/atomic.go#newcode20
src/cmd/vet/atomic.go:20: if call, ok := rexp.(*ast.CallExpr); ok {
the indentation level is crazy: you could say:

call, ok := rexp.(*ast.CallExpr)
if !ok {
  continue
}
sel, ok := ...
if !ok {
  continue
}
etc.

https://codereview.appspot.com/7097048/diff/6001/src/cmd/vet/atomic.go#newcode27
src/cmd/vet/atomic.go:27: name := e.X.(*ast.Ident).Name
the type assertion should be sanitized
e.Op should be checked for token.AND

[divoxx, 2013-01-12 11:35:04]

Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[divoxx, 2013-01-12 11:50:39]

Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[divoxx, 2013-01-12 23:28:31]

Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[remyoudompheng, 2013-01-12 23:48:01]

https://codereview.appspot.com/7097048/diff/9002/src/cmd/vet/atomic.go
File src/cmd/vet/atomic.go (right):

https://codereview.appspot.com/7097048/diff/9002/src/cmd/vet/atomic.go#newcode1
src/cmd/vet/atomic.go:1: // Copyright 2012 The Go Authors. All rights reserved.
we are now in 2013

https://codereview.appspot.com/7097048/diff/9002/src/cmd/vet/atomic.go#newcode80
src/cmd/vet/atomic.go:80: }
can you check your code works properly on:

var s struct { Counter uint64 }
z := atomic.AddUint64(&s.Counter, 1)
p := &s.Counter
var u [2]uint64
u[0] = atomic.AddUint64(p, 1)

thanks.

[divoxx, 2013-01-12 23:51:20]

On 2013/01/12 23:48:01, remyoudompheng wrote:
> https://codereview.appspot.com/7097048/diff/9002/src/cmd/vet/atomic.go
> File src/cmd/vet/atomic.go (right):
> 
>
https://codereview.appspot.com/7097048/diff/9002/src/cmd/vet/atomic.go#newcode1
> src/cmd/vet/atomic.go:1: // Copyright 2012 The Go Authors. All rights
reserved.
> we are now in 2013
> 
>
https://codereview.appspot.com/7097048/diff/9002/src/cmd/vet/atomic.go#newcode80
> src/cmd/vet/atomic.go:80: }
> can you check your code works properly on:
> 
> var s struct { Counter uint64 }
> z := atomic.AddUint64(&s.Counter, 1)
> p := &s.Counter
> var u [2]uint64
> u[0] = atomic.AddUint64(p, 1)
> 
> thanks.

Unless I'm missing something the code you posted should be fine, shouldn't it?
You asking to make sure vet does not show the warning/err for this?

[divoxx, 2013-01-13 02:31:02]

Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[divoxx, 2013-01-13 02:36:37]

On 2013/01/13 02:31:02, divoxx wrote:
> Hello mailto:dvyukov@google.com, mailto:golang-dev@googlegroups.com,
mailto:remyoudompheng@gmail.com
> (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

The latest patch include support for checking of structs, pointer to structs,
slices/arrays and slice/arrays of pointers.

It has been refactored to be more readable and easier to understand. New test
cases included.

[remyoudompheng, 2013-01-13 05:42:27]

No, the problem is that your code panicked on my examples because you
hadn't addressed all comments.

My examples were not supposed to raise any warning, they look like proper
usage.
On 13 Jan 2013 00:51, <divoxx@gmail.com> wrote:

> On 2013/01/12 23:48:01, remyoudompheng wrote:
>
>> https://codereview.appspot.**com/7097048/diff/9002/src/cmd/**
>>
vet/atomic.go<https://codereview.appspot.com/7097048/diff/9002/src/cmd/vet/atomic.go>
>> File src/cmd/vet/atomic.go (right):
>>
>
>
> https://codereview.appspot.**com/7097048/diff/9002/src/cmd/**
>
vet/atomic.go#newcode1<https://codereview.appspot.com/7097048/diff/9002/src/cmd/vet/atomic.go#newcode1>
>
>> src/cmd/vet/atomic.go:1: // Copyright 2012 The Go Authors. All rights
>>
> reserved.
>
>> we are now in 2013
>>
>
>
> https://codereview.appspot.**com/7097048/diff/9002/src/cmd/**
>
vet/atomic.go#newcode80<https://codereview.appspot.com/7097048/diff/9002/src/cmd/vet/atomic.go#newcode80>
>
>> src/cmd/vet/atomic.go:80: }
>> can you check your code works properly on:
>>
>
>  var s struct { Counter uint64 }
>> z := atomic.AddUint64(&s.Counter, 1)
>> p := &s.Counter
>> var u [2]uint64
>> u[0] = atomic.AddUint64(p, 1)
>>
>
>  thanks.
>>
>
> Unless I'm missing something the code you posted should be fine,
> shouldn't it? You asking to make sure vet does not show the warning/err
> for this?
>
>
https://codereview.appspot.**com/7097048/<https://codereview.appspot.com/7097...
>

[divoxx, 2013-01-13 06:00:13]

Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-01-18 20:36:51]

Add this:

func (f *File) gofmt(x ast.Expr) string {
    f.buf.Reset()
    printer.Fprint(&f.b, f.fset, x)
    return f.String()
}

Then you can use it in the error message and the tester. Once you have

lhs = atomic.AddXXX(&arg)

you can check if f.gofmt(lhs) == f.gofmt(arg).

[divoxx, 2013-01-21 16:42:52]

Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com,
rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[divoxx, 2013-01-21 16:44:15]

Russ,

I changed the implementation to do as you suggested. Let me know if that is what
you meant.

Cheers

On Jan 18, 2013, at 12:36 PM, rsc@golang.org wrote:

> Add this:
> 
> func (f *File) gofmt(x ast.Expr) string {
>    f.buf.Reset()
>    printer.Fprint(&f.b, f.fset, x)
>    return f.String()
> }
> 
> Then you can use it in the error message and the tester. Once you have
> 
> lhs = atomic.AddXXX(&arg)
> 
> you can check if f.gofmt(lhs) == f.gofmt(arg).
> 
> https://codereview.appspot.com/7097048/

[rsc, 2013-01-23 03:20:24]

There should be a 'Fixes issue NNN.' at the bottom of the CL description, right?

https://codereview.appspot.com/7097048/diff/24001/src/cmd/vet/atomic.go
File src/cmd/vet/atomic.go (right):

https://codereview.appspot.com/7097048/diff/24001/src/cmd/vet/atomic.go#newco...
src/cmd/vet/atomic.go:19: 
if len(n.Lhs) != len(n.Rhs) {
    return
}

https://codereview.appspot.com/7097048/diff/24001/src/cmd/vet/atomic.go#newco...
src/cmd/vet/atomic.go:20: for i, rexp := range n.Rhs {
s/rexp/right/
s/lhe/left/ below

https://codereview.appspot.com/7097048/diff/24001/src/cmd/vet/atomic.go#newco...
src/cmd/vet/atomic.go:21: // Skip If the expr in the right side is not a method
call
Can tighten this up considerably just by spacing:

// Look for call to atomic.AddXXX with at least one argument.
call, ok := right.(*ast.CallExpr)
if !ok || len(call.Args) < 1 {
    continue
}
sel, ok := call.Func.(*ast.SelectorExpr)
if !ok {
    continue
}
if pkg, ok := sel.X.(*ast.Ident); !ok || pkg.Name != "atomic" {
    continue
}
switch sel.Sel.Name {
default:
    continue
case "AddInt32", "AddInt64", ...:
}

// Check that return value is not assigned to atomic word.
left := n.Lhs[i]
arg := call.Args[0]
<body from below here>

https://codereview.appspot.com/7097048/diff/24001/src/cmd/vet/atomic.go#newco...
src/cmd/vet/atomic.go:48: func (f *File) checkAtomicAddAssignment(call
*ast.CallExpr, lhe ast.Expr) {
suggest swapping the arguments to match program: left = call

https://codereview.appspot.com/7097048/diff/24001/src/cmd/vet/atomic.go#newco...
src/cmd/vet/atomic.go:49: switch arg := call.Args[0].(type) {

arg := call.Args[0]
broken := false
if uarg, ok := arg.(*ast.UnaryExpr); ok && uarg.Op == token.AND {
    broken = gofmt(lhe) == gofmt(uarg.X)
} else if star, ok := lhe.(*ast.StarExpr); ok {
    broken = gofmt(star.X) == gofmt(arg)
}

if broken {
    f.Warn(lhe.Pos(), "direct assignment to atomic value")
}

https://codereview.appspot.com/7097048/diff/24001/src/cmd/vet/atomic.go#newco...
src/cmd/vet/atomic.go:109: /* TEST CASE */
None of the other files have this comment. Drop.

https://codereview.appspot.com/7097048/diff/24001/src/cmd/vet/main.go
File src/cmd/vet/main.go (right):

https://codereview.appspot.com/7097048/diff/24001/src/cmd/vet/main.go#newcode272
src/cmd/vet/main.go:272: // goFmt returns a string representation of the
expression
Please call this plain gofmt not goFmt. It's confusing not to spell it like the
command.

[divoxx, 2013-01-23 07:37:07]

Hello dvyukov@google.com, golang-dev@googlegroups.com, remyoudompheng@gmail.com,
rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-01-29 23:21:12]

LGTM

[rsc, 2013-01-30 15:57:15]

*** Submitted as https://code.google.com/p/go/source/detail?r=c8cd1f608cd2 ***

cmd/vet: detect misuse of atomic.Add*

Re-assigning the return value of an atomic operation to the same variable being
operated is a common mistake:

x = atomic.AddUint64(&x, 1)

Add this check to go vet.

Fixes issue 4065.

R=dvyukov, golang-dev, remyoudompheng, rsc
CC=golang-dev
https://codereview.appspot.com/7097048

Committer: Russ Cox <rsc@golang.org>

[divoxx, 2013-03-01 17:07:27]

*** Abandoned ***