Merge pull request #1955 from RasmusWL/python-modernise-explicit-return-in-init

Python: Modernise the `py/explicit-return-in-init` query.

python/ql/src/Classes/Equality.qll

@@ -38,7 +38,7 @@ class GenericEqMethod extends Function {
     }
 }
 
-/** An __eq__ method that just does self is other */
+/** An `__eq__` method that just does `self is other` */
 class IdentityEqMethod extends Function {
  
     IdentityEqMethod() {

python/ql/src/Functions/ExplicitReturnInInit.ql

@@ -12,12 +12,12 @@
 
 import python
 
-from Return r
-where exists(Function init | init.isInitMethod() and
-r.getScope() = init and exists(r.getValue())) and
-not r.getValue() instanceof None and
-not exists(FunctionObject f | f.getACall() = r.getValue().getAFlowNode() |
-    f.neverReturns()
-) and
-not exists(Attribute meth | meth = ((Call)r.getValue()).getFunc() | meth.getName() = "__init__")
+from Return r, Expr rv
+where
+    exists(Function init | init.isInitMethod() and r.getScope() = init) and
+    r.getValue() = rv and
+    not rv.pointsTo(Value::none_()) and
+    not exists(FunctionValue f | f.getACall() = rv.getAFlowNode() | f.neverReturns()) and
+    // to avoid double reporting, don't trigger if returning result from other __init__ function
+    not exists(Attribute meth | meth = rv.(Call).getFunc() | meth.getName() = "__init__")
 select r, "Explicit return in __init__ method."

python/ql/src/semmle/python/Function.qll

@@ -41,7 +41,7 @@ class Function extends Function_, Scope, AstNode {
         exists(YieldFrom y | y.getScope() = this)
     }
 
-    /** Whether this function is declared in a class and is named "__init__" */
+    /** Whether this function is declared in a class and is named `__init__` */
     predicate isInitMethod() {
         this.isMethod() and this.getName() = "__init__"
     }

python/ql/test/query-tests/Functions/general/ExplicitReturnInInit.expected

@@ -1 +1,2 @@
 | explicit_return_in_init.py:4:9:4:19 | Return | Explicit return in __init__ method. |
+| explicit_return_in_init.py:102:9:102:18 | Return | Explicit return in __init__ method. |

python/ql/test/query-tests/Functions/general/explicit_return_in_init.py

@@ -3,7 +3,7 @@ class ExplicitReturnInInit(object):
     def __init__(self):
         return self
 
-#These are OK
+# These are OK
 class ExplicitReturnNoneInInit(object):
 
     def __init__(self):
@@ -32,8 +32,71 @@ class InitIsGenerator(object):
     def __init__(self):
         yield self
 
-#OK as it returns result of a call to super().__init__()
+# OK as it returns result of a call to super().__init__()
 class InitCallsInit(InitCallsError):
 
     def __init__(self):
         return super(InitCallsInit, self).__init__()
+
+# This is not ok, but we will only report root cause (ExplicitReturnInInit)
+class InitCallsBadInit(ExplicitReturnInInit):
+
+    def __init__(self):
+        return ExplicitReturnInInit.__init__(self)
+
+# OK as procedure implicitly returns None
+#
+# this was seen in the wild: https://lgtm.com/projects/b/jjburton/cgmtools/snapshot/0d8a429b7ea17854a5e5341df98b1cbd54d7fe6c/files/mayaTools/cgm/lib/classes/AttrFactory.py?sort=name&dir=ASC&mode=heatmap#L90
+# using a pattern of `return procedure_that_logs_error()`
+
+def procedure():
+    pass
+
+def explicit_none():
+    return None
+
+def explicit_none_nested():
+    return explicit_none()
+
+class InitReturnsCallResult1(object):
+
+    def __init__(self):
+        return procedure()
+
+class InitReturnsCallResult2(object):
+
+    def __init__(self):
+        return explicit_none()
+
+class InitReturnsCallResult3(object):
+
+    def __init__(self):
+        return explicit_none_nested()
+
+class InitReturnsCallResult4(object):
+
+    def __init__(self, b):
+        if b:
+            p = procedure
+        else:
+            p = explicit_none
+        return p()
+
+class InitReturnsCallResult5(object):
+
+    def __init__(self, b):
+        return procedure() if b else explicit_none()
+
+# Not OK
+
+def not_ok():
+    return 42
+
+class InitReturnsCallResult6(object):
+
+    def __init__(self, b):
+        if b:
+            p = procedure_implicit_none()
+        else:
+            p = not_ok
+        return p()