Skip to content

Commit 9309a65

Browse files
smowtonsmowton
authored
Merge pull request #8493 from JLLeitschuh/feat/JLL/test_assertion_guard_preconditions
[Java]: Add precondition support for testing library asserts
2 parents f1ec2e3 + 9bcf466 commit 9309a65

File tree

13 files changed

+298
-26
lines changed

13 files changed

+298
-26
lines changed

‎java/ql/lib/change-notes/2022-03-18-test-assertion-guards.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Added guard preconditon support for assertion methods for popular testing libraries (e.g. Junit 4, Junit 5, TestNG).

‎java/ql/lib/semmle/code/java/ControlFlowGraph.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ private module ControlFlowGraphImpl {
185185
* Bind `t` to an unchecked exception that may occur in a precondition check.
186186
*/
187187
private predicate uncheckedExceptionFromMethod(MethodAccess ma, ThrowableType t) {
188-
conditionCheck(ma, _) and
188+
conditionCheckArgument(ma, _, _) and
189189
(t instanceof TypeError or t instanceof TypeRuntimeException)
190190
}
191191

‎java/ql/lib/semmle/code/java/controlflow/Guards.qll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ class Guard extends ExprParent {
8686
or
8787
this instanceof SwitchCase
8888
or
89-
conditionCheck(this, _)
89+
conditionCheckArgument(this, _, _)
9090
}
9191

9292
/** Gets the immediately enclosing callable whose body contains this guard. */
@@ -189,7 +189,7 @@ private predicate switchCaseControls(SwitchCase sc, BasicBlock bb) {
189189
private predicate preconditionBranchEdge(
190190
MethodAccess ma, BasicBlock bb1, BasicBlock bb2, boolean branch
191191
) {
192-
conditionCheck(ma, branch) and
192+
conditionCheckArgument(ma, _, branch) and
193193
bb1.getLastNode() = ma.getControlFlowNode() and
194194
bb2 = bb1.getLastNode().getANormalSuccessor()
195195
}

‎java/ql/lib/semmle/code/java/controlflow/internal/GuardsLogic.qll

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,9 @@ predicate implies_v1(Guard g1, boolean b1, Guard g2, boolean b2) {
5757
or
5858
g1.(DefaultCase).getSwitch().getAConstCase() = g2 and b1 = true and b2 = false
5959
or
60-
exists(MethodAccess check | check = g1 |
61-
conditionCheck(check, _) and
62-
g2 = check.getArgument(0) and
60+
exists(MethodAccess check, int argIndex | check = g1 |
61+
conditionCheckArgument(check, argIndex, _) and
62+
g2 = check.getArgument(argIndex) and
6363
b1 = [true, false] and
6464
b2 = b1
6565
)

‎java/ql/lib/semmle/code/java/controlflow/internal/Preconditions.qll

Lines changed: 83 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,20 +7,41 @@
77
import java
88

99
/**
10+
* DEPRECATED: Use `conditionCheckMethodArgument` instead.
1011
* Holds if `m` is a non-overridable method that checks that its first argument
1112
* is equal to `checkTrue` and throws otherwise.
1213
*/
13-
predicate conditionCheckMethod(Method m, boolean checkTrue) {
14-
m.getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions") and
15-
checkTrue = true and
16-
(m.hasName("checkArgument") or m.hasName("checkState"))
14+
deprecated predicate conditionCheckMethod(Method m, boolean checkTrue) {
15+
conditionCheckMethodArgument(m, 0, checkTrue)
16+
}
17+
18+
/**
19+
* Holds if `m` is a non-overridable method that checks that its zero-indexed `argument`
20+
* is equal to `checkTrue` and throws otherwise.
21+
*/
22+
predicate conditionCheckMethodArgument(Method m, int argument, boolean checkTrue) {
23+
condtionCheckMethodGooglePreconditions(m, checkTrue) and argument = 0
1724
or
18-
m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate") and
19-
checkTrue = true and
20-
(m.hasName("isTrue") or m.hasName("validState"))
25+
conditionCheckMethodApacheCommonsLang3Validate(m, checkTrue) and argument = 0
26+
or
27+
condtionCheckMethodTestingFramework(m, argument, checkTrue)
28+
or
29+
exists(Parameter p, MethodAccess ma, int argIndex, boolean ct, Expr arg |
30+
p = m.getParameter(argument) and
31+
not m.isOverridable() and
32+
m.getBody().getStmt(0).(ExprStmt).getExpr() = ma and
33+
conditionCheckArgument(ma, argIndex, ct) and
34+
ma.getArgument(argIndex) = arg and
35+
(
36+
arg.(LogNotExpr).getExpr().(VarAccess).getVariable() = p and
37+
checkTrue = ct.booleanNot()
38+
or
39+
arg.(VarAccess).getVariable() = p and checkTrue = ct
40+
)
41+
)
2142
or
2243
exists(Parameter p, IfStmt ifstmt, Expr cond |
23-
p = m.getParameter(0) and
44+
p = m.getParameter(argument) and
2445
not m.isOverridable() and
2546
p.getType() instanceof BooleanType and
2647
m.getBody().getStmt(0) = ifstmt and
@@ -35,26 +56,68 @@ predicate conditionCheckMethod(Method m, boolean checkTrue) {
3556
ifstmt.getThen().(SingletonBlock).getStmt() instanceof ThrowStmt
3657
)
3758
)
38-
or
39-
exists(Parameter p, MethodAccess ma, boolean ct, Expr arg |
40-
p = m.getParameter(0) and
41-
not m.isOverridable() and
42-
m.getBody().getStmt(0).(ExprStmt).getExpr() = ma and
43-
conditionCheck(ma, ct) and
44-
ma.getArgument(0) = arg and
59+
}
60+
61+
private predicate condtionCheckMethodGooglePreconditions(Method m, boolean checkTrue) {
62+
m.getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions") and
63+
checkTrue = true and
64+
(m.hasName("checkArgument") or m.hasName("checkState"))
65+
}
66+
67+
private predicate conditionCheckMethodApacheCommonsLang3Validate(Method m, boolean checkTrue) {
68+
m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate") and
69+
checkTrue = true and
70+
(m.hasName("isTrue") or m.hasName("validState"))
71+
}
72+
73+
/**
74+
* Holds if `m` is a non-overridable testing framework method that checks that its first argument
75+
* is equal to `checkTrue` and throws otherwise.
76+
*/
77+
private predicate condtionCheckMethodTestingFramework(Method m, int argument, boolean checkTrue) {
78+
argument = 0 and
79+
(
80+
m.getDeclaringType().hasQualifiedName("org.junit", "Assume") and
81+
checkTrue = true and
82+
m.hasName("assumeTrue")
83+
or
84+
m.getDeclaringType().hasQualifiedName("org.junit.jupiter.api", "Assertions") and
4585
(
46-
arg.(LogNotExpr).getExpr().(VarAccess).getVariable() = p and
47-
checkTrue = ct.booleanNot()
86+
checkTrue = true and m.hasName("assertTrue")
4887
or
49-
arg.(VarAccess).getVariable() = p and checkTrue = ct
88+
checkTrue = false and m.hasName("assertFalse")
89+
)
90+
or
91+
m.getDeclaringType().hasQualifiedName("org.junit.jupiter.api", "Assumptions") and
92+
(
93+
checkTrue = true and m.hasName("assumeTrue")
94+
or
95+
checkTrue = false and m.hasName("assumeFalse")
5096
)
5197
)
98+
or
99+
m.getDeclaringType().hasQualifiedName(["org.junit", "org.testng"], "Assert") and
100+
m.getParameter(argument).getType() instanceof BooleanType and
101+
(
102+
checkTrue = true and m.hasName("assertTrue")
103+
or
104+
checkTrue = false and m.hasName("assertFalse")
105+
)
52106
}
53107

54108
/**
109+
* DEPRECATED: Use `conditionCheckArgument` instead.
55110
* Holds if `ma` is an access to a non-overridable method that checks that its
56111
* first argument is equal to `checkTrue` and throws otherwise.
57112
*/
58-
predicate conditionCheck(MethodAccess ma, boolean checkTrue) {
59-
conditionCheckMethod(ma.getMethod().getSourceDeclaration(), checkTrue)
113+
deprecated predicate conditionCheck(MethodAccess ma, boolean checkTrue) {
114+
conditionCheckArgument(ma, 0, checkTrue)
115+
}
116+
117+
/**
118+
* Holds if `ma` is an access to a non-overridable method that checks that its
119+
* zero-indexed `argument` is equal to `checkTrue` and throws otherwise.
120+
*/
121+
predicate conditionCheckArgument(MethodAccess ma, int argument, boolean checkTrue) {
122+
conditionCheckMethodArgument(ma.getMethod().getSourceDeclaration(), argument, checkTrue)
60123
}

‎java/ql/test/library-tests/guards/Logic.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,4 +47,21 @@ private static void checkTrue(boolean b, String msg) {
4747
private static void checkFalse(boolean b, String msg) {
4848
checkTrue(!b, msg);
4949
}
50+
51+
void f3(int i) {
52+
checkTrue("i pos", i > 0);
53+
checkFalse("g", g(100));
54+
if (i > 10) {
55+
checkTrue("", i > 20);
56+
}
57+
int dummy = 0;
58+
}
59+
60+
private static void checkTrue(String msg, boolean b) {
61+
if (!b) throw new Error (msg);
62+
}
63+
64+
private static void checkFalse(String msg, boolean b) {
65+
checkTrue(!b, msg);
66+
}
5067
}

‎java/ql/test/library-tests/guards/Preconditions.java

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
import org.junit.Assert;
2+
import org.junit.jupiter.api.Assertions;
3+
4+
public class Preconditions {
5+
public static void guarded() {}
6+
7+
void test1() {
8+
Assert.assertTrue(true);
9+
guarded();
10+
}
11+
12+
void test2() {
13+
Assert.assertTrue(false);
14+
guarded();
15+
}
16+
17+
void test3() {
18+
Assert.assertFalse(false);
19+
guarded();
20+
}
21+
22+
void test4() {
23+
Assert.assertFalse(true);
24+
guarded();
25+
}
26+
27+
void test5() {
28+
Assert.assertTrue("Reason", true);
29+
guarded();
30+
}
31+
32+
void test6() {
33+
Assert.assertTrue("Reason", false);
34+
guarded();
35+
}
36+
37+
void test7() {
38+
Assert.assertFalse("Reason", false);
39+
guarded();
40+
}
41+
42+
void test8() {
43+
Assert.assertFalse("Reason", true);
44+
guarded();
45+
}
46+
47+
void test9() {
48+
Assertions.assertTrue(true);
49+
guarded();
50+
}
51+
52+
void test10() {
53+
Assertions.assertTrue(false);
54+
guarded();
55+
}
56+
57+
void test11() {
58+
Assertions.assertFalse(false);
59+
guarded();
60+
}
61+
62+
void test12() {
63+
Assertions.assertFalse(true);
64+
guarded();
65+
}
66+
67+
void test13() {
68+
Assertions.assertTrue(true, "Reason");
69+
guarded();
70+
}
71+
72+
void test14() {
73+
Assertions.assertTrue(false, "Reason");
74+
guarded();
75+
}
76+
77+
void test15() {
78+
Assertions.assertFalse(false, "Reason");
79+
guarded();
80+
}
81+
82+
void test16() {
83+
Assertions.assertFalse(true, "Reason");
84+
guarded();
85+
}
86+
87+
void test17() {
88+
t(true);
89+
guarded();
90+
}
91+
92+
void test18() {
93+
t(false);
94+
guarded();
95+
}
96+
97+
void test19() {
98+
f(false);
99+
guarded();
100+
}
101+
102+
void test20() {
103+
f(true);
104+
guarded();
105+
}
106+
107+
static void t(boolean b) {
108+
Assert.assertTrue("Unified Reason", b);
109+
}
110+
111+
static void f(boolean b) {
112+
Assert.assertFalse("Unified Reason", b);
113+
}
114+
}

‎java/ql/test/library-tests/guards/guardslogic.expected

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,3 +46,19 @@
4646
| Logic.java:36:16:36:21 | g(...) | false | Logic.java:40:5:40:18 | var ...; |
4747
| Logic.java:37:9:37:14 | ... > ... | true | Logic.java:37:17:39:5 | { ... } |
4848
| Logic.java:44:10:44:10 | b | false | Logic.java:44:33:44:35 | msg |
49+
| Logic.java:52:5:52:29 | checkTrue(...) | true | Logic.java:53:5:53:28 | <Expr>; |
50+
| Logic.java:52:5:52:29 | checkTrue(...) | true | Logic.java:54:5:54:15 | if (...) |
51+
| Logic.java:52:5:52:29 | checkTrue(...) | true | Logic.java:54:17:56:5 | { ... } |
52+
| Logic.java:52:5:52:29 | checkTrue(...) | true | Logic.java:57:5:57:18 | var ...; |
53+
| Logic.java:52:24:52:28 | ... > ... | true | Logic.java:53:5:53:28 | <Expr>; |
54+
| Logic.java:52:24:52:28 | ... > ... | true | Logic.java:54:5:54:15 | if (...) |
55+
| Logic.java:52:24:52:28 | ... > ... | true | Logic.java:54:17:56:5 | { ... } |
56+
| Logic.java:52:24:52:28 | ... > ... | true | Logic.java:57:5:57:18 | var ...; |
57+
| Logic.java:53:5:53:27 | checkFalse(...) | false | Logic.java:54:5:54:15 | if (...) |
58+
| Logic.java:53:5:53:27 | checkFalse(...) | false | Logic.java:54:17:56:5 | { ... } |
59+
| Logic.java:53:5:53:27 | checkFalse(...) | false | Logic.java:57:5:57:18 | var ...; |
60+
| Logic.java:53:21:53:26 | g(...) | false | Logic.java:54:5:54:15 | if (...) |
61+
| Logic.java:53:21:53:26 | g(...) | false | Logic.java:54:17:56:5 | { ... } |
62+
| Logic.java:53:21:53:26 | g(...) | false | Logic.java:57:5:57:18 | var ...; |
63+
| Logic.java:54:9:54:14 | ... > ... | true | Logic.java:54:17:56:5 | { ... } |
64+
| Logic.java:61:10:61:10 | b | false | Logic.java:61:33:61:35 | msg |

‎java/ql/test/library-tests/guards/guardspreconditions.expected

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
| Preconditions.java:8:9:8:31 | assertTrue(...) | true | Preconditions.java:9:9:9:18 | <Expr>; |
2+
| Preconditions.java:13:9:13:32 | assertTrue(...) | true | Preconditions.java:14:9:14:18 | <Expr>; |
3+
| Preconditions.java:18:9:18:33 | assertFalse(...) | false | Preconditions.java:19:9:19:18 | <Expr>; |
4+
| Preconditions.java:23:9:23:32 | assertFalse(...) | false | Preconditions.java:24:9:24:18 | <Expr>; |
5+
| Preconditions.java:28:9:28:41 | assertTrue(...) | true | Preconditions.java:29:9:29:18 | <Expr>; |
6+
| Preconditions.java:33:9:33:42 | assertTrue(...) | true | Preconditions.java:34:9:34:18 | <Expr>; |
7+
| Preconditions.java:38:9:38:43 | assertFalse(...) | false | Preconditions.java:39:9:39:18 | <Expr>; |
8+
| Preconditions.java:43:9:43:42 | assertFalse(...) | false | Preconditions.java:44:9:44:18 | <Expr>; |
9+
| Preconditions.java:48:9:48:35 | assertTrue(...) | true | Preconditions.java:49:9:49:18 | <Expr>; |
10+
| Preconditions.java:53:9:53:36 | assertTrue(...) | true | Preconditions.java:54:9:54:18 | <Expr>; |
11+
| Preconditions.java:58:9:58:37 | assertFalse(...) | false | Preconditions.java:59:9:59:18 | <Expr>; |
12+
| Preconditions.java:63:9:63:36 | assertFalse(...) | false | Preconditions.java:64:9:64:18 | <Expr>; |
13+
| Preconditions.java:68:9:68:45 | assertTrue(...) | true | Preconditions.java:69:9:69:18 | <Expr>; |
14+
| Preconditions.java:73:9:73:46 | assertTrue(...) | true | Preconditions.java:74:9:74:18 | <Expr>; |
15+
| Preconditions.java:78:9:78:47 | assertFalse(...) | false | Preconditions.java:79:9:79:18 | <Expr>; |
16+
| Preconditions.java:83:9:83:46 | assertFalse(...) | false | Preconditions.java:84:9:84:18 | <Expr>; |
17+
| Preconditions.java:88:9:88:15 | t(...) | true | Preconditions.java:89:9:89:18 | <Expr>; |
18+
| Preconditions.java:93:9:93:16 | t(...) | true | Preconditions.java:94:9:94:18 | <Expr>; |
19+
| Preconditions.java:98:9:98:16 | f(...) | false | Preconditions.java:99:9:99:18 | <Expr>; |
20+
| Preconditions.java:103:9:103:15 | f(...) | false | Preconditions.java:104:9:104:18 | <Expr>; |

‎java/ql/test/library-tests/guards/guardspreconditions.ql

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import java
2+
import semmle.code.java.controlflow.Guards
3+
4+
from Guard g, BasicBlock bb, boolean branch
5+
where
6+
g.controls(bb, branch) and
7+
g.getEnclosingCallable().getDeclaringType().hasName("Preconditions")
8+
select g, branch, bb

0 commit comments

Comments
 (0)