Merge pull request #2042 from tausbn/python-fix-unused-import-fps

Python: Fix false positives in `py/unused-import`.

Author: RasmusWL

python/ql/src/Imports/UnusedImport.ql

@@ -14,13 +14,13 @@ import python
 import Variables.Definition
 
 predicate global_name_used(Module m, Variable name) {
-    exists (Name u, GlobalVariable v |
+    exists(Name u, GlobalVariable v |
         u.uses(v) and
         v.getId() = name.getId() and
         u.getEnclosingModule() = m
     )
     or
-    /* A use of an undefined class local variable, will use the global variable */
+    // A use of an undefined class local variable, will use the global variable
     exists(Name u, LocalVariable v |
         u.uses(v) and
         v.getId() = name.getId() and
@@ -31,24 +31,21 @@ predicate global_name_used(Module m, Variable name) {
 
 /** Holds if a module has `__all__` but we don't understand it */
 predicate all_not_understood(Module m) {
-    exists(GlobalVariable a | 
-        a.getId() = "__all__" and a.getScope() = m |
-        /* __all__ is not defined as a simple list */
+    exists(GlobalVariable a | a.getId() = "__all__" and a.getScope() = m |
+        // `__all__` is not defined as a simple list
         not m.declaredInAll(_)
         or
-        /* __all__ is modified */
+        // `__all__` is modified
         exists(Call c | c.getFunc().(Attribute).getObject() = a.getALoad())
     )
 }
 
 predicate imported_module_used_in_doctest(Import imp) {
     exists(string modname |
-        imp.getAName().getAsname().(Name).getId() = modname
-        and
-        /* Look for doctests containing the patterns:
-         * >>> …name…
-         * ... …name…
-         */
+        imp.getAName().getAsname().(Name).getId() = modname and
+        // Look for doctests containing the patterns:
+        // >>> …name…
+        // ... …name…
         exists(StrConst doc |
             doc.getEnclosingModule() = imp.getScope() and
             doc.isDocString() and
@@ -58,52 +55,52 @@ predicate imported_module_used_in_doctest(Import imp) {
 }
 
 predicate imported_module_used_in_typehint(Import imp) {
-    exists(string modname |
-        imp.getAName().getAsname().(Name).getId() = modname
-        and
-        /* Look for typehints containing the patterns:
-         * # type: …name…
-         */
+    exists(string modname, Location loc |
+        imp.getAName().getAsname().(Name).getId() = modname and
+        loc.getFile() = imp.getScope().(Module).getFile()
+    |
+        // Look for type hints containing the patterns:
+        // # type: …name…
         exists(Comment typehint |
-            typehint.getLocation().getFile() = imp.getScope().(Module).getFile() and
+            loc = typehint.getLocation() and
             typehint.getText().regexpMatch("# type:.*" + modname + ".*")
         )
+        or
+        // Type hint is inside a string annotation, as needed for forward references
+        exists(string typehint, Expr annotation |
+            annotation = any(Arguments a).getAnAnnotation()
+            or
+            annotation = any(AnnAssign a).getAnnotation()
+        |
+            annotation.pointsTo(Value::forString(typehint)) and
+            loc = annotation.getLocation() and
+            typehint.regexpMatch(".*\\b" + modname + "\\b.*")
+        )
     )
 }
 
-
 predicate unused_import(Import imp, Variable name) {
-    ((Name)imp.getAName().getAsname()).getVariable() = name
-    and
-    not imp.getAnImportedModuleName() = "__future__"
-    and
-    not imp.getEnclosingModule().declaredInAll(name.getId())
-    and
-    imp.getScope() = imp.getEnclosingModule()
-    and
-    not global_name_used(imp.getScope(), name)
-    and
-    /* Imports in __init__.py are used to force module loading */
-    not imp.getEnclosingModule().isPackageInit()
-    and
-    /* Name may be imported for use in epytext documentation */
-    not exists(Comment cmt |
-        cmt.getText().matches("%L{" + name.getId() + "}%") |
+    imp.getAName().getAsname().(Name).getVariable() = name and
+    not imp.getAnImportedModuleName() = "__future__" and
+    not imp.getEnclosingModule().declaredInAll(name.getId()) and
+    imp.getScope() = imp.getEnclosingModule() and
+    not global_name_used(imp.getScope(), name) and
+    // Imports in `__init__.py` are used to force module loading
+    not imp.getEnclosingModule().isPackageInit() and
+    // Name may be imported for use in epytext documentation
+    not exists(Comment cmt | cmt.getText().matches("%L{" + name.getId() + "}%") |
         cmt.getLocation().getFile() = imp.getLocation().getFile()
-    )
-    and
-    not name_acceptable_for_unused_variable(name)
-    and
-    /* Assume that opaque `__all__` includes imported module */
-    not all_not_understood(imp.getEnclosingModule())
-    and
-    not imported_module_used_in_doctest(imp)
-    and
-    not imported_module_used_in_typehint(imp)
+    ) and
+    not name_acceptable_for_unused_variable(name) and
+    // Assume that opaque `__all__` includes imported module
+    not all_not_understood(imp.getEnclosingModule()) and
+    not imported_module_used_in_doctest(imp) and
+    not imported_module_used_in_typehint(imp) and
+    // Only consider import statements that actually point-to something (possibly an unknown module).
+    // If this is not the case, it's likely that the import statement never gets executed.
+    imp.getAName().getValue().pointsTo(_)
 }
 
-
 from Stmt s, Variable name
 where unused_import(s, name)
 select s, "Import of '" + name.getId() + "' is not used."
-

python/ql/test/query-tests/Imports/unused/imports_test.py

@@ -76,3 +76,17 @@ def f():
 
 foo = None # type: typing.Optional[int]
 
+
+# OK since the import statement is never executed
+if False:
+    import never_imported
+
+# OK since the imported module is used in a forward-referenced type annotation.
+import only_used_in_parameter_annotation
+
+def func(x: 'Optional[only_used_in_parameter_annotation]'):
+    pass
+
+import only_used_in_annotated_assignment
+
+var : 'Optional[only_used_in_annotated_assignment]' = 5