QL: Refine 'redundant override' query

2026-04-25 16:55:19 +02:00 · 2022-08-18 19:04:36 +02:00
parent 4f93f2b9ba
commit 50a53008cd
3 changed files with 217 additions and 40 deletions
--- a/ql/ql/src/queries/style/RedundantOverride.ql
+++ b/ql/ql/src/queries/style/RedundantOverride.ql
@@ -10,15 +10,14 @@

 import ql

-private predicate redundantOverride(ClassPredicate pred, ClassPredicate sup) {
+/** Holds if `pred` overrides super predicate `sup` by forwarding via `mc`. */
+private predicate forwardingOverride(ClassPredicate pred, MemberCall mc, ClassPredicate sup) {
  pred.overrides(sup) and
-  // Can be made more precise, but rules out overrides needed for disambiguation
-  count(pred.getDeclaringType().getASuperType()) <= 1 and
-  exists(MemberCall mc |
-    mc.getBase() instanceof Super and
-    mc.getTarget() = sup and
-    not exists(pred.getQLDoc())
-  |
+  mc.getBase() instanceof Super and
+  mc.getTarget() = sup and
+  not exists(pred.getQLDoc()) and
+  forall(int i, VarDecl p | p = pred.getParameter(i) | mc.getArgument(i) = p.getAnAccess()) and
+  (
    pred.getBody() =
      any(ComparisonFormula comp |
        comp.getOperator() = "=" and
@@ -31,6 +30,31 @@ private predicate redundantOverride(ClassPredicate pred, ClassPredicate sup) {
  )
 }

+private predicate forwardingOverrideProj(ClassPredicate pred, ClassPredicate sup) {
+  forwardingOverride(pred, _, sup)
+}
+
+private ClassPredicate getUltimateDef(ClassPredicate p) {
+  forwardingOverrideProj*(p, result) and
+  not forwardingOverrideProj(result, _)
+}
+
+private predicate redundantOverride(ClassPredicate pred, ClassPredicate sup) {
+  exists(MemberCall mc |
+    forwardingOverride(pred, mc, sup) and
+    // overridden to provide more precise QL doc
+    not exists(pred.getQLDoc()) and
+    // overridden to disambiguate
+    not exists(ClassPredicate other |
+      getUltimateDef(sup) != getUltimateDef(other) and
+      pred.getDeclaringType().getASuperType+() = other.getDeclaringType() and
+      not sup.overrides*(other) and
+      other.getName() = pred.getName() and
+      other.getArity() = pred.getArity()
+    )
+  )
+}
+
 from ClassPredicate pred, ClassPredicate sup
 where redundantOverride(pred, sup)
 select pred, "Redundant override of $@ predicate", sup, "this"
--- a/ql/ql/test/queries/style/RedundantOverride/RedundantOverride.expected
+++ b/ql/ql/test/queries/style/RedundantOverride/RedundantOverride.expected
@@ -1,4 +1,11 @@
-| RedundantOverride.qll:12:16:12:19 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:4:7:4:10 | ClassPredicate pred | this |
-| RedundantOverride.qll:16:16:16:19 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:4:7:4:10 | ClassPredicate pred | this |
-| RedundantOverride.qll:47:22:47:26 | ClassPredicate pred3 | Redundant override of $@ predicate | RedundantOverride.qll:8:13:8:17 | ClassPredicate pred3 | this |
-| RedundantOverride.qll:51:16:51:19 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:4:7:4:10 | ClassPredicate pred | this |
+| RedundantOverride.qll:9:18:9:21 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:5:9:5:12 | ClassPredicate pred | this |
+| RedundantOverride.qll:21:18:21:21 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:17:9:17:12 | ClassPredicate pred | this |
+| RedundantOverride.qll:110:24:110:27 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:106:15:106:18 | ClassPredicate pred | this |
+| RedundantOverride.qll:124:18:124:21 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:118:9:118:12 | ClassPredicate pred | this |
+| RedundantOverride.qll:128:18:128:21 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:118:9:118:12 | ClassPredicate pred | this |
+| RedundantOverride.qll:132:18:132:21 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:128:18:128:21 | ClassPredicate pred | this |
+| RedundantOverride.qll:150:18:150:21 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:140:9:140:12 | ClassPredicate pred | this |
+| RedundantOverride.qll:164:18:164:21 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:158:9:158:12 | ClassPredicate pred | this |
+| RedundantOverride.qll:168:18:168:21 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:158:9:158:12 | ClassPredicate pred | this |
+| RedundantOverride.qll:172:18:172:21 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:168:18:168:21 | ClassPredicate pred | this |
+| RedundantOverride.qll:176:18:176:21 | ClassPredicate pred | Redundant override of $@ predicate | RedundantOverride.qll:168:18:168:21 | ClassPredicate pred | this |
--- a/ql/ql/test/queries/style/RedundantOverride/RedundantOverride.qll
+++ b/ql/ql/test/queries/style/RedundantOverride/RedundantOverride.qll
@@ -1,52 +1,198 @@
-class Foo extends string {
-  Foo() { this = "Foo" }
+module Test1 {
+  class Foo extends int {
+    Foo() { this = 1 }

-  Foo pred() { none() }
+    Foo pred() { result = this }
+  }

-  Foo pred2() { none() }
-
-  predicate pred3() { none() }
+  class Bar extends Foo {
+    override Foo pred() { result = Foo.super.pred() } // BAD
+  }
 }

-class Bar1 extends Foo {
-  override Foo pred() { result = Foo.super.pred() } // BAD
+module Test2 {
+  class Foo extends int {
+    Foo() { this = 1 }
+
+    Foo pred() { result = this }
+  }
+
+  class Bar extends Foo {
+    override Foo pred() { result = super.pred() } // BAD
+  }
 }

-class Bar2 extends Foo {
-  override Foo pred() { result = super.pred() } // BAD
+module Test3 {
+  class Foo extends int {
+    Foo() { this = [1 .. 3] }
+
+    Foo pred() { any() }
+  }
+
+  class Bar extends Foo {
+    Bar() { this = 2 }
+  }
+
+  class Baz extends Foo {
+    Baz() { this = 3 }
+
+    override Bar pred() { result = super.pred() } // GOOD (refined return type)
+  }
 }

-class Bar3 extends Foo {
-  override Bar3 pred() { result = super.pred() } // GOOD (refined return type)
+module Test4 {
+  class Foo extends int {
+    Foo() { this = [1, 2] }
+
+    Foo pred() { result = 2 }
+  }
+
+  class Bar extends Foo {
+    Bar() { this = 1 }
+
+    override Foo pred() { result = this } // GOOD
+  }
 }

-class Bar4 extends Foo {
-  override Foo pred() { any() } // GOOD
+module Test5 {
+  class Foo extends int {
+    Foo() { this = 1 }
+
+    Foo pred() { result = this }
+  }
+
+  class Bar extends Foo {
+    /** My own overriding QL doc. */
+    override Foo pred() { result = super.pred() } // GOOD
+  }
 }

-class Bar5 extends Foo {
-  /** My own overriding QL doc. */
-  override Foo pred() { result = super.pred() } // GOOD
+module Test6 {
+  class Foo extends int {
+    Foo() { this = [1, 2] }
+
+    Foo pred() { result = 1 }
+
+    Foo pred2() { result = 2 }
+  }
+
+  class Bar extends Foo {
+    override Foo pred() { result = super.pred2() } // GOOD
+  }
 }

-class Bar6 extends Foo {
-  override Foo pred() { result = super.pred2() } // GOOD
+module Test7 {
+  class Foo extends int {
+    Foo() { this = [1, 2] }
+
+    Foo pred() { result = 2 }
+  }
+
+  class Bar extends int {
+    Bar() { this = 1 }
+
+    Foo pred() { result = this }
+  }
+
+  class Baz extends Foo, Bar {
+    override Foo pred() { result = Foo.super.pred() } // GOOD (disambiguate)
+  }
 }

-class Bar7 extends string {
-  Bar7() { this = "Bar7" }
+module Test8 {
+  class Foo extends int {
+    Foo() { this = 1 }

-  Foo pred() { any() }
+    predicate pred(Foo f) { f = this }
+  }
+
+  class Bar extends Foo {
+    override predicate pred(Foo f) { super.pred(f) } // BAD
+  }
 }

-class Bar8 extends Foo, Bar7 {
-  override Foo pred() { result = Foo.super.pred() } // GOOD
+module Test9 {
+  class Foo extends int {
+    Foo() { this = [1, 2] }
+
+    Foo pred() { result = this }
+  }
+
+  class Bar extends Foo {
+    Bar() { this = 1 }
+
+    override Foo pred() { Foo.super.pred() = result } // BAD
+  }
+
+  class Baz1 extends Foo, Bar {
+    override Foo pred() { Foo.super.pred() = result } // BAD
+  }
+
+  class Baz2 extends Foo, Baz1 {
+    override Foo pred() { Baz1.super.pred() = result } // BAD
+  }
 }

-class Bar9 extends Foo {
-  override predicate pred3() { super.pred3() } // BAD
+module Test10 {
+  class Foo extends int {
+    Foo() { this = [1, 2] }
+
+    Foo pred() { result = 1 }
+  }
+
+  class Bar extends int {
+    Bar() { this = 1 }
+
+    Foo pred(int i) { none() }
+  }
+
+  class Baz1 extends Foo, Bar {
+    override Foo pred() { result = Foo.super.pred() } // BAD
+  }
 }

-class Bar10 extends Foo {
-  override Foo pred() { Foo.super.pred() = result } // BAD
+module Test11 {
+  class Foo extends int {
+    Foo() { this = [1 .. 4] }
+
+    Foo pred() { result = 1 }
+  }
+
+  class Bar1 extends Foo {
+    Bar1() { this = [1 .. 3] }
+
+    override Foo pred() { Foo.super.pred() = result } // BAD
+  }
+
+  class Bar2 extends Foo, Bar1 {
+    override Foo pred() { Foo.super.pred() = result } // BAD
+  }
+
+  class Bar3 extends Foo, Bar2 {
+    override Foo pred() { Bar2.super.pred() = result } // BAD
+  }
+
+  class Bar4 extends Bar2, Bar3 {
+    override Foo pred() { result = Bar2.super.pred() } // BAD
+  }
+
+  class Bar5 extends Foo {
+    Bar5() { this = [1 .. 2] }
+
+    override Foo pred() { result = this } // GOOD
+  }
+
+  class Bar6 extends Bar4, Bar5 {
+    override Foo pred() { result = Bar4.super.pred() } // GOOD (dismambiguate)
+  }
+
+  class Bar7 extends Bar6 {
+    Bar7() { this = 1 }
+
+    override Foo pred() { result = 2 } // GOOD
+  }
+
+  class Bar8 extends Bar6, Bar7 {
+    override Foo pred() { result = Bar6.super.pred() } // GOOD (specialize)
+  }
 }