few join order fixes

erik-krogh · erik-krogh · commit 758a5d7a85e4 · 2022-03-30T22:54:00.000+02:00
diff --git a/python/ql/lib/semmle/python/AstExtended.qll b/python/ql/lib/semmle/python/AstExtended.qll
@@ -54,6 +54,7 @@ abstract class AstNode extends AstNode_ {
   }
 
   /** Whether this contains `inner` syntactically */
+  pragma[nomagic]
   predicate contains(AstNode inner) { this.getAChildNode+() = inner }
 
   pragma[noinline]
diff --git a/python/ql/lib/semmle/python/Function.qll b/python/ql/lib/semmle/python/Function.qll
@@ -167,17 +167,19 @@ class Function extends Function_, Scope, AstNode {
 
 /** A def statement. Note that FunctionDef extends Assign as a function definition binds the newly created function */
 class FunctionDef extends Assign {
+  FunctionExpr f;
+
   /* syntax: def name(...): ... */
   FunctionDef() {
     /* This is an artificial assignment the rhs of which is a (possibly decorated) FunctionExpr */
-    exists(FunctionExpr f | this.getValue() = f or this.getValue() = f.getADecoratorCall())
+    this.getValue() = f or this.getValue() = f.getADecoratorCall()
   }
 
   override string toString() { result = "FunctionDef" }
 
   /** Gets the function for this statement */
   Function getDefinedFunction() {
-    exists(FunctionExpr func | this.containsInScope(func) and result = func.getInnerScope())
+    result = f.getInnerScope() // XXX: This behaves very differently. But from inspecting the results of the previous version, that had every function in the same scope as the result.
   }
 
   override Stmt getLastStatement() { result = this.getDefinedFunction().getLastStatement() }
diff --git a/python/ql/src/Statements/UnnecessaryDelete.ql b/python/ql/src/Statements/UnnecessaryDelete.ql
@@ -14,14 +14,22 @@
 
 import python
 
+predicate isInsideLoop(AstNode node) {
+  node.getParentNode() instanceof While
+  or
+  node.getParentNode() instanceof For
+  or
+  exists(AstNode prev | isInsideLoop(prev) | node = prev.getAChildNode())
+}
+
 from Delete del, Expr e, Function f
 where
   f.getLastStatement() = del and
   e = del.getATarget() and
   f.containsInScope(e) and
   not e instanceof Subscript and
   not e instanceof Attribute and
-  not exists(Stmt s | s.(While).contains(del) or s.(For).contains(del)) and
+  not isInsideLoop(del) and
   // False positive: calling `sys.exc_info` within a function results in a
   //       reference cycle, and an explicit call to `del` helps break this cycle.
   not exists(FunctionValue ex |

Original file line number	Diff line number	Diff line change
`@@ -54,6 +54,7 @@ abstract class AstNode extends AstNode_ {`
`54`	`54`	`}`
`55`	`55`
`56`	`56`	/** Whether this contains `inner` syntactically */
	`57`	`+ pragma[nomagic]`
`57`	`58`	`predicate contains(AstNode inner) { this.getAChildNode+() = inner }`
`58`	`59`
`59`	`60`	`pragma[noinline]`