Skip to content

Commit

Permalink
Revert "Fix instrumentation of types based on bool"
Browse files Browse the repository at this point in the history
This reverts commit 935290e.
  • Loading branch information
rillig committed Mar 8, 2024
1 parent 54ef6b4 commit ec6c8c6
Show file tree
Hide file tree
Showing 36 changed files with 205 additions and 276 deletions.
57 changes: 13 additions & 44 deletions instrumenter.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,10 @@ type cond struct {

// exprSubst prepares to later replace '*ref' with 'expr'.
type exprSubst struct {
ref *ast.Expr
expr ast.Expr
pos token.Pos
text string
convert bool
ref *ast.Expr
expr ast.Expr
pos token.Pos
text string
}

// instrumenter rewrites the code of a go package
Expand Down Expand Up @@ -249,7 +248,7 @@ func (i *instrumenter) findRefsField(field reflect.Value) {
delete(i.marked, expr)
ref := field.Addr().Interface().(*ast.Expr)
i.exprSubst[expr] = &exprSubst{
ref, expr, expr.Pos(), i.str(expr), true,
ref, expr, expr.Pos(), i.str(expr),
}
}

Expand All @@ -258,7 +257,7 @@ func (i *instrumenter) findRefsField(field reflect.Value) {
if i.marked[expr] {
delete(i.marked, expr)
i.exprSubst[expr] = &exprSubst{
&val[ei], expr, expr.Pos(), i.str(expr), true,
&val[ei], expr, expr.Pos(), i.str(expr),
}
}
}
Expand Down Expand Up @@ -293,13 +292,6 @@ func (i *instrumenter) prepareStmts(n ast.Node) bool {

func (i *instrumenter) prepareSwitchStmt(n *ast.SwitchStmt) {
if n.Tag == nil {
for _, clause := range n.Body.List {
for _, expr := range clause.(*ast.CaseClause).List {
if s := i.exprSubst[expr]; s != nil {
s.convert = false
}
}
}
return // Already handled in instrumenter.markConds.
}

Expand All @@ -326,7 +318,6 @@ func (i *instrumenter) prepareSwitchStmt(n *ast.SwitchStmt) {
gen.eql(tagExprName, expr),
expr.Pos(),
i.strEql(n.Tag, expr),
false,
}
tagExprUsed = true
}
Expand Down Expand Up @@ -430,7 +421,7 @@ func (i *instrumenter) prepareTypeSwitchStmt(ts *ast.TypeSwitchStmt) {

gen := codeGenerator{test.pos}
ident := gen.ident(test.varname)
wrapped := i.callCover(ident, test.pos, test.code, false)
wrapped := i.callCover(ident, test.pos, test.code)
newList = append(newList, wrapped)
}

Expand Down Expand Up @@ -467,7 +458,7 @@ func (i *instrumenter) replace(n ast.Node) bool {

case ast.Expr:
if s := i.exprSubst[n]; s != nil {
*s.ref = i.callCover(s.expr, s.pos, s.text, s.convert)
*s.ref = i.callCover(s.expr, s.pos, s.text)
}

case ast.Stmt:
Expand All @@ -487,7 +478,7 @@ func (i *instrumenter) replace(n ast.Node) bool {
// that is most closely related to the instrumented condition.
// Especially for switch statements,
// the position may differ from the expression that is wrapped.
func (i *instrumenter) callCover(expr ast.Expr, pos token.Pos, code string, convert bool) ast.Expr {
func (i *instrumenter) callCover(expr ast.Expr, pos token.Pos, code string) ast.Expr {
assert(pos.IsValid(), "pos must refer to the code from before instrumentation")

start := i.fset.Position(pos)
Expand All @@ -500,7 +491,7 @@ func (i *instrumenter) callCover(expr ast.Expr, pos token.Pos, code string, conv
idx := len(i.conds) - 1

gen := codeGenerator{pos}
return gen.callGobcoCover(idx, expr, convert)
return gen.callGobcoCover(idx, expr)
}

// strEql returns the string representation of (lhs == rhs).
Expand Down Expand Up @@ -721,8 +712,8 @@ func (gen codeGenerator) callFinish(arg ast.Expr) ast.Expr {
}
}

func (gen codeGenerator) callGobcoCover(idx int, cond ast.Expr, convert bool) ast.Expr {
call := &ast.CallExpr{
func (gen codeGenerator) callGobcoCover(idx int, cond ast.Expr) ast.Expr {
return &ast.CallExpr{
Fun: gen.ident("GobcoCover"),
Lparen: gen.pos,
Args: []ast.Expr{
Expand All @@ -731,32 +722,10 @@ func (gen codeGenerator) callGobcoCover(idx int, cond ast.Expr, convert bool) as
Kind: token.INT,
Value: fmt.Sprint(idx),
},
gen.toBool(cond),
cond,
},
Rparen: gen.pos,
}
if !convert {
return call
}
return gen.toCustomBool(call)
}

func (gen codeGenerator) toBool(cond ast.Expr) ast.Expr {
switch cond.(type) {
case *ast.UnaryExpr, *ast.BinaryExpr:
return cond
default:
return gen.toCustomBool(cond)
}
}

func (gen codeGenerator) toCustomBool(cond ast.Expr) ast.Expr {
return &ast.BinaryExpr{
X: cond,
OpPos: gen.pos,
Op: token.EQL,
Y: gen.ident("true"),
}
}

func (gen codeGenerator) define(lhs string, rhs ast.Expr) *ast.AssignStmt {
Expand Down
14 changes: 7 additions & 7 deletions testdata/instrumenter/AssignStmt.cond
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,20 @@ func assignStmt() {
assertEquals(i, 3)

// Most assignments assign to a single variable.
mm[GobcoCover(0, i > 0) == true] = GobcoCover(1, i > 3) == true
mm[GobcoCover(0, i > 0)] = GobcoCover(1, i > 3)

// An assignment can assign multiple variables simultaneously.
b1, m[GobcoCover(2, i > 0) == true], b2 = b2, m[GobcoCover(3, i > 1) == true], b1
mm[GobcoCover(4, i > 10) == true], mm[GobcoCover(5, i > 11) == true], mm[GobcoCover(6, i > 12) == true] =
GobcoCover(7, i > 13) == true, GobcoCover(8, i > 14) == true, GobcoCover(9, i > 15) == true
b1, m[GobcoCover(2, i > 0)], b2 = b2, m[GobcoCover(3, i > 1)], b1
mm[GobcoCover(4, i > 10)], mm[GobcoCover(5, i > 11)], mm[GobcoCover(6, i > 12)] =
GobcoCover(7, i > 13), GobcoCover(8, i > 14), GobcoCover(9, i > 15)

// Assignments from a function call can assign to multiple variables.
mm[GobcoCover(10, i > 0) == true], mm[GobcoCover(11, i > 1) == true], mm[GobcoCover(12, i > 2) == true] =
mm[GobcoCover(10, i > 0)], mm[GobcoCover(11, i > 1)], mm[GobcoCover(12, i > 2)] =
func() (bool, bool, bool) { return false, false, false }()

// Operands may be parenthesized.
(m[GobcoCover(13, i > 21) == true]), (m[GobcoCover(14, i > 22) == true]) =
(m[GobcoCover(15, i > 23) == true]), (m[GobcoCover(16, i > 24) == true])
(m[GobcoCover(13, i > 21)]), (m[GobcoCover(14, i > 22)]) =
(m[GobcoCover(15, i > 23)]), (m[GobcoCover(16, i > 24)])

// Since the left-hand side in an assignment must be a variable,
// and since only those expressions are instrumented that are
Expand Down
4 changes: 2 additions & 2 deletions testdata/instrumenter/BinaryExpr.branch
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,10 @@ func binaryExpr(i int, a bool, b bool, c bool) {
// if statements; in branch coverage mode, only instrument the main
// condition.
mi := map[bool]int{}
if GobcoCover(0, i == mi[i > 51]) == true {
if GobcoCover(0, i == mi[i > 51]) {
_ = i == mi[i > 52]
}
for GobcoCover(1, i == mi[i > 61]) == true {
for GobcoCover(1, i == mi[i > 61]) {
_ = i == mi[i > 62]
}
}
Expand Down
52 changes: 26 additions & 26 deletions testdata/instrumenter/BinaryExpr.cond
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ package instrumenter
func binaryExpr(i int, a bool, b bool, c bool) {
// Comparison expressions have return type boolean and are
// therefore instrumented.
_ = GobcoCover(0, i > 0) == true
pos := GobcoCover(1, i > 0) == true
_ = GobcoCover(0, i > 0)
pos := GobcoCover(1, i > 0)

// Expressions consisting of a single identifier do not look like boolean
// expressions, therefore they are not instrumented.
Expand All @@ -32,50 +32,50 @@ func binaryExpr(i int, a bool, b bool, c bool) {
//
// Also, gobco only looks at the parse tree without any type resolution.
// Therefore it cannot decide whether a variable is boolean or not.
both := GobcoCover(2, a == true) == true && GobcoCover(3, b == true) == true
either := GobcoCover(4, a == true) == true || GobcoCover(5, b == true) == true
both := GobcoCover(2, a) && GobcoCover(3, b)
either := GobcoCover(4, a) || GobcoCover(5, b)
_, _ = both, either

// When a long chain of '&&' or '||' is parsed, it is split into
// the rightmost operand and the rest, instrumenting both these
// parts.
_ = GobcoCover(6, i == 11) == true ||
GobcoCover(7, i == 12) == true ||
GobcoCover(8, i == 13) == true ||
GobcoCover(9, i == 14) == true ||
GobcoCover(10, i == 15) == true
_ = GobcoCover(11, i != 21) == true &&
GobcoCover(12, i != 22) == true &&
GobcoCover(13, i != 23) == true &&
GobcoCover(14, i != 24) == true &&
GobcoCover(15, i != 25) == true
_ = GobcoCover(6, i == 11) ||
GobcoCover(7, i == 12) ||
GobcoCover(8, i == 13) ||
GobcoCover(9, i == 14) ||
GobcoCover(10, i == 15)
_ = GobcoCover(11, i != 21) &&
GobcoCover(12, i != 22) &&
GobcoCover(13, i != 23) &&
GobcoCover(14, i != 24) &&
GobcoCover(15, i != 25)

// The operators '&&' and '||' can be mixed as well.
_ = GobcoCover(16, i == 31) == true ||
GobcoCover(17, i >= 32) == true && GobcoCover(18, i <= 33) == true ||
GobcoCover(19, i >= 34) == true && GobcoCover(20, i <= 35) == true
_ = GobcoCover(16, i == 31) ||
GobcoCover(17, i >= 32) && GobcoCover(18, i <= 33) ||
GobcoCover(19, i >= 34) && GobcoCover(20, i <= 35)

m := map[bool]int{}
_ = GobcoCover(21, m[GobcoCover(22, i == 41) == true] == m[GobcoCover(23, i == 42) == true]) == true
_ = GobcoCover(21, m[GobcoCover(22, i == 41)] == m[GobcoCover(23, i == 42)])

// In condition coverage mode, do not instrument complex conditions
// but instead their terminal conditions, in this case 'a', 'b' and
// 'c', to avoid large and redundant conditions in the output.
f := func(args ...bool) {}
f(GobcoCover(24, a == true) == true && GobcoCover(25, b == true) == true)
f(GobcoCover(26, a == true) == true && GobcoCover(27, b == true) == true && GobcoCover(28, c == true) == true)
f(!(GobcoCover(29, a == true) == true))
f(!(GobcoCover(30, a == true) == true) && !(GobcoCover(31, b == true) == true) && !(GobcoCover(32, c == true) == true))
f(GobcoCover(24, a) && GobcoCover(25, b))
f(GobcoCover(26, a) && GobcoCover(27, b) && GobcoCover(28, c))
f(!GobcoCover(29, a))
f(!GobcoCover(30, a) && !GobcoCover(31, b) && !GobcoCover(32, c))

// In condition coverage mode, instrument deeply nested conditions in
// if statements; in branch coverage mode, only instrument the main
// condition.
mi := map[bool]int{}
if GobcoCover(33, i == mi[GobcoCover(34, i > 51) == true]) == true {
_ = GobcoCover(35, i == mi[GobcoCover(36, i > 52) == true]) == true
if GobcoCover(33, i == mi[GobcoCover(34, i > 51)]) {
_ = GobcoCover(35, i == mi[GobcoCover(36, i > 52)])
}
for GobcoCover(37, i == mi[GobcoCover(38, i > 61) == true]) == true {
_ = GobcoCover(39, i == mi[GobcoCover(40, i > 62) == true]) == true
for GobcoCover(37, i == mi[GobcoCover(38, i > 61)]) {
_ = GobcoCover(39, i == mi[GobcoCover(40, i > 62)])
}
}

Expand Down
15 changes: 1 addition & 14 deletions testdata/instrumenter/CallExpr.branch
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ func callExpr(a bool, b string) bool {
// not wrapped since, as of January 2023, gobco does not use type
// resolution.

if GobcoCover(0, len(b) > 0) == true {
if GobcoCover(0, len(b) > 0) {
return callExpr(len(b)%2 == 0, b[1:])
}

Expand All @@ -29,20 +29,7 @@ func callExpr(a bool, b string) bool {
type myBool bool
_ = myBool(3 > 0)

// There may be custom types based on bool,
// and these types cannot be directly assigned to bool.
// https://github.com/rillig/gobco/issues/35
toMyBool := func(i int) myBool { return i%2 == 0 }
if GobcoCover(1, toMyBool(4) == true) == true {
}

fromMyBool := func(m myBool) bool { return bool(m) }
if GobcoCover(2, fromMyBool(toMyBool(3) && toMyBool(6)) == true) == true {
}

return false
}

// :17:5: "len(b) > 0"
// :36:5: "toMyBool(4)"
// :40:5: "fromMyBool(toMyBool(3) && toMyBool(6))"
26 changes: 5 additions & 21 deletions testdata/instrumenter/CallExpr.cond
Original file line number Diff line number Diff line change
Expand Up @@ -14,31 +14,20 @@ func callExpr(a bool, b string) bool {
// not wrapped since, as of January 2023, gobco does not use type
// resolution.

if GobcoCover(0, len(b) > 0) == true {
return callExpr(GobcoCover(1, len(b)%2 == 0) == true, b[1:])
if GobcoCover(0, len(b) > 0) {
return callExpr(GobcoCover(1, len(b)%2 == 0), b[1:])
}

// A CallExpr without identifier is also covered.
(func(a bool) {})(GobcoCover(2, 1 != 2) == true)
(func(a bool) {})(GobcoCover(2, 1 != 2))

// The function expression can contain conditions as well.
m := map[bool]func(){}
m[GobcoCover(3, 3 != 0) == true]()
m[GobcoCover(3, 3 != 0)]()

// Type conversions end up as CallExpr as well.
type myBool bool
_ = myBool(GobcoCover(4, 3 > 0) == true)

// There may be custom types based on bool,
// and these types cannot be directly assigned to bool.
// https://github.com/rillig/gobco/issues/35
toMyBool := func(i int) myBool { return GobcoCover(5, i%2 == 0) == true }
if GobcoCover(6, toMyBool(4) == true) == true {
}

fromMyBool := func(m myBool) bool { return bool(m) }
if GobcoCover(7, fromMyBool(GobcoCover(8, toMyBool(3) == true) == true && GobcoCover(9, toMyBool(6) == true) == true) == true) == true {
}
_ = myBool(GobcoCover(4, 3 > 0))

return false
}
Expand All @@ -48,8 +37,3 @@ func callExpr(a bool, b string) bool {
// :22:20: "1 != 2"
// :26:4: "3 != 0"
// :30:13: "3 > 0"
// :35:42: "i%2 == 0"
// :36:5: "toMyBool(4)"
// :40:5: "fromMyBool(toMyBool(3) && toMyBool(6))"
// :40:16: "toMyBool(3)"
// :40:31: "toMyBool(6)"
11 changes: 0 additions & 11 deletions testdata/instrumenter/CallExpr.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,5 @@ func callExpr(a bool, b string) bool {
type myBool bool
_ = myBool(3 > 0)

// There may be custom types based on bool,
// and these types cannot be directly assigned to bool.
// https://github.com/rillig/gobco/issues/35
toMyBool := func(i int) myBool { return i%2 == 0 }
if toMyBool(4) {
}

fromMyBool := func(m myBool) bool { return bool(m) }
if fromMyBool(toMyBool(3) && toMyBool(6)) {
}

return false
}
10 changes: 5 additions & 5 deletions testdata/instrumenter/Comment.branch
Original file line number Diff line number Diff line change
Expand Up @@ -33,23 +33,23 @@ func comment() {
_, gobco4 := gobco0.([][]int)
_, gobco5 := gobco0.([]int)
switch {
case GobcoCover(0, gobco1 == true):
case GobcoCover(0, gobco1):
// begin int
_ = 1
// end int
case GobcoCover(1, gobco2 == true):
case GobcoCover(1, gobco2):
// begin int-4D
_ = 1
// end int-4D
case GobcoCover(2, gobco3 == true):
case GobcoCover(2, gobco3):
// begin int-3D
_ = 1
// end int-3D
case GobcoCover(3, gobco4 == true):
case GobcoCover(3, gobco4):
// begin int-2D
_ = 1
// end int-2D
case GobcoCover(4, gobco5 == true):
case GobcoCover(4, gobco5):
// begin int-1D
_ = 1
}
Expand Down
Loading

0 comments on commit ec6c8c6

Please sign in to comment.