From 9681818114f8b98a6ff1d8fac4f64a923c81637c Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 4 Oct 2022 10:46:48 +0200 Subject: [PATCH] recognize when this or result is only used in one side of a disjunct --- .../performance/VarUnusedInDisjunct.ql | 119 ++++++++++++++---- .../performance/VarUnusedInDisjunct/Test.qll | 16 +++ .../VarUnusedInDisjunct.expected | 2 + 3 files changed, 115 insertions(+), 22 deletions(-) diff --git a/ql/ql/src/queries/performance/VarUnusedInDisjunct.ql b/ql/ql/src/queries/performance/VarUnusedInDisjunct.ql index c26b47554fe2..9e3f600b6add 100644 --- a/ql/ql/src/queries/performance/VarUnusedInDisjunct.ql +++ b/ql/ql/src/queries/performance/VarUnusedInDisjunct.ql @@ -11,29 +11,105 @@ import ql +newtype TVar = + TVarDef(VarDef var) or + TThis(Predicate pred) { exists(ThisAccess t | t.getEnclosingPredicate() = pred) } or + TResult(Predicate pred) { exists(ResultAccess t | t.getEnclosingPredicate() = pred) } + +class Var extends TVar { + string getName() { + exists(VarDef def | this = TVarDef(def) | result = def.getName()) + or + result = "this" and this instanceof TThis + or + result = "result" and this instanceof TResult + } + + string toString() { result = this.getName() } + + Expr getAnAccess() { + exists(VarDef var | this = TVarDef(var) | + // base case + result.(VarAccess).getDeclaration() = var + or + exists(Class clz | + result.(FieldAccess).getDeclaration().getVarDecl() = var and + result.(FieldAccess).getDeclaration() = clz.getAField() and // <- ensuring that the field is not inherited from a super class + result.getEnclosingPredicate() = clz.getCharPred() // <- in non-charpred, the fields are implicitly bound by their relation to `this`. + ) + ) + or + exists(Predicate pred | + this = TThis(pred) and + ( + result instanceof ThisAccess + or + result instanceof Super // super binds `this` + ) + or + this = TResult(pred) and result instanceof ResultAccess + | + result.getEnclosingPredicate() = pred + ) + or + exists(Predicate pred | this = TThis(pred) | + // a field access implicitly binds `this` to the enclosing predicate + result.(FieldAccess).getEnclosingPredicate() = pred + or + // a call to a class method implicitly binds `this` to the enclosing predicate + pred.getParent() instanceof Class and // class-predicate or charpred + // a call of the form `foo()`, not a member-call. + exists(PredicateCall call | + result = call and + call.getEnclosingPredicate() = pred and + call.getTarget() instanceof ClassPredicate + ) + ) + } + + Location getLocation() { + exists(VarDef def | this = TVarDef(def) | result = def.getLocation()) + or + exists(Predicate pred | this = TThis(pred) | result = pred.getLocation()) + or + exists(Predicate pred | this = TResult(pred) | result = pred.getLocation()) + } + + pragma[noinline] + Type getType() { + result = this.getAnAccess().getType() // the easy implementation. + } + + VarDef asVarDef() { this = TVarDef(result) } + + string describe() { + exists(VarDef var | this = TVarDef(var) | + if var.getParent() instanceof FieldDecl + then result = "field " + this.getName() + else result = "variable " + this.getName() + ) + or + result = "this variable" and this instanceof TThis + or + result = "result variable" and this instanceof TResult + } +} + /** * Holds if `node` bind `var` in a (transitive) child node. * Is a practical approximation that ignores `not` and many other features. */ pragma[noinline] -predicate alwaysBindsVar(VarDef var, AstNode node) { - ( - // base case - node.(VarAccess).getDeclaration() = var - or - exists(Class clz | - node.(FieldAccess).getDeclaration().getVarDecl() = var and - node.(FieldAccess).getDeclaration() = clz.getAField() and // <- ensuring that the field is not inherited from a super class - node.getEnclosingPredicate() = clz.getCharPred() // <- in non-charpred, the fields are implicitly bound by their relation to `this`. - ) - ) and +predicate alwaysBindsVar(Var var, AstNode node) { + node = var.getAnAccess() and not isSmallType(var.getType()) // <- early pruning or // recursive cases alwaysBindsVar(var, node.getAChild(_)) and // the recursive step, go one step up to the parent. - not node.(FullAggregate).getAnArgument() = var and // except if the parent defines the variable, then we stop. - not node.(Quantifier).getAnArgument() = var and - not node instanceof EffectiveDisjunction // for disjunctions, we need to check both sides. + not node.(FullAggregate).getAnArgument() = var.asVarDef() and // except if the parent defines the variable, then we stop. + not node.(Quantifier).getAnArgument() = var.asVarDef() and + not node instanceof EffectiveDisjunction and // for disjunctions, we need to check both sides. + not node instanceof Predicate // stop. or exists(EffectiveDisjunction disj | disj = node | alwaysBindsVar(var, disj.getLeft()) and @@ -119,7 +195,7 @@ class EffectiveDisjunction extends AstNode { * Holds if `disj` only uses `var` in one of its branches. */ pragma[noinline] -predicate onlyUseInOneBranch(EffectiveDisjunction disj, VarDef var, AstNode notBoundIn) { +predicate onlyUseInOneBranch(EffectiveDisjunction disj, Var var, AstNode notBoundIn) { alwaysBindsVar(var, disj.getLeft()) and not alwaysBindsVar(var, disj.getRight()) and notBoundIn = disj.getRight() @@ -169,7 +245,7 @@ class EffectiveConjunction extends AstNode { /** * Holds if `node` is a sub-node of a node that always binds `var`. */ -predicate varIsAlwaysBound(VarDef var, AstNode node) { +predicate varIsAlwaysBound(Var var, AstNode node) { // base case alwaysBindsVar(var, node) and onlyUseInOneBranch(_, var, _) // <- manual magic @@ -196,7 +272,7 @@ predicate varIsAlwaysBound(VarDef var, AstNode node) { * Holds if `disj` only uses `var` in one of its branches. * And we should report it as being a bad thing. */ -predicate badDisjunction(EffectiveDisjunction disj, VarDef var, AstNode notBoundIn) { +predicate badDisjunction(EffectiveDisjunction disj, Var var, AstNode notBoundIn) { onlyUseInOneBranch(disj, var, notBoundIn) and // it's fine if it's always bound further up not varIsAlwaysBound(var, disj) and @@ -205,7 +281,7 @@ predicate badDisjunction(EffectiveDisjunction disj, VarDef var, AstNode notBound // inlined predicates might bind unused variables in the context they are used in. not ( isInlined(disj.getEnclosingPredicate()) and - var = disj.getEnclosingPredicate().getParameter(_) + var.asVarDef() = disj.getEnclosingPredicate().getParameter(_) ) and // recursion prevention never runs, it's a compile-time check, so we remove those results here not disj.getEnclosingPredicate().getParent().(Class).getName().matches("%RecursionPrevention") and // these are by design @@ -215,9 +291,8 @@ predicate badDisjunction(EffectiveDisjunction disj, VarDef var, AstNode notBound not isTinyAssignment(disj.getAnOperand()) } -from EffectiveDisjunction disj, VarDef var, AstNode notBoundIn, string type +from EffectiveDisjunction disj, Var var, AstNode notBoundIn where badDisjunction(disj, var, notBoundIn) and - not badDisjunction(disj.getParent(), var, _) and // avoid duplicate reporting of the same error - if var.getParent() instanceof FieldDecl then type = "field" else type = "variable" -select disj, "The $@ is only used in one side of disjunct.", var, type + " " + var.getName() + not badDisjunction(disj.getParent(), var, _) // avoid duplicate reporting of the same error +select disj, "The $@ is only used in one side of disjunct.", var, var.describe() diff --git a/ql/ql/test/queries/performance/VarUnusedInDisjunct/Test.qll b/ql/ql/test/queries/performance/VarUnusedInDisjunct/Test.qll index 520acbaa1b3a..e7a087b7f5ed 100644 --- a/ql/ql/test/queries/performance/VarUnusedInDisjunct/Test.qll +++ b/ql/ql/test/queries/performance/VarUnusedInDisjunct/Test.qll @@ -165,4 +165,20 @@ class HasField extends Big { or this.toString().matches("%foo") // <- field only defined here. } + + predicate isInScope(Big big) { + big.toString().matches("%foo") // <- `this` is not bound here + or + this.toString().matches("%foo") and this.toString() = big.toString() + } + + string getThing() { result = "foo" and result = this.toString() } + + string getPart(int i) { + i = 0 and result = "?" + or + i = 1 and result = " extends " and exists(this.getThing()) + or + i = 1 and result = " super " and exists(this.getThing()) + } } diff --git a/ql/ql/test/queries/performance/VarUnusedInDisjunct/VarUnusedInDisjunct.expected b/ql/ql/test/queries/performance/VarUnusedInDisjunct/VarUnusedInDisjunct.expected index 6304bcb7a524..65fea33dca9c 100644 --- a/ql/ql/test/queries/performance/VarUnusedInDisjunct/VarUnusedInDisjunct.expected +++ b/ql/ql/test/queries/performance/VarUnusedInDisjunct/VarUnusedInDisjunct.expected @@ -7,3 +7,5 @@ | Test.qll:127:5:129:9 | Disjunction | The $@ is only used in one side of disjunct. | Test.qll:125:16:125:20 | a | variable a | | Test.qll:132:5:134:9 | Disjunction | The $@ is only used in one side of disjunct. | Test.qll:125:16:125:20 | a | variable a | | Test.qll:164:5:166:35 | Disjunction | The $@ is only used in one side of disjunct. | Test.qll:161:3:161:11 | field | field field | +| Test.qll:170:5:172:72 | Disjunction | The $@ is only used in one side of disjunct. | Test.qll:169:13:169:21 | this | this variable | +| Test.qll:178:5:182:60 | Disjunction | The $@ is only used in one side of disjunct. | Test.qll:177:10:177:16 | this | this variable |