diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index 8edb61b33b8..681bc85783e 100644 --- a/change-notes/1.24/analysis-go.md +++ b/change-notes/1.24/analysis-go.md @@ -18,4 +18,5 @@ |-----------------------------------------------------|------------------------------|-----------------------------------------------------------| | Reflected cross-site scripting (`go/reflected-xss`) | Fewer results | Untrusted input flowing into an HTTP header definition is no longer flagged, since this is often harmless. | | Useless assignment to field (`go/useless-assignment-to-field`) | Fewer false positives | The query now conservatively handles fields promoted through embedded pointer types. | +| Identical operands (`go/redundant-operation`) | Fewer false positives | The query no longer flags cases where the operands have the same value but are syntactically distinct, since this is usually intentional. | | Incomplete regular expression for hostnames (`go/incomplete-hostname-regexp`) | More results | The query now flags unescaped dots before the TLD in a hostname regex. | diff --git a/ql/src/RedundantCode/RedundantExpr.ql b/ql/src/RedundantCode/RedundantExpr.ql index a7edc4e75f5..86c70b4094f 100644 --- a/ql/src/RedundantCode/RedundantExpr.ql +++ b/ql/src/RedundantCode/RedundantExpr.ql @@ -1,6 +1,7 @@ /** * @name Identical operands - * @description Passing identical operands to an operator such as subtraction or conjunction may indicate a typo; + * @description Passing identical, or seemingly identical, operands to an + * operator such as subtraction or conjunction may indicate a typo; * even if it is intentional, it makes the code hard to read. * @kind problem * @problem.severity warning @@ -11,54 +12,81 @@ * @precision very-high */ -import go +import Clones /** - * Holds if `e` is a binary expression that is redundant if both operands are the same. + * A binary expression that is redundant if both operands are the same. */ -predicate potentiallyRedundant(BinaryExpr e) { - e instanceof SubExpr - or - e instanceof DivExpr - or - e instanceof ModExpr - or - e instanceof XorExpr - or - e instanceof AndNotExpr - or - e instanceof LogicalBinaryExpr - or - e instanceof BitAndExpr - or - e instanceof BitOrExpr - or - // an expression of the form `(e + f)/2` - exists(DivExpr div | - e.(AddExpr) = div.getLeftOperand().stripParens() and - div.getRightOperand().getNumericValue() = 2 - ) +abstract class PotentiallyRedundantExpr extends BinaryExpr, HashRoot { + predicate operands(Expr left, Expr right) { + left = getLeftOperand() and right = getRightOperand() + } } -/** Gets the global value number of `e`, which is the `i`th operand of `red`. */ -GVN redundantOperandGVN(BinaryExpr red, int i, Expr e) { - potentiallyRedundant(red) and - ( - i = 0 and e = red.getLeftOperand() +/** + * A binary expression whose operator is idemnecant. + * + * An idemnecant operator is an operator that yields a trivial result when applied to + * identical operands (disregarding overflow and special floating point values). + * + * For instance, subtraction is idemnecant (since `e-e=0`), and so is division (since `e/e=1`). + */ +class IdemnecantExpr extends PotentiallyRedundantExpr { + IdemnecantExpr() { + ( + this instanceof SubExpr or + this instanceof DivExpr or + this instanceof ModExpr or + this instanceof XorExpr or + this instanceof AndNotExpr + ) and + getLeftOperand().getKind() = getRightOperand().getKind() and + // exclude trivial cases like `1-1` + not getLeftOperand().stripParens() instanceof BasicLit + } +} + +/** + * An idempotent expressions. + */ +class IdempotentExpr extends PotentiallyRedundantExpr { + IdempotentExpr() { + ( + this instanceof LogicalBinaryExpr or + this instanceof BitAndExpr or + this instanceof BitOrExpr + ) and + getLeftOperand().getKind() = getRightOperand().getKind() + } +} + +/** + * An expression of the form `(e + f)/2`. + */ +class AverageExpr extends PotentiallyRedundantExpr, AddExpr { + AverageExpr() { + exists(DivExpr div | + this = div.getLeftOperand().stripParens() and + div.getRightOperand().getNumericValue() = 2 and + getLeftOperand().getKind() = getRightOperand().getKind() + ) + } + + override predicate operands(Expr left, Expr right) { + left = getLeftOperand() and right = getRightOperand() + } +} + +/** Gets the hash of `nd`, which is the `i`th operand of `red`. */ +HashedNode hashRedundantOperand(PotentiallyRedundantExpr red, int i, HashableNode nd) { + exists(Expr left, Expr right | red.operands(left, right) | + i = 0 and nd = left or - i = 1 and e = red.getRightOperand() + i = 1 and nd = right ) and - result = e.getGlobalValueNumber() + result = nd.hash() } -from BinaryExpr red, Expr e, Expr f -where - redundantOperandGVN(red, 0, e) = redundantOperandGVN(red, 1, f) and - // whitelist trivial cases - not (e instanceof BasicLit and f instanceof BasicLit) and - // whitelist operations involving a symbolic constants and a literal constant; these are often feature flags - not exists(DeclaredConstant decl | - red.getAnOperand() = decl.getAReference() and - red.getAnOperand() instanceof BasicLit - ) +from PotentiallyRedundantExpr red, Expr e, Expr f +where hashRedundantOperand(red, 0, e) = hashRedundantOperand(red, 1, f) select red, "The $@ and $@ operand of this operation are identical.", e, "left", f, "right" diff --git a/ql/test/query-tests/RedundantCode/RedundantExpr/RedundantExpr.expected b/ql/test/query-tests/RedundantCode/RedundantExpr/RedundantExpr.expected index 678f0857c68..9bd3b944f74 100644 --- a/ql/test/query-tests/RedundantCode/RedundantExpr/RedundantExpr.expected +++ b/ql/test/query-tests/RedundantCode/RedundantExpr/RedundantExpr.expected @@ -2,4 +2,3 @@ | tst.go:4:9:4:13 | ...-... | The $@ and $@ operand of this operation are identical. | tst.go:4:9:4:9 | x | left | tst.go:4:13:4:13 | x | right | | tst.go:4:31:4:35 | ...&... | The $@ and $@ operand of this operation are identical. | tst.go:4:31:4:31 | x | left | tst.go:4:35:4:35 | x | right | | tst.go:9:11:9:15 | ...+... | The $@ and $@ operand of this operation are identical. | tst.go:9:11:9:11 | x | left | tst.go:9:15:9:15 | x | right | -| tst.go:20:10:20:14 | ...-... | The $@ and $@ operand of this operation are identical. | tst.go:20:10:20:10 | d | left | tst.go:20:14:20:14 | 1 | right | diff --git a/ql/test/query-tests/RedundantCode/RedundantExpr/tst.go b/ql/test/query-tests/RedundantCode/RedundantExpr/tst.go index 197213479d1..1b2f59c922c 100644 --- a/ql/test/query-tests/RedundantCode/RedundantExpr/tst.go +++ b/ql/test/query-tests/RedundantCode/RedundantExpr/tst.go @@ -17,7 +17,7 @@ const c = 1 func baz(b bool) int { var d = 1 if b { - return d - 1 // NOT OK + return d - 1 // OK } else { return c - 1 // OK }