Skip to content

Commit b3ee1bd

Browse files
JLLeitschuhJLLeitschuh
committed
Refactor Preconditions and add Tests
1 parent db0879e commit b3ee1bd

File tree

6 files changed

+140
-66
lines changed

6 files changed

+140
-66
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -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+
conditionCheck(ma, _, branch) and
193193
bb1.getLastNode() = ma.getControlFlowNode() and
194194
bb2 = bb1.getLastNode().getANormalSuccessor()
195195
}

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

Lines changed: 58 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -12,35 +12,35 @@ import java
1212
*/
1313
predicate conditionCheckMethod(Method m, boolean checkTrue) {
1414
conditionCheckMethod(m, 0, checkTrue)
15+
}
16+
17+
/**
18+
* Holds if `m` is a non-overridable method that checks that its zero-indexed `argument`
19+
* is equal to `checkTrue` and throws otherwise.
20+
*/
21+
predicate conditionCheckMethod(Method m, int argument, boolean checkTrue) {
22+
condtionCheckMethodGooglePreconditions(m, checkTrue) and argument = 0
1523
or
16-
m.getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions") and
17-
checkTrue = true and
18-
(m.hasName("checkArgument") or m.hasName("checkState"))
19-
or
20-
m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate") and
21-
checkTrue = true and
22-
(m.hasName("isTrue") or m.hasName("validState"))
24+
conditionCheckMethodApacheCommonsLang3Validate(m, checkTrue) and argument = 0
2325
or
24-
m.getDeclaringType().hasQualifiedName("org.junit", "Assume") and
25-
checkTrue = true and
26-
m.hasName("assumeTrue")
26+
condtionCheckMethodTestingFramework(m, argument, checkTrue)
2727
or
28-
m.getDeclaringType().hasQualifiedName("org.junit.jupiter.api", "Assertions") and
29-
(
30-
checkTrue = true and m.hasName("assertTrue")
31-
or
32-
checkTrue = false and m.hasName("assertFalse")
33-
)
34-
or
35-
m.getDeclaringType().hasQualifiedName("org.junit.jupiter.api", "Assumptions") and
36-
(
37-
checkTrue = true and m.hasName("assumeTrue")
38-
or
39-
checkTrue = false and m.hasName("assumeFalse")
28+
exists(Parameter p, MethodAccess ma, int argIndex, boolean ct, Expr arg |
29+
p = m.getParameter(argument) and
30+
not m.isOverridable() and
31+
m.getBody().getStmt(0).(ExprStmt).getExpr() = ma and
32+
conditionCheck(ma, argIndex, ct) and
33+
ma.getArgument(argIndex) = arg and
34+
(
35+
arg.(LogNotExpr).getExpr().(VarAccess).getVariable() = p and
36+
checkTrue = ct.booleanNot()
37+
or
38+
arg.(VarAccess).getVariable() = p and checkTrue = ct
39+
)
4040
)
4141
or
4242
exists(Parameter p, IfStmt ifstmt, Expr cond |
43-
p = m.getParameter(0) and
43+
p = m.getParameter(argument) and
4444
not m.isOverridable() and
4545
p.getType() instanceof BooleanType and
4646
m.getBody().getStmt(0) = ifstmt and
@@ -57,12 +57,43 @@ predicate conditionCheckMethod(Method m, boolean checkTrue) {
5757
)
5858
}
5959

60+
private predicate condtionCheckMethodGooglePreconditions(Method m, boolean checkTrue) {
61+
m.getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions") and
62+
checkTrue = true and
63+
(m.hasName("checkArgument") or m.hasName("checkState"))
64+
}
65+
66+
private predicate conditionCheckMethodApacheCommonsLang3Validate(Method m, boolean checkTrue) {
67+
m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate") and
68+
checkTrue = true and
69+
(m.hasName("isTrue") or m.hasName("validState"))
70+
}
71+
6072
/**
61-
* Holds if `m` is a non-overridable method that checks that its zero-indexed `argument`
73+
* Holds if `m` is a non-overridable testing framework methopd that checks that its first argument
6274
* is equal to `checkTrue` and throws otherwise.
6375
*/
64-
predicate conditionCheckMethod(Method m, int argument, boolean checkTrue) {
65-
conditionCheckMethod(m, checkTrue) and argument = 0
76+
private predicate condtionCheckMethodTestingFramework(Method m, int argument, boolean checkTrue) {
77+
argument = 0 and
78+
(
79+
m.getDeclaringType().hasQualifiedName("org.junit", "Assume") and
80+
checkTrue = true and
81+
m.hasName("assumeTrue")
82+
or
83+
m.getDeclaringType().hasQualifiedName("org.junit.jupiter.api", "Assertions") and
84+
(
85+
checkTrue = true and m.hasName("assertTrue")
86+
or
87+
checkTrue = false and m.hasName("assertFalse")
88+
)
89+
or
90+
m.getDeclaringType().hasQualifiedName("org.junit.jupiter.api", "Assumptions") and
91+
(
92+
checkTrue = true and m.hasName("assumeTrue")
93+
or
94+
checkTrue = false and m.hasName("assumeFalse")
95+
)
96+
)
6697
or
6798
m.getDeclaringType().hasQualifiedName(["org.junit", "org.testng"], "Assert") and
6899
m.getParameter(argument).getType() instanceof BooleanType and
@@ -71,29 +102,13 @@ predicate conditionCheckMethod(Method m, int argument, boolean checkTrue) {
71102
or
72103
checkTrue = false and m.hasName("assertFalse")
73104
)
74-
or
75-
exists(Parameter p, MethodAccess ma, int argIndex, boolean ct, Expr arg |
76-
p = m.getParameter(argument) and
77-
not m.isOverridable() and
78-
m.getBody().getStmt(0).(ExprStmt).getExpr() = ma and
79-
conditionCheck(ma, argIndex, ct) and
80-
ma.getArgument(argIndex) = arg and
81-
(
82-
arg.(LogNotExpr).getExpr().(VarAccess).getVariable() = p and
83-
checkTrue = ct.booleanNot()
84-
or
85-
arg.(VarAccess).getVariable() = p and checkTrue = ct
86-
)
87-
)
88105
}
89106

90107
/**
91108
* Holds if `ma` is an access to a non-overridable method that checks that its
92109
* first argument is equal to `checkTrue` and throws otherwise.
93110
*/
94-
predicate conditionCheck(MethodAccess ma, boolean checkTrue) {
95-
conditionCheckMethod(ma.getMethod().getSourceDeclaration(), checkTrue)
96-
}
111+
predicate conditionCheck(MethodAccess ma, boolean checkTrue) { conditionCheck(ma, 0, checkTrue) }
97112

98113
/**
99114
* Holds if `ma` is an access to a non-overridable method that checks that its

‎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: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -45,51 +45,70 @@ void test8() {
4545
}
4646

4747
void test9() {
48-
r(true);
48+
Assertions.assertTrue(true);
4949
guarded();
5050
}
5151

52-
static void r(boolean b) {
53-
Assert.assertTrue("Unified Reason", b);
54-
}
55-
5652
void test10() {
57-
Assertions.assertTrue(true);
53+
Assertions.assertTrue(false);
5854
guarded();
5955
}
6056

6157
void test11() {
62-
Assertions.assertTrue(false);
58+
Assertions.assertFalse(false);
6359
guarded();
6460
}
6561

6662
void test12() {
67-
Assertions.assertFalse(false);
63+
Assertions.assertFalse(true);
6864
guarded();
6965
}
7066

7167
void test13() {
72-
Assertions.assertFalse(true);
68+
Assertions.assertTrue(true, "Reason");
7369
guarded();
7470
}
7571

7672
void test14() {
77-
Assertions.assertTrue(true, "Reason");
73+
Assertions.assertTrue(false, "Reason");
7874
guarded();
7975
}
8076

8177
void test15() {
82-
Assertions.assertTrue(false, "Reason");
78+
Assertions.assertFalse(false, "Reason");
8379
guarded();
8480
}
8581

8682
void test16() {
87-
Assertions.assertFalse(false, "Reason");
83+
Assertions.assertFalse(true, "Reason");
8884
guarded();
8985
}
9086

9187
void test17() {
92-
Assertions.assertFalse(true, "Reason");
88+
t(true);
89+
guarded();
90+
}
91+
92+
void test18() {
93+
t(false);
94+
guarded();
95+
}
96+
97+
void test19() {
98+
f(false);
9399
guarded();
94100
}
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+
}
95114
}

‎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: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,19 @@
22
| Preconditions.java:13:9:13:32 | assertTrue(...) | true | Preconditions.java:14:9:14:18 | <Expr>; |
33
| Preconditions.java:18:9:18:33 | assertFalse(...) | false | Preconditions.java:19:9:19:18 | <Expr>; |
44
| Preconditions.java:23:9:23:32 | assertFalse(...) | false | Preconditions.java:24:9:24:18 | <Expr>; |
5-
| Preconditions.java:48:9:48:15 | r(...) | true | Preconditions.java:49:9:49:18 | <Expr>; |
6-
| Preconditions.java:57:9:57:35 | assertTrue(...) | true | Preconditions.java:58:9:58:18 | <Expr>; |
7-
| Preconditions.java:62:9:62:36 | assertTrue(...) | true | Preconditions.java:63:9:63:18 | <Expr>; |
8-
| Preconditions.java:67:9:67:37 | assertFalse(...) | false | Preconditions.java:68:9:68:18 | <Expr>; |
9-
| Preconditions.java:72:9:72:36 | assertFalse(...) | false | Preconditions.java:73:9:73:18 | <Expr>; |
10-
| Preconditions.java:77:9:77:45 | assertTrue(...) | true | Preconditions.java:78:9:78:18 | <Expr>; |
11-
| Preconditions.java:82:9:82:46 | assertTrue(...) | true | Preconditions.java:83:9:83:18 | <Expr>; |
12-
| Preconditions.java:87:9:87:47 | assertFalse(...) | false | Preconditions.java:88:9:88:18 | <Expr>; |
13-
| Preconditions.java:92:9:92:46 | assertFalse(...) | false | Preconditions.java:93:9:93: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>; |

0 commit comments

Comments
 (0)