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>

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
-		}
-	}
-}

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
+}

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")
+}

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()]

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
-		}
-	}
-}

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
+		}
+	}
+}

go/analysis/passes/vet/assign.go

@@ -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)
-		}
-	}
-}