Python: modernise variables queries

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,55 +90,45 @@ 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())
     }
-
 }
 
 /**
  * Check whether basic block `a` reaches basic block `b` without an intervening
  * 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() }
 }

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"
-
-

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."
+select g, "Declaring '" + g.getAName() + "' as global at module-level is redundant."

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"

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())
     )
 }

python/ql/src/Variables/LoopVariableCapture.ql

@@ -30,11 +30,11 @@ 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.refersTo(_, _, capturing) | not forloop.contains(e))
+        loop instanceof For and
+        exists(Expr e | e.pointsTo(_, _, capturing) | not loop.contains(e))
         or
         loop.(Comp).getElt() = capturing
         or

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())
     )
 }

python/ql/src/Variables/MultiplyDefined.ql

@@ -15,29 +15,39 @@ 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()
     )
 }
 
 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"

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
@@ -42,10 +43,11 @@ predicate white_list(string name) {
     name = "_"
 }
 
-predicate shadows(Name d, string name, Scope scope, int line) {
-    exists(LocalVariable l | d.defines(l) and scope instanceof Function and
-           l.getId() = name and
-           exists(Object::builtin(l.getId()))
+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
     d.getScope() = scope and
     d.getLocation().getStartLine() = line and
@@ -56,7 +58,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