Skip to content

Commit 2d39a2b

Browse files
aschackmullaschackmull
committed
C#: Simplify the ConstantCondition query.
1 parent 898d12b commit 2d39a2b

File tree

9 files changed

+32
-119
lines changed

9 files changed

+32
-119
lines changed

‎csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql‎

Lines changed: 9 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,9 @@ module ConstCondInput implements ConstCond::InputSig<ControlFlow::BasicBlock> {
4141
module ConstCondImpl = ConstCond::Make<Location, Cfg, ConstCondInput>;
4242

4343
predicate nullCheck(Expr e, boolean direct) {
44-
exists(QualifiableExpr qe | qe.isConditional() and qe.getQualifier() = e and direct = true)
44+
exists(QualifiableExpr qe | qe.isConditional() and direct = true |
45+
qe.getQualifier() = e or qe.(ExtensionMethodCall).getArgument(0) = e
46+
)
4547
or
4648
exists(NullCoalescingOperation nce | nce.getLeftOperand() = e and direct = true)
4749
or
@@ -108,72 +110,21 @@ class ConstantGuard extends ConstantCondition {
108110
class ConstantBooleanCondition extends ConstantCondition {
109111
boolean b;
110112

111-
ConstantBooleanCondition() { isConstantCondition(this, b) }
113+
ConstantBooleanCondition() { isConstantComparison(this, b) }
112114

113115
override string getMessage() { result = "Condition always evaluates to '" + b + "'." }
114-
115-
override predicate isWhiteListed() {
116-
// E.g. `x ?? false`
117-
this.(BoolLiteral) = any(NullCoalescingOperation nce).getRightOperand() or
118-
// No need to flag logical operations when the operands are constant
119-
isConstantCondition(this.(LogicalNotExpr).getOperand(), _) or
120-
this =
121-
any(LogicalAndExpr lae |
122-
isConstantCondition(lae.getAnOperand(), false)
123-
or
124-
isConstantCondition(lae.getLeftOperand(), true) and
125-
isConstantCondition(lae.getRightOperand(), true)
126-
) or
127-
this =
128-
any(LogicalOrExpr loe |
129-
isConstantCondition(loe.getAnOperand(), true)
130-
or
131-
isConstantCondition(loe.getLeftOperand(), false) and
132-
isConstantCondition(loe.getRightOperand(), false)
133-
)
134-
}
135-
}
136-
137-
/** A constant condition in an `if` statement or a conditional expression. */
138-
class ConstantIfCondition extends ConstantBooleanCondition {
139-
ConstantIfCondition() {
140-
this = any(IfStmt is).getCondition().getAChildExpr*() or
141-
this = any(ConditionalExpr ce).getCondition().getAChildExpr*()
142-
}
143-
144-
override predicate isWhiteListed() {
145-
ConstantBooleanCondition.super.isWhiteListed()
146-
or
147-
// It is a common pattern to use a local constant/constant field to control
148-
// whether code parts must be executed or not
149-
this instanceof AssignableRead and
150-
not this instanceof ParameterRead
151-
}
152-
}
153-
154-
/** A constant loop condition. */
155-
class ConstantLoopCondition extends ConstantBooleanCondition {
156-
ConstantLoopCondition() { this = any(LoopStmt ls).getCondition() }
157-
158-
override predicate isWhiteListed() {
159-
// Clearly intentional infinite loops are allowed
160-
this.(BoolLiteral).getBoolValue() = true
161-
}
162116
}
163117

164118
/** A constant nullness condition. */
165119
class ConstantNullnessCondition extends ConstantCondition {
166120
boolean b;
167121

168122
ConstantNullnessCondition() {
169-
forex(ControlFlow::Node cfn | cfn = this.getAControlFlowNode() |
170-
exists(ControlFlow::NullnessSuccessor t, ControlFlow::Node s |
171-
s = cfn.getASuccessorByType(t)
172-
|
173-
b = t.getValue() and
174-
not s.isJoin()
175-
) and
176-
strictcount(ControlFlow::SuccessorType t | exists(cfn.getASuccessorByType(t))) = 1
123+
nullCheck(this, true) and
124+
(
125+
Guards::Internal::nullValueImplied(this) and b = true
126+
or
127+
Guards::Internal::nonNullValueImplied(this) and b = false
177128
)
178129
}
179130

@@ -184,39 +135,6 @@ class ConstantNullnessCondition extends ConstantCondition {
184135
}
185136
}
186137

187-
/** A constant matching condition. */
188-
class ConstantMatchingCondition extends ConstantCondition {
189-
boolean b;
190-
191-
ConstantMatchingCondition() {
192-
this instanceof Expr and
193-
forex(ControlFlow::Node cfn | cfn = this.getAControlFlowNode() |
194-
exists(ControlFlow::MatchingSuccessor t | exists(cfn.getASuccessorByType(t)) |
195-
b = t.getValue()
196-
) and
197-
strictcount(ControlFlow::SuccessorType t | exists(cfn.getASuccessorByType(t))) = 1
198-
)
199-
}
200-
201-
override predicate isWhiteListed() {
202-
exists(Switch se, Case c, int i |
203-
c = se.getCase(i) and
204-
c.getPattern() = this.(DiscardExpr)
205-
|
206-
i > 0
207-
or
208-
i = 0 and
209-
exists(Expr cond | c.getCondition() = cond and not isConstantCondition(cond, true))
210-
)
211-
or
212-
this = any(PositionalPatternExpr ppe).getPattern(_)
213-
}
214-
215-
override string getMessage() {
216-
if b = true then result = "Pattern always matches." else result = "Pattern never matches."
217-
}
218-
}
219-
220138
from ConstantCondition c, string msg, Guards::Guards::Guard reason, string reasonMsg
221139
where
222140
msg = c.getMessage() and

‎csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,9 @@ void M1()
5959
{
6060
switch (1 + 2)
6161
{
62-
case 2: // $ Alert
62+
case 2: // Missing Alert
6363
break;
64-
case 3: // $ Alert
64+
case 3: // Missing Alert
6565
break;
6666
case int _: // GOOD
6767
break;
@@ -72,7 +72,7 @@ void M2(string s)
7272
{
7373
switch ((object)s)
7474
{
75-
case int _: // $ Alert
75+
case int _: // Missing Alert
7676
break;
7777
case "": // GOOD
7878
break;
@@ -92,7 +92,7 @@ string M4(object o)
9292
{
9393
return o switch
9494
{
95-
_ => o.ToString() // $ Alert
95+
_ => o.ToString() // GOOD, catch-all pattern is fine
9696
};
9797
}
9898

@@ -138,7 +138,7 @@ string M9(int i)
138138
{
139139
switch (i)
140140
{
141-
case var _: // $ Alert
141+
case var _: // GOOD, catch-all pattern is fine
142142
return "even";
143143
}
144144
}

‎csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,28 +4,18 @@
44
| ConstantCondition.cs:47:17:47:18 | "" | Expression is never 'null'. | ConstantCondition.cs:47:17:47:18 | "" | dummy |
55
| ConstantCondition.cs:48:13:48:19 | (...) ... | Expression is never 'null'. | ConstantCondition.cs:48:13:48:19 | (...) ... | dummy |
66
| ConstantCondition.cs:49:13:49:14 | "" | Expression is never 'null'. | ConstantCondition.cs:49:13:49:14 | "" | dummy |
7-
| ConstantCondition.cs:62:18:62:18 | 2 | Pattern never matches. | ConstantCondition.cs:62:18:62:18 | 2 | dummy |
8-
| ConstantCondition.cs:64:18:64:18 | 3 | Pattern always matches. | ConstantCondition.cs:64:18:64:18 | 3 | dummy |
9-
| ConstantCondition.cs:75:18:75:20 | access to type Int32 | Pattern never matches. | ConstantCondition.cs:75:18:75:20 | access to type Int32 | dummy |
10-
| ConstantCondition.cs:95:13:95:13 | _ | Pattern always matches. | ConstantCondition.cs:95:13:95:13 | _ | dummy |
117
| ConstantCondition.cs:114:13:114:14 | access to parameter b1 | Condition is always true because of $@. | ConstantCondition.cs:110:14:110:15 | access to parameter b1 | access to parameter b1 |
128
| ConstantCondition.cs:114:19:114:20 | access to parameter b2 | Condition is always true because of $@. | ConstantCondition.cs:112:14:112:15 | access to parameter b2 | access to parameter b2 |
13-
| ConstantCondition.cs:141:22:141:22 | _ | Pattern always matches. | ConstantCondition.cs:141:22:141:22 | _ | dummy |
149
| ConstantConditionBad.cs:5:16:5:20 | ... > ... | Condition always evaluates to 'false'. | ConstantConditionBad.cs:5:16:5:20 | ... > ... | dummy |
1510
| ConstantConditionalExpressionCondition.cs:11:22:11:34 | ... == ... | Condition always evaluates to 'true'. | ConstantConditionalExpressionCondition.cs:11:22:11:34 | ... == ... | dummy |
16-
| ConstantConditionalExpressionCondition.cs:12:21:12:25 | false | Condition always evaluates to 'false'. | ConstantConditionalExpressionCondition.cs:12:21:12:25 | false | dummy |
1711
| ConstantConditionalExpressionCondition.cs:13:21:13:30 | ... == ... | Condition always evaluates to 'true'. | ConstantConditionalExpressionCondition.cs:13:21:13:30 | ... == ... | dummy |
18-
| ConstantForCondition.cs:9:29:9:33 | false | Condition always evaluates to 'false'. | ConstantForCondition.cs:9:29:9:33 | false | dummy |
12+
| ConstantDoCondition.cs:15:22:15:34 | ... == ... | Condition always evaluates to 'true'. | ConstantDoCondition.cs:15:22:15:34 | ... == ... | dummy |
13+
| ConstantDoCondition.cs:32:22:32:31 | ... == ... | Condition always evaluates to 'true'. | ConstantDoCondition.cs:32:22:32:31 | ... == ... | dummy |
1914
| ConstantForCondition.cs:11:29:11:34 | ... == ... | Condition always evaluates to 'false'. | ConstantForCondition.cs:11:29:11:34 | ... == ... | dummy |
2015
| ConstantIfCondition.cs:11:17:11:29 | ... == ... | Condition always evaluates to 'true'. | ConstantIfCondition.cs:11:17:11:29 | ... == ... | dummy |
21-
| ConstantIfCondition.cs:14:17:14:21 | false | Condition always evaluates to 'false'. | ConstantIfCondition.cs:14:17:14:21 | false | dummy |
2216
| ConstantIfCondition.cs:17:17:17:26 | ... == ... | Condition always evaluates to 'true'. | ConstantIfCondition.cs:17:17:17:26 | ... == ... | dummy |
23-
| ConstantIsNullOrEmpty.cs:10:21:10:54 | call to method IsNullOrEmpty | Condition always evaluates to 'false'. | ConstantIsNullOrEmpty.cs:10:21:10:54 | call to method IsNullOrEmpty | dummy |
24-
| ConstantIsNullOrEmpty.cs:46:21:46:46 | call to method IsNullOrEmpty | Condition always evaluates to 'true'. | ConstantIsNullOrEmpty.cs:46:21:46:46 | call to method IsNullOrEmpty | dummy |
25-
| ConstantIsNullOrEmpty.cs:50:21:50:44 | call to method IsNullOrEmpty | Condition always evaluates to 'true'. | ConstantIsNullOrEmpty.cs:50:21:50:44 | call to method IsNullOrEmpty | dummy |
26-
| ConstantIsNullOrEmpty.cs:54:21:54:45 | call to method IsNullOrEmpty | Condition always evaluates to 'false'. | ConstantIsNullOrEmpty.cs:54:21:54:45 | call to method IsNullOrEmpty | dummy |
17+
| ConstantIfCondition.cs:35:20:35:25 | ... >= ... | Condition always evaluates to 'true'. | ConstantIfCondition.cs:35:20:35:25 | ... >= ... | dummy |
2718
| ConstantNullCoalescingLeftHandOperand.cs:11:24:11:34 | access to constant NULL_OBJECT | Expression is never 'null'. | ConstantNullCoalescingLeftHandOperand.cs:11:24:11:34 | access to constant NULL_OBJECT | dummy |
2819
| ConstantNullCoalescingLeftHandOperand.cs:12:24:12:27 | null | Expression is always 'null'. | ConstantNullCoalescingLeftHandOperand.cs:12:24:12:27 | null | dummy |
2920
| ConstantWhileCondition.cs:12:20:12:32 | ... == ... | Condition always evaluates to 'true'. | ConstantWhileCondition.cs:12:20:12:32 | ... == ... | dummy |
30-
| ConstantWhileCondition.cs:16:20:16:24 | false | Condition always evaluates to 'false'. | ConstantWhileCondition.cs:16:20:16:24 | false | dummy |
3121
| ConstantWhileCondition.cs:24:20:24:29 | ... == ... | Condition always evaluates to 'true'. | ConstantWhileCondition.cs:24:20:24:29 | ... == ... | dummy |

‎csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantConditionalExpressionCondition.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ class Main
99
public void Foo()
1010
{
1111
int i = (ZERO == 1 - 1) ? 0 : 1; // $ Alert
12-
int j = false ? 0 : 1; // $ Alert
12+
int j = false ? 0 : 1; // GOOD, literal false is likely intentional
1313
int k = " " == " " ? 0 : 1; // $ Alert
1414
int l = (" "[0] == ' ') ? 0 : 1; // Missing Alert
1515
int m = Bar() == 0 ? 0 : 1; // GOOD

‎csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantDoCondition.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ public void Foo()
1212
do
1313
{
1414
break;
15-
} while (ZERO == 1 - 1); // BAD
15+
} while (ZERO == 1 - 1); // $ Alert
1616
do
1717
{
1818
break;
@@ -29,7 +29,7 @@ public void Foo()
2929
do
3030
{
3131
break;
32-
} while (" " == " "); // BAD
32+
} while (" " == " "); // $ Alert
3333
do
3434
{
3535
break;

‎csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantForCondition.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ class Main
66
{
77
public void M()
88
{
9-
for (int i = 0; false; i++) // $ Alert
9+
for (int i = 0; false; i++) // GOOD, literal false is likely intentional
1010
;
1111
for (int i = 0; 0 == 1; i++) // $ Alert
1212
;

‎csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIfCondition.cs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ public void Foo()
1111
if (ZERO == 1 - 1) // $ Alert
1212
{
1313
}
14-
if (false) // $ Alert
14+
if (false) // GOOD
1515
{
1616
}
1717
if (" " == " ") // $ Alert
@@ -30,6 +30,11 @@ public int Bar()
3030
return ZERO;
3131
}
3232

33+
public void UnsignedCheck(byte n)
34+
{
35+
while (n >= 0) { n--; } // $ Alert
36+
}
37+
3338
}
3439

3540
}

‎csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIsNullOrEmpty.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ internal class Program
77
static void Main(string[] args)
88
{
99
{
10-
if (string.IsNullOrEmpty(nameof(args))) // $ Alert
10+
if (string.IsNullOrEmpty(nameof(args))) // Missing Alert (always false)
1111
{
1212
}
1313

@@ -43,15 +43,15 @@ static void Main(string[] args)
4343
{
4444
}
4545

46-
if (string.IsNullOrEmpty(null)) // $ Alert
46+
if (string.IsNullOrEmpty(null)) // Missing Alert
4747
{
4848
}
4949

50-
if (string.IsNullOrEmpty("")) // $ Alert
50+
if (string.IsNullOrEmpty("")) // Missing Alert
5151
{
5252
}
5353

54-
if (string.IsNullOrEmpty(" ")) // $ Alert
54+
if (string.IsNullOrEmpty(" ")) // Missing Alert
5555
{
5656
}
5757
}

‎csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantWhileCondition.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ public void Foo()
1313
{
1414
break;
1515
}
16-
while (false) // $ Alert
16+
while (false) // Silly, but likely intentional
1717
{
1818
break;
1919
}

0 commit comments

Comments
 (0)