From e6242fd34912535e9325a6c423e2d7f26d672a22 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 13 Oct 2021 13:43:30 +0100 Subject: [PATCH 1/6] Add ql/use-set-literal query. --- ql/src/queries/style/UseSetLiteral.ql | 89 +++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 ql/src/queries/style/UseSetLiteral.ql diff --git a/ql/src/queries/style/UseSetLiteral.ql b/ql/src/queries/style/UseSetLiteral.ql new file mode 100644 index 00000000000..0657cef831d --- /dev/null +++ b/ql/src/queries/style/UseSetLiteral.ql @@ -0,0 +1,89 @@ +/** + * @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 +import codeql_ql.ast.internal.Predicate // TODO: for PredicateOrBuiltin + +/** + * 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) + } +} + +from DisjunctionChain d, int c +where + d instanceof DisjunctionEqualsLiteral and + c = count(d.getAnOperandRec()) and + c >= 4 +select d, "This formula can be replaced with equality on a set literal, improving readability." From c8c23a6eb495e16712f30ca9ed36e53a4da08506 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 13 Oct 2021 13:58:32 +0100 Subject: [PATCH 2/6] Support hasName(x) pattern as well. --- ql/src/queries/style/UseSetLiteral.ql | 47 +++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/ql/src/queries/style/UseSetLiteral.ql b/ql/src/queries/style/UseSetLiteral.ql index 0657cef831d..4bd13a22c38 100644 --- a/ql/src/queries/style/UseSetLiteral.ql +++ b/ql/src/queries/style/UseSetLiteral.ql @@ -81,9 +81,50 @@ class DisjunctionEqualsLiteral extends DisjunctionChain { } } -from DisjunctionChain d, int c +/** + * 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 + ( + 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, "This formula can be replaced with equality on a set literal, improving readability." +select d, msg From 0704ab7bd325722e50baf3b70fd3150b212a2c2d Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 13 Oct 2021 14:25:14 +0100 Subject: [PATCH 3/6] Add tests. --- .../UseSetLiteral/UseSetLiteral.expected | 8 ++ .../style/UseSetLiteral/UseSetLiteral.qlref | 1 + ql/test/queries/style/UseSetLiteral/test.qll | 127 ++++++++++++++++++ 3 files changed, 136 insertions(+) create mode 100644 ql/test/queries/style/UseSetLiteral/UseSetLiteral.expected create mode 100644 ql/test/queries/style/UseSetLiteral/UseSetLiteral.qlref create mode 100644 ql/test/queries/style/UseSetLiteral/test.qll diff --git a/ql/test/queries/style/UseSetLiteral/UseSetLiteral.expected b/ql/test/queries/style/UseSetLiteral/UseSetLiteral.expected new file mode 100644 index 00000000000..053a8b55512 --- /dev/null +++ b/ql/test/queries/style/UseSetLiteral/UseSetLiteral.expected @@ -0,0 +1,8 @@ +| test.qll:4:2:7:6 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | +| test.qll:29:2:32:9 | Disjunction | This formula of 4 predicate calls can be replaced with a single call on a set literal, improving readability. | +| test.qll:43:2:46:11 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | +| test.qll:62:4:65:11 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | +| test.qll:67:4:70:10 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | +| test.qll:72:4:75:10 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | +| test.qll:89:2:92:8 | Disjunction | This formula of 4 predicate calls can be replaced with a single call on a set literal, improving readability. | +| test.qll:126:2:126:37 | 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..5016e032c11 --- /dev/null +++ b/ql/test/queries/style/UseSetLiteral/test.qll @@ -0,0 +1,127 @@ +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) or // GOOD + (a = 2 and b = 4) or + (a = 3 and b = 9) or + (a = 4 and b = 16) +} + +from int a +where + a = 1 or ((a = 2 or a = 3) or a = 4) // BAD +select a From 9b52ad2d3d00ef349493ba43c9c22af94dfa647d Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 13 Oct 2021 15:17:00 +0100 Subject: [PATCH 4/6] Work around import of internal file. --- ql/src/queries/style/UseSetLiteral.ql | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ql/src/queries/style/UseSetLiteral.ql b/ql/src/queries/style/UseSetLiteral.ql index 4bd13a22c38..0481ac39299 100644 --- a/ql/src/queries/style/UseSetLiteral.ql +++ b/ql/src/queries/style/UseSetLiteral.ql @@ -9,7 +9,6 @@ */ import ql -import codeql_ql.ast.internal.Predicate // TODO: for PredicateOrBuiltin /** * A chain of disjunctions treated as one object. For example the following is @@ -106,7 +105,7 @@ class CallLiteral extends Call { class DisjunctionPredicateLiteral extends DisjunctionChain { DisjunctionPredicateLiteral() { // Call to the same target - exists(PredicateOrBuiltin target | + exists(AstNode target | forex(Formula f | f = getAnOperandRec() | f.(CallLiteral).getTarget() = target) ) } From 6af28e37ae8cefbc4c3876a675c0d9f8d4ed24c4 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 14 Oct 2021 11:22:49 +0100 Subject: [PATCH 5/6] We can use PredicateOrBuiltin now. --- ql/src/queries/style/UseSetLiteral.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/queries/style/UseSetLiteral.ql b/ql/src/queries/style/UseSetLiteral.ql index 0481ac39299..d0e983a1774 100644 --- a/ql/src/queries/style/UseSetLiteral.ql +++ b/ql/src/queries/style/UseSetLiteral.ql @@ -105,7 +105,7 @@ class CallLiteral extends Call { class DisjunctionPredicateLiteral extends DisjunctionChain { DisjunctionPredicateLiteral() { // Call to the same target - exists(AstNode target | + exists(PredicateOrBuiltin target | forex(Formula f | f = getAnOperandRec() | f.(CallLiteral).getTarget() = target) ) } From 76880e8f9346a35962cd7372b8f4ea9fe347fea0 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 14 Oct 2021 14:31:42 +0100 Subject: [PATCH 6/6] Autoformat and fix test. --- .../UseSetLiteral/UseSetLiteral.expected | 16 +- ql/test/queries/style/UseSetLiteral/test.qll | 176 +++++++++--------- 2 files changed, 100 insertions(+), 92 deletions(-) diff --git a/ql/test/queries/style/UseSetLiteral/UseSetLiteral.expected b/ql/test/queries/style/UseSetLiteral/UseSetLiteral.expected index 053a8b55512..fac79ff078e 100644 --- a/ql/test/queries/style/UseSetLiteral/UseSetLiteral.expected +++ b/ql/test/queries/style/UseSetLiteral/UseSetLiteral.expected @@ -1,8 +1,8 @@ -| test.qll:4:2:7:6 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | -| test.qll:29:2:32:9 | Disjunction | This formula of 4 predicate calls can be replaced with a single call on a set literal, improving readability. | -| test.qll:43:2:46:11 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | -| test.qll:62:4:65:11 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | -| test.qll:67:4:70:10 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | -| test.qll:72:4:75:10 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | -| test.qll:89:2:92:8 | Disjunction | This formula of 4 predicate calls can be replaced with a single call on a set literal, improving readability. | -| test.qll:126:2:126:37 | Disjunction | This formula of 4 comparisons can be replaced with a single equality on a set literal, improving readability. | +| 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/test.qll b/ql/test/queries/style/UseSetLiteral/test.qll index 5016e032c11..36a5f938f89 100644 --- a/ql/test/queries/style/UseSetLiteral/test.qll +++ b/ql/test/queries/style/UseSetLiteral/test.qll @@ -1,127 +1,135 @@ import ql predicate test1(int a) { - a = 1 or // BAD - a = 2 or - a = 3 or - a = 4 + a = 1 or // BAD + a = 2 or + a = 3 or + a = 4 } predicate test2(int a) { - a = [1, 2, 3, 4] // GOOD + 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 + 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 +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) + 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) + 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 + 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 + test7() = 1 or // BAD [NOT DETECTED] + test7() = 2 or + test7() = 3 or + test7() = 4 } -class MyTest8Class extends int -{ - string s; +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 - ) - } + 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 - } + predicate is(int x) { x = this } - int get() { - result = 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) + 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 + 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, 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 +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) or // GOOD - (a = 2 and b = 4) or - (a = 3 and b = 9) or - (a = 4 and b = 16) + a = 1 and b = 1 // GOOD + or + a = 2 and b = 4 + or + a = 3 and b = 9 + or + a = 4 and b = 16 } -from int a -where - a = 1 or ((a = 2 or a = 3) or a = 4) // BAD -select a +predicate test14(int a) { + a = 1 // BAD + or + ( + (a = 2 or a = 3) + or + a = 4 + ) +}