From 25985e901bf5020f0fb6d9e7857d6c081d47e1fe Mon Sep 17 00:00:00 2001 From: Taus Brock-Nannestad Date: Thu, 26 Sep 2019 17:53:10 +0200 Subject: [PATCH 1/3] Python: Remove a few false positives from `py/unused-import`. --- python/ql/src/Imports/UnusedImport.ql | 25 ++++++++++++++++--- .../Imports/unused/imports_test.py | 14 +++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/python/ql/src/Imports/UnusedImport.ql b/python/ql/src/Imports/UnusedImport.ql index 6d927550c0c7..d87b0efd254d 100644 --- a/python/ql/src/Imports/UnusedImport.ql +++ b/python/ql/src/Imports/UnusedImport.ql @@ -58,16 +58,28 @@ 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 + exists(string modname, Location loc | + imp.getAName().getAsname().(Name).getId() = modname and + loc.getFile() = imp.getScope().(Module).getFile() + | /* Look for typehints 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.*") + ) ) } @@ -100,6 +112,11 @@ predicate unused_import(Import imp, Variable name) { 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(_) } diff --git a/python/ql/test/query-tests/Imports/unused/imports_test.py b/python/ql/test/query-tests/Imports/unused/imports_test.py index a1b457f04228..cf2024ac1833 100644 --- a/python/ql/test/query-tests/Imports/unused/imports_test.py +++ b/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 From 4341e88fc4dbeeb24aae204c0288693d3e87bd04 Mon Sep 17 00:00:00 2001 From: Taus Brock-Nannestad Date: Fri, 27 Sep 2019 13:03:27 +0200 Subject: [PATCH 2/3] Python: Clean up comments in preparation for autoformat. --- python/ql/src/Imports/UnusedImport.ql | 29 ++++++++++++--------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/python/ql/src/Imports/UnusedImport.ql b/python/ql/src/Imports/UnusedImport.ql index d87b0efd254d..951e7f875132 100644 --- a/python/ql/src/Imports/UnusedImport.ql +++ b/python/ql/src/Imports/UnusedImport.ql @@ -20,7 +20,7 @@ predicate global_name_used(Module m, Variable name) { 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 @@ -33,10 +33,10 @@ predicate global_name_used(Module m, Variable name) { predicate all_not_understood(Module m) { exists(GlobalVariable a | a.getId() = "__all__" and a.getScope() = m | - /* __all__ is not defined as a simple list */ + // `__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()) ) } @@ -45,10 +45,9 @@ 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… - */ + // Look for doctests containing the patterns: + // >>> …name… + // ... …name… exists(StrConst doc | doc.getEnclosingModule() = imp.getScope() and doc.isDocString() and @@ -62,9 +61,8 @@ predicate imported_module_used_in_typehint(Import imp) { imp.getAName().getAsname().(Name).getId() = modname and loc.getFile() = imp.getScope().(Module).getFile() | - /* Look for typehints containing the patterns: - * # type: …name… - */ + // Look for type hints containing the patterns: + // # type: …name… exists(Comment typehint | loc = typehint.getLocation() and typehint.getText().regexpMatch("# type:.*" + modname + ".*") @@ -95,10 +93,10 @@ predicate unused_import(Import imp, Variable name) { and not global_name_used(imp.getScope(), name) and - /* Imports in __init__.py are used to force module loading */ + // Imports in `__init__.py` are used to force module loading not imp.getEnclosingModule().isPackageInit() and - /* Name may be imported for use in epytext documentation */ + // 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() @@ -106,16 +104,15 @@ predicate unused_import(Import imp, Variable name) { and not name_acceptable_for_unused_variable(name) and - /* Assume that opaque `__all__` includes imported module */ + // 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. - */ + // 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 9878e4fe263f54120fe282810274377a06f9cc01 Mon Sep 17 00:00:00 2001 From: Taus Brock-Nannestad Date: Fri, 27 Sep 2019 13:04:17 +0200 Subject: [PATCH 3/3] Python: Apply four-space autoformat. --- python/ql/src/Imports/UnusedImport.ql | 53 +++++++++------------------ 1 file changed, 18 insertions(+), 35 deletions(-) diff --git a/python/ql/src/Imports/UnusedImport.ql b/python/ql/src/Imports/UnusedImport.ql index 951e7f875132..3ac04e2e4d2f 100644 --- a/python/ql/src/Imports/UnusedImport.ql +++ b/python/ql/src/Imports/UnusedImport.ql @@ -14,7 +14,7 @@ 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 @@ -31,8 +31,7 @@ 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 | + exists(GlobalVariable a | a.getId() = "__all__" and a.getScope() = m | // `__all__` is not defined as a simple list not m.declaredInAll(_) or @@ -43,8 +42,7 @@ predicate all_not_understood(Module m) { predicate imported_module_used_in_doctest(Import imp) { exists(string modname | - imp.getAName().getAsname().(Name).getId() = modname - and + imp.getAName().getAsname().(Name).getId() = modname and // Look for doctests containing the patterns: // >>> …name… // ... …name… @@ -60,7 +58,7 @@ predicate imported_module_used_in_typehint(Import imp) { 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 | @@ -73,7 +71,7 @@ predicate imported_module_used_in_typehint(Import imp) { 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.*") @@ -81,43 +79,28 @@ predicate imported_module_used_in_typehint(Import imp) { ) } - 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 + 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 + not imp.getEnclosingModule().isPackageInit() and // Name may be imported for use in epytext documentation - not exists(Comment cmt | - cmt.getText().matches("%L{" + name.getId() + "}%") | + 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 + ) 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). + 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." -