Skip to content

Commit e8ddac0

Browse files
aschackmullaschackmull
authored
Merge pull request #20377 from aschackmull/java/preconditions
Java: Consolidate Assertions.qll and Preconditions.qll.
2 parents e9fcd98 + e7df1b2 commit e8ddac0

File tree

8 files changed

+158
-111
lines changed

8 files changed

+158
-111
lines changed

‎java/ql/lib/change-notes/2025-09-11-assertions-cfg.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+
* Improved support for various assertion libraries, in particular JUnit. This affects the control-flow graph slightly, and in turn affects several queries (mainly quality queries). Most queries should see improved precision (new true positives and fewer false positives), in particular `java/constant-comparison`, `java/index-out-of-bounds`, `java/dereferenced-value-may-be-null`, and `java/useless-null-check`. Some medium precision queries like `java/toctou-race-condition` and `java/unreleased-lock` may see mixed result changes (both slight improvements and slight regressions).

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -365,10 +365,10 @@ private module ControlFlowGraphImpl {
365365
* Bind `t` to an unchecked exception that may occur in a precondition check or guard wrapper.
366366
*/
367367
private predicate uncheckedExceptionFromMethod(MethodCall ma, ThrowableType t) {
368-
conditionCheckArgument(ma, _, _) and
368+
(methodCallChecksArgument(ma) or methodCallUnconditionallyThrows(ma)) and
369369
(t instanceof TypeError or t instanceof TypeRuntimeException)
370370
or
371-
methodMayThrow(ma.getMethod(), t)
371+
methodMayThrow(ma.getMethod().getSourceDeclaration(), t)
372372
}
373373

374374
/**
@@ -586,6 +586,7 @@ private module ControlFlowGraphImpl {
586586
* Gets a `MethodCall` that always throws an exception or calls `exit`.
587587
*/
588588
private MethodCall nonReturningMethodCall() {
589+
methodCallUnconditionallyThrows(result) or
589590
result.getMethod().getSourceDeclaration() = nonReturningMethod() or
590591
result = likelyNonReturningMethod().getAnAccess()
591592
}

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -395,11 +395,13 @@ private module LogicInputCommon {
395395
predicate additionalImpliesStep(
396396
GuardsImpl::PreGuard g1, GuardValue v1, GuardsImpl::PreGuard g2, GuardValue v2
397397
) {
398-
exists(MethodCall check, int argIndex |
398+
exists(MethodCall check |
399399
g1 = check and
400-
v1.getDualValue().isThrowsException() and
401-
conditionCheckArgument(check, argIndex, v2.asBooleanValue()) and
402-
g2 = check.getArgument(argIndex)
400+
v1.getDualValue().isThrowsException()
401+
|
402+
methodCallChecksBoolean(check, g2, v2.asBooleanValue())
403+
or
404+
methodCallChecksNotNull(check, g2) and v2.isNonNullValue()
403405
)
404406
}
405407
}

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

Lines changed: 122 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/**
2-
* Provides predicates for identifying precondition checks like
2+
* Provides predicates for identifying precondition and assertion checks like
33
* `com.google.common.base.Preconditions` and
44
* `org.apache.commons.lang3.Validate`.
55
*/
@@ -9,99 +9,150 @@ module;
99
import java
1010

1111
/**
12-
* Holds if `m` is a non-overridable method that checks that its zero-indexed `argument`
13-
* is equal to `checkTrue` and throws otherwise.
12+
* Holds if `m` is a method that checks that its argument at position `arg` is
13+
* equal to true and throws otherwise.
1414
*/
15-
predicate conditionCheckMethodArgument(Method m, int argument, boolean checkTrue) {
16-
condtionCheckMethodGooglePreconditions(m, checkTrue) and argument = 0
17-
or
18-
conditionCheckMethodApacheCommonsLang3Validate(m, checkTrue) and argument = 0
15+
private predicate methodCheckTrue(Method m, int arg) {
16+
arg = 0 and
17+
(
18+
m.hasQualifiedName("com.google.common.base", "Preconditions", ["checkArgument", "checkState"]) or
19+
m.hasQualifiedName("com.google.common.base", "Verify", "verify") or
20+
m.hasQualifiedName("org.apache.commons.lang3", "Validate", ["isTrue", "validState"]) or
21+
m.hasQualifiedName("org.junit.jupiter.api", "Assertions", "assertTrue") or
22+
m.hasQualifiedName("org.junit.jupiter.api", "Assumptions", "assumeTrue") or
23+
m.hasQualifiedName("org.testng", "Assert", "assertTrue")
24+
)
1925
or
20-
condtionCheckMethodTestingFramework(m, argument, checkTrue)
26+
m.getParameter(arg).getType() instanceof BooleanType and
27+
(
28+
m.hasQualifiedName("org.junit", "Assert", "assertTrue") or
29+
m.hasQualifiedName("org.junit", "Assume", "assumeTrue") or
30+
m.hasQualifiedName("junit.framework", _, "assertTrue")
31+
)
32+
}
33+
34+
/**
35+
* Holds if `m` is a method that checks that its argument at position `arg` is
36+
* equal to false and throws otherwise.
37+
*/
38+
private predicate methodCheckFalse(Method m, int arg) {
39+
arg = 0 and
40+
(
41+
m.hasQualifiedName("org.junit.jupiter.api", "Assertions", "assertFalse") or
42+
m.hasQualifiedName("org.junit.jupiter.api", "Assumptions", "assumeFalse") or
43+
m.hasQualifiedName("org.testng", "Assert", "assertFalse")
44+
)
2145
or
22-
exists(Parameter p, MethodCall ma, int argIndex, boolean ct, Expr arg |
23-
p = m.getParameter(argument) and
24-
not m.isOverridable() and
25-
m.getBody().getStmt(0).(ExprStmt).getExpr() = ma and
26-
conditionCheckArgument(ma, argIndex, ct) and
27-
ma.getArgument(argIndex) = arg and
28-
(
29-
arg.(LogNotExpr).getExpr().(VarAccess).getVariable() = p and
30-
checkTrue = ct.booleanNot()
31-
or
32-
arg.(VarAccess).getVariable() = p and checkTrue = ct
33-
)
46+
m.getParameter(arg).getType() instanceof BooleanType and
47+
(
48+
m.hasQualifiedName("org.junit", "Assert", "assertFalse") or
49+
m.hasQualifiedName("org.junit", "Assume", "assumeFalse") or
50+
m.hasQualifiedName("junit.framework", _, "assertFalse")
51+
)
52+
}
53+
54+
/**
55+
* Holds if `m` is a method that checks that its argument at position `arg` is
56+
* not null and throws otherwise.
57+
*/
58+
private predicate methodCheckNotNull(Method m, int arg) {
59+
arg = 0 and
60+
(
61+
m.hasQualifiedName("com.google.common.base", "Preconditions", "checkNotNull") or
62+
m.hasQualifiedName("com.google.common.base", "Verify", "verifyNotNull") or
63+
m.hasQualifiedName("org.apache.commons.lang3", "Validate", "notNull") or
64+
m.hasQualifiedName("java.util", "Objects", "requireNonNull") or
65+
m.hasQualifiedName("org.junit.jupiter.api", "Assertions", "assertNotNull") or
66+
m.hasQualifiedName("org.junit", "Assume", "assumeNotNull") or // vararg
67+
m.hasQualifiedName("org.testng", "Assert", "assertNotNull")
3468
)
3569
or
36-
exists(Parameter p, IfStmt ifstmt, Expr cond |
37-
p = m.getParameter(argument) and
38-
not m.isOverridable() and
39-
p.getType() instanceof BooleanType and
40-
m.getBody().getStmt(0) = ifstmt and
41-
ifstmt.getCondition() = cond and
42-
(
43-
cond.(LogNotExpr).getExpr().(VarAccess).getVariable() = p and checkTrue = true
44-
or
45-
cond.(VarAccess).getVariable() = p and checkTrue = false
46-
) and
47-
(
48-
ifstmt.getThen() instanceof ThrowStmt or
49-
ifstmt.getThen().(SingletonBlock).getStmt() instanceof ThrowStmt
50-
)
70+
arg = m.getNumberOfParameters() - 1 and
71+
(
72+
m.hasQualifiedName("org.junit", "Assert", "assertNotNull") or
73+
m.hasQualifiedName("junit.framework", _, "assertNotNull")
5174
)
5275
}
5376

54-
private predicate condtionCheckMethodGooglePreconditions(Method m, boolean checkTrue) {
55-
m.getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions") and
56-
checkTrue = true and
57-
(m.hasName("checkArgument") or m.hasName("checkState"))
77+
/**
78+
* Holds if `m` is a method that checks that its argument at position `arg`
79+
* satisfies a property specified by another argument and throws otherwise.
80+
*/
81+
private predicate methodCheckThat(Method m, int arg) {
82+
m.getParameter(arg).getType().getErasure() instanceof TypeObject and
83+
(
84+
m.hasQualifiedName("org.hamcrest", "MatcherAssert", "assertThat") or
85+
m.hasQualifiedName("org.junit", "Assert", "assertThat") or
86+
m.hasQualifiedName("org.junit", "Assume", "assumeThat")
87+
)
5888
}
5989

60-
private predicate conditionCheckMethodApacheCommonsLang3Validate(Method m, boolean checkTrue) {
61-
m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate") and
62-
checkTrue = true and
63-
(m.hasName("isTrue") or m.hasName("validState"))
90+
/** Holds if `m` is a method that unconditionally throws. */
91+
private predicate methodUnconditionallyThrows(Method m) {
92+
m.hasQualifiedName("org.junit.jupiter.api", "Assertions", "fail") or
93+
m.hasQualifiedName("org.junit", "Assert", "fail") or
94+
m.hasQualifiedName("junit.framework", _, "fail") or
95+
m.hasQualifiedName("org.testng", "Assert", "fail")
6496
}
6597

6698
/**
67-
* Holds if `m` is a non-overridable testing framework method that checks that its first argument
68-
* is equal to `checkTrue` and throws otherwise.
99+
* Holds if `mc` is a call to a method that checks that its argument `arg` is
100+
* equal to `checkTrue` and throws otherwise.
69101
*/
70-
private predicate condtionCheckMethodTestingFramework(Method m, int argument, boolean checkTrue) {
71-
argument = 0 and
72-
(
73-
m.getDeclaringType().hasQualifiedName("org.junit", "Assume") and
74-
checkTrue = true and
75-
m.hasName("assumeTrue")
102+
predicate methodCallChecksBoolean(MethodCall mc, Expr arg, boolean checkTrue) {
103+
exists(int pos | mc.getArgument(pos) = arg |
104+
methodCheckTrue(mc.getMethod().getSourceDeclaration(), pos) and checkTrue = true
76105
or
77-
m.getDeclaringType().hasQualifiedName("org.junit.jupiter.api", "Assertions") and
78-
(
79-
checkTrue = true and m.hasName("assertTrue")
80-
or
81-
checkTrue = false and m.hasName("assertFalse")
82-
)
83-
or
84-
m.getDeclaringType().hasQualifiedName("org.junit.jupiter.api", "Assumptions") and
85-
(
86-
checkTrue = true and m.hasName("assumeTrue")
87-
or
88-
checkTrue = false and m.hasName("assumeFalse")
89-
)
106+
methodCheckFalse(mc.getMethod().getSourceDeclaration(), pos) and checkTrue = false
90107
)
91-
or
92-
m.getDeclaringType().hasQualifiedName(["org.junit", "org.testng"], "Assert") and
93-
m.getParameter(argument).getType() instanceof BooleanType and
94-
(
95-
checkTrue = true and m.hasName("assertTrue")
108+
}
109+
110+
/**
111+
* Holds if `mc` is a call to a method that checks that its argument `arg` is
112+
* not null and throws otherwise.
113+
*/
114+
predicate methodCallChecksNotNull(MethodCall mc, Expr arg) {
115+
exists(int pos | mc.getArgument(pos) = arg |
116+
methodCheckNotNull(mc.getMethod().getSourceDeclaration(), pos)
96117
or
97-
checkTrue = false and m.hasName("assertFalse")
118+
methodCheckThat(mc.getMethod().getSourceDeclaration(), pos) and
119+
mc.getAnArgument().(MethodCall).getMethod().getName() = "notNullValue"
98120
)
99121
}
100122

101123
/**
124+
* Holds if `mc` is a call to a method that checks one of its arguments in some
125+
* way and possibly throws.
126+
*/
127+
predicate methodCallChecksArgument(MethodCall mc) {
128+
methodCallChecksBoolean(mc, _, _) or
129+
methodCallChecksNotNull(mc, _)
130+
}
131+
132+
/** Holds if `mc` is a call to a method that unconditionally throws. */
133+
predicate methodCallUnconditionallyThrows(MethodCall mc) {
134+
methodUnconditionallyThrows(mc.getMethod().getSourceDeclaration()) or
135+
exists(BooleanLiteral b | methodCallChecksBoolean(mc, b, b.getBooleanValue().booleanNot()))
136+
}
137+
138+
/**
139+
* DEPRECATED: Use `methodCallChecksBoolean` instead.
140+
*
141+
* Holds if `m` is a non-overridable method that checks that its zero-indexed `argument`
142+
* is equal to `checkTrue` and throws otherwise.
143+
*/
144+
deprecated predicate conditionCheckMethodArgument(Method m, int argument, boolean checkTrue) {
145+
methodCheckTrue(m, argument) and checkTrue = true
146+
or
147+
methodCheckFalse(m, argument) and checkTrue = false
148+
}
149+
150+
/**
151+
* DEPRECATED: Use `methodCallChecksBoolean` instead.
152+
*
102153
* Holds if `ma` is an access to a non-overridable method that checks that its
103154
* zero-indexed `argument` is equal to `checkTrue` and throws otherwise.
104155
*/
105-
predicate conditionCheckArgument(MethodCall ma, int argument, boolean checkTrue) {
156+
deprecated predicate conditionCheckArgument(MethodCall ma, int argument, boolean checkTrue) {
106157
conditionCheckMethodArgument(ma.getMethod().getSourceDeclaration(), argument, checkTrue)
107158
}

‎java/ql/lib/semmle/code/java/dataflow/Nullness.qll‎

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ private import RangeUtils
4242
private import IntegerGuards
4343
private import NullGuards
4444
private import semmle.code.java.Collections
45-
private import semmle.code.java.frameworks.Assertions
45+
private import semmle.code.java.controlflow.internal.Preconditions
4646

4747
/** Gets an expression that may be `null`. */
4848
Expr nullExpr() {
@@ -140,20 +140,11 @@ private ControlFlowNode varDereference(SsaVariable v, VarAccess va) {
140140
* A `ControlFlowNode` that ensures that the SSA variable is not null in any
141141
* subsequent use, either by dereferencing it or by an assertion.
142142
*/
143-
private ControlFlowNode ensureNotNull(SsaVariable v) {
144-
result = varDereference(v, _)
145-
or
146-
exists(AssertTrueMethod m | result.asCall() = m.getACheck(directNullGuard(v, true, false)))
147-
or
148-
exists(AssertFalseMethod m | result.asCall() = m.getACheck(directNullGuard(v, false, false)))
149-
or
150-
exists(AssertNotNullMethod m | result.asCall() = m.getACheck(v.getAUse()))
151-
or
152-
exists(AssertThatMethod m, MethodCall ma |
153-
result.asCall() = m.getACheck(v.getAUse()) and ma.getControlFlowNode() = result
154-
|
155-
ma.getAnArgument().(MethodCall).getMethod().getName() = "notNullValue"
156-
)
143+
private ControlFlowNode ensureNotNull(SsaVariable v) { result = varDereference(v, _) }
144+
145+
private predicate assertFail(BasicBlock bb, ControlFlowNode n) {
146+
bb = n.getBasicBlock() and
147+
methodCallUnconditionallyThrows(n.asExpr())
157148
}
158149

159150
/**

‎java/ql/lib/semmle/code/java/frameworks/Assertions.qll‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
/**
2+
* DEPRECATED.
3+
*
24
* A library providing uniform access to various assertion frameworks.
35
*
46
* Currently supports `org.junit.Assert`, `junit.framework.*`,
57
* `org.junit.jupiter.api.Assertions`, `com.google.common.base.Preconditions`,
68
* and `java.util.Objects`.
79
*/
810
overlay[local?]
9-
module;
11+
deprecated module;
1012

1113
import java
1214

‎java/ql/src/Violations of Best Practice/Dead Code/DeadLocals.qll‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
import java
66
import semmle.code.java.dataflow.SSA
7-
private import semmle.code.java.frameworks.Assertions
7+
private import semmle.code.java.controlflow.internal.Preconditions
88

99
private predicate emptyDecl(LocalVariableDeclExpr decl) {
1010
not exists(decl.getInit()) and
@@ -22,7 +22,19 @@ predicate deadLocal(VariableUpdate upd) {
2222
/**
2323
* A dead variable update that is expected to be dead as indicated by an assertion.
2424
*/
25-
predicate expectedDead(VariableUpdate upd) { assertFail(upd.getBasicBlock(), _) }
25+
predicate expectedDead(VariableUpdate upd) {
26+
exists(BasicBlock bb, ControlFlowNode n |
27+
bb = upd.getBasicBlock() and
28+
bb = n.getBasicBlock()
29+
|
30+
methodCallUnconditionallyThrows(n.asExpr())
31+
or
32+
exists(AssertStmt a |
33+
n.asExpr() = a.getExpr() and
34+
a.getExpr().(BooleanLiteral).getBooleanValue() = false
35+
)
36+
)
37+
}
2638

2739
/**
2840
* A dead update that is overwritten by a live update.

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

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,35 +1,19 @@
11
| Preconditions.java:8:9:8:31 | assertTrue(...) | exception | Preconditions.java:7:10:7:14 | Exceptional Exit |
22
| Preconditions.java:8:9:8:31 | assertTrue(...) | no exception | Preconditions.java:9:9:9:18 | <Expr>; |
3-
| Preconditions.java:13:9:13:32 | assertTrue(...) | exception | Preconditions.java:12:10:12:14 | Exceptional Exit |
4-
| Preconditions.java:13:9:13:32 | assertTrue(...) | no exception | Preconditions.java:14:9:14:18 | <Expr>; |
53
| Preconditions.java:18:9:18:33 | assertFalse(...) | exception | Preconditions.java:17:10:17:14 | Exceptional Exit |
64
| Preconditions.java:18:9:18:33 | assertFalse(...) | no exception | Preconditions.java:19:9:19:18 | <Expr>; |
7-
| Preconditions.java:23:9:23:32 | assertFalse(...) | exception | Preconditions.java:22:10:22:14 | Exceptional Exit |
8-
| Preconditions.java:23:9:23:32 | assertFalse(...) | no exception | Preconditions.java:24:9:24:18 | <Expr>; |
95
| Preconditions.java:28:9:28:41 | assertTrue(...) | exception | Preconditions.java:27:10:27:14 | Exceptional Exit |
106
| Preconditions.java:28:9:28:41 | assertTrue(...) | no exception | Preconditions.java:29:9:29:18 | <Expr>; |
11-
| Preconditions.java:33:9:33:42 | assertTrue(...) | exception | Preconditions.java:32:10:32:14 | Exceptional Exit |
12-
| Preconditions.java:33:9:33:42 | assertTrue(...) | no exception | Preconditions.java:34:9:34:18 | <Expr>; |
137
| Preconditions.java:38:9:38:43 | assertFalse(...) | exception | Preconditions.java:37:10:37:14 | Exceptional Exit |
148
| Preconditions.java:38:9:38:43 | assertFalse(...) | no exception | Preconditions.java:39:9:39:18 | <Expr>; |
15-
| Preconditions.java:43:9:43:42 | assertFalse(...) | exception | Preconditions.java:42:10:42:14 | Exceptional Exit |
16-
| Preconditions.java:43:9:43:42 | assertFalse(...) | no exception | Preconditions.java:44:9:44:18 | <Expr>; |
179
| Preconditions.java:48:9:48:35 | assertTrue(...) | exception | Preconditions.java:47:10:47:14 | Exceptional Exit |
1810
| Preconditions.java:48:9:48:35 | assertTrue(...) | no exception | Preconditions.java:49:9:49:18 | <Expr>; |
19-
| Preconditions.java:53:9:53:36 | assertTrue(...) | exception | Preconditions.java:52:10:52:15 | Exceptional Exit |
20-
| Preconditions.java:53:9:53:36 | assertTrue(...) | no exception | Preconditions.java:54:9:54:18 | <Expr>; |
2111
| Preconditions.java:58:9:58:37 | assertFalse(...) | exception | Preconditions.java:57:10:57:15 | Exceptional Exit |
2212
| Preconditions.java:58:9:58:37 | assertFalse(...) | no exception | Preconditions.java:59:9:59:18 | <Expr>; |
23-
| Preconditions.java:63:9:63:36 | assertFalse(...) | exception | Preconditions.java:62:10:62:15 | Exceptional Exit |
24-
| Preconditions.java:63:9:63:36 | assertFalse(...) | no exception | Preconditions.java:64:9:64:18 | <Expr>; |
2513
| Preconditions.java:68:9:68:45 | assertTrue(...) | exception | Preconditions.java:67:10:67:15 | Exceptional Exit |
2614
| Preconditions.java:68:9:68:45 | assertTrue(...) | no exception | Preconditions.java:69:9:69:18 | <Expr>; |
27-
| Preconditions.java:73:9:73:46 | assertTrue(...) | exception | Preconditions.java:72:10:72:15 | Exceptional Exit |
28-
| Preconditions.java:73:9:73:46 | assertTrue(...) | no exception | Preconditions.java:74:9:74:18 | <Expr>; |
2915
| Preconditions.java:78:9:78:47 | assertFalse(...) | exception | Preconditions.java:77:10:77:15 | Exceptional Exit |
3016
| Preconditions.java:78:9:78:47 | assertFalse(...) | no exception | Preconditions.java:79:9:79:18 | <Expr>; |
31-
| Preconditions.java:83:9:83:46 | assertFalse(...) | exception | Preconditions.java:82:10:82:15 | Exceptional Exit |
32-
| Preconditions.java:83:9:83:46 | assertFalse(...) | no exception | Preconditions.java:84:9:84:18 | <Expr>; |
3317
| Preconditions.java:88:9:88:15 | t(...) | exception | Preconditions.java:87:10:87:15 | Exceptional Exit |
3418
| Preconditions.java:88:9:88:15 | t(...) | no exception | Preconditions.java:89:9:89:18 | <Expr>; |
3519
| Preconditions.java:93:9:93:16 | t(...) | exception | Preconditions.java:92:10:92:15 | Exceptional Exit |

0 commit comments

Comments
 (0)