Merge pull request #7132 from aschackmull/java/overrides

Java: Fix overrides to not be transitive.

Author: aschackmull

java/change-notes/2021-11-15-overrides.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The predicate `Method.overrides(Method)` was accidentally transitive. This has been fixed. This fix also affects `Method.overridesOrInstantiates(Method)` and `Method.getASourceOverriddenMethod()`.

java/ql/lib/semmle/code/java/Member.qll

@@ -285,7 +285,20 @@ private predicate overrides(Method m1, Method m2) {
     or
     m2.isProtected()
     or
-    m2.isPackageProtected() and t1.getPackage() = t2.getPackage()
+    m2.isPackageProtected() and
+    pragma[only_bind_out](t1.getPackage()) = pragma[only_bind_out](t2.getPackage())
+  )
+}
+
+pragma[nomagic]
+private predicate overridesCandidateType(RefType tsup, string sig, RefType t, Method m) {
+  virtualMethodWithSignature(sig, t, m) and
+  t.extendsOrImplements(tsup)
+  or
+  exists(RefType mid |
+    overridesCandidateType(mid, sig, t, m) and
+    mid.extendsOrImplements(tsup) and
+    not virtualMethodWithSignature(sig, mid, _)
   )
 }
 
@@ -294,11 +307,10 @@ private predicate overrides(Method m1, Method m2) {
  * ignoring any access modifiers. Additionally, this predicate binds
  * `t1` to the type declaring `m1` and `t2` to the type declaring `m2`.
  */
-pragma[noopt]
+cached
 predicate overridesIgnoringAccess(Method m1, RefType t1, Method m2, RefType t2) {
   exists(string sig |
-    virtualMethodWithSignature(sig, t1, m1) and
-    t1.extendsOrImplements+(t2) and
+    overridesCandidateType(t2, sig, t1, m1) and
     virtualMethodWithSignature(sig, t2, m2)
   )
 }

java/ql/lib/semmle/code/java/deadcode/EntryPoints.qll

@@ -262,7 +262,7 @@ class ManagedBeanImplEntryPoint extends EntryPoint, RegisteredManagedBeanImpl {
     // Find the method that will be called for each method on each managed bean that this class
     // implements.
     this.inherits(result) and
-    result.overrides(this.getAnImplementedManagedBean().getAMethod())
+    result.overrides+(this.getAnImplementedManagedBean().getAMethod())
   }
 }
 

java/ql/lib/semmle/code/java/deadcode/StrutsEntryPoints.qll

@@ -19,7 +19,7 @@ class Struts1ActionEntryPoint extends EntryPoint, Class {
       exists(Method methodFromAction |
         methodFromAction.getDeclaringType().hasQualifiedName("org.apache.struts.action", "Action")
       |
-        result.(Method).overrides(methodFromAction)
+        result.(Method).overrides+(methodFromAction)
       )
       or
       this.getASupertype*().hasQualifiedName("org.apache.struts.actions", "DispatchAction") and

java/ql/lib/semmle/code/java/frameworks/Thrift.qll

@@ -27,7 +27,7 @@ class ThriftIface extends Interface {
 
   Method getAnImplementingMethod() {
     result.getDeclaringType().(Class).getASupertype+() = this and
-    result.overrides(this.getAMethod()) and
+    result.overrides+(this.getAMethod()) and
     not result.getFile() = this.getFile()
   }
 }

java/ql/lib/semmle/code/java/frameworks/javaee/ejb/EJB.qll

@@ -572,7 +572,7 @@ class RemoteInterface extends Interface {
    * abstract methods or overriding within an interface hierarchy.
    */
   Method getARemoteMethodImplementationChecked() {
-    result.overrides(this.getARemoteMethod()) and
+    result.overrides+(this.getARemoteMethod()) and
     exists(result.getBody())
   }
 

java/ql/src/Likely Bugs/Concurrency/NonSynchronizedOverride.ql

@@ -54,7 +54,6 @@ where
   sup.isSynchronized() and
   not sub.isSynchronized() and
   not delegatingOverride(sub, sup) and
-  not exists(Method mid | sub.overrides(mid) and mid.overrides(sup)) and
   supSrc = sup.getDeclaringType().getSourceDeclaration()
 select sub,
   "Method '" + sub.getName() + "' overrides a synchronized method in $@ but is not synchronized.",

java/ql/src/Likely Bugs/Inheritance/NoNonFinalInConstructor.ql

@@ -45,7 +45,7 @@ where
   // which is an access to the object being initialized, ...
   ma = unqualifiedCallToNonAbstractMethod(c, m) and
   // ... there exists an overriding method in a subtype,
-  n.overrides(m) and
+  n.overrides+(m) and
   n.getDeclaringType().getASupertype+() = c.getDeclaringType() and
   // ... the method is in a supertype of c,
   m.getDeclaringType() = c.getDeclaringType().getASupertype*() and

java/ql/test/library-tests/overrides/ConstructedOverrides2.expected

@@ -3,4 +3,3 @@
 | ConstructedOverrides.java:17:7:17:9 | Sub | usedGeneric(U, String) | Super.class:0:0:0:0 | Super<String> | usedGeneric(U, String) |
 | ConstructedOverrides.java:23:7:23:10 | Sub2 | unusedGeneric(V, String) | Super.class:0:0:0:0 | Super<String> | unusedGeneric(U, String) |
 | ConstructedOverrides.java:23:7:23:10 | Sub2 | usedGeneric(V, String) | ConstructedOverrides.java:17:7:17:9 | Sub | usedGeneric(U, String) |
-| ConstructedOverrides.java:23:7:23:10 | Sub2 | usedGeneric(V, String) | Super.class:0:0:0:0 | Super<String> | usedGeneric(U, String) |