diff --git a/go/analysis/passes/vet/bool.go b/go/analysis/passes/bools/bools.go similarity index 66% rename from go/analysis/passes/vet/bool.go rename to go/analysis/passes/bools/bools.go index 56e33ece..40ca730d 100644 --- a/go/analysis/passes/vet/bool.go +++ b/go/analysis/passes/bools/bools.go @@ -1,43 +1,56 @@ -// +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 boolean condition tests. - -package main +package bools 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" ) -func init() { - register("bool", - "check for mistakes involving boolean operators", - checkBool, - binaryExpr) +var Analyzer = &analysis.Analyzer{ + Name: "bools", + Doc: "check for common mistakes involving boolean operators", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, } -func checkBool(f *File, n ast.Node) { - e := n.(*ast.BinaryExpr) +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - var op boolOp - switch e.Op { - case token.LOR: - op = or - case token.LAND: - op = and - default: - return + nodeFilter := []ast.Node{ + (*ast.BinaryExpr)(nil), } + inspect.Preorder(nodeFilter, func(n ast.Node) { + e := n.(*ast.BinaryExpr) - comm := op.commutativeSets(f, e) - for _, exprs := range comm { - op.checkRedundant(f, exprs) - op.checkSuspect(f, exprs) - } + var op boolOp + switch e.Op { + case token.LOR: + op = or + case token.LAND: + op = and + default: + return + } + + // TODO(adonovan): this reports n(n-1)/2 errors for an + // expression e||...||e of depth n. Fix. + // See https://github.com/golang/go/issues/28086. + comm := op.commutativeSets(pass.TypesInfo, e) + for _, exprs := range comm { + op.checkRedundant(pass, exprs) + op.checkSuspect(pass, exprs) + } + }) + return nil, nil } type boolOp struct { @@ -55,14 +68,14 @@ var ( // expressions in e that are connected by op. // For example, given 'a || b || f() || c || d' with the or op, // commutativeSets returns {{b, a}, {d, c}}. -func (op boolOp) commutativeSets(f *File, e *ast.BinaryExpr) [][]ast.Expr { +func (op boolOp) commutativeSets(info *types.Info, e *ast.BinaryExpr) [][]ast.Expr { exprs := op.split(e) // Partition the slice of expressions into commutative sets. i := 0 var sets [][]ast.Expr for j := 0; j <= len(exprs); j++ { - if j == len(exprs) || hasSideEffects(f, exprs[j]) { + if j == len(exprs) || hasSideEffects(info, exprs[j]) { if i < j { sets = append(sets, exprs[i:j]) } @@ -77,12 +90,12 @@ func (op boolOp) commutativeSets(f *File, e *ast.BinaryExpr) [][]ast.Expr { // e && e // e || e // Exprs must contain only side effect free expressions. -func (op boolOp) checkRedundant(f *File, exprs []ast.Expr) { +func (op boolOp) checkRedundant(pass *analysis.Pass, exprs []ast.Expr) { seen := make(map[string]bool) for _, e := range exprs { - efmt := f.gofmt(e) + efmt := analysisutil.Format(pass.Fset, e) if seen[efmt] { - f.Badf(e.Pos(), "redundant %s: %s %s %s", op.name, efmt, op.tok, efmt) + pass.Reportf(e.Pos(), "redundant %s: %s %s %s", op.name, efmt, op.tok, efmt) } else { seen[efmt] = true } @@ -96,7 +109,7 @@ func (op boolOp) checkRedundant(f *File, exprs []ast.Expr) { // If c1 and c2 are the same then it's redundant; // if c1 and c2 are different then it's always true or always false. // Exprs must contain only side effect free expressions. -func (op boolOp) checkSuspect(f *File, exprs []ast.Expr) { +func (op boolOp) checkSuspect(pass *analysis.Pass, exprs []ast.Expr) { // seen maps from expressions 'x' to equality expressions 'x != c'. seen := make(map[string]string) @@ -115,21 +128,21 @@ func (op boolOp) checkSuspect(f *File, exprs []ast.Expr) { // code is written. var x ast.Expr switch { - case f.pkg.types[bin.Y].Value != nil: + case pass.TypesInfo.Types[bin.Y].Value != nil: x = bin.X - case f.pkg.types[bin.X].Value != nil: + case pass.TypesInfo.Types[bin.X].Value != nil: x = bin.Y default: continue } // e is of the form 'x != c' or 'x == c'. - xfmt := f.gofmt(x) - efmt := f.gofmt(e) + xfmt := analysisutil.Format(pass.Fset, x) + efmt := analysisutil.Format(pass.Fset, e) if prev, found := seen[xfmt]; found { // checkRedundant handles the case in which efmt == prev. if efmt != prev { - f.Badf(e.Pos(), "suspect %s: %s %s %s", op.name, efmt, op.tok, prev) + pass.Reportf(e.Pos(), "suspect %s: %s %s %s", op.name, efmt, op.tok, prev) } } else { seen[xfmt] = efmt @@ -138,12 +151,12 @@ func (op boolOp) checkSuspect(f *File, exprs []ast.Expr) { } // hasSideEffects reports whether evaluation of e has side effects. -func hasSideEffects(f *File, e ast.Expr) bool { +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 := f.pkg.types[n.Fun] + typVal := info.Types[n.Fun] switch { case typVal.IsType(): // Type conversion, which is safe. diff --git a/go/analysis/passes/bools/bools_test.go b/go/analysis/passes/bools/bools_test.go new file mode 100644 index 00000000..ed959598 --- /dev/null +++ b/go/analysis/passes/bools/bools_test.go @@ -0,0 +1,13 @@ +package bools_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/bools" +) + +func TestFromFileSystem(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, bools.Analyzer, "a") +} diff --git a/go/analysis/passes/bools/testdata/src/a/a.go b/go/analysis/passes/bools/testdata/src/a/a.go new file mode 100644 index 00000000..fc4b8fbf --- /dev/null +++ b/go/analysis/passes/bools/testdata/src/a/a.go @@ -0,0 +1,137 @@ +// 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 bool checker. + +package a + +import "io" + +type T int + +func (t T) Foo() int { return int(t) } + +type FT func() int + +var S []int + +func RatherStupidConditions() { + var f, g func() int + if f() == 0 || f() == 0 { // OK f might have side effects + } + var t T + _ = t.Foo() == 2 || t.Foo() == 2 // OK Foo might have side effects + if v, w := f(), g(); v == w || v == w { // want `redundant or: v == w \|\| v == w` + } + _ = f == nil || f == nil // want `redundant or: f == nil \|\| f == nil` + + var B byte + _ = B == byte(1) || B == byte(1) // want `redundant or: B == byte\(1\) \|\| B == byte\(1\)` + _ = t == T(2) || t == T(2) // want `redundant or: t == T\(2\) \|\| t == T\(2\)` + _ = FT(f) == nil || FT(f) == nil // want `redundant or: FT\(f\) == nil \|\| FT\(f\) == nil` + + _ = (func() int)(f) == nil || (func() int)(f) == nil // want `redundant or: \(func\(\) int\)\(f\) == nil \|\| \(func\(\) int\)\(f\) == nil` + _ = append(S, 3) == nil || append(S, 3) == nil // OK append has side effects + + var namedFuncVar FT + _ = namedFuncVar() == namedFuncVar() // OK still func calls + + var c chan int + _ = 0 == <-c || 0 == <-c // OK subsequent receives may yield different values + for i, j := <-c, <-c; i == j || i == j; i, j = <-c, <-c { // want `redundant or: i == j \|\| i == j` + } + + var i, j, k int + _ = i+1 == 1 || i+1 == 1 // want `redundant or: i\+1 == 1 \|\| i\+1 == 1` + _ = i == 1 || j+1 == i || i == 1 // want `redundant or: i == 1 \|\| i == 1` + + // The various r.* patterns are intended to match duplicate + // diagnostics reported for the same underlying problem. + // See github.com/golang/go/issues/28086. + // TODO(adonovan): fix the checker. + + _ = i == 1 || i == 1 || f() == 1 // want `redundant or: i == 1 \|\| i == 1` `r.*` + _ = i == 1 || f() == 1 || i == 1 // OK f may alter i as a side effect + _ = f() == 1 || i == 1 || i == 1 // want `redundant or: i == 1 \|\| i == 1` + + // Test partition edge cases + _ = f() == 1 || i == 1 || i == 1 || j == 1 // want `redundant or: i == 1 \|\| i == 1` `r.*` + _ = f() == 1 || j == 1 || i == 1 || i == 1 // want `redundant or: i == 1 \|\| i == 1` + _ = i == 1 || f() == 1 || i == 1 || i == 1 // want `redundant or: i == 1 \|\| i == 1` + _ = i == 1 || i == 1 || f() == 1 || i == 1 // want `redundant or: i == 1 \|\| i == 1` `r.*` `r.*` + _ = i == 1 || i == 1 || j == 1 || f() == 1 // want `redundant or: i == 1 \|\| i == 1` `r.*` `r.*` + _ = j == 1 || i == 1 || i == 1 || f() == 1 // want `redundant or: i == 1 \|\| i == 1` `r.*` + _ = i == 1 || f() == 1 || f() == 1 || i == 1 + + _ = i == 1 || (i == 1 || i == 2) // want `redundant or: i == 1 \|\| i == 1` + _ = i == 1 || (f() == 1 || i == 1) // OK f may alter i as a side effect + _ = i == 1 || (i == 1 || f() == 1) // want `redundant or: i == 1 \|\| i == 1` + _ = i == 1 || (i == 2 || (i == 1 || i == 3)) // want `redundant or: i == 1 \|\| i == 1` + + var a, b bool + _ = i == 1 || (a || (i == 1 || b)) // want `redundant or: i == 1 \|\| i == 1` + + // Check that all redundant ors are flagged + _ = j == 0 || + i == 1 || + f() == 1 || + j == 0 || // want `redundant or: j == 0 \|\| j == 0` `r.*` + i == 1 || // want `redundant or: i == 1 \|\| i == 1` `r.*` `r.*` `r.*` + i == 1 || // want `redundant or: i == 1 \|\| i == 1` `r.*` `r.*` + i == 1 || + j == 0 || + k == 0 + + _ = i == 1*2*3 || i == 1*2*3 // want `redundant or: i == 1\*2\*3 \|\| i == 1\*2\*3` + + // These test that redundant, suspect expressions do not trigger multiple errors. + _ = i != 0 || i != 0 // want `redundant or: i != 0 \|\| i != 0` + _ = i == 0 && i == 0 // want `redundant and: i == 0 && i == 0` + + // and is dual to or; check the basics and + // let the or tests pull the rest of the weight. + _ = 0 != <-c && 0 != <-c // OK subsequent receives may yield different values + _ = f() != 0 && f() != 0 // OK f might have side effects + _ = f != nil && f != nil // want `redundant and: f != nil && f != nil` + _ = i != 1 && i != 1 && f() != 1 // want `redundant and: i != 1 && i != 1` `r.*` + _ = i != 1 && f() != 1 && i != 1 // OK f may alter i as a side effect + _ = f() != 1 && i != 1 && i != 1 // want `redundant and: i != 1 && i != 1` +} + +func RoyallySuspectConditions() { + var i, j int + + _ = i == 0 || i == 1 // OK + _ = i != 0 || i != 1 // want `suspect or: i != 0 \|\| i != 1` + _ = i != 0 || 1 != i // want `suspect or: i != 0 \|\| 1 != i` + _ = 0 != i || 1 != i // want `suspect or: 0 != i \|\| 1 != i` + _ = 0 != i || i != 1 // want `suspect or: 0 != i \|\| i != 1` + + _ = (0 != i) || i != 1 // want `suspect or: 0 != i \|\| i != 1` + + _ = i+3 != 7 || j+5 == 0 || i+3 != 9 // want `suspect or: i\+3 != 7 \|\| i\+3 != 9` + + _ = i != 0 || j == 0 || i != 1 // want `suspect or: i != 0 \|\| i != 1` + + _ = i != 0 || i != 1<<4 // want `suspect or: i != 0 \|\| i != 1<<4` + + _ = i != 0 || j != 0 + _ = 0 != i || 0 != j + + var s string + _ = s != "one" || s != "the other" // want `suspect or: s != .one. \|\| s != .the other.` + + _ = "et" != "alii" || "et" != "cetera" // want `suspect or: .et. != .alii. \|\| .et. != .cetera.` + _ = "me gustas" != "tu" || "le gustas" != "tu" // OK we could catch this case, but it's not worth the code + + var err error + _ = err != nil || err != io.EOF // TODO catch this case? + + // Sanity check and. + _ = i != 0 && i != 1 // OK + _ = i == 0 && i == 1 // want `suspect and: i == 0 && i == 1` + _ = i == 0 && 1 == i // want `suspect and: i == 0 && 1 == i` + _ = 0 == i && 1 == i // want `suspect and: 0 == i && 1 == i` + _ = 0 == i && i == 1 // want `suspect and: 0 == i && i == 1` +} diff --git a/go/analysis/passes/vet/testdata/bool.go b/go/analysis/passes/vet/testdata/bool.go deleted file mode 100644 index 80c44d25..00000000 --- a/go/analysis/passes/vet/testdata/bool.go +++ /dev/null @@ -1,131 +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 bool checker. - -package testdata - -import "io" - -type T int - -func (t T) Foo() int { return int(t) } - -type FT func() int - -var S []int - -func RatherStupidConditions() { - var f, g func() int - if f() == 0 || f() == 0 { // OK f might have side effects - } - var t T - _ = t.Foo() == 2 || t.Foo() == 2 // OK Foo might have side effects - if v, w := f(), g(); v == w || v == w { // ERROR "redundant or: v == w || v == w" - } - _ = f == nil || f == nil // ERROR "redundant or: f == nil || f == nil" - - _ = i == byte(1) || i == byte(1) // ERROR "redundant or: i == byte(1) || i == byte(1)" - _ = i == T(2) || i == T(2) // ERROR "redundant or: i == T(2) || i == T(2)" - _ = FT(f) == nil || FT(f) == nil // ERROR "redundant or: FT(f) == nil || FT(f) == nil" - - _ = (func() int)(f) == nil || (func() int)(f) == nil // ERROR "redundant or: (func() int)(f) == nil || (func() int)(f) == nil" - _ = append(S, 3) == nil || append(S, 3) == nil // OK append has side effects - - var namedFuncVar FT - _ = namedFuncVar() == namedFuncVar() // OK still func calls - - var c chan int - _ = 0 == <-c || 0 == <-c // OK subsequent receives may yield different values - for i, j := <-c, <-c; i == j || i == j; i, j = <-c, <-c { // ERROR "redundant or: i == j || i == j" - } - - var i, j, k int - _ = i+1 == 1 || i+1 == 1 // ERROR "redundant or: i\+1 == 1 || i\+1 == 1" - _ = i == 1 || j+1 == i || i == 1 // ERROR "redundant or: i == 1 || i == 1" - - _ = i == 1 || i == 1 || f() == 1 // ERROR "redundant or: i == 1 || i == 1" - _ = i == 1 || f() == 1 || i == 1 // OK f may alter i as a side effect - _ = f() == 1 || i == 1 || i == 1 // ERROR "redundant or: i == 1 || i == 1" - - // Test partition edge cases - _ = f() == 1 || i == 1 || i == 1 || j == 1 // ERROR "redundant or: i == 1 || i == 1" - _ = f() == 1 || j == 1 || i == 1 || i == 1 // ERROR "redundant or: i == 1 || i == 1" - _ = i == 1 || f() == 1 || i == 1 || i == 1 // ERROR "redundant or: i == 1 || i == 1" - _ = i == 1 || i == 1 || f() == 1 || i == 1 // ERROR "redundant or: i == 1 || i == 1" - _ = i == 1 || i == 1 || j == 1 || f() == 1 // ERROR "redundant or: i == 1 || i == 1" - _ = j == 1 || i == 1 || i == 1 || f() == 1 // ERROR "redundant or: i == 1 || i == 1" - _ = i == 1 || f() == 1 || f() == 1 || i == 1 - - _ = i == 1 || (i == 1 || i == 2) // ERROR "redundant or: i == 1 || i == 1" - _ = i == 1 || (f() == 1 || i == 1) // OK f may alter i as a side effect - _ = i == 1 || (i == 1 || f() == 1) // ERROR "redundant or: i == 1 || i == 1" - _ = i == 1 || (i == 2 || (i == 1 || i == 3)) // ERROR "redundant or: i == 1 || i == 1" - - var a, b bool - _ = i == 1 || (a || (i == 1 || b)) // ERROR "redundant or: i == 1 || i == 1" - - // Check that all redundant ors are flagged - _ = j == 0 || - i == 1 || - f() == 1 || - j == 0 || // ERROR "redundant or: j == 0 || j == 0" - i == 1 || // ERROR "redundant or: i == 1 || i == 1" - i == 1 || // ERROR "redundant or: i == 1 || i == 1" - i == 1 || - j == 0 || - k == 0 - - _ = i == 1*2*3 || i == 1*2*3 // ERROR "redundant or: i == 1\*2\*3 || i == 1\*2\*3" - - // These test that redundant, suspect expressions do not trigger multiple errors. - _ = i != 0 || i != 0 // ERROR "redundant or: i != 0 || i != 0" - _ = i == 0 && i == 0 // ERROR "redundant and: i == 0 && i == 0" - - // and is dual to or; check the basics and - // let the or tests pull the rest of the weight. - _ = 0 != <-c && 0 != <-c // OK subsequent receives may yield different values - _ = f() != 0 && f() != 0 // OK f might have side effects - _ = f != nil && f != nil // ERROR "redundant and: f != nil && f != nil" - _ = i != 1 && i != 1 && f() != 1 // ERROR "redundant and: i != 1 && i != 1" - _ = i != 1 && f() != 1 && i != 1 // OK f may alter i as a side effect - _ = f() != 1 && i != 1 && i != 1 // ERROR "redundant and: i != 1 && i != 1" -} - -func RoyallySuspectConditions() { - var i, j int - - _ = i == 0 || i == 1 // OK - _ = i != 0 || i != 1 // ERROR "suspect or: i != 0 || i != 1" - _ = i != 0 || 1 != i // ERROR "suspect or: i != 0 || 1 != i" - _ = 0 != i || 1 != i // ERROR "suspect or: 0 != i || 1 != i" - _ = 0 != i || i != 1 // ERROR "suspect or: 0 != i || i != 1" - - _ = (0 != i) || i != 1 // ERROR "suspect or: 0 != i || i != 1" - - _ = i+3 != 7 || j+5 == 0 || i+3 != 9 // ERROR "suspect or: i\+3 != 7 || i\+3 != 9" - - _ = i != 0 || j == 0 || i != 1 // ERROR "suspect or: i != 0 || i != 1" - - _ = i != 0 || i != 1<<4 // ERROR "suspect or: i != 0 || i != 1<<4" - - _ = i != 0 || j != 0 - _ = 0 != i || 0 != j - - var s string - _ = s != "one" || s != "the other" // ERROR "suspect or: s != .one. || s != .the other." - - _ = "et" != "alii" || "et" != "cetera" // ERROR "suspect or: .et. != .alii. || .et. != .cetera." - _ = "me gustas" != "tu" || "le gustas" != "tu" // OK we could catch this case, but it's not worth the code - - var err error - _ = err != nil || err != io.EOF // TODO catch this case? - - // Sanity check and. - _ = i != 0 && i != 1 // OK - _ = i == 0 && i == 1 // ERROR "suspect and: i == 0 && i == 1" - _ = i == 0 && 1 == i // ERROR "suspect and: i == 0 && 1 == i" - _ = 0 == i && 1 == i // ERROR "suspect and: 0 == i && 1 == i" - _ = 0 == i && i == 1 // ERROR "suspect and: 0 == i && i == 1" -}