From 6a73479ea395ce56d61279d17a8f4f65bef4fc93 Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Thu, 12 Dec 2019 14:52:23 +0100
Subject: [PATCH 01/17] Python: Modernise py/loop-variable-capture

---
 python/ql/src/Variables/LoopVariableCapture.ql | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/python/ql/src/Variables/LoopVariableCapture.ql b/python/ql/src/Variables/LoopVariableCapture.ql
index 307da04861d0..69c9ab96c007 100644
--- a/python/ql/src/Variables/LoopVariableCapture.ql
+++ b/python/ql/src/Variables/LoopVariableCapture.ql
@@ -34,7 +34,7 @@ predicate escaping_capturing_looping_construct(CallableExpr capturing, AstNode l
     and
     // Escapes if used out side of for loop or is a lambda in a comprehension
     (
-        exists(Expr e, For forloop | forloop = loop and e.refersTo(_, _, capturing) | not forloop.contains(e))
+        exists(Expr e, For forloop | forloop = loop and e.pointsTo(_, _, capturing) | not forloop.contains(e))
         or
         loop.(Comp).getElt() = capturing
         or

From 9f4088413a4afc441b1eafe929a1a099ac6c482c Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Thu, 12 Dec 2019 16:12:39 +0100
Subject: [PATCH 02/17] Python: Modernise py/local-shadows-builtin

+ moved `scope instanceof Function` so it makes more sense :)
---
 python/ql/src/Variables/ShadowBuiltin.ql | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/python/ql/src/Variables/ShadowBuiltin.ql b/python/ql/src/Variables/ShadowBuiltin.ql
index d0b80ef9bbb3..facda4d56331 100644
--- a/python/ql/src/Variables/ShadowBuiltin.ql
+++ b/python/ql/src/Variables/ShadowBuiltin.ql
@@ -11,10 +11,11 @@
  * @precision medium
  * @id py/local-shadows-builtin
  */
- 
+
 import python
 import Shadowing
- 
+import semmle.python.types.Builtins
+
 predicate white_list(string name) {
     /* These are rarely used and thus unlikely to be confusing */
     name = "iter" or
@@ -43,10 +44,11 @@ predicate white_list(string name) {
 }
 
 predicate shadows(Name d, string name, Scope scope, int line) {
-    exists(LocalVariable l | d.defines(l) and scope instanceof Function and
+    exists(LocalVariable l | d.defines(l) and
            l.getId() = name and
-           exists(Object::builtin(l.getId()))
+           exists(Builtin::builtin(l.getId()))
     ) and
+    scope instanceof Function and
     d.getScope() = scope and
     d.getLocation().getStartLine() = line and
     not white_list(name) and

From 8f7ba0a06d2049b08dfb42c6a6793df36e1a6d12 Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Fri, 13 Dec 2019 14:09:50 +0100
Subject: [PATCH 03/17] Python: Modernise py/local-shadows-global

---
 python/ql/src/Variables/ShadowGlobal.ql | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/python/ql/src/Variables/ShadowGlobal.ql b/python/ql/src/Variables/ShadowGlobal.ql
index 8a8df6694c58..24e09d6e3db1 100644
--- a/python/ql/src/Variables/ShadowGlobal.ql
+++ b/python/ql/src/Variables/ShadowGlobal.ql
@@ -14,6 +14,7 @@
 
 import python
 import Shadowing
+import semmle.python.types.Builtins
 
 predicate shadows(Name d, GlobalVariable g, Scope scope, int line) {
     exists(LocalVariable l | d.defines(l) and l.getId() = g.getId() and
@@ -21,11 +22,11 @@ predicate shadows(Name d, GlobalVariable g, Scope scope, int line) {
         not exists(Import il, Import ig, Name gd | il.contains(d) and gd.defines(g) and ig.contains(gd)) and
         not exists(Assign a | a.getATarget() = d and a.getValue() = g.getAnAccess())
     ) and
-    not exists(Object::builtin(g.getId())) and
+    not exists(Builtin::builtin(g.getId())) and
     d.getScope() = scope and
     d.getLocation().getStartLine() = line and
     exists(Name defn | defn.defines(g) |
-        not exists(If i | i.isNameEqMain() | 
+        not exists(If i | i.isNameEqMain() |
             i.contains(defn)
         )
     ) and
@@ -34,24 +35,24 @@ predicate shadows(Name d, GlobalVariable g, Scope scope, int line) {
 
 /* pytest dynamically populates its namespace so, we cannot look directly for the pytest.fixture function */
 AttrNode pytest_fixture_attr() {
-    exists(ModuleObject pytest |
-        result.getObject("fixture").refersTo(pytest)
+    exists(ModuleValue pytest |
+        result.getObject("fixture").pointsTo(pytest)
     )
 }
 
-Object pytest_fixture() {
+Value pytest_fixture() {
     exists(CallNode call |
         call.getFunction() = pytest_fixture_attr()
         or
         call.getFunction().(CallNode).getFunction() = pytest_fixture_attr()
         |
-        call.refersTo(result)
+        call.pointsTo(result)
     )
 }
 
 /* pytest fixtures require that the parameter name is also a global */
 predicate assigned_pytest_fixture(GlobalVariable v) {
-    exists(NameNode def | def.defines(v) and def.(DefinitionNode).getValue().refersTo(pytest_fixture()))
+    exists(NameNode def | def.defines(v) and def.(DefinitionNode).getValue().pointsTo(pytest_fixture()))
 }
 
 predicate first_shadowing_definition(Name d, GlobalVariable g) {
@@ -61,6 +62,6 @@ predicate first_shadowing_definition(Name d, GlobalVariable g) {
 }
 
 from Name d, GlobalVariable g, Name def
-where first_shadowing_definition(d, g) and not exists(Name n | n.deletes(g)) and 
+where first_shadowing_definition(d, g) and not exists(Name n | n.deletes(g)) and
       def.defines(g) and not assigned_pytest_fixture(g) and not g.getId() = "_"
 select d, "Local variable '" + g.getId() + "' shadows a global variable defined $@.", def, "here"

From 81e27aab8d4e17e1b4e4badbc819db81478b4655 Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Mon, 16 Dec 2019 13:19:17 +0100
Subject: [PATCH 04/17] Python: Modernise py/unused-loop-variable

---
 .../SuspiciousUnusedLoopIterationVariable.ql  | 27 +++++++++----------
 .../src/semmle/python/objects/ObjectAPI.qll   |  7 ++++-
 .../test/query-tests/Variables/unused/test.py | 15 ++++++++---
 3 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/python/ql/src/Variables/SuspiciousUnusedLoopIterationVariable.ql b/python/ql/src/Variables/SuspiciousUnusedLoopIterationVariable.ql
index 52d8768f404f..5d76bd869911 100644
--- a/python/ql/src/Variables/SuspiciousUnusedLoopIterationVariable.ql
+++ b/python/ql/src/Variables/SuspiciousUnusedLoopIterationVariable.ql
@@ -15,7 +15,7 @@ import Definition
 
 predicate is_increment(Stmt s) {
     /* x += n */
-    s.(AugAssign).getValue() instanceof IntegerLiteral  
+    s.(AugAssign).getValue() instanceof IntegerLiteral
     or
     /* x = x + n */
     exists(Name t, BinaryExpr add |
@@ -36,7 +36,7 @@ predicate empty_loop(For f) {
 
 predicate one_item_only(For f) {
     not exists(Continue c | f.contains(c)) and
-    exists(Stmt s | 
+    exists(Stmt s |
         s = f.getBody().getLastItem() |
         s instanceof Return
         or
@@ -45,13 +45,13 @@ predicate one_item_only(For f) {
 }
 
 predicate points_to_call_to_range(ControlFlowNode f) {
-    /* (x)range is a function in Py2 and a class in Py3, so we must treat it as a plain object */ 
-    exists(Object range, Object call |
-        range = Object::builtin("range") or
-        range = Object::builtin("xrange")
+    /* (x)range is a function in Py2 and a class in Py3, so we must treat it as a plain object */
+    exists(Value range, Value call |
+        range = Value::named("range") or
+        range = Value::named("xrange")
     |
-        f.refersTo(call) and
-        call.(CallNode).getFunction().refersTo(range)
+        f.pointsTo(call) and
+        call.getACall().getFunction().pointsTo(range)
     )
     or
     /* In case points-to fails due to 'from six.moves import range' or similar. */
@@ -60,11 +60,10 @@ predicate points_to_call_to_range(ControlFlowNode f) {
         range = "range" or range = "xrange"
     )
     or
-    /* If range is wrapped in a list it is still a range */
-    exists(CallNode call |
-        f.refersTo(call) and
-        call = theListType().getACall() and
-        points_to_call_to_range(call.getArg(0))
+    /* Handle list(range(...)) and list(list(range(...))) */
+    (
+        f.(CallNode).pointsTo().getClass() = ClassValue::list() and
+        points_to_call_to_range(f.(CallNode).getArg(0))
     )
 }
 
@@ -100,7 +99,7 @@ predicate implicit_repeat(For f) {
  * E.g. gets `x` from `{ y for y in x }`.
  */
 ControlFlowNode get_comp_iterable(For f) {
-    exists(Comp c | 
+    exists(Comp c |
         c.getFunction().getStmt(0) = f |
         c.getAFlowNode().getAPredecessor() = result
     )
diff --git a/python/ql/src/semmle/python/objects/ObjectAPI.qll b/python/ql/src/semmle/python/objects/ObjectAPI.qll
index c6eb6db65172..00b94b323d6e 100644
--- a/python/ql/src/semmle/python/objects/ObjectAPI.qll
+++ b/python/ql/src/semmle/python/objects/ObjectAPI.qll
@@ -211,7 +211,7 @@ module Value {
     }
 
     /** Gets the `Value` for the integer constant `i`, if it exists.
-     * There will be no `Value` for most integers, but the following are 
+     * There will be no `Value` for most integers, but the following are
      * guaranteed to exist:
      * * From zero to 511 inclusive.
      * * All powers of 2 (up to 2**30)
@@ -634,6 +634,11 @@ module ClassValue {
         result = TBuiltinClassObject(Builtin::special("float"))
     }
 
+    /** Get the `ClassValue` for the `list` class. */
+    ClassValue list() {
+        result = TBuiltinClassObject(Builtin::special("list"))
+    }
+
     /** Get the `ClassValue` for the `bytes` class (also called `str` in Python 2). */
     ClassValue bytes() {
         result = TBuiltinClassObject(Builtin::special("bytes"))
diff --git a/python/ql/test/query-tests/Variables/unused/test.py b/python/ql/test/query-tests/Variables/unused/test.py
index 855e9a5ddfd0..18dd2020306e 100644
--- a/python/ql/test/query-tests/Variables/unused/test.py
+++ b/python/ql/test/query-tests/Variables/unused/test.py
@@ -10,7 +10,7 @@ def OK1(seq):
     for _ in seq:
         do_something()
         print("Hi")
-        
+
 #OK counting
 def OK2(seq):
     i = 3
@@ -29,7 +29,7 @@ def OK4(n):
     r = range(n)
     for i in r:
         print("x")
-        
+
 #OK named as unused
 def OK5(seq):
     for unused_x in seq:
@@ -77,7 +77,7 @@ def fail4(coll, sequence):
         x = coll.pop()
     for s in sequence:
         do_something(x+1)
-        
+
 #OK See ODASA-4153 and ODASA-4533
 def fail5(t):
     x, y = t
@@ -106,3 +106,12 @@ def cleanup(sessions):
     for sess in sessions:
         # Original code had some comment about deleting sessions
         del sess
+
+# For SuspiciousUnusedLoopIterationVariable.ql
+# ok
+for x in list(range(100)):
+        print('hi')
+
+# ok
+for y in list(list(range(100))):
+        print('hi')

From 3ffea599f1b47e8232badf65e878883a1993b068 Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Mon, 16 Dec 2019 15:56:56 +0100
Subject: [PATCH 05/17] Python: Rewrite casts for py/undefined-global-variable

---
 python/ql/src/Variables/UndefinedGlobal.ql | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/python/ql/src/Variables/UndefinedGlobal.ql b/python/ql/src/Variables/UndefinedGlobal.ql
index e2c5331e97c2..12fe0aa017f4 100644
--- a/python/ql/src/Variables/UndefinedGlobal.ql
+++ b/python/ql/src/Variables/UndefinedGlobal.ql
@@ -17,7 +17,7 @@ import semmle.python.pointsto.PointsTo
 
 predicate guarded_against_name_error(Name u) {
     exists(Try t | t.getBody().getAnItem().contains(u) |
-                   ((Name)((ExceptStmt)t.getAHandler()).getType()).getId() = "NameError"
+                   t.getAHandler().getType().(Name).getId() = "NameError"
           )
     or
     exists(ConditionBlock guard, BasicBlock controlled, Call globals |
@@ -33,7 +33,7 @@ predicate contains_unknown_import_star(Module m) {
      exists(ImportStar imp | imp.getScope() = m |
         not exists(ModuleObject imported | imported.importedAs(imp.getImportedModuleName()))
         or
-        exists(ModuleObject imported | 
+        exists(ModuleObject imported |
             imported.importedAs(imp.getImportedModuleName()) |
             not imported.exportsComplete()
         )
@@ -42,15 +42,15 @@ predicate contains_unknown_import_star(Module m) {
 
 predicate undefined_use_in_function(Name u) {
     exists(Function f | u.getScope().getScope*() = f and
-        /* Either function is a method or inner function or it is live at the end of the module scope */
-        (not f.getScope() = u.getEnclosingModule() or ((ImportTimeScope)u.getEnclosingModule()).definesName(f.getName()))
+        // Either function is a method or inner function or it is live at the end of the module scope
+        (not f.getScope() = u.getEnclosingModule() or u.getEnclosingModule().(ImportTimeScope).definesName(f.getName()))
         and
-        /* There is a use, but not a definition of this global variable in the function or enclosing scope */
+        // There is a use, but not a definition of this global variable in the function or enclosing scope
         exists(GlobalVariable v | u.uses(v) |
-            not exists(Assign a, Scope defnScope | 
+            not exists(Assign a, Scope defnScope |
                 a.getATarget() = v.getAnAccess() and a.getScope() = defnScope |
-                defnScope = f or 
-                /* Exclude modules as that case is handled more precisely below. */
+                defnScope = f or
+                // Exclude modules as that case is handled more precisely below.
                 (defnScope = f.getScope().getScope*() and not defnScope instanceof Module)
             )
         )
@@ -120,7 +120,7 @@ predicate first_undefined_use(Name use) {
     exists(GlobalVariable v |
         v.getALoad() = use |
         first_use_in_a_block(use) and
-        not exists(ControlFlowNode other | 
+        not exists(ControlFlowNode other |
             other.getNode() = v.getALoad() and
             other.getBasicBlock().strictlyDominates(use.getAFlowNode().getBasicBlock())
         )

From 5faa7e71270e0823f9c7ef39570e3026dd5d6b93 Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Mon, 16 Dec 2019 17:19:04 +0100
Subject: [PATCH 06/17] Python: Add ModuleValue::hasCompleteExportInfo

---
 .../ql/src/semmle/python/objects/Modules.qll  | 28 ++++++++++++++++++-
 .../src/semmle/python/objects/ObjectAPI.qll   |  5 ++++
 .../src/semmle/python/types/ModuleObject.qll  | 10 +++++--
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/python/ql/src/semmle/python/objects/Modules.qll b/python/ql/src/semmle/python/objects/Modules.qll
index 5c2599b578cb..f3d4333177cd 100644
--- a/python/ql/src/semmle/python/objects/Modules.qll
+++ b/python/ql/src/semmle/python/objects/Modules.qll
@@ -71,6 +71,9 @@ abstract class ModuleObjectInternal extends ObjectInternal {
         py_exports(this.getSourceModule(), name)
     }
 
+    /** Whether the complete set of names "exported" by this module can be accurately determined */
+    abstract predicate hasCompleteExportInfo();
+
     override predicate isNotSubscriptedType() { any() }
 
 }
@@ -125,6 +128,10 @@ class BuiltinModuleObjectInternal extends ModuleObjectInternal, TBuiltinModuleOb
         none()
     }
 
+    override predicate hasCompleteExportInfo() {
+        any()
+    }
+
 }
 
 /** A class representing packages */
@@ -230,6 +237,12 @@ class PackageObjectInternal extends ModuleObjectInternal, TPackageObject {
         exists(this.submodule(name))
     }
 
+    override predicate hasCompleteExportInfo() {
+
+        not exists(this.getInitModule())
+        or
+        this.getInitModule().hasCompleteExportInfo()
+    }
 }
 
 /** A class representing Python modules */
@@ -300,6 +313,17 @@ class PythonModuleObjectInternal extends ModuleObjectInternal, TPythonModule {
         )
     }
 
+    override predicate hasCompleteExportInfo() {
+        exists(Module m |
+            m = this.getSourceModule() |
+            not exists(Call modify, Attribute attr, GlobalVariable all |
+                modify.getScope() = m and modify.getFunc() = attr and
+                all.getId() = "__all__" |
+                attr.getObject().(Name).uses(all)
+            )
+        )
+    }
+
 }
 
 /** A class representing a module that is missing from the DB, but inferred to exists from imports. */
@@ -354,6 +378,9 @@ class AbsentModuleObjectInternal extends ModuleObjectInternal, TAbsentModule {
         none()
     }
 
+    override predicate hasCompleteExportInfo() {
+        none()
+    }
 }
 
 /** A class representing an attribute of a missing module. */
@@ -453,4 +480,3 @@ class AbsentModuleAttributeObjectInternal extends ObjectInternal, TAbsentModuleA
     override predicate isNotSubscriptedType() { any() }
 
 }
-
diff --git a/python/ql/src/semmle/python/objects/ObjectAPI.qll b/python/ql/src/semmle/python/objects/ObjectAPI.qll
index 00b94b323d6e..afb7fdd0559e 100644
--- a/python/ql/src/semmle/python/objects/ObjectAPI.qll
+++ b/python/ql/src/semmle/python/objects/ObjectAPI.qll
@@ -150,6 +150,11 @@ class ModuleValue extends Value {
         this instanceof PackageObjectInternal
     }
 
+    /** Whether the complete set of names "exported" by this module can be accurately determined */
+    predicate hasCompleteExportInfo() {
+        this.(ModuleObjectInternal).hasCompleteExportInfo()
+    }
+
 }
 
 module Module {
diff --git a/python/ql/src/semmle/python/types/ModuleObject.qll b/python/ql/src/semmle/python/types/ModuleObject.qll
index 8a6f008ef767..e0a64ac6ec7e 100644
--- a/python/ql/src/semmle/python/types/ModuleObject.qll
+++ b/python/ql/src/semmle/python/types/ModuleObject.qll
@@ -83,9 +83,13 @@ abstract class ModuleObject extends Object {
     predicate exports(string name) {
         theModule().exports(name)
     }
- 
-    /** Whether the complete set of names "exported" by this module can be accurately determined */
-    abstract predicate exportsComplete();
+
+    /**
+      * Whether the complete set of names "exported" by this module can be accurately determined
+      *
+      * DEPRECATED: Use ModuleValue::hasCompleteExportInfo instead
+      */
+    deprecated abstract predicate exportsComplete();
 
     /** Gets the short name of the module. For example the short name of module x.y.z is 'z' */
     string getShortName() {

From 697a006ef2f3af98e23b2e91a5787fc75ad5f222 Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Mon, 16 Dec 2019 17:24:26 +0100
Subject: [PATCH 07/17] Python: Modernise py/undefined-global-variable

---
 python/ql/src/Variables/UndefinedGlobal.ql | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/python/ql/src/Variables/UndefinedGlobal.ql b/python/ql/src/Variables/UndefinedGlobal.ql
index 12fe0aa017f4..88bfc55d9525 100644
--- a/python/ql/src/Variables/UndefinedGlobal.ql
+++ b/python/ql/src/Variables/UndefinedGlobal.ql
@@ -31,11 +31,11 @@ predicate guarded_against_name_error(Name u) {
 
 predicate contains_unknown_import_star(Module m) {
      exists(ImportStar imp | imp.getScope() = m |
-        not exists(ModuleObject imported | imported.importedAs(imp.getImportedModuleName()))
+        exists(ModuleValue imported | imported.importedAs(imp.getImportedModuleName()) and imported.isAbsent())
         or
-        exists(ModuleObject imported |
+        exists(ModuleValue imported |
             imported.importedAs(imp.getImportedModuleName()) |
-            not imported.exportsComplete()
+            not imported.hasCompleteExportInfo()
         )
     )
 }
@@ -58,7 +58,7 @@ predicate undefined_use_in_function(Name u) {
     and
     not ((ImportTimeScope)u.getEnclosingModule()).definesName(u.getId())
     and
-    not exists(ModuleObject m | m.getModule() = u.getEnclosingModule() | m.hasAttribute(u.getId()))
+    not exists(ModuleValue m | m.getScope() = u.getEnclosingModule() | m.hasAttribute(u.getId()))
     and
     not globallyDefinedName(u.getId())
     and
@@ -78,7 +78,7 @@ predicate undefined_use_in_class_or_module(Name u) {
     and
     not guarded_against_name_error(u)
     and
-    not exists(ModuleObject m | m.getModule() = u.getEnclosingModule() | m.hasAttribute(u.getId()))
+    not exists(ModuleValue m | m.getScope() = u.getEnclosingModule() | m.hasAttribute(u.getId()))
     and
     not (u.getEnclosingModule().isPackageInit() and u.getId() = "__path__")
     and
@@ -88,10 +88,10 @@ predicate undefined_use_in_class_or_module(Name u) {
 predicate use_of_exec(Module m) {
     exists(Exec exec | exec.getScope() = m)
     or
-    exists(CallNode call, FunctionObject exec |
+    exists(CallNode call, FunctionValue exec |
         exec.getACall() = call and call.getScope() = m |
-        exec = Object::builtin("exec") or
-        exec = Object::builtin("execfile")
+        exec = Value::named("exec") or
+        exec = Value::named("execfile")
     )
 }
 
@@ -105,7 +105,7 @@ predicate undefined_use(Name u) {
     not contains_unknown_import_star(u.getEnclosingModule()) and
     not use_of_exec(u.getEnclosingModule()) and
     not exists(u.getVariable().getAStore()) and
-    not u.refersTo(_) and
+    not u.pointsTo(_) and
     not  probably_defined_in_loop(u)
 }
 

From aba3ac7b666c4338630df07dc8e8deba9b44536c Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Tue, 17 Dec 2019 11:29:44 +0100
Subject: [PATCH 08/17] Python: Modernise py/uninitialized-local-variable

---
 python/ql/src/Variables/UninitializedLocal.ql     | 4 +---
 python/ql/src/semmle/python/objects/ObjectAPI.qll | 5 +++++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/python/ql/src/Variables/UninitializedLocal.ql b/python/ql/src/Variables/UninitializedLocal.ql
index 66ed8bc9405d..0e52f7567ea4 100644
--- a/python/ql/src/Variables/UninitializedLocal.ql
+++ b/python/ql/src/Variables/UninitializedLocal.ql
@@ -30,7 +30,7 @@ predicate uninitialized_local(NameNode use) {
 predicate explicitly_guarded(NameNode u) {
     exists(Try t |
         t.getBody().contains(u.getNode()) and
-        t.getAHandler().getType().refersTo(theNameErrorType())
+        t.getAHandler().getType().pointsTo(ClassValue::nameError())
     )
 }
 
@@ -38,5 +38,3 @@ predicate explicitly_guarded(NameNode u) {
 from NameNode u
 where uninitialized_local(u) and not explicitly_guarded(u)
 select u.getNode(), "Local variable '" + u.getId() + "' may be used before it is initialized."
-
-
diff --git a/python/ql/src/semmle/python/objects/ObjectAPI.qll b/python/ql/src/semmle/python/objects/ObjectAPI.qll
index afb7fdd0559e..31f0b7b513bc 100644
--- a/python/ql/src/semmle/python/objects/ObjectAPI.qll
+++ b/python/ql/src/semmle/python/objects/ObjectAPI.qll
@@ -675,4 +675,9 @@ module ClassValue {
         result = TBuiltinClassObject(Builtin::special("NoneType"))
     }
 
+    /** Get the `ClassValue` for the `NameError` class. */
+    ClassValue nameError() {
+        result = TBuiltinClassObject(Builtin::builtin("NameError"))
+    }
+
 }

From 15bc4cd09026584e54d4de014d1ec10cf94ac447 Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Tue, 17 Dec 2019 16:23:10 +0100
Subject: [PATCH 09/17] Python: Add override helpers to Value classes

---
 .../src/semmle/python/objects/ObjectAPI.qll   | 20 +++++++
 .../overrides/FunctionOverrides.expected      |  9 ++++
 .../overrides/FunctionOverrides.ql            |  7 +++
 .../ql/test/library-tests/overrides/test.py   | 53 +++++++++++++++++++
 4 files changed, 89 insertions(+)
 create mode 100644 python/ql/test/library-tests/overrides/FunctionOverrides.expected
 create mode 100644 python/ql/test/library-tests/overrides/FunctionOverrides.ql
 create mode 100644 python/ql/test/library-tests/overrides/test.py

diff --git a/python/ql/src/semmle/python/objects/ObjectAPI.qll b/python/ql/src/semmle/python/objects/ObjectAPI.qll
index 31f0b7b513bc..7c18914045d1 100644
--- a/python/ql/src/semmle/python/objects/ObjectAPI.qll
+++ b/python/ql/src/semmle/python/objects/ObjectAPI.qll
@@ -105,6 +105,18 @@ class Value extends TObject {
         or
         this instanceof AbsentModuleAttributeObjectInternal
     }
+
+    /** Whether this overrides v. In this context, "overrides" means that this object
+     *  is a named attribute of a some class C and `v` is a named attribute of another
+     *  class S, both attributes having the same name, and S is a super class of C.
+     */
+    predicate overrides(Value v) {
+        exists(ClassValue my_class, ClassValue other_class, string name |
+            my_class.declaredAttribute(name) = this and
+            other_class.declaredAttribute(name) = v and
+            my_class.getABaseType+() = other_class
+        )
+    }
 }
 
 /** Class representing modules in the Python program
@@ -461,6 +473,14 @@ abstract class FunctionValue extends CallableValue {
 
     /** Gets the maximum number of parameters that can be correctly passed to this function */
     abstract int maxParameters();
+
+    predicate isOverridingMethod() {
+        exists(Value f | this.overrides(f))
+    }
+
+    predicate isOverriddenMethod() {
+        exists(Value f | f.overrides(this))
+    }
 }
 
 /** Class representing Python functions */
diff --git a/python/ql/test/library-tests/overrides/FunctionOverrides.expected b/python/ql/test/library-tests/overrides/FunctionOverrides.expected
new file mode 100644
index 000000000000..78ba5023f38f
--- /dev/null
+++ b/python/ql/test/library-tests/overrides/FunctionOverrides.expected
@@ -0,0 +1,9 @@
+| test.py:3:5:3:20 | Function Wat.upper | overriding | not overridden |
+| test.py:6:1:6:26 | Function outside_func | not overriding | overridden |
+| test.py:11:11:11:29 | Function Base.lambda | not overriding | overridden |
+| test.py:13:24:13:36 | Function Base.lambda | not overriding | not overridden |
+| test.py:17:5:17:21 | Function Base.normal | not overriding | overridden |
+| test.py:28:5:28:21 | Function Sub2.foo | overriding | overridden |
+| test.py:31:5:31:24 | Function Sub2.tricky | overriding | not overridden |
+| test.py:36:5:36:21 | Function Sub2Sub.foo | overriding | not overridden |
+| test.py:39:5:39:18 | Function Sub2Sub.baz | overriding | not overridden |
diff --git a/python/ql/test/library-tests/overrides/FunctionOverrides.ql b/python/ql/test/library-tests/overrides/FunctionOverrides.ql
new file mode 100644
index 000000000000..1493223aa8e5
--- /dev/null
+++ b/python/ql/test/library-tests/overrides/FunctionOverrides.ql
@@ -0,0 +1,7 @@
+import python
+
+from PythonFunctionValue f, string overriding, string overridden
+where
+    (if f.isOverridingMethod() then overriding = "overriding" else overriding = "not overriding") and
+    (if f.isOverriddenMethod() then overridden = "overridden" else overridden = "not overridden")
+select f, overriding, overridden
diff --git a/python/ql/test/library-tests/overrides/test.py b/python/ql/test/library-tests/overrides/test.py
new file mode 100644
index 000000000000..b88e2ceef077
--- /dev/null
+++ b/python/ql/test/library-tests/overrides/test.py
@@ -0,0 +1,53 @@
+class Wat(str):
+
+    def upper(self):
+        return self.lower()
+
+def outside_func(self, x):
+    print(x)
+
+class Base(object):
+
+    foo = lambda self, x: x+1
+
+    bar = staticmethod(lambda x: x+1)
+
+    baz = 123
+
+    def normal(self):
+        pass
+
+    tricky = outside_func
+
+class Sub(Base):
+
+    normal = False
+
+class Sub2(Base):
+
+    def foo(self, y):
+        return y * 100
+
+    def tricky(self, x):
+        print('nice!', x)
+
+class Sub2Sub(Sub2):
+
+    def foo(self, z):
+        return z / 123
+
+    def baz(self):
+        print('python is a bit crazy sometimes')
+
+
+ws = Wat('asdf')
+print(ws.upper(), len(ws))
+
+b = Base()
+print(b.foo(1))
+print(b.bar(10))
+
+
+ss = SubSub()
+print(ss.foo(1))
+ss.baz()

From 34f9135492e1a724d92e2a4ec1bf14eb9528febc Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Tue, 17 Dec 2019 16:23:33 +0100
Subject: [PATCH 10/17] Python: Modernise py/unused-parameter

---
 python/ql/src/Variables/UnusedParameter.ql | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/python/ql/src/Variables/UnusedParameter.ql b/python/ql/src/Variables/UnusedParameter.ql
index e27d151b72ea..b8b20d717dde 100644
--- a/python/ql/src/Variables/UnusedParameter.ql
+++ b/python/ql/src/Variables/UnusedParameter.ql
@@ -13,19 +13,19 @@ import python
 import Definition
 
 
-predicate unused_parameter(FunctionObject f, LocalVariable v) {
+predicate unused_parameter(FunctionValue f, LocalVariable v) {
   v.isParameter() and
-  v.getScope() = f.getFunction() and
+  v.getScope() = f.getScope() and
   not name_acceptable_for_unused_variable(v) and
   not exists(NameNode u | u.uses(v)) and
   not exists(Name inner, LocalVariable iv | inner.uses(iv) and iv.getId() = v.getId() and inner.getScope().getScope() = v.getScope())
 }
 
-predicate is_abstract(FunctionObject func) {
-    ((Name)func.getFunction().getADecorator()).getId().matches("%abstract%")
+predicate is_abstract(FunctionValue func) {
+    func.getScope().getADecorator().(Name).getId().matches("%abstract%")
 }
 
-from PyFunctionObject f, LocalVariable v
+from PythonFunctionValue f, LocalVariable v
 where v.getId() != "self" and unused_parameter(f, v) and not f.isOverridingMethod() and not f.isOverriddenMethod() and
 not is_abstract(f)
 select f, "The parameter '" + v.getId() + "' is never used."

From 58bb16e5dda035544fe3fafaf7d2346b56f8f7de Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Tue, 17 Dec 2019 17:08:12 +0100
Subject: [PATCH 11/17] Python: Modernise Variables/Undefined.qll

---
 python/ql/src/Variables/Undefined.qll | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/python/ql/src/Variables/Undefined.qll b/python/ql/src/Variables/Undefined.qll
index fffe96e6e267..1495180da25b 100644
--- a/python/ql/src/Variables/Undefined.qll
+++ b/python/ql/src/Variables/Undefined.qll
@@ -37,11 +37,11 @@ private predicate first_use(NameNode u, EssaVariable v) {
     )
 }
 
-/* Holds if `call` is a call of the form obj.method_name(...) and 
+/* Holds if `call` is a call of the form obj.method_name(...) and
  * there is a function called `method_name` that can exit the program.
  */
 private predicate maybe_call_to_exiting_function(CallNode call) {
-    exists(FunctionObject exits, string name |
+    exists(FunctionValue exits, string name |
         exits.neverReturns() and exits.getName() = name
         |
         call.getFunction().(NameNode).getId() = name or
@@ -119,5 +119,3 @@ class UninitializedConfig extends TaintTracking::Configuration {
     }
 
 }
-
-

From 994ad197c4f48126407e799a551f8462e3f86bdf Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Tue, 17 Dec 2019 17:29:21 +0100
Subject: [PATCH 12/17] Python: Add Module::builtinModule()

---
 python/ql/src/semmle/python/objects/ObjectAPI.qll | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/python/ql/src/semmle/python/objects/ObjectAPI.qll b/python/ql/src/semmle/python/objects/ObjectAPI.qll
index 7c18914045d1..9cac5b5e4e30 100644
--- a/python/ql/src/semmle/python/objects/ObjectAPI.qll
+++ b/python/ql/src/semmle/python/objects/ObjectAPI.qll
@@ -198,6 +198,10 @@ module Module {
         )
     }
 
+    /** Get the `ModuleValue` for the `builtin` module. */
+    ModuleValue builtinModule() {
+        result = TBuiltinModuleObject(Builtin::builtinModule())
+    }
 }
 
 module Value {

From 25ab0ed20f5a18b864750ab0024db351912d7e7c Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Tue, 17 Dec 2019 17:29:32 +0100
Subject: [PATCH 13/17] Python: Modernise Variables/MonkeyPatched.qll

---
 python/ql/src/Variables/MonkeyPatched.qll | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/python/ql/src/Variables/MonkeyPatched.qll b/python/ql/src/Variables/MonkeyPatched.qll
index 50108aae4ea6..41e19a425f26 100644
--- a/python/ql/src/Variables/MonkeyPatched.qll
+++ b/python/ql/src/Variables/MonkeyPatched.qll
@@ -1,25 +1,24 @@
 import python
 
-
 predicate monkey_patched_builtin(string name) {
-    exists(AttrNode attr, SubscriptNode subscr, StrConst s | 
+    exists(AttrNode attr, SubscriptNode subscr, StrConst s |
         subscr.isStore() and
         subscr.getIndex().getNode() = s and
         s.getText() = name and
         subscr.getValue() = attr and
-        attr.getObject("__dict__").refersTo(theBuiltinModuleObject())
+        attr.getObject("__dict__").pointsTo(Module::builtinModule())
     )
     or
-    exists(CallNode call, ControlFlowNode bltn, StrConst s | 
+    exists(CallNode call, ControlFlowNode bltn, StrConst s |
         call.getArg(0) = bltn and
-        bltn.refersTo(theBuiltinModuleObject()) and
+        bltn.pointsTo(Module::builtinModule()) and
         call.getArg(1).getNode() = s and
         s.getText() = name and
-        call.getFunction().refersTo(Object::builtin("setattr"))
+        call.getFunction().pointsTo(Value::named("setattr"))
     )
     or
-    exists(AttrNode attr | 
+    exists(AttrNode attr |
         attr.isStore() and
-        attr.getObject(name).refersTo(theBuiltinModuleObject())
+        attr.getObject(name).pointsTo(Module::builtinModule())
     )
 }

From b8a9a353b878f4b399905b9fdf1c3d2374fd7ae4 Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Fri, 20 Dec 2019 15:08:20 +0100
Subject: [PATCH 14/17] Python: Autoformat Variables/*

---
 python/ql/src/Variables/Definition.qll        | 63 ++++++---------
 python/ql/src/Variables/Global.ql             |  2 -
 .../ql/src/Variables/GlobalAtModuleLevel.ql   |  2 +-
 .../src/Variables/LeakingListComprehension.ql | 27 ++++---
 python/ql/src/Variables/Loop.qll              | 19 ++---
 .../ql/src/Variables/LoopVariableCapture.ql   |  7 +-
 python/ql/src/Variables/MultiplyDefined.ql    | 38 +++++----
 python/ql/src/Variables/ShadowBuiltin.ql      | 10 ++-
 python/ql/src/Variables/ShadowGlobal.ql       | 34 ++++----
 python/ql/src/Variables/Shadowing.qll         |  9 ++-
 .../SuspiciousUnusedLoopIterationVariable.ql  | 74 ++++++++---------
 python/ql/src/Variables/Undefined.qll         | 26 +++---
 python/ql/src/Variables/UndefinedGlobal.ql    | 79 +++++++++----------
 .../ql/src/Variables/UndefinedPlaceHolder.ql  | 30 +++----
 python/ql/src/Variables/UninitializedLocal.ql | 10 +--
 .../ql/src/Variables/UnusedModuleVariable.ql  | 40 +++++-----
 python/ql/src/Variables/UnusedParameter.ql    | 21 +++--
 17 files changed, 231 insertions(+), 260 deletions(-)

diff --git a/python/ql/src/Variables/Definition.qll b/python/ql/src/Variables/Definition.qll
index 0f0fc7f730b0..e8bc95ef79cc 100644
--- a/python/ql/src/Variables/Definition.qll
+++ b/python/ql/src/Variables/Definition.qll
@@ -1,26 +1,20 @@
 import python
 
-
 /**
  * A control-flow node that defines a variable
  */
 class Definition extends NameNode, DefinitionNode {
-
     /**
      * The variable defined by this control-flow node.
      */
-    Variable getVariable() {
-        this.defines(result)
-    }
+    Variable getVariable() { this.defines(result) }
 
     /**
      * The SSA variable corresponding to the current definition. Since SSA variables
      * are only generated for definitions with at least one use, not all definitions
      * will have an SSA variable.
      */
-    SsaVariable getSsaVariable() {
-        result.getDefinition() = this
-    }
+    SsaVariable getSsaVariable() { result.getDefinition() = this }
 
     /**
      * The index of this definition in its basic block.
@@ -42,9 +36,7 @@ class Definition extends NameNode, DefinitionNode {
     }
 
     /** Is this definition the first in its basic block for its variable? */
-    predicate isFirst() {
-        this.rankInBB(_, _) = 1
-    }
+    predicate isFirst() { this.rankInBB(_, _) = 1 }
 
     /** Is this definition the last in its basic block for its variable? */
     predicate isLast() {
@@ -66,8 +58,10 @@ class Definition extends NameNode, DefinitionNode {
         this.getVariable().getScope() = this.getScope() and
         // A call to locals() or vars() in the variable scope counts as a use
         not exists(Function f, Call c, string locals_or_vars |
-            c.getScope() = f and this.getScope() = f and
-            c.getFunc().(Name).getId() = locals_or_vars |
+            c.getScope() = f and
+            this.getScope() = f and
+            c.getFunc().(Name).getId() = locals_or_vars
+        |
             locals_or_vars = "locals" or locals_or_vars = "vars"
         )
     }
@@ -96,19 +90,14 @@ class Definition extends NameNode, DefinitionNode {
      * We also ignore anything named "_", "empty", "unused" or "dummy"
      */
     predicate isRelevant() {
-        exists(AstNode p |
-            p = this.getNode().getParentNode() |
+        exists(AstNode p | p = this.getNode().getParentNode() |
             p instanceof Assign or p instanceof AugAssign or p instanceof Tuple
-        )
-        and
-        not name_acceptable_for_unused_variable(this.getVariable())
-        and
+        ) and
+        not name_acceptable_for_unused_variable(this.getVariable()) and
         /* Decorated classes and functions are used */
-        not exists(this.getNode().getParentNode().(FunctionDef).getDefinedFunction().getADecorator())
-        and
+        not exists(this.getNode().getParentNode().(FunctionDef).getDefinedFunction().getADecorator()) and
         not exists(this.getNode().getParentNode().(ClassDef).getDefinedClass().getADecorator())
     }
-
 }
 
 /**
@@ -116,35 +105,30 @@ class Definition extends NameNode, DefinitionNode {
  * definition of variable `v`. The relation is not transitive by default, so any
  * observed transitivity will be caused by loops in the control-flow graph.
  */
-private
-predicate reaches_without_redef(Variable v, BasicBlock a, BasicBlock b) {
+private predicate reaches_without_redef(Variable v, BasicBlock a, BasicBlock b) {
     exists(Definition def | a.getASuccessor() = b |
         def.getBasicBlock() = a and def.getVariable() = v and maybe_redefined(v)
-    ) or
+    )
+    or
     exists(BasicBlock mid | reaches_without_redef(v, a, mid) |
-        not exists(NameNode cfn | cfn.defines(v) |
-            cfn.getBasicBlock() = mid
-        ) and
+        not exists(NameNode cfn | cfn.defines(v) | cfn.getBasicBlock() = mid) and
         mid.getASuccessor() = b
     )
 }
 
-private predicate maybe_redefined(Variable v) {
-    strictcount(Definition d | d.defines(v)) > 1
-}
+private predicate maybe_redefined(Variable v) { strictcount(Definition d | d.defines(v)) > 1 }
 
 predicate name_acceptable_for_unused_variable(Variable var) {
-    exists(string name |
-        var.getId() = name |
-        name.regexpMatch("_+") or name = "empty" or
-        name.matches("%unused%") or name = "dummy" or
+    exists(string name | var.getId() = name |
+        name.regexpMatch("_+") or
+        name = "empty" or
+        name.matches("%unused%") or
+        name = "dummy" or
         name.regexpMatch("__.*")
     )
 }
 
-
 class ListComprehensionDeclaration extends ListComp {
-
     Name getALeakedVariableUse() {
         major_version() = 2 and
         this.getIterationVariable(_).getId() = result.getId() and
@@ -153,8 +137,5 @@ class ListComprehensionDeclaration extends ListComp {
         result.isUse()
     }
 
-    Name getDefinition() {
-        result = this.getIterationVariable(0).getAStore()
-    }
-
+    Name getDefinition() { result = this.getIterationVariable(0).getAStore() }
 }
diff --git a/python/ql/src/Variables/Global.ql b/python/ql/src/Variables/Global.ql
index 8adbd06bcf51..d1ecadb1710f 100644
--- a/python/ql/src/Variables/Global.ql
+++ b/python/ql/src/Variables/Global.ql
@@ -14,5 +14,3 @@ import python
 from Global g
 where not g.getScope() instanceof Module
 select g, "Updating global variables except at module initialization is discouraged"
-
-
diff --git a/python/ql/src/Variables/GlobalAtModuleLevel.ql b/python/ql/src/Variables/GlobalAtModuleLevel.ql
index f3dc9e21440a..e0ac59d2e721 100644
--- a/python/ql/src/Variables/GlobalAtModuleLevel.ql
+++ b/python/ql/src/Variables/GlobalAtModuleLevel.ql
@@ -14,4 +14,4 @@ import python
 
 from Global g
 where g.getScope() instanceof Module
-select g, "Declaring '" + g.getAName() + "' as global at module-level is redundant."
\ No newline at end of file
+select g, "Declaring '" + g.getAName() + "' as global at module-level is redundant."
diff --git a/python/ql/src/Variables/LeakingListComprehension.ql b/python/ql/src/Variables/LeakingListComprehension.ql
index efec82af4adc..32a338d31c12 100644
--- a/python/ql/src/Variables/LeakingListComprehension.ql
+++ b/python/ql/src/Variables/LeakingListComprehension.ql
@@ -15,16 +15,17 @@ import Definition
 
 from ListComprehensionDeclaration l, Name use, Name defn
 where
-  use = l.getALeakedVariableUse() and
-  defn = l.getDefinition() and
-  l.getAFlowNode().strictlyReaches(use.getAFlowNode()) and
-  /* Make sure we aren't in a loop, as the variable may be redefined */
-  not use.getAFlowNode().strictlyReaches(l.getAFlowNode()) and
-  not l.contains(use) and
-  not use.deletes(_) and
-  not exists(SsaVariable v |
-      v.getAUse() = use.getAFlowNode() and
-      not v.getDefinition().strictlyDominates(l.getAFlowNode())
-  )
-
-select use, use.getId() + " may have a different value in Python 3, as the $@ will not be in scope.", defn, "list comprehension variable" 
+    use = l.getALeakedVariableUse() and
+    defn = l.getDefinition() and
+    l.getAFlowNode().strictlyReaches(use.getAFlowNode()) and
+    /* Make sure we aren't in a loop, as the variable may be redefined */
+    not use.getAFlowNode().strictlyReaches(l.getAFlowNode()) and
+    not l.contains(use) and
+    not use.deletes(_) and
+    not exists(SsaVariable v |
+        v.getAUse() = use.getAFlowNode() and
+        not v.getDefinition().strictlyDominates(l.getAFlowNode())
+    )
+select use,
+    use.getId() + " may have a different value in Python 3, as the $@ will not be in scope.", defn,
+    "list comprehension variable"
diff --git a/python/ql/src/Variables/Loop.qll b/python/ql/src/Variables/Loop.qll
index f3b105463ac1..cc998f70e940 100644
--- a/python/ql/src/Variables/Loop.qll
+++ b/python/ql/src/Variables/Loop.qll
@@ -1,17 +1,19 @@
 import python
 
-
 private predicate empty_sequence(Expr e) {
-    exists(SsaVariable var | var.getAUse().getNode() = e | empty_sequence(var.getDefinition().getNode())) or
-    e instanceof List and not exists(e.(List).getAnElt()) or
-    e instanceof Tuple and not exists(e.(Tuple).getAnElt()) or
+    exists(SsaVariable var | var.getAUse().getNode() = e |
+        empty_sequence(var.getDefinition().getNode())
+    )
+    or
+    e instanceof List and not exists(e.(List).getAnElt())
+    or
+    e instanceof Tuple and not exists(e.(Tuple).getAnElt())
+    or
     e.(StrConst).getText().length() = 0
 }
 
 /* This has the potential for refinement, but we err on the side of fewer false positives for now. */
-private predicate probably_non_empty_sequence(Expr e) {
-    not empty_sequence(e)
-}
+private predicate probably_non_empty_sequence(Expr e) { not empty_sequence(e) }
 
 /** A loop which probably defines v */
 private Stmt loop_probably_defines(Variable v) {
@@ -24,8 +26,7 @@ private Stmt loop_probably_defines(Variable v) {
 
 /** Holds if the variable used by `use` is probably defined in a loop */
 predicate probably_defined_in_loop(Name use) {
-    exists(Stmt loop |
-        loop = loop_probably_defines(use.getVariable()) |
+    exists(Stmt loop | loop = loop_probably_defines(use.getVariable()) |
         loop.getAFlowNode().strictlyReaches(use.getAFlowNode())
     )
 }
diff --git a/python/ql/src/Variables/LoopVariableCapture.ql b/python/ql/src/Variables/LoopVariableCapture.ql
index 69c9ab96c007..4159a08c0ef1 100644
--- a/python/ql/src/Variables/LoopVariableCapture.ql
+++ b/python/ql/src/Variables/LoopVariableCapture.ql
@@ -30,11 +30,12 @@ predicate capturing_looping_construct(CallableExpr capturing, AstNode loop, Vari
 }
 
 predicate escaping_capturing_looping_construct(CallableExpr capturing, AstNode loop, Variable var) {
-    capturing_looping_construct(capturing, loop, var) 
-    and
+    capturing_looping_construct(capturing, loop, var) and
     // Escapes if used out side of for loop or is a lambda in a comprehension
     (
-        exists(Expr e, For forloop | forloop = loop and e.pointsTo(_, _, capturing) | not forloop.contains(e))
+        exists(Expr e, For forloop | forloop = loop and e.pointsTo(_, _, capturing) |
+            not forloop.contains(e)
+        )
         or
         loop.(Comp).getElt() = capturing
         or
diff --git a/python/ql/src/Variables/MultiplyDefined.ql b/python/ql/src/Variables/MultiplyDefined.ql
index 14c95acb1fde..cae39729b9bf 100644
--- a/python/ql/src/Variables/MultiplyDefined.ql
+++ b/python/ql/src/Variables/MultiplyDefined.ql
@@ -15,13 +15,17 @@ import python
 import Definition
 
 predicate multiply_defined(AstNode asgn1, AstNode asgn2, Variable v) {
-    /* Must be redefined on all possible paths in the CFG corresponding to the original source.
+    /*
+     * Must be redefined on all possible paths in the CFG corresponding to the original source.
      * For example, splitting may create a path where `def` is unconditionally redefined, even though
-     * it is not in the original source. */
+     * it is not in the original source.
+     */
+
     forex(Definition def, Definition redef |
         def.getVariable() = v and
         def = asgn1.getAFlowNode() and
-        redef = asgn2.getAFlowNode() |
+        redef = asgn2.getAFlowNode()
+    |
         def.isUnused() and
         def.getARedef() = redef and
         def.isRelevant()
@@ -29,15 +33,21 @@ predicate multiply_defined(AstNode asgn1, AstNode asgn2, Variable v) {
 }
 
 predicate simple_literal(Expr e) {
-    e.(Num).getN() = "0" or
-    e instanceof NameConstant or
-    e instanceof List and not exists(e.(List).getAnElt()) or
-    e instanceof Tuple and not exists(e.(Tuple).getAnElt()) or
-    e instanceof Dict and not exists(e.(Dict).getAKey()) or
+    e.(Num).getN() = "0"
+    or
+    e instanceof NameConstant
+    or
+    e instanceof List and not exists(e.(List).getAnElt())
+    or
+    e instanceof Tuple and not exists(e.(Tuple).getAnElt())
+    or
+    e instanceof Dict and not exists(e.(Dict).getAKey())
+    or
     e.(StrConst).getText() = ""
 }
 
-/** A multiple definition is 'uninteresting' if it sets a variable to a 
+/**
+ * A multiple definition is 'uninteresting' if it sets a variable to a
  * simple literal before reassigning it.
  * x = None
  * if cond:
@@ -46,16 +56,14 @@ predicate simple_literal(Expr e) {
  *     x = value2
  */
 predicate uninteresting_definition(AstNode asgn1) {
-    exists(AssignStmt a | 
-        a.getATarget() = asgn1 |
-        simple_literal(a.getValue())
-    )
+    exists(AssignStmt a | a.getATarget() = asgn1 | simple_literal(a.getValue()))
 }
 
-
 from AstNode asgn1, AstNode asgn2, Variable v
 where
     multiply_defined(asgn1, asgn2, v) and
     forall(Name el | el = asgn1.getParentNode().(Tuple).getAnElt() | multiply_defined(el, _, _)) and
     not uninteresting_definition(asgn1)
-select asgn1, "This assignment to '" + v.getId() + "' is unnecessary as it is redefined $@ before this value is used.", asgn2 as t, "here"
+select asgn1,
+    "This assignment to '" + v.getId() +
+        "' is unnecessary as it is redefined $@ before this value is used.", asgn2 as t, "here"
diff --git a/python/ql/src/Variables/ShadowBuiltin.ql b/python/ql/src/Variables/ShadowBuiltin.ql
index facda4d56331..cd45132cd8a6 100644
--- a/python/ql/src/Variables/ShadowBuiltin.ql
+++ b/python/ql/src/Variables/ShadowBuiltin.ql
@@ -44,9 +44,10 @@ predicate white_list(string name) {
 }
 
 predicate shadows(Name d, string name, Scope scope, int line) {
-    exists(LocalVariable l | d.defines(l) and
-           l.getId() = name and
-           exists(Builtin::builtin(l.getId()))
+    exists(LocalVariable l |
+        d.defines(l) and
+        l.getId() = name and
+        exists(Builtin::builtin(l.getId()))
     ) and
     scope instanceof Function and
     d.getScope() = scope and
@@ -58,7 +59,8 @@ predicate shadows(Name d, string name, Scope scope, int line) {
 predicate first_shadowing_definition(Name d, string name) {
     exists(int first, Scope scope |
         shadows(d, name, scope, first) and
-        first = min(int line | shadows(_, name, scope, line)))
+        first = min(int line | shadows(_, name, scope, line))
+    )
 }
 
 from Name d, string name
diff --git a/python/ql/src/Variables/ShadowGlobal.ql b/python/ql/src/Variables/ShadowGlobal.ql
index 24e09d6e3db1..97d92f4c7b4d 100644
--- a/python/ql/src/Variables/ShadowGlobal.ql
+++ b/python/ql/src/Variables/ShadowGlobal.ql
@@ -17,27 +17,24 @@ import Shadowing
 import semmle.python.types.Builtins
 
 predicate shadows(Name d, GlobalVariable g, Scope scope, int line) {
-    exists(LocalVariable l | d.defines(l) and l.getId() = g.getId() and
-        scope instanceof Function and g.getScope() = scope.getScope() and
+    exists(LocalVariable l |
+        d.defines(l) and
+        l.getId() = g.getId() and
+        scope instanceof Function and
+        g.getScope() = scope.getScope() and
         not exists(Import il, Import ig, Name gd | il.contains(d) and gd.defines(g) and ig.contains(gd)) and
         not exists(Assign a | a.getATarget() = d and a.getValue() = g.getAnAccess())
     ) and
     not exists(Builtin::builtin(g.getId())) and
     d.getScope() = scope and
     d.getLocation().getStartLine() = line and
-    exists(Name defn | defn.defines(g) |
-        not exists(If i | i.isNameEqMain() |
-            i.contains(defn)
-        )
-    ) and
+    exists(Name defn | defn.defines(g) | not exists(If i | i.isNameEqMain() | i.contains(defn))) and
     not optimizing_parameter(d)
 }
 
 /* pytest dynamically populates its namespace so, we cannot look directly for the pytest.fixture function */
 AttrNode pytest_fixture_attr() {
-    exists(ModuleValue pytest |
-        result.getObject("fixture").pointsTo(pytest)
-    )
+    exists(ModuleValue pytest | result.getObject("fixture").pointsTo(pytest))
 }
 
 Value pytest_fixture() {
@@ -45,23 +42,30 @@ Value pytest_fixture() {
         call.getFunction() = pytest_fixture_attr()
         or
         call.getFunction().(CallNode).getFunction() = pytest_fixture_attr()
-        |
+    |
         call.pointsTo(result)
     )
 }
 
 /* pytest fixtures require that the parameter name is also a global */
 predicate assigned_pytest_fixture(GlobalVariable v) {
-    exists(NameNode def | def.defines(v) and def.(DefinitionNode).getValue().pointsTo(pytest_fixture()))
+    exists(NameNode def |
+        def.defines(v) and def.(DefinitionNode).getValue().pointsTo(pytest_fixture())
+    )
 }
 
 predicate first_shadowing_definition(Name d, GlobalVariable g) {
     exists(int first, Scope scope |
         shadows(d, g, scope, first) and
-        first = min(int line | shadows(_, g, scope, line)))
+        first = min(int line | shadows(_, g, scope, line))
+    )
 }
 
 from Name d, GlobalVariable g, Name def
-where first_shadowing_definition(d, g) and not exists(Name n | n.deletes(g)) and
-      def.defines(g) and not assigned_pytest_fixture(g) and not g.getId() = "_"
+where
+    first_shadowing_definition(d, g) and
+    not exists(Name n | n.deletes(g)) and
+    def.defines(g) and
+    not assigned_pytest_fixture(g) and
+    not g.getId() = "_"
 select d, "Local variable '" + g.getId() + "' shadows a global variable defined $@.", def, "here"
diff --git a/python/ql/src/Variables/Shadowing.qll b/python/ql/src/Variables/Shadowing.qll
index 5c56f5cacc27..a3dabb0a4e68 100644
--- a/python/ql/src/Variables/Shadowing.qll
+++ b/python/ql/src/Variables/Shadowing.qll
@@ -1,13 +1,14 @@
 import python
 
-/* Parameters with defaults that are used as an optimization. 
+/*
+ * Parameters with defaults that are used as an optimization.
  * E.g. def f(x, len=len): ...
  * (In general, this kind of optimization is not recommended.)
  */
+
 predicate optimizing_parameter(Parameter p) {
-    exists(string name, Name glob |
-        p.getDefault() = glob |
-        glob.getId() = name and 
+    exists(string name, Name glob | p.getDefault() = glob |
+        glob.getId() = name and
         p.asName().getId() = name
     )
 }
diff --git a/python/ql/src/Variables/SuspiciousUnusedLoopIterationVariable.ql b/python/ql/src/Variables/SuspiciousUnusedLoopIterationVariable.ql
index 5d76bd869911..91dd3bd11018 100644
--- a/python/ql/src/Variables/SuspiciousUnusedLoopIterationVariable.ql
+++ b/python/ql/src/Variables/SuspiciousUnusedLoopIterationVariable.ql
@@ -26,18 +26,13 @@ predicate is_increment(Stmt s) {
     )
 }
 
-predicate counting_loop(For f) {
-    is_increment(f.getAStmt())
-}
+predicate counting_loop(For f) { is_increment(f.getAStmt()) }
 
-predicate empty_loop(For f) {
-    not exists(f.getStmt(1)) and f.getStmt(0) instanceof Pass
-}
+predicate empty_loop(For f) { not exists(f.getStmt(1)) and f.getStmt(0) instanceof Pass }
 
 predicate one_item_only(For f) {
     not exists(Continue c | f.contains(c)) and
-    exists(Stmt s |
-        s = f.getBody().getLastItem() |
+    exists(Stmt s | s = f.getBody().getLastItem() |
         s instanceof Return
         or
         s instanceof Break
@@ -55,16 +50,13 @@ predicate points_to_call_to_range(ControlFlowNode f) {
     )
     or
     /* In case points-to fails due to 'from six.moves import range' or similar. */
-    exists(string range |
-        f.getNode().(Call).getFunc().(Name).getId() = range |
+    exists(string range | f.getNode().(Call).getFunc().(Name).getId() = range |
         range = "range" or range = "xrange"
     )
     or
     /* Handle list(range(...)) and list(list(range(...))) */
-    (
-        f.(CallNode).pointsTo().getClass() = ClassValue::list() and
-        points_to_call_to_range(f.(CallNode).getArg(0))
-    )
+    f.(CallNode).pointsTo().getClass() = ClassValue::list() and
+    points_to_call_to_range(f.(CallNode).getArg(0))
 }
 
 /** Whether n is a use of a variable that is a not effectively a constant. */
@@ -74,52 +66,50 @@ predicate use_of_non_constant(Name n) {
         /* use is local */
         not n.getScope() instanceof Module and
         /* variable is not global */
-        not var.getScope() instanceof Module |
+        not var.getScope() instanceof Module
+    |
         /* Defined more than once (dynamically) */
-        strictcount(Name def | def.defines(var)) > 1 or
-        exists(For f, Name def | f.contains(def) and def.defines(var)) or
+        strictcount(Name def | def.defines(var)) > 1
+        or
+        exists(For f, Name def | f.contains(def) and def.defines(var))
+        or
         exists(While w, Name def | w.contains(def) and def.defines(var))
     )
 }
 
-/** Whether loop body is implicitly repeating something N times.
+/**
+ * Whether loop body is implicitly repeating something N times.
  * E.g. queue.add(None)
  */
 predicate implicit_repeat(For f) {
     not exists(f.getStmt(1)) and
-    exists(ImmutableLiteral imm |
-        f.getStmt(0).contains(imm)
-    ) and
+    exists(ImmutableLiteral imm | f.getStmt(0).contains(imm)) and
     not exists(Name n | f.getBody().contains(n) and use_of_non_constant(n))
 }
 
-/** Get the CFG node for the iterable relating to the for-statement `f` in a comprehension.
+/**
+ * Get the CFG node for the iterable relating to the for-statement `f` in a comprehension.
  * The for-statement `f` is the artificial for-statement in a comprehension
  * and the result is the iterable in that comprehension.
  * E.g. gets `x` from `{ y for y in x }`.
  */
 ControlFlowNode get_comp_iterable(For f) {
-    exists(Comp c |
-        c.getFunction().getStmt(0) = f |
-        c.getAFlowNode().getAPredecessor() = result
-    )
+    exists(Comp c | c.getFunction().getStmt(0) = f | c.getAFlowNode().getAPredecessor() = result)
 }
 
 from For f, Variable v, string msg
-
-where f.getTarget() = v.getAnAccess() and
-      not f.getAStmt().contains(v.getAnAccess()) and
-      not points_to_call_to_range(f.getIter().getAFlowNode()) and
-      not points_to_call_to_range(get_comp_iterable(f)) and
-      not name_acceptable_for_unused_variable(v) and
-      not f.getScope().getName() = "genexpr" and
-      not empty_loop(f) and
-      not one_item_only(f) and
-      not counting_loop(f) and
-      not implicit_repeat(f) and
-      if exists(Name del | del.deletes(v) and f.getAStmt().contains(del)) then
-          msg = "' is deleted, but not used, in the loop body."
-      else
-          msg = "' is not used in the loop body."
-
+where
+    f.getTarget() = v.getAnAccess() and
+    not f.getAStmt().contains(v.getAnAccess()) and
+    not points_to_call_to_range(f.getIter().getAFlowNode()) and
+    not points_to_call_to_range(get_comp_iterable(f)) and
+    not name_acceptable_for_unused_variable(v) and
+    not f.getScope().getName() = "genexpr" and
+    not empty_loop(f) and
+    not one_item_only(f) and
+    not counting_loop(f) and
+    not implicit_repeat(f) and
+    if exists(Name del | del.deletes(v) and f.getAStmt().contains(del))
+    then msg = "' is deleted, but not used, in the loop body."
+    else msg = "' is not used in the loop body."
 select f, "For loop variable '" + v.getId() + msg
diff --git a/python/ql/src/Variables/Undefined.qll b/python/ql/src/Variables/Undefined.qll
index 1495180da25b..78e6438ec692 100644
--- a/python/ql/src/Variables/Undefined.qll
+++ b/python/ql/src/Variables/Undefined.qll
@@ -4,9 +4,7 @@ import semmle.python.security.TaintTracking
 
 /** Marker for "uninitialized". */
 class Uninitialized extends TaintKind {
-
     Uninitialized() { this = "undefined" }
-
 }
 
 private predicate loop_entry_variables(EssaVariable pred, EssaVariable succ) {
@@ -26,7 +24,8 @@ private predicate loop_entry_edge(BasicBlock pred, BasicBlock loop) {
     )
 }
 
-/** Since any use of a local will raise if it is uninitialized, then
+/**
+ * Since any use of a local will raise if it is uninitialized, then
  * any use dominated by another use of the same variable must be defined, or is unreachable.
  */
 private predicate first_use(NameNode u, EssaVariable v) {
@@ -37,19 +36,17 @@ private predicate first_use(NameNode u, EssaVariable v) {
     )
 }
 
-/* Holds if `call` is a call of the form obj.method_name(...) and
+/**
+ * Holds if `call` is a call of the form obj.method_name(...) and
  * there is a function called `method_name` that can exit the program.
  */
 private predicate maybe_call_to_exiting_function(CallNode call) {
-    exists(FunctionValue exits, string name |
-        exits.neverReturns() and exits.getName() = name
-        |
+    exists(FunctionValue exits, string name | exits.neverReturns() and exits.getName() = name |
         call.getFunction().(NameNode).getId() = name or
         call.getFunction().(AttrNode).getName() = name
     )
 }
 
-
 predicate exitFunctionGuardedEdge(EssaVariable pred, EssaVariable succ) {
     exists(CallNode exit_call |
         succ.(PhiFunction).getInput(exit_call.getBasicBlock()) = pred and
@@ -58,17 +55,15 @@ predicate exitFunctionGuardedEdge(EssaVariable pred, EssaVariable succ) {
 }
 
 class UninitializedConfig extends TaintTracking::Configuration {
-
-    UninitializedConfig() {
-        this = "Unitialized local config"
-    }
+    UninitializedConfig() { this = "Unitialized local config" }
 
     override predicate isSource(DataFlow::Node source, TaintKind kind) {
         kind instanceof Uninitialized and
         exists(EssaVariable var |
             source.asVariable() = var and
             var.getSourceVariable() instanceof FastLocalVariable and
-            not var.getSourceVariable().(Variable).escapes() |
+            not var.getSourceVariable().(Variable).escapes()
+        |
             var instanceof ScopeEntryDefinition
             or
             var instanceof DeletionDefinition
@@ -110,12 +105,13 @@ class UninitializedConfig extends TaintTracking::Configuration {
     }
 
     override predicate isBarrierEdge(DataFlow::Node src, DataFlow::Node dest) {
-        /* If we are guaranteed to iterate over a loop at least once, then we can prune any edges that
+        /*
+         * If we are guaranteed to iterate over a loop at least once, then we can prune any edges that
          * don't pass through the body.
          */
+
         loop_entry_variables(src.asVariable(), dest.asVariable())
         or
         exitFunctionGuardedEdge(src.asVariable(), dest.asVariable())
     }
-
 }
diff --git a/python/ql/src/Variables/UndefinedGlobal.ql b/python/ql/src/Variables/UndefinedGlobal.ql
index 88bfc55d9525..0c45725cd236 100644
--- a/python/ql/src/Variables/UndefinedGlobal.ql
+++ b/python/ql/src/Variables/UndefinedGlobal.ql
@@ -17,12 +17,13 @@ import semmle.python.pointsto.PointsTo
 
 predicate guarded_against_name_error(Name u) {
     exists(Try t | t.getBody().getAnItem().contains(u) |
-                   t.getAHandler().getType().(Name).getId() = "NameError"
-          )
+        t.getAHandler().getType().(Name).getId() = "NameError"
+    )
     or
     exists(ConditionBlock guard, BasicBlock controlled, Call globals |
         guard.getLastNode().getNode().contains(globals) or
-        guard.getLastNode().getNode() = globals |
+        guard.getLastNode().getNode() = globals
+    |
         globals.getFunc().(Name).getId() = "globals" and
         guard.controls(controlled, _) and
         controlled.contains(u.getAFlowNode())
@@ -30,66 +31,59 @@ predicate guarded_against_name_error(Name u) {
 }
 
 predicate contains_unknown_import_star(Module m) {
-     exists(ImportStar imp | imp.getScope() = m |
-        exists(ModuleValue imported | imported.importedAs(imp.getImportedModuleName()) and imported.isAbsent())
-        or
+    exists(ImportStar imp | imp.getScope() = m |
         exists(ModuleValue imported |
-            imported.importedAs(imp.getImportedModuleName()) |
+            imported.importedAs(imp.getImportedModuleName()) and imported.isAbsent()
+        )
+        or
+        exists(ModuleValue imported | imported.importedAs(imp.getImportedModuleName()) |
             not imported.hasCompleteExportInfo()
         )
     )
 }
 
 predicate undefined_use_in_function(Name u) {
-    exists(Function f | u.getScope().getScope*() = f and
+    exists(Function f |
+        u.getScope().getScope*() = f and
         // Either function is a method or inner function or it is live at the end of the module scope
-        (not f.getScope() = u.getEnclosingModule() or u.getEnclosingModule().(ImportTimeScope).definesName(f.getName()))
-        and
+        (
+            not f.getScope() = u.getEnclosingModule() or
+            u.getEnclosingModule().(ImportTimeScope).definesName(f.getName())
+        ) and
         // There is a use, but not a definition of this global variable in the function or enclosing scope
         exists(GlobalVariable v | u.uses(v) |
             not exists(Assign a, Scope defnScope |
-                a.getATarget() = v.getAnAccess() and a.getScope() = defnScope |
-                defnScope = f or
+                a.getATarget() = v.getAnAccess() and a.getScope() = defnScope
+            |
+                defnScope = f
+                or
                 // Exclude modules as that case is handled more precisely below.
-                (defnScope = f.getScope().getScope*() and not defnScope instanceof Module)
+                defnScope = f.getScope().getScope*() and not defnScope instanceof Module
             )
         )
-    )
-    and
-    not ((ImportTimeScope)u.getEnclosingModule()).definesName(u.getId())
-    and
-    not exists(ModuleValue m | m.getScope() = u.getEnclosingModule() | m.hasAttribute(u.getId()))
-    and
-    not globallyDefinedName(u.getId())
-    and
-    not exists(SsaVariable var | var.getAUse().getNode() = u and not var.maybeUndefined())
-    and
-    not guarded_against_name_error(u)
-    and
+    ) and
+    not u.getEnclosingModule().(ImportTimeScope).definesName(u.getId()) and
+    not exists(ModuleValue m | m.getScope() = u.getEnclosingModule() | m.hasAttribute(u.getId())) and
+    not globallyDefinedName(u.getId()) and
+    not exists(SsaVariable var | var.getAUse().getNode() = u and not var.maybeUndefined()) and
+    not guarded_against_name_error(u) and
     not (u.getEnclosingModule().isPackageInit() and u.getId() = "__path__")
 }
 
 predicate undefined_use_in_class_or_module(Name u) {
-    exists(GlobalVariable v | u.uses(v))
-    and
-    not exists(Function f | u.getScope().getScope*() = f)
-    and
-    exists(SsaVariable var | var.getAUse().getNode() = u | var.maybeUndefined())
-    and
-    not guarded_against_name_error(u)
-    and
-    not exists(ModuleValue m | m.getScope() = u.getEnclosingModule() | m.hasAttribute(u.getId()))
-    and
-    not (u.getEnclosingModule().isPackageInit() and u.getId() = "__path__")
-    and
+    exists(GlobalVariable v | u.uses(v)) and
+    not exists(Function f | u.getScope().getScope*() = f) and
+    exists(SsaVariable var | var.getAUse().getNode() = u | var.maybeUndefined()) and
+    not guarded_against_name_error(u) and
+    not exists(ModuleValue m | m.getScope() = u.getEnclosingModule() | m.hasAttribute(u.getId())) and
+    not (u.getEnclosingModule().isPackageInit() and u.getId() = "__path__") and
     not globallyDefinedName(u.getId())
 }
 
 predicate use_of_exec(Module m) {
     exists(Exec exec | exec.getScope() = m)
     or
-    exists(CallNode call, FunctionValue exec |
-        exec.getACall() = call and call.getScope() = m |
+    exists(CallNode call, FunctionValue exec | exec.getACall() = call and call.getScope() = m |
         exec = Value::named("exec") or
         exec = Value::named("execfile")
     )
@@ -106,7 +100,7 @@ predicate undefined_use(Name u) {
     not use_of_exec(u.getEnclosingModule()) and
     not exists(u.getVariable().getAStore()) and
     not u.pointsTo(_) and
-    not  probably_defined_in_loop(u)
+    not probably_defined_in_loop(u)
 }
 
 private predicate first_use_in_a_block(Name use) {
@@ -117,8 +111,7 @@ private predicate first_use_in_a_block(Name use) {
 
 predicate first_undefined_use(Name use) {
     undefined_use(use) and
-    exists(GlobalVariable v |
-        v.getALoad() = use |
+    exists(GlobalVariable v | v.getALoad() = use |
         first_use_in_a_block(use) and
         not exists(ControlFlowNode other |
             other.getNode() = v.getALoad() and
@@ -129,4 +122,4 @@ predicate first_undefined_use(Name use) {
 
 from Name u
 where first_undefined_use(u)
-select u, "This use of global variable '" + u.getId()  + "' may be undefined."
+select u, "This use of global variable '" + u.getId() + "' may be undefined."
diff --git a/python/ql/src/Variables/UndefinedPlaceHolder.ql b/python/ql/src/Variables/UndefinedPlaceHolder.ql
index 0e6e5f07078f..1ec4f85749ff 100644
--- a/python/ql/src/Variables/UndefinedPlaceHolder.ql
+++ b/python/ql/src/Variables/UndefinedPlaceHolder.ql
@@ -14,7 +14,6 @@ import python
 import Variables.MonkeyPatched
 
 /* Local variable part */
-
 predicate initialized_as_local(PlaceHolder use) {
     exists(SsaVariable l, Function f | f = use.getScope() and l.getAUse() = use.getAFlowNode() |
         l.getVariable() instanceof LocalVariable and
@@ -23,34 +22,25 @@ predicate initialized_as_local(PlaceHolder use) {
 }
 
 /* Not a template member */
-
-Class enclosing_class(PlaceHolder use) {
-    result.getAMethod() = use.getScope()
-}
+Class enclosing_class(PlaceHolder use) { result.getAMethod() = use.getScope() }
 
 predicate template_attribute(PlaceHolder use) {
-    exists(ImportTimeScope cls |
-        cls = enclosing_class(use) |
-        cls.definesName(use.getId())
-    )
+    exists(ImportTimeScope cls | cls = enclosing_class(use) | cls.definesName(use.getId()))
 }
 
 /* Global Stuff */
-
 predicate not_a_global(PlaceHolder use) {
-    not exists(PythonModuleObject mo | mo.hasAttribute(use.getId()) and mo.getModule() = use.getEnclosingModule())
-    and
+    not exists(PythonModuleObject mo |
+        mo.hasAttribute(use.getId()) and mo.getModule() = use.getEnclosingModule()
+    ) and
     not globallyDefinedName(use.getId()) and
     not monkey_patched_builtin(use.getId()) and
     not globallyDefinedName(use.getId())
 }
 
 from PlaceHolder p
-where 
-not initialized_as_local(p) and
-not template_attribute(p) and
-not_a_global(p)
-select p, "This use of place-holder variable '" + p.getId()  + "' may be undefined"
-
-
-
+where
+    not initialized_as_local(p) and
+    not template_attribute(p) and
+    not_a_global(p)
+select p, "This use of place-holder variable '" + p.getId() + "' may be undefined"
diff --git a/python/ql/src/Variables/UninitializedLocal.ql b/python/ql/src/Variables/UninitializedLocal.ql
index 0e52f7567ea4..343036be152f 100644
--- a/python/ql/src/Variables/UninitializedLocal.ql
+++ b/python/ql/src/Variables/UninitializedLocal.ql
@@ -15,13 +15,10 @@ import Undefined
 import semmle.python.pointsto.PointsTo
 
 predicate uninitialized_local(NameNode use) {
-    exists(FastLocalVariable local |
-        use.uses(local) or use.deletes(local) |
-        not local.escapes()
-    )
-    and
+    exists(FastLocalVariable local | use.uses(local) or use.deletes(local) | not local.escapes()) and
     (
-        any(Uninitialized uninit).taints(use) and PointsToInternal::reachableBlock(use.getBasicBlock(), _)
+        any(Uninitialized uninit).taints(use) and
+        PointsToInternal::reachableBlock(use.getBasicBlock(), _)
         or
         not exists(EssaVariable var | var.getASourceUse() = use)
     )
@@ -34,7 +31,6 @@ predicate explicitly_guarded(NameNode u) {
     )
 }
 
-
 from NameNode u
 where uninitialized_local(u) and not explicitly_guarded(u)
 select u.getNode(), "Local variable '" + u.getId() + "' may be used before it is initialized."
diff --git a/python/ql/src/Variables/UnusedModuleVariable.ql b/python/ql/src/Variables/UnusedModuleVariable.ql
index 888b9546ce1a..2d873908ba55 100644
--- a/python/ql/src/Variables/UnusedModuleVariable.ql
+++ b/python/ql/src/Variables/UnusedModuleVariable.ql
@@ -14,36 +14,39 @@
 import python
 import Definition
 
-/** Whether the module contains an __all__ definition, 
- * but it is more complex than a simple list of strings */
+/**
+ * Whether the module contains an __all__ definition,
+ * but it is more complex than a simple list of strings
+ */
 predicate complex_all(Module m) {
-    exists(Assign a, GlobalVariable all | 
-        a.defines(all) and a.getScope() = m and all.getId() = "__all__" |
-        not a.getValue() instanceof List or
-        exists(Expr e |
-            e = a.getValue().(List).getAnElt() |
-            not e instanceof StrConst
-        )
+    exists(Assign a, GlobalVariable all |
+        a.defines(all) and a.getScope() = m and all.getId() = "__all__"
+    |
+        not a.getValue() instanceof List
+        or
+        exists(Expr e | e = a.getValue().(List).getAnElt() | not e instanceof StrConst)
     )
     or
     exists(Call c, GlobalVariable all |
         c.getFunc().(Attribute).getObject() = all.getALoad() and
-        c.getScope() = m and all.getId() = "__all__"
+        c.getScope() = m and
+        all.getId() = "__all__"
     )
 }
 
 predicate unused_global(Name unused, GlobalVariable v) {
     not exists(ImportingStmt is | is.contains(unused)) and
-    forex(DefinitionNode defn |
-        defn.getNode() = unused |
+    forex(DefinitionNode defn | defn.getNode() = unused |
         not defn.getValue().getNode() instanceof FunctionExpr and
         not defn.getValue().getNode() instanceof ClassExpr and
-        not exists(Name u | 
+        not exists(Name u |
             // A use of the variable
-            u.uses(v) |
+            u.uses(v)
+        |
             // That is reachable from this definition, directly
             defn.strictlyReaches(u.getAFlowNode())
-            or // indirectly
+            or
+            // indirectly
             defn.getBasicBlock().reachesExit() and u.getScope() != unused.getScope()
         ) and
         not unused.getEnclosingModule().getAnExport() = v.getId() and
@@ -56,7 +59,8 @@ predicate unused_global(Name unused, GlobalVariable v) {
 }
 
 from Name unused, GlobalVariable v
-where unused_global(unused, v) and
-// If unused is part of a tuple, count it as unused if all elements of that tuple are unused.
-forall(Name el | el = unused.getParentNode().(Tuple).getAnElt() | unused_global(el, _))
+where
+    unused_global(unused, v) and
+    // If unused is part of a tuple, count it as unused if all elements of that tuple are unused.
+    forall(Name el | el = unused.getParentNode().(Tuple).getAnElt() | unused_global(el, _))
 select unused, "The global variable '" + v.getId() + "' is not used."
diff --git a/python/ql/src/Variables/UnusedParameter.ql b/python/ql/src/Variables/UnusedParameter.ql
index b8b20d717dde..af3b6c6b3cd8 100644
--- a/python/ql/src/Variables/UnusedParameter.ql
+++ b/python/ql/src/Variables/UnusedParameter.ql
@@ -12,13 +12,14 @@
 import python
 import Definition
 
-
 predicate unused_parameter(FunctionValue f, LocalVariable v) {
-  v.isParameter() and
-  v.getScope() = f.getScope() and
-  not name_acceptable_for_unused_variable(v) and
-  not exists(NameNode u | u.uses(v)) and
-  not exists(Name inner, LocalVariable iv | inner.uses(iv) and iv.getId() = v.getId() and inner.getScope().getScope() = v.getScope())
+    v.isParameter() and
+    v.getScope() = f.getScope() and
+    not name_acceptable_for_unused_variable(v) and
+    not exists(NameNode u | u.uses(v)) and
+    not exists(Name inner, LocalVariable iv |
+        inner.uses(iv) and iv.getId() = v.getId() and inner.getScope().getScope() = v.getScope()
+    )
 }
 
 predicate is_abstract(FunctionValue func) {
@@ -26,6 +27,10 @@ predicate is_abstract(FunctionValue func) {
 }
 
 from PythonFunctionValue f, LocalVariable v
-where v.getId() != "self" and unused_parameter(f, v) and not f.isOverridingMethod() and not f.isOverriddenMethod() and
-not is_abstract(f)
+where
+    v.getId() != "self" and
+    unused_parameter(f, v) and
+    not f.isOverridingMethod() and
+    not f.isOverriddenMethod() and
+    not is_abstract(f)
 select f, "The parameter '" + v.getId() + "' is never used."

From 92e272cc03821a0aa69dbf62d56a0f8b86736a41 Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Fri, 20 Dec 2019 15:58:51 +0100
Subject: [PATCH 15/17] Python: Address comments for modernising Variables/

---
 python/ql/src/Variables/LoopVariableCapture.ql      |  4 ++--
 python/ql/src/Variables/ShadowBuiltin.ql            |  3 +--
 python/ql/src/Variables/ShadowGlobal.ql             | 13 ++++++-------
 .../SuspiciousUnusedLoopIterationVariable.ql        |  5 ++---
 python/ql/src/Variables/UndefinedGlobal.ql          |  4 ----
 python/ql/src/semmle/python/objects/Modules.qll     | 12 +++++-------
 6 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/python/ql/src/Variables/LoopVariableCapture.ql b/python/ql/src/Variables/LoopVariableCapture.ql
index 4159a08c0ef1..37ac541d6226 100644
--- a/python/ql/src/Variables/LoopVariableCapture.ql
+++ b/python/ql/src/Variables/LoopVariableCapture.ql
@@ -33,8 +33,8 @@ predicate escaping_capturing_looping_construct(CallableExpr capturing, AstNode l
     capturing_looping_construct(capturing, loop, var) and
     // Escapes if used out side of for loop or is a lambda in a comprehension
     (
-        exists(Expr e, For forloop | forloop = loop and e.pointsTo(_, _, capturing) |
-            not forloop.contains(e)
+        exists(Expr e | e.pointsTo(_, _, capturing) |
+            not loop.(For).contains(e)
         )
         or
         loop.(Comp).getElt() = capturing
diff --git a/python/ql/src/Variables/ShadowBuiltin.ql b/python/ql/src/Variables/ShadowBuiltin.ql
index cd45132cd8a6..781d1c54ad31 100644
--- a/python/ql/src/Variables/ShadowBuiltin.ql
+++ b/python/ql/src/Variables/ShadowBuiltin.ql
@@ -43,13 +43,12 @@ predicate white_list(string name) {
     name = "_"
 }
 
-predicate shadows(Name d, string name, Scope scope, int line) {
+predicate shadows(Name d, string name, Function scope, int line) {
     exists(LocalVariable l |
         d.defines(l) and
         l.getId() = name and
         exists(Builtin::builtin(l.getId()))
     ) and
-    scope instanceof Function and
     d.getScope() = scope and
     d.getLocation().getStartLine() = line and
     not white_list(name) and
diff --git a/python/ql/src/Variables/ShadowGlobal.ql b/python/ql/src/Variables/ShadowGlobal.ql
index 97d92f4c7b4d..c7140c75d834 100644
--- a/python/ql/src/Variables/ShadowGlobal.ql
+++ b/python/ql/src/Variables/ShadowGlobal.ql
@@ -16,17 +16,16 @@ import python
 import Shadowing
 import semmle.python.types.Builtins
 
-predicate shadows(Name d, GlobalVariable g, Scope scope, int line) {
+predicate shadows(Name d, GlobalVariable g, Function scope, int line) {
+    g.getScope() = scope.getScope() and
+    d.getScope() = scope and
     exists(LocalVariable l |
         d.defines(l) and
-        l.getId() = g.getId() and
-        scope instanceof Function and
-        g.getScope() = scope.getScope() and
-        not exists(Import il, Import ig, Name gd | il.contains(d) and gd.defines(g) and ig.contains(gd)) and
-        not exists(Assign a | a.getATarget() = d and a.getValue() = g.getAnAccess())
+        l.getId() = g.getId()
     ) and
+    not exists(Import il, Import ig, Name gd | il.contains(d) and gd.defines(g) and ig.contains(gd)) and
+    not exists(Assign a | a.getATarget() = d and a.getValue() = g.getAnAccess()) and
     not exists(Builtin::builtin(g.getId())) and
-    d.getScope() = scope and
     d.getLocation().getStartLine() = line and
     exists(Name defn | defn.defines(g) | not exists(If i | i.isNameEqMain() | i.contains(defn))) and
     not optimizing_parameter(d)
diff --git a/python/ql/src/Variables/SuspiciousUnusedLoopIterationVariable.ql b/python/ql/src/Variables/SuspiciousUnusedLoopIterationVariable.ql
index 91dd3bd11018..77d1e5ca67c7 100644
--- a/python/ql/src/Variables/SuspiciousUnusedLoopIterationVariable.ql
+++ b/python/ql/src/Variables/SuspiciousUnusedLoopIterationVariable.ql
@@ -41,12 +41,11 @@ predicate one_item_only(For f) {
 
 predicate points_to_call_to_range(ControlFlowNode f) {
     /* (x)range is a function in Py2 and a class in Py3, so we must treat it as a plain object */
-    exists(Value range, Value call |
+    exists(Value range |
         range = Value::named("range") or
         range = Value::named("xrange")
     |
-        f.pointsTo(call) and
-        call.getACall().getFunction().pointsTo(range)
+        f = range.getACall()
     )
     or
     /* In case points-to fails due to 'from six.moves import range' or similar. */
diff --git a/python/ql/src/Variables/UndefinedGlobal.ql b/python/ql/src/Variables/UndefinedGlobal.ql
index 0c45725cd236..c21c048a3e1e 100644
--- a/python/ql/src/Variables/UndefinedGlobal.ql
+++ b/python/ql/src/Variables/UndefinedGlobal.ql
@@ -32,10 +32,6 @@ predicate guarded_against_name_error(Name u) {
 
 predicate contains_unknown_import_star(Module m) {
     exists(ImportStar imp | imp.getScope() = m |
-        exists(ModuleValue imported |
-            imported.importedAs(imp.getImportedModuleName()) and imported.isAbsent()
-        )
-        or
         exists(ModuleValue imported | imported.importedAs(imp.getImportedModuleName()) |
             not imported.hasCompleteExportInfo()
         )
diff --git a/python/ql/src/semmle/python/objects/Modules.qll b/python/ql/src/semmle/python/objects/Modules.qll
index f3d4333177cd..fdc748ae0e9d 100644
--- a/python/ql/src/semmle/python/objects/Modules.qll
+++ b/python/ql/src/semmle/python/objects/Modules.qll
@@ -314,13 +314,11 @@ class PythonModuleObjectInternal extends ModuleObjectInternal, TPythonModule {
     }
 
     override predicate hasCompleteExportInfo() {
-        exists(Module m |
-            m = this.getSourceModule() |
-            not exists(Call modify, Attribute attr, GlobalVariable all |
-                modify.getScope() = m and modify.getFunc() = attr and
-                all.getId() = "__all__" |
-                attr.getObject().(Name).uses(all)
-            )
+        not exists(Call modify, Attribute attr, GlobalVariable all |
+            modify.getScope() = this.getSourceModule() and
+            modify.getFunc() = attr and
+            all.getId() = "__all__" |
+            attr.getObject().(Name).uses(all)
         )
     }
 

From 9b0b0c338f62282e6e4ef6dd28b00d8eb0feb51b Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Mon, 6 Jan 2020 10:55:37 +0100
Subject: [PATCH 16/17] Python: Cleanup overrides tests

---
 .../overrides/FunctionOverrides.expected      | 18 ++++++-------
 .../ql/test/library-tests/overrides/test.py   | 27 ++++++++++---------
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/python/ql/test/library-tests/overrides/FunctionOverrides.expected b/python/ql/test/library-tests/overrides/FunctionOverrides.expected
index 78ba5023f38f..33d5447621a3 100644
--- a/python/ql/test/library-tests/overrides/FunctionOverrides.expected
+++ b/python/ql/test/library-tests/overrides/FunctionOverrides.expected
@@ -1,9 +1,9 @@
-| test.py:3:5:3:20 | Function Wat.upper | overriding | not overridden |
-| test.py:6:1:6:26 | Function outside_func | not overriding | overridden |
-| test.py:11:11:11:29 | Function Base.lambda | not overriding | overridden |
-| test.py:13:24:13:36 | Function Base.lambda | not overriding | not overridden |
-| test.py:17:5:17:21 | Function Base.normal | not overriding | overridden |
-| test.py:28:5:28:21 | Function Sub2.foo | overriding | overridden |
-| test.py:31:5:31:24 | Function Sub2.tricky | overriding | not overridden |
-| test.py:36:5:36:21 | Function Sub2Sub.foo | overriding | not overridden |
-| test.py:39:5:39:18 | Function Sub2Sub.baz | overriding | not overridden |
+| test.py:3:5:3:20 | Function MyStr.upper | overriding | not overridden |
+| test.py:11:1:11:26 | Function outside_func | not overriding | overridden |
+| test.py:16:11:16:29 | Function Base.lambda | not overriding | overridden |
+| test.py:18:24:18:36 | Function Base.lambda | not overriding | not overridden |
+| test.py:22:5:22:21 | Function Base.normal | not overriding | overridden |
+| test.py:33:5:33:21 | Function Bar.foo | overriding | overridden |
+| test.py:36:5:36:24 | Function Bar.tricky | overriding | not overridden |
+| test.py:41:5:41:21 | Function SpecialBar.foo | overriding | not overridden |
+| test.py:44:5:44:18 | Function SpecialBar.baz | overriding | not overridden |
diff --git a/python/ql/test/library-tests/overrides/test.py b/python/ql/test/library-tests/overrides/test.py
index b88e2ceef077..0392fbc8ef0e 100644
--- a/python/ql/test/library-tests/overrides/test.py
+++ b/python/ql/test/library-tests/overrides/test.py
@@ -1,8 +1,13 @@
-class Wat(str):
+class MyStr(str):
 
     def upper(self):
         return self.lower()
 
+
+s = MyStr('asdf')
+print(s.upper(), len(s))
+
+
 def outside_func(self, x):
     print(x)
 
@@ -19,35 +24,31 @@ def normal(self):
 
     tricky = outside_func
 
-class Sub(Base):
+class Foo(Base):
 
     normal = False
 
-class Sub2(Base):
+class Bar(Base):
 
     def foo(self, y):
         return y * 100
 
     def tricky(self, x):
-        print('nice!', x)
+        print('tricky!', x)
 
-class Sub2Sub(Sub2):
+class SpecialBar(Bar):
 
     def foo(self, z):
         return z / 123
 
     def baz(self):
-        print('python is a bit crazy sometimes')
+        print('baz')
 
 
-ws = Wat('asdf')
-print(ws.upper(), len(ws))
-
 b = Base()
 print(b.foo(1))
 print(b.bar(10))
 
-
-ss = SubSub()
-print(ss.foo(1))
-ss.baz()
+sb = SpecialBar()
+print(sb.foo(1))
+sb.baz()

From 5d01cb7c28da7a7e05f23657146ace0b37150808 Mon Sep 17 00:00:00 2001
From: Rasmus Wriedt Larsen <rasmuswl@github.com>
Date: Mon, 6 Jan 2020 13:30:37 +0100
Subject: [PATCH 17/17] Python: Fix bad QL-rewriting

Original code:

exists(Expr e, For forloop | forloop = loop and e.pointsTo(_, _, capturing) |
    not loop.contains(e)
)

The new version will preserve the same semantics. The problem with the first
rewrite was that `not loop.(For).somethingMore` would hold for any AstNode that
was not a For
---
 python/ql/src/Variables/LoopVariableCapture.ql | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/python/ql/src/Variables/LoopVariableCapture.ql b/python/ql/src/Variables/LoopVariableCapture.ql
index 37ac541d6226..a88abb8b5f5b 100644
--- a/python/ql/src/Variables/LoopVariableCapture.ql
+++ b/python/ql/src/Variables/LoopVariableCapture.ql
@@ -33,9 +33,8 @@ predicate escaping_capturing_looping_construct(CallableExpr capturing, AstNode l
     capturing_looping_construct(capturing, loop, var) and
     // Escapes if used out side of for loop or is a lambda in a comprehension
     (
-        exists(Expr e | e.pointsTo(_, _, capturing) |
-            not loop.(For).contains(e)
-        )
+        loop instanceof For and
+        exists(Expr e | e.pointsTo(_, _, capturing) | not loop.contains(e))
         or
         loop.(Comp).getElt() = capturing
         or