wip2

Author: hvitved

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

@@ -165,8 +165,8 @@ private module Input implements LocalNameBindingInputSig<Location> {
     AstNode getElse() { none() }
   }
 
-  class ShadowingDecl extends AstNode {
-    ShadowingDecl() { none() }
+  class SiblingShadowingDecl extends AstNode {
+    SiblingShadowingDecl() { none() }
 
     AstNode getLhs() { none() }
 
@@ -175,31 +175,37 @@ private module Input implements LocalNameBindingInputSig<Location> {
     AstNode getElse() { none() }
   }
 
+  private Scope::Range getTopScope(Scope::Range scope) {
+    isTopScope(result) and result = scope
+    or
+    not isTopScope(scope) and
+    result = getTopScope(scopeOf(scope))
+  }
+
   predicate declInScope(AstNode definingNode, string name, AstNode scope) {
     scopeDefinesParameterVariable(scope, name, definingNode, _)
     or
-    definingNode =
-      min(Ruby::AstNode other, int startline, int startcolumn |
-        scopeAssigns(scope, name, other) and
-        other.getLocation().hasLocationInfo(_, startline, startcolumn, _, _)
-      |
-        other order by startline, startcolumn
-      ) and
+    /*
+     * Variables are not declared explicitly in Ruby, so we consider the _first_ assignment to
+     * be the declaration:
+     *
+     * ```rb
+     * a = 1 # declares `a`
+     * a = 2 # does not declare `a`
+     * 1.times do | x | # declares `x`
+     *   a = 2 # does not declare `a`
+     * end
+     * ```
+     */
+
+    scopeAssigns(scope, name, definingNode) and
     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())
+    not exists(AstNode topScope, AstNode prev |
+      topScope = getTopScope(scope) and
+      prev.getLocation().strictlyBefore(definingNode.getLocation())
+    |
+      scopeDefinesParameterVariable(topScope, name, prev, _) or
+      scopeAssigns(topScope, name, prev)
     )
   }
 
@@ -427,7 +433,7 @@ private module Cached {
       or
       /*
        * In the example below, `a` is declared in the scope of `M`, but only the
-       * second mention of `a` is an actual access.
+       * second mention of `a` is an actual access:
        *
        * ```rb
        * module M

rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll

@@ -188,8 +188,8 @@ module Impl {
       AstNode getElse() { result = this.(IfExpr).getElse() }
     }
 
-    class ShadowingDecl extends AstNode {
-      ShadowingDecl() { this instanceof LetStmt or this instanceof LetExpr }
+    class SiblingShadowingDecl extends AstNode {
+      SiblingShadowingDecl() { this instanceof LetStmt or this instanceof LetExpr }
 
       AstNode getLhs() {
         result = this.(LetStmt).getPat()
@@ -217,7 +217,7 @@ module Impl {
         |
           scope = any(MatchArm arm | pat = arm.getPat())
           or
-          scope = any(Input::ShadowingDecl let | pat = let.getLhs())
+          scope = any(Input::SiblingShadowingDecl let | pat = let.getLhs())
           or
           scope = any(ForExpr fe | pat = fe.getPat()).getLoopBody()
           or

shared/namebinding/codeql/namebinding/LocalNameBinding.qll

@@ -1,6 +1,6 @@
 /**
  * Provides a library for resolving local names based on syntactic scopes, including
- * handling of shadowing declarations.
+ * handling of shadowing sibling declarations.
  */
 overlay[local?]
 module;
@@ -25,7 +25,12 @@ signature module LocalNameBindingInputSig<LocationSig Location> {
     Location getLocation();
   }
 
-  /** Gets the child of AST node `n` at the specified index. */
+  /**
+   * Gets the child of AST node `n` at the specified index.
+   *
+   * The order of the children is only relevant for determining nearest preceding
+   * shadowing sibling declarations.
+   */
   AstNode getChild(AstNode n, int index);
 
   /**
@@ -54,18 +59,25 @@ signature module LocalNameBindingInputSig<LocationSig Location> {
   }
 
   /**
-   * A shadowing declaration where any local declarations in the left-hand side
-   * are in scope _after_ the declaration and shadow any declarations with the
-   * same name preceding it.
+   * A declaration where all local declarations in the left-hand side are in
+   * scope _after_ the declaration, and where any sibling declarations with
+   * the same name and syntactic scope preceding it are shadowed.
    *
    * Example:
+   *
    * ```rust
-   * let x = 1;
-   * let x = x + 1;     // this declaration of `x` shadows the previous one, but the `x` in the right-hand side still refers to the first declaration
-   * println!("{}", x); // this access of `x` refers to the second declaration
+   * fn f() {
+   *   let x = 1;
+   *   // this declaration of `x` shadows the previous one (in the syntactic scope
+   *   // being the body of `f`), but the `x` in the right-hand side still refers
+   *   // to the first declaration
+   *   let x = x + 1;
+   *   // this access of `x` refers to the second declaration
+   *   println!("{}", x);
+   * }
    * ```
    */
-  class ShadowingDecl extends AstNode {
+  class SiblingShadowingDecl extends AstNode {
     /** Gets the left-hand side of this declaration. */
     AstNode getLhs();
 
@@ -91,7 +103,7 @@ signature module LocalNameBindingInputSig<LocationSig Location> {
    * the syntactic scope `scope`.
    *
    * Note that declarations with a `definingNode` in the left-hand side of a
-   * shadowing declaration `decl` should use `scope = decl`.
+   * shadowing sibling declaration `decl` should use `scope = decl`.
    */
   predicate declInScope(AstNode definingNode, string name, AstNode scope);
 
@@ -127,7 +139,7 @@ signature module LocalNameBindingInputSig<LocationSig Location> {
 
 /**
  * Provides logic for resolving local names based on syntactic scopes, including
- * handling of shadowing declarations.
+ * handling of shadowing sibling declarations.
  */
 module LocalNameBinding<LocationSig Location, LocalNameBindingInputSig<Location> Input> {
   private import Input
@@ -160,8 +172,8 @@ module LocalNameBinding<LocationSig Location, LocalNameBindingInputSig<Location>
   /**
    * An adjusted version of `getChild` from the `Input` module where in conditionals like
    * `if cond body`, instead of letting `body` be a child of `if`, we make it the last
-   * child of `cond`. This ensures that shadowing declarations inside `cond` are properly
-   * handled inside `body`.
+   * child of `cond`. This ensures that shadowing sibling declarations inside `cond` are
+   * properly handled inside `body`.
    *
    * Example:
    *
@@ -171,8 +183,8 @@ module LocalNameBinding<LocationSig Location, LocalNameBindingInputSig<Location>
    * }
    * ```
    *
-   * We also move any `else` branch _before_ the condition to ensure that shadowing declarations
-   * inside the condition are not in scope.
+   * We also move any `else` branch _before_ the condition to ensure that shadowing sibling
+   * declarations inside the condition are not in scope.
    */
   private AstNode getChildAdj(AstNode parent, int index) {
     result = getChild(parent, index) and
@@ -198,29 +210,31 @@ module LocalNameBinding<LocationSig Location, LocalNameBindingInputSig<Location>
 
     int getRank(C parent, Ranked child) {
       child = getChildAdj(parent, result) and
-      getChildAdj(parent, _) instanceof ShadowingDecl
+      getChildAdj(parent, _) instanceof SiblingShadowingDecl
     }
   }
 
   private predicate getRankedChild = DenseRank1<DenseRankInput>::denseRank/2;
 
   /**
    * Holds if `n` is the `i`th child of `parent`, but should instead be considered
-   * a child of a shadowing declaration `decl` when resolving accesses.
+   * a child of a shadowing sibling declaration `decl` when resolving accesses.
    *
-   * This is the case when `decl` is the nearest shadowing declaration preceding `n`
-   * amongst all the children of `parent`.
+   * This is the case when `decl` is the nearest shadowing sibling declaration
+   * preceding `n` amongst all the children of `parent`.
    *
    * Note that `decl` may itself also have to be nested under another shadowing
-   * declaration.
+   * sibling declaration.
    */
-  private predicate shouldBeShadowingDeclChild(AstNode parent, ShadowingDecl decl, int i, AstNode n) {
+  private predicate shouldBeShadowingDeclChild(
+    AstNode parent, SiblingShadowingDecl decl, int i, AstNode n
+  ) {
     n = getRankedChild(parent, i) and
     (
       decl = getRankedChild(parent, i - 1)
       or
       shouldBeShadowingDeclChild(parent, decl, i - 1,
-        any(AstNode prev | not prev instanceof ShadowingDecl))
+        any(AstNode prev | not prev instanceof SiblingShadowingDecl))
     )
   }
 
@@ -269,12 +283,12 @@ module LocalNameBinding<LocationSig Location, LocalNameBindingInputSig<Location>
    */
   private AstNode getParentForScoping(AstNode n) {
     not shouldBeShadowingDeclChild(_, _, _, n) and
-    not exists(ShadowingDecl decl | n = [decl.getRhs(), decl.getElse()]) and
+    not exists(SiblingShadowingDecl decl | n = [decl.getRhs(), decl.getElse()]) and
     n = getChildAdj(result, _)
     or
     shouldBeShadowingDeclChild(_, result, _, n)
     or
-    exists(ShadowingDecl decl |
+    exists(SiblingShadowingDecl decl |
       result = getParentForScoping(decl) and
       n = [decl.getRhs(), decl.getElse()]
     )