From 33d363eb940ffdd59b738b2afe96c0a69747bd89 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 18 Nov 2021 12:12:47 +0100 Subject: [PATCH] move use-instanceof implementation to Query.qll, and rename the .ql file --- .../style/UseInstanceofExtensionQuery.qll | 70 +++++++++++++++++++ .../queries/style/UseInstanceofExtension.ql | 21 ++++++ .../style/suggestInstanceofExtension.ql | 47 ------------- 3 files changed, 91 insertions(+), 47 deletions(-) create mode 100644 ql/src/codeql_ql/style/UseInstanceofExtensionQuery.qll create mode 100644 ql/src/queries/style/UseInstanceofExtension.ql delete mode 100644 ql/src/queries/style/suggestInstanceofExtension.ql diff --git a/ql/src/codeql_ql/style/UseInstanceofExtensionQuery.qll b/ql/src/codeql_ql/style/UseInstanceofExtensionQuery.qll new file mode 100644 index 00000000000..c4154612b4d --- /dev/null +++ b/ql/src/codeql_ql/style/UseInstanceofExtensionQuery.qll @@ -0,0 +1,70 @@ +import ql + +/** + * Gets a class where the charpred has an `this instanceof type` expression. + */ +predicate instanceofThisInCharPred(Class c, Type type) { + exists(InstanceOf instanceOf | + instanceOf = c.getCharPred().getBody() + or + exists(Conjunction conj | + conj = c.getCharPred().getBody() and + instanceOf = conj.getAnOperand() + ) + | + instanceOf.getExpr() instanceof ThisAccess and + type = instanceOf.getType().getResolvedType() + ) +} + +/** + * Holds if `c` uses the casting based range pattern, which could be replaced with `instanceof type`. + */ +predicate usesCastingBasedInstanceof(Class c, Type type) { + instanceofThisInCharPred(c, type) and + // require that there is a call to the range class that matches the name of the enclosing predicate + exists(InlineCast cast, MemberCall call | + cast = getAThisCast(c, type) and + call.getBase() = cast and + cast.getEnclosingPredicate().getName() = call.getMemberName() + ) +} + +/** Gets an inline cast that cases `this` to `type` inside a class predicate for `c`. */ +InlineCast getAThisCast(Class c, Type type) { + exists(MemberCall call | + call.getEnclosingPredicate() = c.getAClassPredicate() and + result = call.getBase() and + result.getBase() instanceof ThisAccess and + result.getTypeExpr().getResolvedType() = type + ) +} + +predicate usesFieldBasedInstanceof(Class c, TypeExpr type, VarDecl field, ComparisonFormula comp) { + exists(FieldAccess fieldAccess | + c.getCharPred().getBody() = comp or + c.getCharPred().getBody().(Conjunction).getAnOperand() = comp + | + comp.getOperator() = "=" and + comp.getEnclosingPredicate() = c.getCharPred() and + comp.getAnOperand() instanceof ThisAccess and + comp.getAnOperand() = fieldAccess and + fieldAccess.getDeclaration() = field and + field.getTypeExpr() = type + ) and + // require that there is a call to the range field that matches the name of the enclosing predicate + exists(FieldAccess access, MemberCall call | + access = getARangeFieldAccess(c, field, _) and + call.getBase() = access and + access.getEnclosingPredicate().getName() = call.getMemberName() + ) +} + +FieldAccess getARangeFieldAccess(Class c, VarDecl field, string name) { + exists(MemberCall call | + result = call.getBase() and + result.getDeclaration() = field and + name = call.getMemberName() and + call.getEnclosingPredicate().(ClassPredicate).getParent() = c + ) +} diff --git a/ql/src/queries/style/UseInstanceofExtension.ql b/ql/src/queries/style/UseInstanceofExtension.ql new file mode 100644 index 00000000000..2e7b8e3de80 --- /dev/null +++ b/ql/src/queries/style/UseInstanceofExtension.ql @@ -0,0 +1,21 @@ +/** + * @name Suggest using non-extending subtype relationships. + * @description Non-extending subtypes ("instanceof extensions") are generally preferrable to instanceof expressions in characteristic predicates. + * @kind problem + * @problem.severity warning + * @id ql/suggest-instanceof-extension + * @tags maintainability + * @precision medium + */ + +import ql +import codeql_ql.style.UseInstanceofExtensionQuery + +from Class c, Type type, string message +where + ( + usesCastingBasedInstanceof(c, type) or + usesFieldBasedInstanceof(c, any(TypeExpr te | te.getResolvedType() = type), _, _) + ) and + message = "consider defining $@ as non-extending subtype of $@" +select c, message, c, c.getName(), type, type.getName() diff --git a/ql/src/queries/style/suggestInstanceofExtension.ql b/ql/src/queries/style/suggestInstanceofExtension.ql deleted file mode 100644 index 465a0b2083b..00000000000 --- a/ql/src/queries/style/suggestInstanceofExtension.ql +++ /dev/null @@ -1,47 +0,0 @@ -/** - * @name Suggest using non-extending subtype relationships. - * @description Non-extending subtypes ("instanceof extensions") are generally preferrable to instanceof expressions in characteristic predicates. - * @kind problem - * @problem.severity warning - * @id ql/suggest-instanceof-extension - * @tags maintainability - * @precision medium - */ - -import ql - -InstanceOf instanceofInCharPred(Class c) { - result = c.getCharPred().getBody() - or - exists(Conjunction conj | - conj = c.getCharPred().getBody() and - result = conj.getAnOperand() - ) -} - -predicate instanceofThisInCharPred(Class c, TypeExpr type) { - exists(InstanceOf instanceOf | - instanceOf = instanceofInCharPred(c) and - instanceOf.getExpr() instanceof ThisAccess and - type = instanceOf.getType() - ) -} - -predicate classWithInstanceofThis(Class c, TypeExpr type) { - instanceofThisInCharPred(c, type) and - exists(ClassPredicate classPred | - classPred = c.getAClassPredicate() and - exists(MemberCall call, InlineCast cast | - call.getEnclosingPredicate() = classPred and - cast = call.getBase() and - cast.getBase() instanceof ThisAccess and - cast.getTypeExpr().getResolvedType() = type.getResolvedType() - ) - ) -} - -from Class c, TypeExpr type, string message -where - classWithInstanceofThis(c, type) and - message = "consider defining $@ as non-extending subtype of $@" -select c, message, c, c.getName(), type, type.getResolvedType().getName()