From a2b3f7f249e90c6fe7ca632df1cbbb6fd73c0cfa Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 8 Oct 2018 13:56:31 -0400 Subject: [PATCH] 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 Run-TryBot: Michael Matloob --- go/analysis/passes/asmdecl/asmdecl.go | 53 +-------- go/analysis/passes/assign/assign.go | 65 +++++++++++ go/analysis/passes/assign/assign_test.go | 13 +++ .../assign.go => assign/testdata/src/a/a.go} | 6 +- go/analysis/passes/buildtag/buildtag.go | 52 +-------- .../passes/internal/analysisutil/util.go | 106 ++++++++++++++++++ go/analysis/passes/vet/assign.go | 54 --------- 7 files changed, 194 insertions(+), 155 deletions(-) create mode 100644 go/analysis/passes/assign/assign.go create mode 100644 go/analysis/passes/assign/assign_test.go rename go/analysis/passes/{vet/testdata/assign.go => assign/testdata/src/a/a.go} (80%) create mode 100644 go/analysis/passes/internal/analysisutil/util.go delete mode 100644 go/analysis/passes/vet/assign.go diff --git a/go/analysis/passes/asmdecl/asmdecl.go b/go/analysis/passes/asmdecl/asmdecl.go index ecd8b412ecf..bafb39e459a 100644 --- a/go/analysis/passes/asmdecl/asmdecl.go +++ b/go/analysis/passes/asmdecl/asmdecl.go @@ -11,12 +11,12 @@ import ( "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 @@ func run(pass *analysis.Pass) (interface{}, error) { 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 @@ Files: 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 @@ Files: 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 @@ func asmCheckVar(badf func(string, ...interface{}), fn *asmFunc, line, expr stri 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 00000000000..271b45e9cf5 --- /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 00000000000..bde77095f3a --- /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 80% rename from go/analysis/passes/vet/testdata/assign.go rename to go/analysis/passes/assign/testdata/src/a/a.go index 6140ad4db8c..aa07ebef2bf 100644 --- a/go/analysis/passes/vet/testdata/assign.go +++ b/go/analysis/passes/assign/testdata/src/a/a.go @@ -15,11 +15,11 @@ type ST struct { 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 6368786485d..7187afaae55 100644 --- a/go/analysis/passes/buildtag/buildtag.go +++ b/go/analysis/passes/buildtag/buildtag.go @@ -8,12 +8,11 @@ import ( "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 checkGoFile(pass *analysis.Pass, f *ast.File) { } 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 @@ func checkOtherFile(pass *analysis.Pass, filename string) error { 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 @@ var ( 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 00000000000..13a458d9d6b --- /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 40ced75200a..00000000000 --- 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 " = ". -// 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) - } - } -}