go/analysis/passes/shift: split out of vet

Change-Id: I9c0c86e7aee4f7bb9239dc4b4836df9bba471b03
Reviewed-on: https://go-review.googlesource.com/c/140757
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>

go/analysis/passes/shift/dead.go

@@ -1,39 +1,50 @@
-// +build ignore
-
 // Copyright 2017 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.
-//
-// Simplified dead code detector. Used for skipping certain checks
-// on unreachable code (for instance, shift checks on arch-specific code).
 
-package main
+package shift
+
+// Simplified dead code detector.
+// Used for skipping shift checks on unreachable arch-specific code.
 
 import (
 	"go/ast"
 	"go/constant"
+	"go/types"
 )
 
-// updateDead puts unreachable "if" and "case" nodes into f.dead.
+// updateDead puts unreachable "if" and "case" nodes into dead.
-func (f *File) updateDead(node ast.Node) {
+func updateDead(info *types.Info, dead map[ast.Node]bool, node ast.Node) {
-	if f.dead[node] {
+	if dead[node] {
 		// The node is already marked as dead.
 		return
 	}
 
+	// setDead marks the node and all the children as dead.
+	setDead := func(n ast.Node) {
+		ast.Inspect(n, func(node ast.Node) bool {
+			if node != nil {
+				dead[node] = true
+			}
+			return true
+		})
+	}
+
 	switch stmt := node.(type) {
 	case *ast.IfStmt:
 		// "if" branch is dead if its condition evaluates
 		// to constant false.
-		v := f.pkg.types[stmt.Cond].Value
+		v := info.Types[stmt.Cond].Value
 		if v == nil {
 			return
 		}
 		if !constant.BoolVal(v) {
-			f.setDead(stmt.Body)
+			setDead(stmt.Body)
 			return
 		}
-		f.setDead(stmt.Else)
+		if stmt.Else != nil {
+			setDead(stmt.Else)
+		}
 	case *ast.SwitchStmt:
 		// Case clause with empty switch tag is dead if it evaluates
 		// to constant false.
@@ -46,12 +57,12 @@ func (f *File) updateDead(node ast.Node) {
 					continue
 				}
 				for _, expr := range cc.List {
-					v := f.pkg.types[expr].Value
+					v := info.Types[expr].Value
 					if v == nil || v.Kind() != constant.Bool || constant.BoolVal(v) {
 						continue BodyLoopBool
 					}
 				}
-				f.setDead(cc)
+				setDead(cc)
 			}
 			return
 		}
@@ -59,7 +70,7 @@ func (f *File) updateDead(node ast.Node) {
 		// Case clause is dead if its constant value doesn't match
 		// the constant value from the switch tag.
 		// TODO: This handles integer comparisons only.
-		v := f.pkg.types[stmt.Tag].Value
+		v := info.Types[stmt.Tag].Value
 		if v == nil || v.Kind() != constant.Int {
 			return
 		}
@@ -75,7 +86,7 @@ func (f *File) updateDead(node ast.Node) {
 				continue
 			}
 			for _, expr := range cc.List {
-				v := f.pkg.types[expr].Value
+				v := info.Types[expr].Value
 				if v == nil {
 					continue BodyLoopInt
 				}
@@ -84,27 +95,7 @@ func (f *File) updateDead(node ast.Node) {
 					continue BodyLoopInt
 				}
 			}
-			f.setDead(cc)
+			setDead(cc)
 		}
 	}
 }
-
-// setDead marks the node and all the children as dead.
-func (f *File) setDead(node ast.Node) {
-	dv := deadVisitor{
-		f: f,
-	}
-	ast.Walk(dv, node)
-}
-
-type deadVisitor struct {
-	f *File
-}
-
-func (dv deadVisitor) Visit(node ast.Node) ast.Visitor {
-	if node == nil {
-		return nil
-	}
-	dv.f.dead[node] = true
-	return dv
-}

go/analysis/passes/shift/shift.go

@@ -0,0 +1,126 @@
+// Copyright 2014 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 shift
+
+// TODO(adonovan): integrate with ctrflow (CFG-based) dead code analysis. May
+// have impedance mismatch due to its (non-)treatment of constant
+// expressions (such as runtime.GOARCH=="386").
+
+import (
+	"go/ast"
+	"go/build"
+	"go/constant"
+	"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:     "shift",
+	Doc:      "check for shifts that equal or exceed the width of the integer",
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	// Do a complete pass to compute dead nodes.
+	dead := make(map[ast.Node]bool)
+	nodeFilter := []ast.Node{
+		(*ast.IfStmt)(nil),
+		(*ast.SwitchStmt)(nil),
+	}
+	inspect.Preorder(nodeFilter, func(n ast.Node) {
+		// TODO(adonovan): move updateDead into this file.
+		updateDead(pass.TypesInfo, dead, n)
+	})
+
+	nodeFilter = []ast.Node{
+		(*ast.AssignStmt)(nil),
+		(*ast.BinaryExpr)(nil),
+	}
+	inspect.Preorder(nodeFilter, func(node ast.Node) {
+		if dead[node] {
+			// Skip shift checks on unreachable nodes.
+			return
+		}
+
+		switch node := node.(type) {
+		case *ast.BinaryExpr:
+			if node.Op == token.SHL || node.Op == token.SHR {
+				checkLongShift(pass, node, node.X, node.Y)
+			}
+		case *ast.AssignStmt:
+			if len(node.Lhs) != 1 || len(node.Rhs) != 1 {
+				return
+			}
+			if node.Tok == token.SHL_ASSIGN || node.Tok == token.SHR_ASSIGN {
+				checkLongShift(pass, node, node.Lhs[0], node.Rhs[0])
+			}
+		}
+	})
+	return nil, nil
+}
+
+// checkLongShift checks if shift or shift-assign operations shift by more than
+// the length of the underlying variable.
+func checkLongShift(pass *analysis.Pass, node ast.Node, x, y ast.Expr) {
+	if pass.TypesInfo.Types[x].Value != nil {
+		// Ignore shifts of constants.
+		// These are frequently used for bit-twiddling tricks
+		// like ^uint(0) >> 63 for 32/64 bit detection and compatibility.
+		return
+	}
+
+	v := pass.TypesInfo.Types[y].Value
+	if v == nil {
+		return
+	}
+	amt, ok := constant.Int64Val(v)
+	if !ok {
+		return
+	}
+	t := pass.TypesInfo.Types[x].Type
+	if t == nil {
+		return
+	}
+	b, ok := t.Underlying().(*types.Basic)
+	if !ok {
+		return
+	}
+	var size int64
+	switch b.Kind() {
+	case types.Uint8, types.Int8:
+		size = 8
+	case types.Uint16, types.Int16:
+		size = 16
+	case types.Uint32, types.Int32:
+		size = 32
+	case types.Uint64, types.Int64:
+		size = 64
+	case types.Int, types.Uint:
+		size = uintBitSize
+	case types.Uintptr:
+		size = uintptrBitSize
+	default:
+		return
+	}
+	if amt >= size {
+		ident := analysisutil.Format(pass.Fset, x)
+		pass.Reportf(node.Pos(), "%s (%d bits) too small for shift of %d", ident, size, amt)
+	}
+}
+
+var (
+	uintBitSize    = 8 * archSizes.Sizeof(types.Typ[types.Uint])
+	uintptrBitSize = 8 * archSizes.Sizeof(types.Typ[types.Uintptr])
+)
+
+var archSizes = types.SizesFor("gc", build.Default.GOARCH)

go/analysis/passes/shift/shift_test.go

@@ -0,0 +1,13 @@
+package shift_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/go/analysis/passes/shift"
+)
+
+func TestFromFileSystem(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, shift.Analyzer, "a")
+}

go/analysis/passes/shift/testdata/src/a/a.go

@@ -0,0 +1,155 @@
+// Copyright 2014 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 tests for the suspicious shift checker.
+
+package shift
+
+import (
+	"unsafe"
+)
+
+func ShiftTest() {
+	var i8 int8
+	_ = i8 << 7
+	_ = (i8 + 1) << 8 // want ".i8 . 1. .8 bits. too small for shift of 8"
+	_ = i8 << (7 + 1) // want "i8 .8 bits. too small for shift of 8"
+	_ = i8 >> 8       // want "i8 .8 bits. too small for shift of 8"
+	i8 <<= 8          // want "i8 .8 bits. too small for shift of 8"
+	i8 >>= 8          // want "i8 .8 bits. too small for shift of 8"
+	var i16 int16
+	_ = i16 << 15
+	_ = i16 << 16 // want "i16 .16 bits. too small for shift of 16"
+	_ = i16 >> 16 // want "i16 .16 bits. too small for shift of 16"
+	i16 <<= 16    // want "i16 .16 bits. too small for shift of 16"
+	i16 >>= 16    // want "i16 .16 bits. too small for shift of 16"
+	var i32 int32
+	_ = i32 << 31
+	_ = i32 << 32 // want "i32 .32 bits. too small for shift of 32"
+	_ = i32 >> 32 // want "i32 .32 bits. too small for shift of 32"
+	i32 <<= 32    // want "i32 .32 bits. too small for shift of 32"
+	i32 >>= 32    // want "i32 .32 bits. too small for shift of 32"
+	var i64 int64
+	_ = i64 << 63
+	_ = i64 << 64 // want "i64 .64 bits. too small for shift of 64"
+	_ = i64 >> 64 // want "i64 .64 bits. too small for shift of 64"
+	i64 <<= 64    // want "i64 .64 bits. too small for shift of 64"
+	i64 >>= 64    // want "i64 .64 bits. too small for shift of 64"
+	var u8 uint8
+	_ = u8 << 7
+	_ = u8 << 8 // want "u8 .8 bits. too small for shift of 8"
+	_ = u8 >> 8 // want "u8 .8 bits. too small for shift of 8"
+	u8 <<= 8    // want "u8 .8 bits. too small for shift of 8"
+	u8 >>= 8    // want "u8 .8 bits. too small for shift of 8"
+	var u16 uint16
+	_ = u16 << 15
+	_ = u16 << 16 // want "u16 .16 bits. too small for shift of 16"
+	_ = u16 >> 16 // want "u16 .16 bits. too small for shift of 16"
+	u16 <<= 16    // want "u16 .16 bits. too small for shift of 16"
+	u16 >>= 16    // want "u16 .16 bits. too small for shift of 16"
+	var u32 uint32
+	_ = u32 << 31
+	_ = u32 << 32 // want "u32 .32 bits. too small for shift of 32"
+	_ = u32 >> 32 // want "u32 .32 bits. too small for shift of 32"
+	u32 <<= 32    // want "u32 .32 bits. too small for shift of 32"
+	u32 >>= 32    // want "u32 .32 bits. too small for shift of 32"
+	var u64 uint64
+	_ = u64 << 63
+	_ = u64 << 64  // want "u64 .64 bits. too small for shift of 64"
+	_ = u64 >> 64  // want "u64 .64 bits. too small for shift of 64"
+	u64 <<= 64     // want "u64 .64 bits. too small for shift of 64"
+	u64 >>= 64     // want "u64 .64 bits. too small for shift of 64"
+	_ = u64 << u64 // Non-constant shifts should succeed.
+
+	var i int
+	_ = i << 31
+	const in = 8 * unsafe.Sizeof(i)
+	_ = i << in // want "too small for shift"
+	_ = i >> in // want "too small for shift"
+	i <<= in    // want "too small for shift"
+	i >>= in    // want "too small for shift"
+	const ix = 8*unsafe.Sizeof(i) - 1
+	_ = i << ix
+	_ = i >> ix
+	i <<= ix
+	i >>= ix
+
+	var u uint
+	_ = u << 31
+	const un = 8 * unsafe.Sizeof(u)
+	_ = u << un // want "too small for shift"
+	_ = u >> un // want "too small for shift"
+	u <<= un    // want "too small for shift"
+	u >>= un    // want "too small for shift"
+	const ux = 8*unsafe.Sizeof(u) - 1
+	_ = u << ux
+	_ = u >> ux
+	u <<= ux
+	u >>= ux
+
+	var p uintptr
+	_ = p << 31
+	const pn = 8 * unsafe.Sizeof(p)
+	_ = p << pn // want "too small for shift"
+	_ = p >> pn // want "too small for shift"
+	p <<= pn    // want "too small for shift"
+	p >>= pn    // want "too small for shift"
+	const px = 8*unsafe.Sizeof(p) - 1
+	_ = p << px
+	_ = p >> px
+	p <<= px
+	p >>= px
+
+	const oneIf64Bit = ^uint(0) >> 63 // allow large shifts of constants; they are used for 32/64 bit compatibility tricks
+
+	var h uintptr
+	h = h<<8 | (h >> (8 * (unsafe.Sizeof(h) - 1)))
+	h <<= 8 * unsafe.Sizeof(h) // want "too small for shift"
+	h >>= 7 * unsafe.Alignof(h)
+	h >>= 8 * unsafe.Alignof(h) // want "too small for shift"
+}
+
+func ShiftDeadCode() {
+	var i int
+	const iBits = 8 * unsafe.Sizeof(i)
+
+	if iBits <= 32 {
+		if iBits == 16 {
+			_ = i >> 8
+		} else {
+			_ = i >> 16
+		}
+	} else {
+		_ = i >> 32
+	}
+
+	if iBits >= 64 {
+		_ = i << 32
+		if iBits == 128 {
+			_ = i << 64
+		}
+	} else {
+		_ = i << 16
+	}
+
+	if iBits == 64 {
+		_ = i << 32
+	}
+
+	switch iBits {
+	case 128, 64:
+		_ = i << 32
+	default:
+		_ = i << 16
+	}
+
+	switch {
+	case iBits < 32:
+		_ = i << 16
+	case iBits > 64:
+		_ = i << 64
+	default:
+		_ = i << 64 // want "too small for shift"
+	}
+}

go/analysis/passes/vet/shift.go

@@ -1,100 +0,0 @@
-// +build ignore
-
-// Copyright 2014 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 suspicious shifts.
-*/
-
-package main
-
-import (
-	"go/ast"
-	"go/constant"
-	"go/token"
-	"go/types"
-)
-
-func init() {
-	register("shift",
-		"check for useless shifts",
-		checkShift,
-		binaryExpr, assignStmt)
-}
-
-func checkShift(f *File, node ast.Node) {
-	if f.dead[node] {
-		// Skip shift checks on unreachable nodes.
-		return
-	}
-
-	switch node := node.(type) {
-	case *ast.BinaryExpr:
-		if node.Op == token.SHL || node.Op == token.SHR {
-			checkLongShift(f, node, node.X, node.Y)
-		}
-	case *ast.AssignStmt:
-		if len(node.Lhs) != 1 || len(node.Rhs) != 1 {
-			return
-		}
-		if node.Tok == token.SHL_ASSIGN || node.Tok == token.SHR_ASSIGN {
-			checkLongShift(f, node, node.Lhs[0], node.Rhs[0])
-		}
-	}
-}
-
-// checkLongShift checks if shift or shift-assign operations shift by more than
-// the length of the underlying variable.
-func checkLongShift(f *File, node ast.Node, x, y ast.Expr) {
-	if f.pkg.types[x].Value != nil {
-		// Ignore shifts of constants.
-		// These are frequently used for bit-twiddling tricks
-		// like ^uint(0) >> 63 for 32/64 bit detection and compatibility.
-		return
-	}
-
-	v := f.pkg.types[y].Value
-	if v == nil {
-		return
-	}
-	amt, ok := constant.Int64Val(v)
-	if !ok {
-		return
-	}
-	t := f.pkg.types[x].Type
-	if t == nil {
-		return
-	}
-	b, ok := t.Underlying().(*types.Basic)
-	if !ok {
-		return
-	}
-	var size int64
-	switch b.Kind() {
-	case types.Uint8, types.Int8:
-		size = 8
-	case types.Uint16, types.Int16:
-		size = 16
-	case types.Uint32, types.Int32:
-		size = 32
-	case types.Uint64, types.Int64:
-		size = 64
-	case types.Int, types.Uint:
-		size = uintBitSize
-	case types.Uintptr:
-		size = uintptrBitSize
-	default:
-		return
-	}
-	if amt >= size {
-		ident := f.gofmt(x)
-		f.Badf(node.Pos(), "%s (%d bits) too small for shift of %d", ident, size, amt)
-	}
-}
-
-var (
-	uintBitSize    = 8 * archSizes.Sizeof(types.Typ[types.Uint])
-	uintptrBitSize = 8 * archSizes.Sizeof(types.Typ[types.Uintptr])
-)

go/analysis/passes/vet/testdata/shift.go

@@ -1,162 +0,0 @@
-// Copyright 2014 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 tests for the suspicious shift checker.
-
-package testdata
-
-import (
-	"fmt"
-	"unsafe"
-)
-
-func ShiftTest() {
-	var i8 int8
-	_ = i8 << 7
-	_ = (i8 + 1) << 8 // ERROR ".i8 . 1. .8 bits. too small for shift of 8"
-	_ = i8 << (7 + 1) // ERROR "i8 .8 bits. too small for shift of 8"
-	_ = i8 >> 8       // ERROR "i8 .8 bits. too small for shift of 8"
-	i8 <<= 8          // ERROR "i8 .8 bits. too small for shift of 8"
-	i8 >>= 8          // ERROR "i8 .8 bits. too small for shift of 8"
-	var i16 int16
-	_ = i16 << 15
-	_ = i16 << 16 // ERROR "i16 .16 bits. too small for shift of 16"
-	_ = i16 >> 16 // ERROR "i16 .16 bits. too small for shift of 16"
-	i16 <<= 16    // ERROR "i16 .16 bits. too small for shift of 16"
-	i16 >>= 16    // ERROR "i16 .16 bits. too small for shift of 16"
-	var i32 int32
-	_ = i32 << 31
-	_ = i32 << 32 // ERROR "i32 .32 bits. too small for shift of 32"
-	_ = i32 >> 32 // ERROR "i32 .32 bits. too small for shift of 32"
-	i32 <<= 32    // ERROR "i32 .32 bits. too small for shift of 32"
-	i32 >>= 32    // ERROR "i32 .32 bits. too small for shift of 32"
-	var i64 int64
-	_ = i64 << 63
-	_ = i64 << 64 // ERROR "i64 .64 bits. too small for shift of 64"
-	_ = i64 >> 64 // ERROR "i64 .64 bits. too small for shift of 64"
-	i64 <<= 64    // ERROR "i64 .64 bits. too small for shift of 64"
-	i64 >>= 64    // ERROR "i64 .64 bits. too small for shift of 64"
-	var u8 uint8
-	_ = u8 << 7
-	_ = u8 << 8 // ERROR "u8 .8 bits. too small for shift of 8"
-	_ = u8 >> 8 // ERROR "u8 .8 bits. too small for shift of 8"
-	u8 <<= 8    // ERROR "u8 .8 bits. too small for shift of 8"
-	u8 >>= 8    // ERROR "u8 .8 bits. too small for shift of 8"
-	var u16 uint16
-	_ = u16 << 15
-	_ = u16 << 16 // ERROR "u16 .16 bits. too small for shift of 16"
-	_ = u16 >> 16 // ERROR "u16 .16 bits. too small for shift of 16"
-	u16 <<= 16    // ERROR "u16 .16 bits. too small for shift of 16"
-	u16 >>= 16    // ERROR "u16 .16 bits. too small for shift of 16"
-	var u32 uint32
-	_ = u32 << 31
-	_ = u32 << 32 // ERROR "u32 .32 bits. too small for shift of 32"
-	_ = u32 >> 32 // ERROR "u32 .32 bits. too small for shift of 32"
-	u32 <<= 32    // ERROR "u32 .32 bits. too small for shift of 32"
-	u32 >>= 32    // ERROR "u32 .32 bits. too small for shift of 32"
-	var u64 uint64
-	_ = u64 << 63
-	_ = u64 << 64  // ERROR "u64 .64 bits. too small for shift of 64"
-	_ = u64 >> 64  // ERROR "u64 .64 bits. too small for shift of 64"
-	u64 <<= 64     // ERROR "u64 .64 bits. too small for shift of 64"
-	u64 >>= 64     // ERROR "u64 .64 bits. too small for shift of 64"
-	_ = u64 << u64 // Non-constant shifts should succeed.
-
-	var i int
-	_ = i << 31
-	const in = 8 * unsafe.Sizeof(i)
-	_ = i << in // ERROR "too small for shift"
-	_ = i >> in // ERROR "too small for shift"
-	i <<= in    // ERROR "too small for shift"
-	i >>= in    // ERROR "too small for shift"
-	const ix = 8*unsafe.Sizeof(i) - 1
-	_ = i << ix
-	_ = i >> ix
-	i <<= ix
-	i >>= ix
-
-	var u uint
-	_ = u << 31
-	const un = 8 * unsafe.Sizeof(u)
-	_ = u << un // ERROR "too small for shift"
-	_ = u >> un // ERROR "too small for shift"
-	u <<= un    // ERROR "too small for shift"
-	u >>= un    // ERROR "too small for shift"
-	const ux = 8*unsafe.Sizeof(u) - 1
-	_ = u << ux
-	_ = u >> ux
-	u <<= ux
-	u >>= ux
-
-	var p uintptr
-	_ = p << 31
-	const pn = 8 * unsafe.Sizeof(p)
-	_ = p << pn // ERROR "too small for shift"
-	_ = p >> pn // ERROR "too small for shift"
-	p <<= pn    // ERROR "too small for shift"
-	p >>= pn    // ERROR "too small for shift"
-	const px = 8*unsafe.Sizeof(p) - 1
-	_ = p << px
-	_ = p >> px
-	p <<= px
-	p >>= px
-
-	const oneIf64Bit = ^uint(0) >> 63 // allow large shifts of constants; they are used for 32/64 bit compatibility tricks
-
-	var h uintptr
-	h = h<<8 | (h >> (8 * (unsafe.Sizeof(h) - 1)))
-	h <<= 8 * unsafe.Sizeof(h) // ERROR "too small for shift"
-	h >>= 7 * unsafe.Alignof(h)
-	h >>= 8 * unsafe.Alignof(h) // ERROR "too small for shift"
-}
-
-func ShiftDeadCode() {
-	var i int
-	const iBits = 8 * unsafe.Sizeof(i)
-
-	if iBits <= 32 {
-		if iBits == 16 {
-			_ = i >> 8
-		} else {
-			_ = i >> 16
-		}
-	} else {
-		_ = i >> 32
-	}
-
-	if iBits >= 64 {
-		_ = i << 32
-		if iBits == 128 {
-			_ = i << 64
-		}
-	} else {
-		_ = i << 16
-	}
-
-	if iBits == 64 {
-		_ = i << 32
-	}
-
-	switch iBits {
-	case 128, 64:
-		_ = i << 32
-	default:
-		_ = i << 16
-	}
-
-	switch {
-	case iBits < 32:
-		_ = i << 16
-	case iBits > 64:
-		_ = i << 64
-	default:
-		_ = i << 64 // ERROR "too small for shift"
-	}
-
-	// Make sure other vet checks work in dead code.
-	if iBits == 1024 {
-		_ = i << 512                  // OK
-		fmt.Printf("foo %s bar", 123) // ERROR "Printf"
-	}
-}