wip

Author: hvitved

ruby/ql/lib/codeql/ruby/ast/internal/Variable.qll

@@ -95,10 +95,11 @@ predicate scopeDefinesParameterVariable(
   // In case of overlapping parameter names (e.g. `_`), only the first
   // parameter will give rise to a variable
   i =
-    min(Ruby::Identifier other |
-      parameterAssignment(scope, name, other, _)
+    min(Ruby::Identifier other, int startline, int startcolumn |
+      parameterAssignment(scope, name, other, _) and
+      other.getLocation().hasLocationInfo(_, startline, startcolumn, _, _)
     |
-      other order by other.getLocation().getStartLine(), other.getLocation().getStartColumn()
+      other order by startline, startcolumn
     ) and
   parameterAssignment(scope, name, _, pos)
   or
@@ -177,17 +178,29 @@ private module Input implements LocalNameBindingInputSig<Location> {
   predicate declInScope(AstNode definingNode, string name, AstNode scope) {
     scopeDefinesParameterVariable(scope, name, definingNode, _)
     or
-    declInScopeIsUncertain(definingNode, name, scope)
-  }
-
-  predicate declInScopeIsUncertain(AstNode definingNode, string name, AstNode scope) {
     definingNode =
-      min(Ruby::AstNode other |
-        scopeAssigns(scope, name, other)
+      min(Ruby::AstNode other, int startline, int startcolumn |
+        scopeAssigns(scope, name, other) and
+        other.getLocation().hasLocationInfo(_, startline, startcolumn, _, _)
       |
-        other order by other.getLocation().getStartLine(), other.getLocation().getStartColumn()
+        other order by startline, startcolumn
       ) and
-    not scopeDefinesParameterVariable(scope, name, _, _)
+    not scopeDefinesParameterVariable(scope, name, _, _) and
+    not exists(AstNode top, Ruby::AstNode outer |
+      /*
+       * ```rb
+       * a = 1 # declares `a`
+       * 1.times do | x | # declares `x`
+       *   a = 2 # does not declare `a`
+       * end
+       * ```
+       */
+
+      not Input::isTopScope(scope) and
+      top = scopeOf(scope) and
+      scopeAssigns(top, name, outer) and
+      outer.getLocation().strictlyBefore(definingNode.getLocation())
+    )
   }
 
   predicate implicitDeclInScope(string name, AstNode scope) {
@@ -223,20 +236,10 @@ private module Input implements LocalNameBindingInputSig<Location> {
     n instanceof Ruby::Self and
     name = "self"
   }
-
-  bindingset[access, definingNode]
-  predicate isValidAccess(AstNode access, AstNode definingNode) {
-    not access.getLocation().strictlyBefore(definingNode.getLocation())
-  }
 }
 
 private import LocalNameBinding<Location, Input>
 
-pragma[nomagic]
-predicate access(Ruby::AstNode access, VariableReal variable) {
-  exists(Local l | variable = TLocalVariableReal(l) | access = l.getAnAccess())
-}
-
 cached
 private module Cached {
   cached
@@ -247,18 +250,20 @@ private module Cached {
     } or
     TClassVariable(Scope::Range scope, string name, Ruby::AstNode decl) {
       decl =
-        min(Ruby::ClassVariable other |
-          classVariableAccess(other, name, scope)
+        min(Ruby::ClassVariable other, int startline, int startcolumn |
+          classVariableAccess(other, name, scope) and
+          other.getLocation().hasLocationInfo(_, startline, startcolumn, _, _)
         |
-          other order by other.getLocation().getStartLine(), other.getLocation().getStartColumn()
+          other order by startline, startcolumn
         )
     } or
     TInstanceVariable(Scope::Range scope, string name, boolean instance, Ruby::AstNode decl) {
       decl =
-        min(Ruby::InstanceVariable other |
-          instanceVariableAccess(other, name, scope, instance)
+        min(Ruby::InstanceVariable other, int startline, int startcolumn |
+          instanceVariableAccess(other, name, scope, instance) and
+          other.getLocation().hasLocationInfo(_, startline, startcolumn, _, _)
         |
-          other order by other.getLocation().getStartLine(), other.getLocation().getStartColumn()
+          other order by startline, startcolumn
         )
     } or
     TLocalVariableReal(Local l) or
@@ -412,6 +417,31 @@ private module Cached {
     i = any(Ruby::ExpressionReferencePattern x).getValue()
   }
 
+  cached
+  predicate access(Ruby::AstNode access, VariableReal variable) {
+    exists(Local l |
+      variable = TLocalVariableReal(l) and
+      access = l.getAnAccess()
+    |
+      l instanceof ImplicitLocal
+      or
+      /*
+       * In the example below, `a` is declared in the scope of `M`, but only the
+       * second mention of `a` is an actual access.
+       *
+       * ```rb
+       * module M
+       *   puts a # calls method `a`
+       *   a = 1  # declares `a`
+       *   puts a # accesses variable `a`
+       * end
+       * ```
+       */
+
+      not access.getLocation().strictlyBefore(l.getDefiningNode().getLocation())
+    )
+  }
+
   private class Access extends Ruby::Token {
     Access() {
       access(this, _) or

shared/namebinding/codeql/namebinding/LocalNameBinding.qll

@@ -95,28 +95,6 @@ signature module LocalNameBindingInputSig<LocationSig Location> {
    */
   predicate declInScope(AstNode definingNode, string name, AstNode scope);
 
-  /**
-   * Holds if it is uncertain whether the reference to local declaration named
-   * `name` at `definingNode` in scope `scope` is a declaration or an access.
-   *
-   * For example, in Ruby variables are not explicitly declared, so we consider
-   * the first mention of a variable to be its declaration:
-   *
-   * ```rb
-   * a = 1 # declares `a`
-   * 1.times do | x | # declares `x`
-   *   # marking this as an uncertain declaration means it is correctly
-   *   # identified as a reassignment instead of a new declaration
-   *   a = 2
-   * end
-   * ```
-   *
-   * This predicate must be a subset of `declInScope`.
-   */
-  default predicate declInScopeIsUncertain(AstNode definingNode, string name, AstNode scope) {
-    none()
-  }
-
   /**
    * Holds if a local declaration named `name` is implicitly in scope in the given `scope`.
    */
@@ -145,26 +123,6 @@ signature module LocalNameBindingInputSig<LocationSig Location> {
    * full control of scope resolution for for specific types of references.
    */
   default predicate lookupStartsAt(AstNode n, AstNode scope) { none() }
-
-  /**
-   * Holds if `access` is valid access of the local declared at `definingNode`.
-   *
-   * This allows for post-filtering of inferred variable accesses, for example
-   * in Ruby:
-   *
-   * ```rb
-   * module M
-   *   puts a # calls method `a`
-   *   a = 1  # declares `a`
-   *   puts a # accesses variable `a`
-   * end
-   * ```
-   *
-   * the scope of `a` is inside `M`, but only the second mention of `a` is an
-   * actual access.
-   */
-  bindingset[access, definingNode]
-  default predicate isValidAccess(AstNode access, AstNode definingNode) { any() }
 }
 
 /**
@@ -341,12 +299,6 @@ module LocalNameBinding<LocationSig Location, LocalNameBindingInputSig<Location>
       not lookupStartsAt(n, _) and
       lookup = getEnclosingScope(n)
     )
-    or
-    exists(Scope scope |
-      declInScopeIsUncertain(n, name, scope) and
-      not isTopScope(scope) and
-      lookup = getEnclosingScope(scope)
-    )
   }
 
   pragma[nomagic]
@@ -356,11 +308,7 @@ module LocalNameBinding<LocationSig Location, LocalNameBindingInputSig<Location>
     or
     exists(Scope mid |
       lookupInScope(name, lookup, mid) and
-      not exists(AstNode definingNode |
-        declInScope(definingNode, name, mid) and
-        // allow to step through uncertain declarations
-        not declInScopeIsUncertain(definingNode, name, mid)
-      ) and
+      not declInScope(_, name, mid) and
       not implicitDeclInScope(name, mid) and
       not isTopScope(mid) and
       scope = getEnclosingScope(mid)
@@ -404,16 +352,7 @@ module LocalNameBinding<LocationSig Location, LocalNameBindingInputSig<Location>
     private string name;
     private AstNode scope;
 
-    cached
-    ExplicitLocal() {
-      CachedStage::ref() and
-      this = TExplicitLocal(definingNode, name, scope) and
-      // skip uncertain declarations that are in fact accesses
-      not exists(AstNode other |
-        accessStep(definingNode, TExplicitLocal(other, _, _)) and
-        other != definingNode
-      )
-    }
+    ExplicitLocal() { this = TExplicitLocal(definingNode, name, scope) }
 
     override AstNode getDefiningNode() { result = definingNode }
 
@@ -441,48 +380,27 @@ module LocalNameBinding<LocationSig Location, LocalNameBindingInputSig<Location>
   }
 
   pragma[nomagic]
-  private predicate resolveInScope(string name, Scope lookup, TLocal l) {
+  private predicate resolveInScope(string name, Scope lookup, Local l) {
     exists(Scope scope | lookupInScope(name, lookup, scope) |
       l = TExplicitLocal(_, name, scope) or
       l = TImplicitLocal(name, scope)
     )
   }
 
-  pragma[nomagic]
-  private predicate accessStep(AstNode access, TLocal l) {
+  cached
+  private predicate access(AstNode access, Local l) {
     CachedStage::ref() and
     exists(Scope lookup, string name |
       accessCandInLookupScope(access, name, lookup) and
       resolveInScope(name, lookup, l)
-    |
-      l instanceof TImplicitLocal
-      or
-      l = TExplicitLocal(any(AstNode definingNode | isValidAccess(access, definingNode)), _, _)
-    )
-  }
-
-  pragma[nomagic]
-  private predicate accessSteps(AstNode access, Local l) {
-    accessStep(access, l)
-    or
-    exists(AstNode mid, string name, AstNode scope |
-      accessSteps(mid, l) and
-      accessStep(access, TExplicitLocal(mid, name, scope)) and
-      declInScopeIsUncertain(mid, name, scope)
     )
   }
 
   /** A local access. */
   final class LocalAccess extends AstNodeFinal {
     private Local l;
 
-    cached
-    LocalAccess() {
-      CachedStage::ref() and
-      // `l` having type `Local` and not `TLocal` ensure that it is a proper declaration
-      accessSteps(this, l) and
-      accessCand(this, _)
-    }
+    LocalAccess() { access(this, l) }
 
     /** Gets the local entity being accessed. */
     Local getLocal() { result = l }