Merge pull request #2047 from taus-semmle/python-modernise-and-fix-cyclic-import-fp

Python: modernise and fix cyclic import false positive.

Author: RasmusWL

python/ql/src/Imports/Cyclic.qll

@@ -1,51 +1,50 @@
 import python
 
-predicate is_import_time(Stmt s) {
-	not s.getScope+() instanceof Function
-}
+predicate is_import_time(Stmt s) { not s.getScope+() instanceof Function }
 
-PythonModuleObject module_imported_by(PythonModuleObject m) {
+ModuleValue module_imported_by(ModuleValue m) {
     exists(Stmt imp |
         result = stmt_imports(imp) and
-        imp.getEnclosingModule() = m.getModule() and
+        imp.getEnclosingModule() = m.getScope() and
         // Import must reach exit to be part of a cycle
         imp.getAnEntryNode().getBasicBlock().reachesExit()
     )
 }
 
 /** Is there a circular import of 'm1' beginning with 'm2'? */
-predicate circular_import(PythonModuleObject m1, PythonModuleObject m2) {
+predicate circular_import(ModuleValue m1, ModuleValue m2) {
     m1 != m2 and
-    m2 = module_imported_by(m1) and m1 = module_imported_by+(m2)
+    m2 = module_imported_by(m1) and
+    m1 = module_imported_by+(m2)
 }
 
-ModuleObject stmt_imports(ImportingStmt s) {
-    exists(string name |
-        result.importedAs(name) and not name = "__main__" |
-        name = s.getAnImportedModuleName()
+ModuleValue stmt_imports(ImportingStmt s) {
+    exists(string name | result.importedAs(name) and not name = "__main__" |
+        name = s.getAnImportedModuleName() and
+        s.getASubExpression().pointsTo(result)
     )
 }
 
-predicate import_time_imported_module(PythonModuleObject m1, PythonModuleObject m2, Stmt imp) {
-    imp.getEnclosingModule() = m1.getModule() and 
+predicate import_time_imported_module(ModuleValue m1, ModuleValue m2, Stmt imp) {
+    imp.getEnclosingModule() = m1.getScope() and
     is_import_time(imp) and
     m2 = stmt_imports(imp)
 }
 
 /** Is there a cyclic import of 'm1' beginning with an import 'm2' at 'imp' where all the imports are top-level? */
-predicate import_time_circular_import(PythonModuleObject m1, PythonModuleObject m2, Stmt imp) {
+predicate import_time_circular_import(ModuleValue m1, ModuleValue m2, Stmt imp) {
     m1 != m2 and
-    import_time_imported_module(m1, m2, imp) and 
+    import_time_imported_module(m1, m2, imp) and
     import_time_transitive_import(m2, _, m1)
 }
 
-predicate import_time_transitive_import(PythonModuleObject base, Stmt imp, PythonModuleObject last) {
+predicate import_time_transitive_import(ModuleValue base, Stmt imp, ModuleValue last) {
     last != base and
     (
         import_time_imported_module(base, last, imp)
         or
-        exists(PythonModuleObject mid | 
-            import_time_transitive_import(base, imp, mid) and 
+        exists(ModuleValue mid |
+            import_time_transitive_import(base, imp, mid) and
             import_time_imported_module(mid, last, _)
         )
     ) and
@@ -56,12 +55,12 @@ predicate import_time_transitive_import(PythonModuleObject base, Stmt imp, Pytho
 /**
  * Returns import-time usages of module 'm' in module 'enclosing'
  */
-predicate import_time_module_use(PythonModuleObject m, PythonModuleObject enclosing, Expr use, string attr) {
-    exists(Expr mod | 
-        use.getEnclosingModule() = enclosing.getModule() and
-        not use.getScope+() instanceof Function
-        and mod.refersTo(m)
-        |
+predicate import_time_module_use(ModuleValue m, ModuleValue enclosing, Expr use, string attr) {
+    exists(Expr mod |
+        use.getEnclosingModule() = enclosing.getScope() and
+        not use.getScope+() instanceof Function and
+        mod.pointsTo(m)
+    |
         // either 'M.foo'
         use.(Attribute).getObject() = mod and use.(Attribute).getName() = attr
         or
@@ -70,20 +69,24 @@ predicate import_time_module_use(PythonModuleObject m, PythonModuleObject enclos
     )
 }
 
-/** Whether importing module 'first' before importing module 'other' will fail at runtime, due to an
-    AttributeError at 'use' (in module 'other') caused by 'first.attr' not being defined as its definition can
-    occur after the import 'other' in 'first'.
-*/
-predicate failing_import_due_to_cycle(PythonModuleObject first, PythonModuleObject other, Stmt imp,
-                                      ControlFlowNode defn, Expr use, string attr) {
+/**
+ * Whether importing module 'first' before importing module 'other' will fail at runtime, due to an
+ * AttributeError at 'use' (in module 'other') caused by 'first.attr' not being defined as its definition can
+ * occur after the import 'other' in 'first'.
+ */
+predicate failing_import_due_to_cycle(
+    ModuleValue first, ModuleValue other, Stmt imp, ControlFlowNode defn, Expr use, string attr
+) {
     import_time_imported_module(other, first, _) and
     import_time_transitive_import(first, imp, other) and
     import_time_module_use(first, other, use, attr) and
-    exists(ImportTimeScope n, SsaVariable v | 
+    exists(ImportTimeScope n, SsaVariable v |
         defn = v.getDefinition() and
-        n = first.getModule() and v.getVariable().getScope() = n and v.getId() = attr |
+        n = first.getScope() and
+        v.getVariable().getScope() = n and
+        v.getId() = attr
+    |
         not defn.strictlyDominates(imp.getAnEntryNode())
-    )
-    and not exists(If i | i.isNameEqMain() and i.contains(use))
+    ) and
+    not exists(If i | i.isNameEqMain() and i.contains(use))
 }
-

python/ql/src/Imports/CyclicImport.ql

@@ -14,14 +14,13 @@
 import python
 import Cyclic
 
-from PythonModuleObject m1, PythonModuleObject m2, Stmt imp
-where 
-  imp.getEnclosingModule() = m1.getModule()
-  and stmt_imports(imp) = m2
-  and circular_import(m1, m2)
-  and m1 != m2
-  // this query finds all cyclic imports that are *not* flagged by ModuleLevelCyclicImport
-  and not failing_import_due_to_cycle(m2, m1, _, _, _, _)
-  and not exists(If i | i.isNameEqMain() and i.contains(imp))
+from ModuleValue m1, ModuleValue m2, Stmt imp
+where
+    imp.getEnclosingModule() = m1.getScope() and
+    stmt_imports(imp) = m2 and
+    circular_import(m1, m2) and
+    m1 != m2 and
+    // this query finds all cyclic imports that are *not* flagged by ModuleLevelCyclicImport
+    not failing_import_due_to_cycle(m2, m1, _, _, _, _) and
+    not exists(If i | i.isNameEqMain() and i.contains(imp))
 select imp, "Import of module $@ begins an import cycle.", m2, m2.getName()
-

python/ql/src/Imports/ModuleLevelCyclicImport.ql

@@ -21,11 +21,10 @@ import Cyclic
 // 3. 'foo' is defined in M after the import in M which completes the cycle.
 // then if we import the 'used' module, we will reach the cyclic import, start importing the 'using'
 // module, hit the 'use', and then crash due to the imported symbol not having been defined yet
-
-from PythonModuleObject m1, Stmt imp, PythonModuleObject m2, string attr, Expr use, ControlFlowNode defn 
+from ModuleValue m1, Stmt imp, ModuleValue m2, string attr, Expr use, ControlFlowNode defn
 where failing_import_due_to_cycle(m1, m2, imp, defn, use, attr)
-select use, "'" + attr + "' may not be defined if module $@ is imported before module $@, " +
-"as the $@ of " + attr + " occurs after the cyclic $@ of " + m2.getName() + ".",
-m1, m1.getName(), m2, m2.getName(), defn, "definition", imp, "import"
-
-  
+select use,
+    "'" + attr + "' may not be defined if module $@ is imported before module $@, as the $@ of " +
+        attr + " occurs after the cyclic $@ of " + m2.getName() + ".",
+    // Arguments for the placeholders in the above message:
+    m1, m1.getName(), m2, m2.getName(), defn, "definition", imp, "import"

python/ql/src/semmle/python/objects/ObjectAPI.qll

@@ -133,6 +133,12 @@ class ModuleValue extends Value {
         result = this.(PythonModuleObjectInternal).getSourceModule().getFile()
     }
 
+    /** Whether this module is imported by 'import name'. For example on a linux system,
+      * the module 'posixpath' is imported as 'os.path' or as 'posixpath' */
+    predicate importedAs(string name) {
+        PointsToInternal::module_imported_as(this, name)
+    }
+
 }
 
 module Module {

python/ql/test/query-tests/Imports/cyclic-module/module10.py

@@ -1,5 +1,9 @@
+from typing import TYPE_CHECKING
 
 if False:
     import module1
 
+if TYPE_CHECKING:
+    import module1
+
 x = 1

python/ql/test/query-tests/Imports/cyclic-module/typing.py

@@ -0,0 +1,2 @@
+TYPE_CHECKING = False
+