C#: Unify logic in guards library #355
Conversation
hvitved
force-pushed
the
csharp/guards-logic
branch
4 times, most recently
from
ac65f55 to
92317f1
Compare
Unify the logic for Boolean/nullness/matching guards.
hvitved
force-pushed
the
csharp/guards-logic
branch
from
92317f1 to
6651736
Compare
| private import semmle.code.csharp.frameworks.System | ||
|
|
||
| /** An abstract value representing one of two possible classes of values. */ |
There was a problem hiding this comment.
This probably needs more explanation - let me think how to word it.
There was a problem hiding this comment.
In the latest version of the commit it is now just "An abstract value".
| } | ||
| } | ||
|
|
||
| /** An empty or non-empty value. */ |
There was a problem hiding this comment.
I the latest version it is
/** A value that represents an empty or non-empty collection. */
class EmptyCollectionValue extends AbstractValue, TEmptyCollectionValue {
| * Gets the conditional qualifier of `e`, if any, where `e` is an access/call | ||
| * with a value type, lifted to a nullable type by the conditional access. | ||
| */ | ||
| Expr getConditionalQualifier(Expr e) { |
There was a problem hiding this comment.
This predicate is confusing because e is the qualifier, rather than result.
There was a problem hiding this comment.
Turns out that this predicate is unused, I will remove it.
| qe.getQualifier() = e and | ||
| qe.isConditional() and | ||
| ( | ||
| result.(FieldAccess).getTarget().getType() instanceof ValueType |
There was a problem hiding this comment.
Why not PropertyAccess as well?
There was a problem hiding this comment.
Because a PropertyAccess is also a PropertyCall which is a Call.
| * if any. For example, `result = x?.y` and `e = x`, or `result = x + 1` | ||
| * and `e = x`. | ||
| */ | ||
| Expr getNullEquivParent(Expr e) { | ||
| exists(QualifiableExpr qe | | ||
| result = qe and | ||
| result = any(QualifiableExpr qe | |
There was a problem hiding this comment.
Use getConditionalQualifier here instead?
|
|
||
| private cached module Cached { | ||
| cached | ||
| newtype TAbstractValue = |
There was a problem hiding this comment.
Is it really necessary to cache these 8 values?
| v1 = TBooleanValue(true) and | ||
| v2 = v1 | ||
| or | ||
| e1.(BitwiseOrExpr).getAnOperand() = e2 and |
There was a problem hiding this comment.
We can't conclude anything about one of the operands from the value of xor, nor the other way around.
There was a problem hiding this comment.
Ah ok. We would need 2 abstract values to deduce the third.
| */ | ||
| cached | ||
| predicate impliesStep(Expr e1, AbstractValue v1, Expr e2, AbstractValue v2) { | ||
| e1.(BitwiseAndExpr).getAnOperand() = e2 and |
There was a problem hiding this comment.
These implications also work in the opposite direction, where e1 is an operand of e2. For example,
e2.(BitwiseAndExpr).getAnOperand() = e1 and
v1 = TBooleanValue(false) and
v2 = v1
etc. for BitwiseOrExpr, LogicalAndExpr and LogicalOrExpr.
| e1 = ct.getExpr() and | ||
| e2 = ct.getAnArgument() | ||
| or | ||
| // e2 === e1 != true, v1 === v2 xor b |
There was a problem hiding this comment.
Yes, but not worth fixing here I think; I have a follow-up PR anyway.
| e1 = ct.getExpr() and | ||
| ct.getAnArgument() = e2 and | ||
| e1 = e2.(LogicalNotExpr).getOperand() and | ||
| v2 = TBooleanValue(v1.(BooleanValue).getValue().booleanNot()) |
There was a problem hiding this comment.
I wonder whether it makes sense to implement BooleanValue BooleanValue.booleanNot() as a utility predicate on BooleanValue ?
Fix data flow through `ExtensionMethodAccess`
Follow-up on #284. This PR joins the predicates
controls(),controlsNullness(), andcontrolsMatching(), as well as the predicatesisGuardedBy(),isGuardedByNullness(), andisGuardedByMatching().@calumgrant