Change implenetation of missing calls to use getASuperCallTarget, and change alerts to alert on the class and provide clearer information, using optional location links.

python/ql/src/Classes/CallsToInitDel/MethodCallOrder.qll

@@ -3,32 +3,38 @@
 import python
 import semmle.python.ApiGraphs
 import semmle.python.dataflow.new.internal.DataFlowDispatch
+import codeql.util.Option
 
 predicate multipleCallsToSuperclassMethod(Function meth, Function calledMulti, string name) {
   exists(DataFlow::MethodCallNode call1, DataFlow::MethodCallNode call2, Class cls |
     meth.getName() = name and
     meth.getScope() = cls and
     call1.asExpr() != call2.asExpr() and
-    calledMulti = getASuperCallTarget(cls, meth, call1) and
+    calledMulti = getASuperCallTargetFromCall(cls, meth, call1, name) and
-    calledMulti = getASuperCallTarget(cls, meth, call2) and
+    calledMulti = getASuperCallTargetFromCall(cls, meth, call2, name) and
     nonTrivial(calledMulti)
   )
 }
 
-Function getASuperCallTarget(Class mroBase, Function meth, DataFlow::MethodCallNode call) {
+Function getASuperCallTargetFromCall(
+  Class mroBase, Function meth, DataFlow::MethodCallNode call, string name
+) {
   meth = call.getScope() and
   getADirectSuperclass*(mroBase) = meth.getScope() and
-  call.calls(_, meth.getName()) and
+  meth.getName() = name and
-  exists(Function target | (result = target or result = getASuperCallTarget(mroBase, target, _)) |
+  call.calls(_, name) and
+  exists(Class targetCls | result = getASuperCallTargetFromClass(mroBase, targetCls, name) |
     superCall(call, _) and
-    target =
+    targetCls = getNextClassInMroKnownStartingClass(meth.getScope(), mroBase)
-      findFunctionAccordingToMroKnownStartingClass(getNextClassInMroKnownStartingClass(meth.getScope(),
-          mroBase), mroBase, meth.getName())
     or
-    exists(Class called |
+    callsMethodOnClassWithSelf(meth, call, targetCls, _)
-      callsMethodOnClassWithSelf(meth, call, called, _) and
+  )
-      target = findFunctionAccordingToMroKnownStartingClass(called, mroBase, meth.getName())
+}
-    )
+
+Function getASuperCallTargetFromClass(Class mroBase, Class cls, string name) {
+  exists(Function target |
+    target = findFunctionAccordingToMroKnownStartingClass(cls, mroBase, name) and
+    (result = target or result = getASuperCallTargetFromCall(mroBase, target, _, name))
   )
 }
 
@@ -78,31 +84,83 @@ predicate callsMethodOnUnknownClassWithSelf(Function meth, string name) {
   )
 }
 
-predicate mayProceedInMro(Class a, Class b, Class mroStart) {
+predicate missingCallToSuperclassMethod(Class base, Function shouldCall, string name) {
-  b = getNextClassInMroKnownStartingClass(a, mroStart)
+  base.getName() = name and
-  or
+  shouldCall.getName() = name and
-  exists(Class mid |
+  base = getADirectSuperclass*(base.getScope()) and
-    mid = getNextClassInMroKnownStartingClass(a, mroStart) and
+  not shouldCall = getASuperCallTargetFromClass(base, base, name) and
-    mayProceedInMro(mid, b, mroStart)
+  nonTrivial(shouldCall) and
+  // "Benefit of the doubt" - if somewhere in the chain we call an unknown superclass, assume all the necessary parent methods are called from it
+  not callsMethodOnUnknownClassWithSelf(getASuperCallTargetFromClass(base, base, name), name)
+}
+
+predicate missingCallToSuperclassMethodRestricted(Class base, Function shouldCall, string name) {
+  missingCallToSuperclassMethod(base, shouldCall, name) and
+  not exists(Class subBase |
+    subBase = getADirectSubclass+(base) and
+    missingCallToSuperclassMethod(subBase, shouldCall, name)
+  ) and
+  not exists(Function superShouldCall |
+    superShouldCall.getScope() = getADirectSuperclass+(shouldCall.getScope()) and
+    missingCallToSuperclassMethod(base, superShouldCall, name)
   )
 }
 
-predicate missingCallToSuperclassMethod(
+Function getPossibleMissingSuper(Class base, Function shouldCall, string name) {
-  Function base, Function shouldCall, Class mroStart, string name
+  missingCallToSuperclassMethod(base, shouldCall, name) and
-) {
+  exists(Function baseMethod |
-  base.getName() = name and
+    baseMethod.getScope() = base and
-  shouldCall.getName() = name and
+    baseMethod.getName() = name and
-  not callsSuper(base) and
+    // the base method calls super, so is presumably expecting every method called in the MRO chain to do so
-  not callsMethodOnUnknownClassWithSelf(base, name) and
+    callsSuper(baseMethod) and
-  nonTrivial(shouldCall) and
+    // result is something that does get called in the chain
-  base.getScope() = getADirectSuperclass*(mroStart) and
+    result = getASuperCallTargetFromClass(base, base, name) and
-  mayProceedInMro(base.getScope(), shouldCall.getScope(), mroStart) and
+    // it doesn't call super
-  not exists(Class called |
+    not callsSuper(result) and
-    (
+    // if it did call super, it would resolve to the missing method
-      callsMethodOnClassWithSelf(base, _, called, name)
+    shouldCall =
-      or
+      findFunctionAccordingToMroKnownStartingClass(getNextClassInMroKnownStartingClass(result
-      callsMethodOnClassWithSelf(findFunctionAccordingToMro(mroStart, name), _, called, name)
+              .getScope(), base), base, name)
-    ) and
-    shouldCall.getScope() = getADirectSuperclass*(called)
   )
 }
+
+private module FunctionOption = Option<Function>;
+
+class FunctionOption extends FunctionOption::Option {
+  /**
+   * Holds if this element is at the specified location.
+   * The location spans column `startcolumn` of line `startline` to
+   * column `endcolumn` of line `endline` in file `filepath`.
+   * For more information, see
+   * [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
+   */
+  predicate hasLocationInfo(
+    string filepath, int startline, int startcolumn, int endline, int endcolumn
+  ) {
+    this.asSome()
+        .getLocation()
+        .hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    or
+    this.isNone() and
+    filepath = "" and
+    startline = 0 and
+    startcolumn = 0 and
+    endline = 0 and
+    endcolumn = 0
+  }
+
+  string getQualifiedName() {
+    result = this.asSome().getQualifiedName()
+    or
+    this.isNone() and
+    result = ""
+  }
+}
+
+bindingset[name]
+FunctionOption getPossibleMissingSuperOption(Class base, Function shouldCall, string name) {
+  result.asSome() = getPossibleMissingSuper(base, shouldCall, name)
+  or
+  not exists(getPossibleMissingSuper(base, shouldCall, name)) and
+  result.isNone()
+}

python/ql/src/Classes/CallsToInitDel/MissingCallToDel.ql

@@ -15,21 +15,35 @@
 import python
 import MethodCallOrder
 
-predicate missingCallToSuperclassDel(Function base, Function shouldCall, Class mroStart) {
+Function getDelMethod(Class c) {
-  missingCallToSuperclassMethod(base, shouldCall, mroStart, "__del__")
+  result = c.getAMethod() and
+  result.getName() = "__del__"
 }
 
-from Function base, Function shouldCall, Class mroStart, string msg
+from Class base, Function shouldCall, FunctionOption possibleIssue, string msg
 where
-  missingCallToSuperclassDel(base, shouldCall, mroStart) and
+  not exists(Function newMethod | newMethod = base.getAMethod() and newMethod.getName() = "__new__") and
-  (
+  exists(FunctionOption possiblyMissingSuper |
-    // Simple case: the method that should be called is directly overridden
+    missingCallToSuperclassMethodRestricted(base, shouldCall, "__del__") and
-    mroStart = base.getScope() and
+    possiblyMissingSuper = getPossibleMissingSuperOption(base, shouldCall, "__del__") and
-    msg = "This delete method does not call $@, which may leave $@ not properly cleaned up."
+    (
-    or
+      not possiblyMissingSuper.isNone() and
-    // Only alert for a different mro base if there are no alerts for direct overrides
+      possibleIssue = possiblyMissingSuper and
-    not missingCallToSuperclassDel(base, _, base.getScope()) and
+      msg =
-    msg =
+        "This class does not call $@ during destruction. ($@ may be missing a call to super().__del__)"
-      "This delete method does not call super().__del__, which may cause $@ to be missed during the cleanup of $@."
+      or
+      possiblyMissingSuper.isNone() and
+      (
+        possibleIssue.asSome() = getDelMethod(base) and
+        msg =
+          "This class does not call $@ during destruction. ($@ may be missing a call to a base class __del__)"
+        or
+        not getDelMethod(base) and
+        possibleIssue.isNone() and
+        msg =
+          "This class does not call $@ during destruction. (The class lacks an __del__ method to ensure every base class __del__ is called.)"
+      )
+    )
   )
-select base, msg, shouldCall, shouldCall.getQualifiedName(), mroStart, mroStart.getName()
+select base, msg, shouldCall, shouldCall.getQualifiedName(), possibleIssue,
+  possibleIssue.getQualifiedName()

python/ql/src/Classes/CallsToInitDel/MissingCallToInit.ql

@@ -14,21 +14,30 @@
 import python
 import MethodCallOrder
 
-predicate missingCallToSuperclassInit(Function base, Function shouldCall, Class mroStart) {
+from Class base, Function shouldCall, FunctionOption possibleIssue, string msg
-  missingCallToSuperclassMethod(base, shouldCall, mroStart, "__init__")
-}
-
-from Function base, Function shouldCall, Class mroStart, string msg
 where
-  missingCallToSuperclassInit(base, shouldCall, mroStart) and
+  not exists(Function newMethod | newMethod = base.getAMethod() and newMethod.getName() = "__new__") and
-  (
+  exists(FunctionOption possiblyMissingSuper |
-    // Simple case: the method that should be called is directly overridden
+    missingCallToSuperclassMethodRestricted(base, shouldCall, "__init__") and
-    mroStart = base.getScope() and
+    possiblyMissingSuper = getPossibleMissingSuperOption(base, shouldCall, "__init__") and
-    msg = "This initialization method does not call $@, which may leave $@ partially initialized."
+    (
-    or
+      not possiblyMissingSuper.isNone() and
-    // Only alert for a different mro base if there are no alerts for direct overrides
+      possibleIssue = possiblyMissingSuper and
-    not missingCallToSuperclassInit(base, _, base.getScope()) and
+      msg =
-    msg =
+        "This class does not call $@ during initialization. ($@ may be missing a call to super().__init__)"
-      "This initialization method does not call super().__init__, which may cause $@ to be missed during the initialization of $@."
+      or
+      possiblyMissingSuper.isNone() and
+      (
+        possibleIssue.asSome() = base.getInitMethod() and
+        msg =
+          "This class does not call $@ during initialization. ($@ may be missing a call to a base class __init__)"
+        or
+        not exists(base.getInitMethod()) and
+        possibleIssue.isNone() and
+        msg =
+          "This class does not call $@ during initialization. (The class lacks an __init__ method to ensure every base class __init__ is called.)"
+      )
+    )
   )
-select base, msg, shouldCall, shouldCall.getQualifiedName(), mroStart, mroStart.getName()
+select base, msg, shouldCall, shouldCall.getQualifiedName(), possibleIssue,
+  possibleIssue.getQualifiedName()