go/analysis/passes/assign: split out from vet
Also: create internal/analysisutil package for
trivial helper functions shared by many vet analyzers.
(Eventually we may want to export some of these functions.)
Change-Id: I2b721a16989826756d0426bc7f70089dfb1ef9ce
Reviewed-on: https://go-review.googlesource.com/c/140577
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
diff --git a/go/analysis/passes/asmdecl/asmdecl.go b/go/analysis/passes/asmdecl/asmdecl.go
index ecd8b41..bafb39e 100644
--- a/go/analysis/passes/asmdecl/asmdecl.go
+++ b/go/analysis/passes/asmdecl/asmdecl.go
@@ -11,12 +11,12 @@
"go/build"
"go/token"
"go/types"
- "io/ioutil"
"regexp"
"strconv"
"strings"
"golang.org/x/tools/go/analysis"
+ "golang.org/x/tools/go/analysis/passes/internal/analysisutil"
)
var Analyzer = &analysis.Analyzer{
@@ -152,7 +152,7 @@
Files:
for _, fname := range sfiles {
- content, tf, err := readFile(pass.Fset, fname)
+ content, tf, err := analysisutil.ReadFile(pass.Fset, fname)
if err != nil {
return nil, err
}
@@ -182,7 +182,7 @@
if fn != nil && fn.vars["ret"] != nil && !haveRetArg && len(retLine) > 0 {
v := fn.vars["ret"]
for _, line := range retLine {
- pass.Reportf(lineStart(tf, line), "[%s] %s: RET without writing to %d-byte ret+%d(FP)", arch, fnName, v.size, v.off)
+ pass.Reportf(analysisutil.LineStart(tf, line), "[%s] %s: RET without writing to %d-byte ret+%d(FP)", arch, fnName, v.size, v.off)
}
}
retLine = nil
@@ -191,7 +191,7 @@
lineno++
badf := func(format string, args ...interface{}) {
- pass.Reportf(lineStart(tf, lineno), "[%s] %s: %s", arch, fnName, fmt.Sprintf(format, args...))
+ pass.Reportf(analysisutil.LineStart(tf, lineno), "[%s] %s: %s", arch, fnName, fmt.Sprintf(format, args...))
}
if arch == "" {
@@ -738,48 +738,3 @@
badf("invalid %s of %s; %s is %d-byte value%s", op, expr, vt, vs, inner.String())
}
}
-
-// -- a copy of these declarations exists in the buildtag Analyzer ---
-
-// readFile reads a file and adds it to the FileSet
-// so that we can report errors against it using lineStart.
-func readFile(fset *token.FileSet, filename string) ([]byte, *token.File, error) {
- content, err := ioutil.ReadFile(filename)
- if err != nil {
- return nil, nil, err
- }
- tf := fset.AddFile(filename, -1, len(content))
- tf.SetLinesForContent(content)
- return content, tf, nil
-}
-
-// lineStart returns the position of the start of the specified line
-// within file f, or NoPos if there is no line of that number.
-func lineStart(f *token.File, line int) token.Pos {
- // Use binary search to find the start offset of this line.
- //
- // TODO(adonovan): eventually replace this function with the
- // simpler and more efficient (*go/token.File).LineStart, added
- // in go1.12.
-
- min := 0 // inclusive
- max := f.Size() // exclusive
- for {
- offset := (min + max) / 2
- pos := f.Pos(offset)
- posn := f.Position(pos)
- if posn.Line == line {
- return 1 + pos - token.Pos(posn.Column)
- }
-
- if min+1 >= max {
- return token.NoPos
- }
-
- if posn.Line < line {
- min = offset
- } else {
- max = offset
- }
- }
-}
diff --git a/go/analysis/passes/assign/assign.go b/go/analysis/passes/assign/assign.go
new file mode 100644
index 0000000..271b45e
--- /dev/null
+++ b/go/analysis/passes/assign/assign.go
@@ -0,0 +1,65 @@
+// 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 assign
+
+// TODO(adonovan): check also for assignments to struct fields inside
+// methods that are on T instead of *T.
+
+import (
+ "go/ast"
+ "go/token"
+ "reflect"
+
+ "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: "assign",
+ Doc: `check for useless assignments
+
+This checker reports assignments of the form x = x or a[i] = a[i].
+These are almost always useless, and even when they aren't they are
+usually a mistake.`,
+ Requires: []*analysis.Analyzer{inspect.Analyzer},
+ 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(n ast.Node) {
+ stmt := n.(*ast.AssignStmt)
+ if stmt.Tok != token.ASSIGN {
+ return // ignore :=
+ }
+ if len(stmt.Lhs) != len(stmt.Rhs) {
+ // If LHS and RHS have different cardinality, they can't be the same.
+ return
+ }
+ for i, lhs := range stmt.Lhs {
+ rhs := stmt.Rhs[i]
+ if analysisutil.HasSideEffects(pass.TypesInfo, lhs) ||
+ analysisutil.HasSideEffects(pass.TypesInfo, rhs) {
+ continue // expressions may not be equal
+ }
+ if reflect.TypeOf(lhs) != reflect.TypeOf(rhs) {
+ continue // short-circuit the heavy-weight gofmt check
+ }
+ le := analysisutil.Format(pass.Fset, lhs)
+ re := analysisutil.Format(pass.Fset, rhs)
+ if le == re {
+ pass.Reportf(stmt.Pos(), "self-assignment of %s to %s", re, le)
+ }
+ }
+ })
+
+ return nil, nil
+}
diff --git a/go/analysis/passes/assign/assign_test.go b/go/analysis/passes/assign/assign_test.go
new file mode 100644
index 0000000..bde7709
--- /dev/null
+++ b/go/analysis/passes/assign/assign_test.go
@@ -0,0 +1,13 @@
+package assign_test
+
+import (
+ "testing"
+
+ "golang.org/x/tools/go/analysis/analysistest"
+ "golang.org/x/tools/go/analysis/passes/assign"
+)
+
+func Test(t *testing.T) {
+ testdata := analysistest.TestData()
+ analysistest.Run(t, testdata, assign.Analyzer, "a")
+}
diff --git a/go/analysis/passes/vet/testdata/assign.go b/go/analysis/passes/assign/testdata/src/a/a.go
similarity index 79%
rename from go/analysis/passes/vet/testdata/assign.go
rename to go/analysis/passes/assign/testdata/src/a/a.go
index 6140ad4..aa07ebe 100644
--- a/go/analysis/passes/vet/testdata/assign.go
+++ b/go/analysis/passes/assign/testdata/src/a/a.go
@@ -15,11 +15,11 @@
func (s *ST) SetX(x int, ch chan int) {
// Accidental self-assignment; it should be "s.x = x"
- x = x // ERROR "self-assignment of x to x"
+ x = x // want "self-assignment of x to x"
// Another mistake
- s.x = s.x // ERROR "self-assignment of s.x to s.x"
+ s.x = s.x // want "self-assignment of s.x to s.x"
- s.l[0] = s.l[0] // ERROR "self-assignment of s.l.0. to s.l.0."
+ s.l[0] = s.l[0] // want "self-assignment of s.l.0. to s.l.0."
// Bail on any potential side effects to avoid false positives
s.l[num()] = s.l[num()]
diff --git a/go/analysis/passes/buildtag/buildtag.go b/go/analysis/passes/buildtag/buildtag.go
index 6368786..7187afa 100644
--- a/go/analysis/passes/buildtag/buildtag.go
+++ b/go/analysis/passes/buildtag/buildtag.go
@@ -8,12 +8,11 @@
"bytes"
"fmt"
"go/ast"
- "go/token"
- "io/ioutil"
"strings"
"unicode"
"golang.org/x/tools/go/analysis"
+ "golang.org/x/tools/go/analysis/passes/internal/analysisutil"
)
var Analyzer = &analysis.Analyzer{
@@ -61,7 +60,7 @@
}
func checkOtherFile(pass *analysis.Pass, filename string) error {
- content, tf, err := readFile(pass.Fset, filename)
+ content, tf, err := analysisutil.ReadFile(pass.Fset, filename)
if err != nil {
return err
}
@@ -96,7 +95,7 @@
continue
}
if err := checkLine(string(line), i >= cutoff); err != nil {
- pass.Reportf(lineStart(tf, i+1), "%s", err)
+ pass.Reportf(analysisutil.LineStart(tf, i+1), "%s", err)
continue
}
}
@@ -157,48 +156,3 @@
nl = []byte("\n")
slashSlash = []byte("//")
)
-
-// -- these declarations are copied from asmdecl --
-
-// readFile reads a file and adds it to the FileSet
-// so that we can report errors against it using lineStart.
-func readFile(fset *token.FileSet, filename string) ([]byte, *token.File, error) {
- content, err := ioutil.ReadFile(filename)
- if err != nil {
- return nil, nil, err
- }
- tf := fset.AddFile(filename, -1, len(content))
- tf.SetLinesForContent(content)
- return content, tf, nil
-}
-
-// lineStart returns the position of the start of the specified line
-// within file f, or NoPos if there is no line of that number.
-func lineStart(f *token.File, line int) token.Pos {
- // Use binary search to find the start offset of this line.
- //
- // TODO(adonovan): eventually replace this function with the
- // simpler and more efficient (*go/token.File).LineStart, added
- // in go1.12.
-
- min := 0 // inclusive
- max := f.Size() // exclusive
- for {
- offset := (min + max) / 2
- pos := f.Pos(offset)
- posn := f.Position(pos)
- if posn.Line == line {
- return pos - (token.Pos(posn.Column) - 1)
- }
-
- if min+1 >= max {
- return token.NoPos
- }
-
- if posn.Line < line {
- min = offset
- } else {
- max = offset
- }
- }
-}
diff --git a/go/analysis/passes/internal/analysisutil/util.go b/go/analysis/passes/internal/analysisutil/util.go
new file mode 100644
index 0000000..13a458d
--- /dev/null
+++ b/go/analysis/passes/internal/analysisutil/util.go
@@ -0,0 +1,106 @@
+// Package analysisutil defines various helper functions
+// used by two or more packages beneath go/analysis.
+package analysisutil
+
+import (
+ "bytes"
+ "go/ast"
+ "go/printer"
+ "go/token"
+ "go/types"
+ "io/ioutil"
+)
+
+// Format returns a string representation of the expression.
+func Format(fset *token.FileSet, x ast.Expr) string {
+ var b bytes.Buffer
+ printer.Fprint(&b, fset, x)
+ return b.String()
+}
+
+// HasSideEffects reports whether evaluation of e has side effects.
+func HasSideEffects(info *types.Info, e ast.Expr) bool {
+ safe := true
+ ast.Inspect(e, func(node ast.Node) bool {
+ switch n := node.(type) {
+ case *ast.CallExpr:
+ typVal := info.Types[n.Fun]
+ switch {
+ case typVal.IsType():
+ // Type conversion, which is safe.
+ case typVal.IsBuiltin():
+ // Builtin func, conservatively assumed to not
+ // be safe for now.
+ safe = false
+ return false
+ default:
+ // A non-builtin func or method call.
+ // Conservatively assume that all of them have
+ // side effects for now.
+ safe = false
+ return false
+ }
+ case *ast.UnaryExpr:
+ if n.Op == token.ARROW {
+ safe = false
+ return false
+ }
+ }
+ return true
+ })
+ return !safe
+}
+
+// Unparen returns e with any enclosing parentheses stripped.
+func Unparen(e ast.Expr) ast.Expr {
+ for {
+ p, ok := e.(*ast.ParenExpr)
+ if !ok {
+ return e
+ }
+ e = p.X
+ }
+}
+
+// ReadFile reads a file and adds it to the FileSet
+// so that we can report errors against it using lineStart.
+func ReadFile(fset *token.FileSet, filename string) ([]byte, *token.File, error) {
+ content, err := ioutil.ReadFile(filename)
+ if err != nil {
+ return nil, nil, err
+ }
+ tf := fset.AddFile(filename, -1, len(content))
+ tf.SetLinesForContent(content)
+ return content, tf, nil
+}
+
+// LineStart returns the position of the start of the specified line
+// within file f, or NoPos if there is no line of that number.
+func LineStart(f *token.File, line int) token.Pos {
+ // Use binary search to find the start offset of this line.
+ //
+ // TODO(adonovan): eventually replace this function with the
+ // simpler and more efficient (*go/token.File).LineStart, added
+ // in go1.12.
+
+ min := 0 // inclusive
+ max := f.Size() // exclusive
+ for {
+ offset := (min + max) / 2
+ pos := f.Pos(offset)
+ posn := f.Position(pos)
+ if posn.Line == line {
+ return pos - (token.Pos(posn.Column) - 1)
+ }
+
+ if min+1 >= max {
+ return token.NoPos
+ }
+
+ if posn.Line < line {
+ min = offset
+ } else {
+ max = offset
+ }
+ }
+}
diff --git a/go/analysis/passes/vet/assign.go b/go/analysis/passes/vet/assign.go
deleted file mode 100644
index 40ced75..0000000
--- a/go/analysis/passes/vet/assign.go
+++ /dev/null
@@ -1,54 +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.
-
-/*
-This file contains the code to check for useless assignments.
-*/
-
-package main
-
-import (
- "go/ast"
- "go/token"
- "reflect"
-)
-
-func init() {
- register("assign",
- "check for useless assignments",
- checkAssignStmt,
- assignStmt)
-}
-
-// TODO: should also check for assignments to struct fields inside methods
-// that are on T instead of *T.
-
-// checkAssignStmt checks for assignments of the form "<expr> = <expr>".
-// These are almost always useless, and even when they aren't they are usually a mistake.
-func checkAssignStmt(f *File, node ast.Node) {
- stmt := node.(*ast.AssignStmt)
- if stmt.Tok != token.ASSIGN {
- return // ignore :=
- }
- if len(stmt.Lhs) != len(stmt.Rhs) {
- // If LHS and RHS have different cardinality, they can't be the same.
- return
- }
- for i, lhs := range stmt.Lhs {
- rhs := stmt.Rhs[i]
- if hasSideEffects(f, lhs) || hasSideEffects(f, rhs) {
- continue // expressions may not be equal
- }
- if reflect.TypeOf(lhs) != reflect.TypeOf(rhs) {
- continue // short-circuit the heavy-weight gofmt check
- }
- le := f.gofmt(lhs)
- re := f.gofmt(rhs)
- if le == re {
- f.Badf(stmt.Pos(), "self-assignment of %s to %s", re, le)
- }
- }
-}
