Skip to content

Commit 2bde364

Browse files
authored
Merge pull request #21599 from aschackmull/csharp/constantcondition-simplify
C#: Simplify the ConstantCondition query.
2 parents 71b38b7 + 29500c7 commit 2bde364

25 files changed

+109
-265
lines changed

csharp/ql/integration-tests/posix/query-suite/csharp-code-quality-extended.qls.expected

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,6 @@ ql/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql
6565
ql/csharp/ql/src/Likely Bugs/Collections/ContainerSizeCmpZero.ql
6666
ql/csharp/ql/src/Likely Bugs/Collections/ReadOnlyContainer.ql
6767
ql/csharp/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql
68-
ql/csharp/ql/src/Likely Bugs/ConstantComparison.ql
6968
ql/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql
7069
ql/csharp/ql/src/Likely Bugs/Dynamic/BadDynamicCall.ql
7170
ql/csharp/ql/src/Likely Bugs/EqualityCheckOnFloats.ql

csharp/ql/integration-tests/posix/query-suite/csharp-code-quality.qls.expected

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ ql/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql
3838
ql/csharp/ql/src/Likely Bugs/Collections/ContainerSizeCmpZero.ql
3939
ql/csharp/ql/src/Likely Bugs/Collections/ReadOnlyContainer.ql
4040
ql/csharp/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql
41-
ql/csharp/ql/src/Likely Bugs/ConstantComparison.ql
4241
ql/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql
4342
ql/csharp/ql/src/Likely Bugs/EqualityCheckOnFloats.ql
4443
ql/csharp/ql/src/Likely Bugs/EqualsArray.ql

csharp/ql/integration-tests/posix/query-suite/csharp-security-and-quality.qls.expected

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,6 @@ ql/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql
6969
ql/csharp/ql/src/Likely Bugs/Collections/ContainerSizeCmpZero.ql
7070
ql/csharp/ql/src/Likely Bugs/Collections/ReadOnlyContainer.ql
7171
ql/csharp/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql
72-
ql/csharp/ql/src/Likely Bugs/ConstantComparison.ql
7372
ql/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql
7473
ql/csharp/ql/src/Likely Bugs/Dynamic/BadDynamicCall.ql
7574
ql/csharp/ql/src/Likely Bugs/EqualityCheckOnFloats.ql

csharp/ql/lib/semmle/code/csharp/commons/Constants.qll

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,19 +4,6 @@ import csharp
44
private import semmle.code.csharp.commons.ComparisonTest
55
private import semmle.code.csharp.commons.StructuralComparison as StructuralComparison
66

7-
pragma[noinline]
8-
private predicate isConstantCondition0(ControlFlow::Node cfn, boolean b) {
9-
exists(cfn.getASuccessorByType(any(ControlFlow::BooleanSuccessor t | t.getValue() = b))) and
10-
strictcount(ControlFlow::SuccessorType t | exists(cfn.getASuccessorByType(t))) = 1
11-
}
12-
13-
/**
14-
* Holds if `e` is a condition that always evaluates to Boolean value `b`.
15-
*/
16-
predicate isConstantCondition(Expr e, boolean b) {
17-
forex(ControlFlow::Node cfn | cfn = e.getAControlFlowNode() | isConstantCondition0(cfn, b))
18-
}
19-
207
/**
218
* Holds if comparison operation `co` is constant with the Boolean value `b`.
229
* For example, the comparison `x > x` is constantly `false` in

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

Lines changed: 24 additions & 89 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,38 @@ 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-
}
135116
}
136117

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-
}
118+
private Expr getQualifier(QualifiableExpr e) {
119+
// `e.getQualifier()` does not work for calls to extension methods
120+
result = e.getChildExpr(-1)
162121
}
163122

164123
/** A constant nullness condition. */
165124
class ConstantNullnessCondition extends ConstantCondition {
166125
boolean b;
167126

168127
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
128+
nullCheck(this, true) and
129+
exists(Expr stripped | stripped = this.(Expr).stripCasts() |
130+
stripped.getType() =
131+
any(ValueType t |
132+
not t instanceof NullableType and
133+
// Extractor bug: the type of `x?.Length` is reported as `int`, but it should
134+
// be `int?`
135+
not getQualifier*(stripped).(QualifiableExpr).isConditional()
136+
) and
137+
b = false
138+
or
139+
stripped instanceof NullLiteral and
140+
b = true
141+
or
142+
stripped.hasValue() and
143+
not stripped instanceof NullLiteral and
144+
b = false
177145
)
178146
}
179147

@@ -184,39 +152,6 @@ class ConstantNullnessCondition extends ConstantCondition {
184152
}
185153
}
186154

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-
220155
from ConstantCondition c, string msg, Guards::Guards::Guard reason, string reasonMsg
221156
where
222157
msg = c.getMessage() and

csharp/ql/src/CSI/CompareIdenticalValues.ql

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ where
4747
not comparesIdenticalValuesNan(ct, _) and msg = "Comparison of identical values."
4848
) and
4949
not isMutatingOperation(ct.getAnArgument().getAChild*()) and
50-
not isConstantCondition(e, _) and // Avoid overlap with cs/constant-condition
51-
not isConstantComparison(e, _) and // Avoid overlap with cs/constant-comparison
50+
not isConstantComparison(e, _) and // Avoid overlap with cs/constant-condition
5251
not isExprInAssertion(e)
5352
select ct, msg

csharp/ql/src/Likely Bugs/ConstantComparison.cs

Lines changed: 0 additions & 2 deletions
This file was deleted.

csharp/ql/src/Likely Bugs/ConstantComparison.qhelp

Lines changed: 0 additions & 46 deletions
This file was deleted.

csharp/ql/src/Likely Bugs/ConstantComparison.ql

Lines changed: 0 additions & 22 deletions
This file was deleted.
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: majorAnalysis
3+
---
4+
* The `cs/constant-condition` query has been simplified. The query no longer reports trivially constant conditions as they were found to generally be intentional. As a result, it should now produce fewer false positives. Additionally, the simplification means that it now reports all the results that `cs/constant-comparison` used to report, and as consequence, that query has been deleted.

0 commit comments

Comments
 (0)