From 3f6d6d9dddbf2dac342afecc00a20476d5e976ec Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 8 Oct 2018 20:33:22 -0400 Subject: [PATCH] go/analysis/passes/atomic: split out of vet Also: don't abort loading just because there were parse/type errors. It's the driver's job to decide whether to fail due to errors. Change-Id: I055033fb89319d957b328c4fa4a30144afc7457c Reviewed-on: https://go-review.googlesource.com/c/140738 Run-TryBot: Michael Matloob Reviewed-by: Michael Matloob --- go/analysis/passes/atomic/atomic.go | 92 +++++++++++++++++++ go/analysis/passes/atomic/atomic_test.go | 13 +++ .../atomic.go => atomic/testdata/src/a/a.go} | 22 ++--- go/analysis/passes/vet/atomic.go | 73 --------------- 4 files changed, 116 insertions(+), 84 deletions(-) create mode 100644 go/analysis/passes/atomic/atomic.go create mode 100644 go/analysis/passes/atomic/atomic_test.go rename go/analysis/passes/{vet/testdata/atomic.go => atomic/testdata/src/a/a.go} (58%) delete mode 100644 go/analysis/passes/vet/atomic.go diff --git a/go/analysis/passes/atomic/atomic.go b/go/analysis/passes/atomic/atomic.go new file mode 100644 index 00000000..1c305709 --- /dev/null +++ b/go/analysis/passes/atomic/atomic.go @@ -0,0 +1,92 @@ +// Copyright 2013 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package atomic + +import ( + "go/ast" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/analysis/passes/internal/analysisutil" + "golang.org/x/tools/go/ast/inspector" +) + +var Analyzer = &analysis.Analyzer{ + Name: "atomic", + Doc: `check for common mistakes using the sync/atomic package + +The atomic checker looks for assignment statements of the form: + + x = atomic.AddUint64(&x, 1) + +which are not atomic.`, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + RunDespiteErrors: true, + Run: run, +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{ + (*ast.AssignStmt)(nil), + } + inspect.Preorder(nodeFilter, func(node ast.Node) { + n := node.(*ast.AssignStmt) + if len(n.Lhs) != len(n.Rhs) { + return + } + if len(n.Lhs) == 1 && n.Tok == token.DEFINE { + return + } + + for i, right := range n.Rhs { + call, ok := right.(*ast.CallExpr) + if !ok { + continue + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + continue + } + pkgIdent, _ := sel.X.(*ast.Ident) + pkgName, ok := pass.TypesInfo.Uses[pkgIdent].(*types.PkgName) + if !ok || pkgName.Imported().Path() != "sync/atomic" { + continue + } + + switch sel.Sel.Name { + case "AddInt32", "AddInt64", "AddUint32", "AddUint64", "AddUintptr": + checkAtomicAddAssignment(pass, n.Lhs[i], call) + } + } + }) + return nil, nil +} + +// checkAtomicAddAssignment walks the atomic.Add* method calls checking +// for assigning the return value to the same variable being used in the +// operation +func checkAtomicAddAssignment(pass *analysis.Pass, left ast.Expr, call *ast.CallExpr) { + if len(call.Args) != 2 { + return + } + arg := call.Args[0] + broken := false + + gofmt := func(e ast.Expr) string { return analysisutil.Format(pass.Fset, e) } + + if uarg, ok := arg.(*ast.UnaryExpr); ok && uarg.Op == token.AND { + broken = gofmt(left) == gofmt(uarg.X) + } else if star, ok := left.(*ast.StarExpr); ok { + broken = gofmt(star.X) == gofmt(arg) + } + + if broken { + pass.Reportf(left.Pos(), "direct assignment to atomic value") + } +} diff --git a/go/analysis/passes/atomic/atomic_test.go b/go/analysis/passes/atomic/atomic_test.go new file mode 100644 index 00000000..d34bb748 --- /dev/null +++ b/go/analysis/passes/atomic/atomic_test.go @@ -0,0 +1,13 @@ +package atomic_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/atomic" +) + +func TestFromFileSystem(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, atomic.Analyzer, "a") +} diff --git a/go/analysis/passes/vet/testdata/atomic.go b/go/analysis/passes/atomic/testdata/src/a/a.go similarity index 58% rename from go/analysis/passes/vet/testdata/atomic.go rename to go/analysis/passes/atomic/testdata/src/a/a.go index 69730b4e..dc12bd01 100644 --- a/go/analysis/passes/vet/testdata/atomic.go +++ b/go/analysis/passes/atomic/testdata/src/a/a.go @@ -4,7 +4,7 @@ // This file contains tests for the atomic checker. -package testdata +package a import ( "sync/atomic" @@ -14,39 +14,39 @@ type Counter uint64 func AtomicTests() { x := uint64(1) - x = atomic.AddUint64(&x, 1) // ERROR "direct assignment to atomic value" - _, x = 10, atomic.AddUint64(&x, 1) // ERROR "direct assignment to atomic value" - x, _ = atomic.AddUint64(&x, 1), 10 // ERROR "direct assignment to atomic value" + x = atomic.AddUint64(&x, 1) // want "direct assignment to atomic value" + _, x = 10, atomic.AddUint64(&x, 1) // want "direct assignment to atomic value" + x, _ = atomic.AddUint64(&x, 1), 10 // want "direct assignment to atomic value" y := &x - *y = atomic.AddUint64(y, 1) // ERROR "direct assignment to atomic value" + *y = atomic.AddUint64(y, 1) // want "direct assignment to atomic value" var su struct{ Counter uint64 } - su.Counter = atomic.AddUint64(&su.Counter, 1) // ERROR "direct assignment to atomic value" + su.Counter = atomic.AddUint64(&su.Counter, 1) // want "direct assignment to atomic value" z1 := atomic.AddUint64(&su.Counter, 1) _ = z1 // Avoid err "z declared and not used" var sp struct{ Counter *uint64 } - *sp.Counter = atomic.AddUint64(sp.Counter, 1) // ERROR "direct assignment to atomic value" + *sp.Counter = atomic.AddUint64(sp.Counter, 1) // want "direct assignment to atomic value" z2 := atomic.AddUint64(sp.Counter, 1) _ = z2 // Avoid err "z declared and not used" au := []uint64{10, 20} - au[0] = atomic.AddUint64(&au[0], 1) // ERROR "direct assignment to atomic value" + au[0] = atomic.AddUint64(&au[0], 1) // want "direct assignment to atomic value" au[1] = atomic.AddUint64(&au[0], 1) ap := []*uint64{&au[0], &au[1]} - *ap[0] = atomic.AddUint64(ap[0], 1) // ERROR "direct assignment to atomic value" + *ap[0] = atomic.AddUint64(ap[0], 1) // want "direct assignment to atomic value" *ap[1] = atomic.AddUint64(ap[0], 1) x = atomic.AddUint64() // Used to make vet crash; now silently ignored. { // A variable declaration creates a new variable in the current scope. - x := atomic.AddUint64(&x, 1) // ERROR "declaration of .x. shadows declaration at atomic.go:16" + x := atomic.AddUint64(&x, 1) // Re-declaration assigns a new value. - x, w := atomic.AddUint64(&x, 1), 10 // ERROR "direct assignment to atomic value" + x, w := atomic.AddUint64(&x, 1), 10 // want "direct assignment to atomic value" _ = w } } diff --git a/go/analysis/passes/vet/atomic.go b/go/analysis/passes/vet/atomic.go deleted file mode 100644 index 50351016..00000000 --- a/go/analysis/passes/vet/atomic.go +++ /dev/null @@ -1,73 +0,0 @@ -// +build ignore - -// Copyright 2013 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -package main - -import ( - "go/ast" - "go/token" - "go/types" -) - -func init() { - register("atomic", - "check for common mistaken usages of the sync/atomic package", - checkAtomicAssignment, - assignStmt) -} - -// checkAtomicAssignment walks the assignment statement checking for common -// mistaken usage of atomic package, such as: x = atomic.AddUint64(&x, 1) -func checkAtomicAssignment(f *File, node ast.Node) { - n := node.(*ast.AssignStmt) - if len(n.Lhs) != len(n.Rhs) { - return - } - if len(n.Lhs) == 1 && n.Tok == token.DEFINE { - return - } - - for i, right := range n.Rhs { - call, ok := right.(*ast.CallExpr) - if !ok { - continue - } - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok { - continue - } - pkgIdent, _ := sel.X.(*ast.Ident) - pkgName, ok := f.pkg.uses[pkgIdent].(*types.PkgName) - if !ok || pkgName.Imported().Path() != "sync/atomic" { - continue - } - - switch sel.Sel.Name { - case "AddInt32", "AddInt64", "AddUint32", "AddUint64", "AddUintptr": - f.checkAtomicAddAssignment(n.Lhs[i], call) - } - } -} - -// checkAtomicAddAssignment walks the atomic.Add* method calls checking for assigning the return value -// to the same variable being used in the operation -func (f *File) checkAtomicAddAssignment(left ast.Expr, call *ast.CallExpr) { - if len(call.Args) != 2 { - return - } - arg := call.Args[0] - broken := false - - if uarg, ok := arg.(*ast.UnaryExpr); ok && uarg.Op == token.AND { - broken = f.gofmt(left) == f.gofmt(uarg.X) - } else if star, ok := left.(*ast.StarExpr); ok { - broken = f.gofmt(star.X) == f.gofmt(arg) - } - - if broken { - f.Bad(left.Pos(), "direct assignment to atomic value") - } -}