Skip to content

Commit 1c93710

Browse files
aschackmullaschackmull
authored
Merge pull request #20826 from aschackmull/guards/disjunctive-implication
Guards: Support disjunctive implications.
2 parents 96f57b2 + 4867306 commit 1c93710

7 files changed

Lines changed: 140 additions & 3 deletions

File tree

‎csharp/ql/src/change-notes/2025-11-14-guards-disjunctive.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+
* An improvement to the Guards library for recognizing disjunctions means improved precision for `cs/constant-condition`, `cs/inefficient-containskey`, and `cs/dereferenced-value-may-be-null`. The two former can have additional findings, and the latter will have fewer false positives.

‎csharp/ql/test/library-tests/controlflow/guards/BooleanGuardedExpr.expected‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,12 @@
2727
| Guards.cs:36:32:36:32 | access to parameter x | Guards.cs:35:13:35:21 | ... == ... | Guards.cs:35:13:35:13 | access to parameter x | false |
2828
| Guards.cs:36:36:36:36 | access to parameter y | Guards.cs:35:26:35:34 | ... == ... | Guards.cs:35:26:35:26 | access to parameter y | false |
2929
| Guards.cs:39:31:39:31 | access to parameter x | Guards.cs:38:15:38:23 | ... == ... | Guards.cs:38:15:38:15 | access to parameter x | false |
30+
| Guards.cs:39:31:39:31 | access to parameter x | Guards.cs:38:15:38:23 | ... == ... | Guards.cs:38:15:38:15 | access to parameter x | true |
3031
| Guards.cs:39:35:39:35 | access to parameter y | Guards.cs:38:28:38:36 | ... == ... | Guards.cs:38:28:38:28 | access to parameter y | false |
32+
| Guards.cs:39:35:39:35 | access to parameter y | Guards.cs:38:28:38:36 | ... == ... | Guards.cs:38:28:38:28 | access to parameter y | true |
33+
| Guards.cs:42:32:42:32 | access to parameter x | Guards.cs:41:17:41:25 | ... != ... | Guards.cs:41:17:41:17 | access to parameter x | false |
3134
| Guards.cs:42:32:42:32 | access to parameter x | Guards.cs:41:17:41:25 | ... != ... | Guards.cs:41:17:41:17 | access to parameter x | true |
35+
| Guards.cs:42:36:42:36 | access to parameter y | Guards.cs:41:30:41:38 | ... != ... | Guards.cs:41:30:41:30 | access to parameter y | false |
3236
| Guards.cs:42:36:42:36 | access to parameter y | Guards.cs:41:30:41:38 | ... != ... | Guards.cs:41:30:41:30 | access to parameter y | true |
3337
| Guards.cs:48:31:48:40 | access to field Field | Guards.cs:47:13:47:25 | ... != ... | Guards.cs:47:13:47:17 | access to field Field | true |
3438
| Guards.cs:55:27:55:27 | access to parameter g | Guards.cs:53:13:53:27 | ... == ... | Guards.cs:53:13:53:13 | access to parameter g | false |

‎csharp/ql/test/library-tests/controlflow/guards/GuardedControlFlowNode.expected‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,12 +64,20 @@
6464
| Guards.cs:36:36:36:36 | access to parameter y | Guards.cs:35:26:35:26 | access to parameter y | Guards.cs:35:26:35:26 | access to parameter y | not null |
6565
| Guards.cs:36:36:36:36 | access to parameter y | Guards.cs:35:26:35:34 | ... == ... | Guards.cs:35:26:35:26 | access to parameter y | false |
6666
| Guards.cs:39:31:39:31 | access to parameter x | Guards.cs:38:15:38:15 | access to parameter x | Guards.cs:38:15:38:15 | access to parameter x | not null |
67+
| Guards.cs:39:31:39:31 | access to parameter x | Guards.cs:38:15:38:15 | access to parameter x | Guards.cs:38:15:38:15 | access to parameter x | null |
6768
| Guards.cs:39:31:39:31 | access to parameter x | Guards.cs:38:15:38:23 | ... == ... | Guards.cs:38:15:38:15 | access to parameter x | false |
69+
| Guards.cs:39:31:39:31 | access to parameter x | Guards.cs:38:15:38:23 | ... == ... | Guards.cs:38:15:38:15 | access to parameter x | true |
6870
| Guards.cs:39:35:39:35 | access to parameter y | Guards.cs:38:28:38:28 | access to parameter y | Guards.cs:38:28:38:28 | access to parameter y | not null |
71+
| Guards.cs:39:35:39:35 | access to parameter y | Guards.cs:38:28:38:28 | access to parameter y | Guards.cs:38:28:38:28 | access to parameter y | null |
6972
| Guards.cs:39:35:39:35 | access to parameter y | Guards.cs:38:28:38:36 | ... == ... | Guards.cs:38:28:38:28 | access to parameter y | false |
73+
| Guards.cs:39:35:39:35 | access to parameter y | Guards.cs:38:28:38:36 | ... == ... | Guards.cs:38:28:38:28 | access to parameter y | true |
7074
| Guards.cs:42:32:42:32 | access to parameter x | Guards.cs:41:17:41:17 | access to parameter x | Guards.cs:41:17:41:17 | access to parameter x | not null |
75+
| Guards.cs:42:32:42:32 | access to parameter x | Guards.cs:41:17:41:17 | access to parameter x | Guards.cs:41:17:41:17 | access to parameter x | null |
76+
| Guards.cs:42:32:42:32 | access to parameter x | Guards.cs:41:17:41:25 | ... != ... | Guards.cs:41:17:41:17 | access to parameter x | false |
7177
| Guards.cs:42:32:42:32 | access to parameter x | Guards.cs:41:17:41:25 | ... != ... | Guards.cs:41:17:41:17 | access to parameter x | true |
7278
| Guards.cs:42:36:42:36 | access to parameter y | Guards.cs:41:30:41:30 | access to parameter y | Guards.cs:41:30:41:30 | access to parameter y | not null |
79+
| Guards.cs:42:36:42:36 | access to parameter y | Guards.cs:41:30:41:30 | access to parameter y | Guards.cs:41:30:41:30 | access to parameter y | null |
80+
| Guards.cs:42:36:42:36 | access to parameter y | Guards.cs:41:30:41:38 | ... != ... | Guards.cs:41:30:41:30 | access to parameter y | false |
7381
| Guards.cs:42:36:42:36 | access to parameter y | Guards.cs:41:30:41:38 | ... != ... | Guards.cs:41:30:41:30 | access to parameter y | true |
7482
| Guards.cs:48:31:48:40 | access to field Field | Guards.cs:47:13:47:17 | access to field Field | Guards.cs:47:13:47:17 | access to field Field | not null |
7583
| Guards.cs:48:31:48:40 | access to field Field | Guards.cs:47:13:47:25 | ... != ... | Guards.cs:47:13:47:17 | access to field Field | true |

‎csharp/ql/test/library-tests/controlflow/guards/GuardedExpr.expected‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,12 +64,20 @@
6464
| Guards.cs:36:36:36:36 | access to parameter y | Guards.cs:35:26:35:26 | access to parameter y | Guards.cs:35:26:35:26 | access to parameter y | not null |
6565
| Guards.cs:36:36:36:36 | access to parameter y | Guards.cs:35:26:35:34 | ... == ... | Guards.cs:35:26:35:26 | access to parameter y | false |
6666
| Guards.cs:39:31:39:31 | access to parameter x | Guards.cs:38:15:38:15 | access to parameter x | Guards.cs:38:15:38:15 | access to parameter x | not null |
67+
| Guards.cs:39:31:39:31 | access to parameter x | Guards.cs:38:15:38:15 | access to parameter x | Guards.cs:38:15:38:15 | access to parameter x | null |
6768
| Guards.cs:39:31:39:31 | access to parameter x | Guards.cs:38:15:38:23 | ... == ... | Guards.cs:38:15:38:15 | access to parameter x | false |
69+
| Guards.cs:39:31:39:31 | access to parameter x | Guards.cs:38:15:38:23 | ... == ... | Guards.cs:38:15:38:15 | access to parameter x | true |
6870
| Guards.cs:39:35:39:35 | access to parameter y | Guards.cs:38:28:38:28 | access to parameter y | Guards.cs:38:28:38:28 | access to parameter y | not null |
71+
| Guards.cs:39:35:39:35 | access to parameter y | Guards.cs:38:28:38:28 | access to parameter y | Guards.cs:38:28:38:28 | access to parameter y | null |
6972
| Guards.cs:39:35:39:35 | access to parameter y | Guards.cs:38:28:38:36 | ... == ... | Guards.cs:38:28:38:28 | access to parameter y | false |
73+
| Guards.cs:39:35:39:35 | access to parameter y | Guards.cs:38:28:38:36 | ... == ... | Guards.cs:38:28:38:28 | access to parameter y | true |
7074
| Guards.cs:42:32:42:32 | access to parameter x | Guards.cs:41:17:41:17 | access to parameter x | Guards.cs:41:17:41:17 | access to parameter x | not null |
75+
| Guards.cs:42:32:42:32 | access to parameter x | Guards.cs:41:17:41:17 | access to parameter x | Guards.cs:41:17:41:17 | access to parameter x | null |
76+
| Guards.cs:42:32:42:32 | access to parameter x | Guards.cs:41:17:41:25 | ... != ... | Guards.cs:41:17:41:17 | access to parameter x | false |
7177
| Guards.cs:42:32:42:32 | access to parameter x | Guards.cs:41:17:41:25 | ... != ... | Guards.cs:41:17:41:17 | access to parameter x | true |
7278
| Guards.cs:42:36:42:36 | access to parameter y | Guards.cs:41:30:41:30 | access to parameter y | Guards.cs:41:30:41:30 | access to parameter y | not null |
79+
| Guards.cs:42:36:42:36 | access to parameter y | Guards.cs:41:30:41:30 | access to parameter y | Guards.cs:41:30:41:30 | access to parameter y | null |
80+
| Guards.cs:42:36:42:36 | access to parameter y | Guards.cs:41:30:41:38 | ... != ... | Guards.cs:41:30:41:30 | access to parameter y | false |
7381
| Guards.cs:42:36:42:36 | access to parameter y | Guards.cs:41:30:41:38 | ... != ... | Guards.cs:41:30:41:30 | access to parameter y | true |
7482
| Guards.cs:48:31:48:40 | access to field Field | Guards.cs:47:13:47:17 | access to field Field | Guards.cs:47:13:47:17 | access to field Field | not null |
7583
| Guards.cs:48:31:48:40 | access to field Field | Guards.cs:47:13:47:25 | ... != ... | Guards.cs:47:13:47:17 | access to field Field | true |

‎csharp/ql/test/library-tests/controlflow/guards/Guards.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,10 @@ void M3(string? x, string? y)
3535
if (x == null || y == null) { }
3636
else Console.WriteLine(x + y); // null guarded
3737

38-
if (!(x == null || y == null))
38+
if (!(x == null || y == null)) // MISHANDLED, likely due to splitting
3939
Console.WriteLine(x + y); // null guarded
4040

41-
if (!!!(x != null && y != null)) { }
41+
if (!!!(x != null && y != null)) { } // MISHANDLED, likely due to splitting
4242
else Console.WriteLine(x + y); // null guarded
4343

4444
if (Field != null)

‎java/ql/test/query-tests/Nullness/C.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,4 +254,15 @@ public void ex18(boolean b, int[] xs, Object related) {
254254
xs[0] = 42; // OK
255255
}
256256
}
257+
258+
public void ex19(Object t, Object x) {
259+
boolean b = t != null || x != null;
260+
if (b) {
261+
if (t != null) {
262+
t.hashCode(); // OK
263+
} else {
264+
x.hashCode(); // OK
265+
}
266+
}
267+
}
257268
}

‎shared/controlflow/codeql/controlflow/Guards.qll‎

Lines changed: 103 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -926,6 +926,9 @@ module Make<
926926
guardControls(g0, v0, tgtGuard, tgtVal) and
927927
additionalImpliesStep(g0, v0, guard, v)
928928
)
929+
or
930+
baseGuardValue(tgtGuard, tgtVal) and
931+
disjunctiveGuardControls(guard, v, tgtGuard, tgtVal)
929932
}
930933

931934
/**
@@ -1003,6 +1006,104 @@ module Make<
10031006
)
10041007
}
10051008

1009+
private import DisjunctiveGuard
1010+
1011+
private module DisjunctiveGuard {
1012+
/**
1013+
* Holds if `disjunction` evaluating to `val` means that either
1014+
* `disjunct1` or `disjunct2` is `val`.
1015+
*/
1016+
private predicate disjunction(
1017+
Guard disjunction, GuardValue val, Guard disjunct1, Guard disjunct2
1018+
) {
1019+
2 =
1020+
strictcount(Guard op |
1021+
disjunction.(OrExpr).getAnOperand() = op or disjunction.(AndExpr).getAnOperand() = op
1022+
) and
1023+
disjunct1 != disjunct2 and
1024+
(
1025+
exists(OrExpr d | d = disjunction |
1026+
d.getAnOperand() = disjunct1 and
1027+
d.getAnOperand() = disjunct2 and
1028+
val.asBooleanValue() = true
1029+
)
1030+
or
1031+
exists(AndExpr d | d = disjunction |
1032+
d.getAnOperand() = disjunct1 and
1033+
d.getAnOperand() = disjunct2 and
1034+
val.asBooleanValue() = false
1035+
)
1036+
)
1037+
}
1038+
1039+
private predicate disjunct(Guard guard, GuardValue val) { disjunction(_, val, guard, _) }
1040+
1041+
module DisjunctImplies = ImpliesTC<disjunct/2>;
1042+
1043+
/**
1044+
* Holds if one of the disjuncts in `disjunction` evaluating to `dv` implies that `def`
1045+
* evaluates to `v`. The other disjunct is `otherDisjunct`.
1046+
*/
1047+
pragma[nomagic]
1048+
private predicate ssaControlsDisjunct(
1049+
SsaDefinition def, GuardValue v, Guard disjunction, Guard otherDisjunct, GuardValue dv
1050+
) {
1051+
exists(Guard disjunct |
1052+
disjunction(disjunction, dv, disjunct, otherDisjunct) and
1053+
DisjunctImplies::ssaControls(def, v, disjunct, dv)
1054+
)
1055+
}
1056+
1057+
/**
1058+
* Holds if the disjunction of `def` evaluating to `v` and
1059+
* `otherDisjunct` evaluating to `dv` controls `bb`.
1060+
*/
1061+
pragma[nomagic]
1062+
private predicate ssaDisjunctionControls(
1063+
SsaDefinition def, GuardValue v, Guard otherDisjunct, GuardValue dv, BasicBlock bb
1064+
) {
1065+
exists(Guard disjunction |
1066+
ssaControlsDisjunct(def, v, disjunction, otherDisjunct, dv) and
1067+
disjunction.valueControls(bb, dv)
1068+
)
1069+
}
1070+
1071+
/**
1072+
* Holds if `tgtGuard` evaluating to `tgtVal` implies that `def`
1073+
* evaluates to `v`. The basic block of `tgtGuard` is `bb`.
1074+
*/
1075+
pragma[nomagic]
1076+
private predicate ssaControlsGuard(
1077+
SsaDefinition def, GuardValue v, Guard tgtGuard, GuardValue tgtVal, BasicBlock bb
1078+
) {
1079+
(
1080+
BranchImplies::ssaControls(def, v, tgtGuard, tgtVal) or
1081+
WrapperGuard::ReturnImplies::ssaControls(def, v, tgtGuard, tgtVal)
1082+
) and
1083+
tgtGuard.getBasicBlock() = bb
1084+
}
1085+
1086+
/**
1087+
* Holds if `tgtGuard` evaluating to `tgtVal` implies that `guard`
1088+
* evaluates to `v`.
1089+
*/
1090+
pragma[nomagic]
1091+
predicate disjunctiveGuardControls(
1092+
Guard guard, GuardValue v, Guard tgtGuard, GuardValue tgtVal
1093+
) {
1094+
exists(SsaDefinition def, GuardValue v1, GuardValue v2, BasicBlock bb |
1095+
// If `def==v1 || guard==v` controls `bb`,
1096+
ssaDisjunctionControls(def, v1, guard, v, bb) and
1097+
// and `tgtGuard==tgtVal` in `bb` implies `def==v2`,
1098+
ssaControlsGuard(def, v2, tgtGuard, tgtVal, bb) and
1099+
// and `v1` and `v2` are disjoint,
1100+
disjointValues(v1, v2)
1101+
// then assuming `tgtGuard==tgtVal` it follows that `def` cannot be `v1`
1102+
// and therefore we must have `guard==v`.
1103+
)
1104+
}
1105+
}
1106+
10061107
/**
10071108
* Provides an implementation of guard implication logic for guard
10081109
* wrappers.
@@ -1042,7 +1143,8 @@ module Make<
10421143

10431144
private predicate relevantCallValue(NonOverridableMethodCall call, GuardValue val) {
10441145
BranchImplies::guardControls(call, val, _, _) or
1045-
ReturnImplies::guardControls(call, val, _, _)
1146+
ReturnImplies::guardControls(call, val, _, _) or
1147+
DisjunctImplies::guardControls(call, val, _, _)
10461148
}
10471149

10481150
/**

0 commit comments

Comments
 (0)