Skip to content

C#: Make a more principled fix in ConstantCondition. #21272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
aschackmull wants to merge 1 commit into
base: main
Choose a base branch
from
+13 −8
Draft

C#: Make a more principled fix in ConstantCondition. #21272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,6 @@ predicate constantGuard(
Guards::Guards::Guard g, string msg, Guards::Guards::Guard reason, string reasonMsg
) {
ConstCondImpl::problems(g, msg, reason, reasonMsg) and
// conditional qualified expressions sit at an akward place in the CFG, which
// leads to FPs
not g.(QualifiableExpr).getQualifier() = reason and
// and if they're extension method calls, the syntactic qualifier is actually argument 0
not g.(ExtensionMethodCall).getArgument(0) = reason and
// if a logical connective is constant, one of its operands is constant, so
// we report that instead
not g instanceof LogicalNotExpr and
Expand Down
16 changes: 13 additions & 3 deletions shared/controlflow/codeql/controlflow/queries/ConstantCondition.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,15 @@ module Make<LocationSig Location, BB::CfgSig<Location> Cfg, InputSig<Cfg::BasicB
private import Cfg
private import Input

/**
* Holds if the edge from `bb1` to `bb2` implies both that `def` evaluates
* to `v` and that `g` evaluates to `gv`. The latter implication is also a
* biimplication in most cases, except when `g` is a short-circuiting
* side-effecting construct.
*
* Checking that `bb1` to `bb2` is a dominating edge is sufficient to ensure
* that the biimplication holds.
*/
private predicate ssaControlsGuardEdge(
SsaDefinition def, GuardValue v, Guard g, BasicBlock bb1, BasicBlock bb2, GuardValue gv
) {
Expand All @@ -81,9 +90,10 @@ module Make<LocationSig Location, BB::CfgSig<Location> Cfg, InputSig<Cfg::BasicB
private predicate impossibleValue(
Guard g, GuardValue gv, SsaDefinition def, BasicBlock bb, GuardValue v2
) {
exists(GuardValue dual, GuardValue v1 |
// If `g` in `bb` evaluates to `gv` then `def` has value `v1`,
ssaControlsGuardEdge(def, v1, g, bb, _, gv) and
exists(GuardValue dual, GuardValue v1, BasicBlock bb2 |
// If `g` evaluates to `gv` and its branch edge `(bb, bb2)` is a dominating edge then `def` has value `v1`,
ssaControlsGuardEdge(def, v1, g, bb, bb2, gv) and
dominatingEdge(bb, bb2) and
dual = gv.getDualValue() and
not gv.isThrowsException() and
not dual.isThrowsException() and
Expand Down
Loading