diff --git a/ql/src/queries/style/UseSetLiteral.ql b/ql/src/queries/style/UseSetLiteral.ql new file mode 100644 index 00000000000..d0e983a1774 --- /dev/null +++ b/ql/src/queries/style/UseSetLiteral.ql @@ -0,0 +1,129 @@ +/** + * @name Use a set literal in place of `or` + * @description A chain of `or`s can be replaced with a set literal, improving readability. + * @kind problem + * @problem.severity recommendation + * @id ql/use-set-literal + * @tags maintainability + * @precision high + */ + +import ql + +/** + * A chain of disjunctions treated as one object. For example the following is + * a chain of disjunctions with three operands: + * ``` + * a or b or c + * ``` + */ +class DisjunctionChain extends Disjunction { + DisjunctionChain() { not exists(Disjunction parent | parent.getAnOperand() = this) } + + /** + * Gets any operand of the chain. + */ + Formula getAnOperandRec() { + result = getAnOperand*() and + not result instanceof Disjunction + } +} + +/** + * An equality comparison with a `Literal`. For example: + * ``` + * x = 4 + * ``` + */ +class EqualsLiteral extends ComparisonFormula { + EqualsLiteral() { + getSymbol() = "=" and + getAnOperand() instanceof Literal + } +} + +/** + * A chain of disjunctions where each operand is an equality comparison between + * the same thing and various `Literal`s. For example: + * ``` + * x = 4 or + * x = 5 or + * x = 6 + * ``` + */ +class DisjunctionEqualsLiteral extends DisjunctionChain { + DisjunctionEqualsLiteral() { + // VarAccess on the same variable + exists(VarDef v | + forex(Formula f | f = getAnOperandRec() | + f.(EqualsLiteral).getAnOperand().(VarAccess).getDeclaration() = v + ) + ) + or + // FieldAccess on the same variable + exists(VarDecl v | + forex(Formula f | f = getAnOperandRec() | + f.(EqualsLiteral).getAnOperand().(FieldAccess).getDeclaration() = v + ) + ) + or + // ThisAccess + forex(Formula f | f = getAnOperandRec() | + f.(EqualsLiteral).getAnOperand() instanceof ThisAccess + ) + or + // ResultAccess + forex(Formula f | f = getAnOperandRec() | + f.(EqualsLiteral).getAnOperand() instanceof ResultAccess + ) + // (in principle something like GlobalValueNumbering could be used to generalize this) + } +} + +/** + * A call with a single `Literal` argument. For example: + * ``` + * myPredicate(4) + * ``` + */ +class CallLiteral extends Call { + CallLiteral() { + getNumberOfArguments() = 1 and + getArgument(0) instanceof Literal + } +} + +/** + * A chain of disjunctions where each operand is a call to the same predicate + * using various `Literal`s. For example: + * ``` + * myPredicate(4) or + * myPredicate(5) or + * myPredicate(6) + * ``` + */ +class DisjunctionPredicateLiteral extends DisjunctionChain { + DisjunctionPredicateLiteral() { + // Call to the same target + exists(PredicateOrBuiltin target | + forex(Formula f | f = getAnOperandRec() | f.(CallLiteral).getTarget() = target) + ) + } +} + +from DisjunctionChain d, string msg, int c +where + ( + d instanceof DisjunctionEqualsLiteral and + msg = + "This formula of " + c.toString() + + " comparisons can be replaced with a single equality on a set literal, improving readability." + or + d instanceof DisjunctionPredicateLiteral and + msg = + "This formula of " + c.toString() + + " predicate calls can be replaced with a single call on a set literal, improving readability." + ) and + c = count(d.getAnOperandRec()) and + c >= 4 +select d, msg diff --git a/ql/test/queries/style/UseSetLiteral/UseSetLiteral.expected b/ql/test/queries/style/UseSetLiteral/UseSetLiteral.expected new file mode 100644 index 00000000000..fac79ff078e --- /dev/null +++ b/ql/test/queries/style/UseSetLiteral/UseSetLiteral.expected @@ -0,0 +1,8 @@ +| test.qll:4:3:7:7 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | +| test.qll:30:3:33:10 | Disjunction | This formula of 4 predicate calls can be replaced with a single call on a set literal, improving readability. | +| test.qll:44:3:47:12 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | +| test.qll:62:7:65:14 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | +| test.qll:68:7:71:13 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | +| test.qll:74:7:77:13 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | +| test.qll:87:3:90:9 | Disjunction | This formula of 4 predicate calls can be replaced with a single call on a set literal, improving readability. | +| test.qll:128:3:134:3 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | diff --git a/ql/test/queries/style/UseSetLiteral/UseSetLiteral.qlref b/ql/test/queries/style/UseSetLiteral/UseSetLiteral.qlref new file mode 100644 index 00000000000..d4047ebc29f --- /dev/null +++ b/ql/test/queries/style/UseSetLiteral/UseSetLiteral.qlref @@ -0,0 +1 @@ +queries/style/UseSetLiteral.ql \ No newline at end of file diff --git a/ql/test/queries/style/UseSetLiteral/test.qll b/ql/test/queries/style/UseSetLiteral/test.qll new file mode 100644 index 00000000000..36a5f938f89 --- /dev/null +++ b/ql/test/queries/style/UseSetLiteral/test.qll @@ -0,0 +1,135 @@ +import ql + +predicate test1(int a) { + a = 1 or // BAD + a = 2 or + a = 3 or + a = 4 +} + +predicate test2(int a) { + a = [1, 2, 3, 4] // GOOD +} + +predicate test3(int a) { + a = 1 and // GOOD (for the purposes of this query) + a = 2 and + a = 3 and + a = 4 +} + +bindingset[a] +predicate test4(int a) { + a < 1 or // GOOD (for the purposes of this query) + a = 2 or + a >= 3 or + a > 4 +} + +predicate test5() { + test1(1) or // BAD + test1(2) or + test1(3) or + test1(4) +} + +predicate test6() { + test1(1) or // GOOD + test2(2) or + test3(3) or + test4(4) +} + +int test7() { + 1 = result or // BAD + 2 = result or + 3 = result or + 4 = result +} + +predicate test8() { + test7() = 1 or // BAD [NOT DETECTED] + test7() = 2 or + test7() = 3 or + test7() = 4 +} + +class MyTest8Class extends int { + string s; + + MyTest8Class() { + ( + this = 1 or // BAD + this = 2 or + this = 3 or + this = 4 + ) and + ( + s = "1" or // BAD + s = "2" or + s = "3" or + s = "4" + ) and + exists(float f | + f = 1.0 or // BAD + f = 1.5 or + f = 2.0 or + f = 2.5 + ) + } + + predicate is(int x) { x = this } + + int get() { result = this } +} + +predicate test9(MyTest8Class c) { + c.is(1) or // BAD + c.is(2) or + c.is(3) or + c.is(4) +} + +predicate test10(MyTest8Class c) { + c.get() = 1 or // BAD [NOT DETECTED] + c.get() = 2 or + c.get() = 3 or + c.get() = 4 +} + +bindingset[a, b, c, d] +predicate test11(int a, int b, int c, int d) { + a = 1 or // GOOD + b = 2 or + c = 3 or + d = 4 +} + +bindingset[a, b] +predicate test12(int a, int b) { + a = 1 or // BAD [NOT DETECTED] + a = 2 or + a = 3 or + a = 4 or + b = 0 +} + +predicate test13(int a, int b) { + a = 1 and b = 1 // GOOD + or + a = 2 and b = 4 + or + a = 3 and b = 9 + or + a = 4 and b = 16 +} + +predicate test14(int a) { + a = 1 // BAD + or + ( + (a = 2 or a = 3) + or + a = 4 + ) +}