Merge pull request #97 from github/hvitved/var-access-categorization

Categorize variable accesses into reads and (implicit or explicit) writes

ql/src/codeql_ruby/ast/Variable.qll

@@ -73,9 +73,56 @@ class VariableAccess extends Expr {
   /** Gets the variable this identifier refers to. */
   Variable getVariable() { result = range.getVariable() }
 
+  /**
+   * Holds if this access is a write access belonging to the explicit
+   * assignment `assignment`. For example, in
+   *
+   * ```rb
+   * a, b = foo
+   * ```
+   *
+   * both `a` and `b` are write accesses belonging to the same assignment.
+   */
+  predicate isExplicitWrite(AstNode assignment) { explicitWriteAccess(this, assignment) }
+
+  /**
+   * Holds if this access is a write access belonging to an implicit assignment.
+   * For example, in
+   *
+   * ```rb
+   * def m elements
+   *   for e in elements do
+   *     puts e
+   *   end
+   * end
+   * ```
+   *
+   * the access to `elements` in the parameter list is an implicit assignment,
+   * as is the first access to `e`.
+   */
+  predicate isImplicitWrite() { implicitWriteAccess(this) }
+
   final override string toString() { result = this.getVariable().getName() }
 }
 
+/** An access to a variable where the value is updated. */
+class VariableWriteAccess extends VariableAccess {
+  VariableWriteAccess() {
+    this.isExplicitWrite(_) or
+    this.isImplicitWrite()
+  }
+}
+
+/** An access to a variable where the value is read. */
+class VariableReadAccess extends VariableAccess {
+  VariableReadAccess() {
+    not this instanceof VariableWriteAccess
+    or
+    // `x` in `x += y` is considered both a read and a write
+    this = any(AssignOperation a).getLhs()
+  }
+}
+
 /** An access to a local variable. */
 class LocalVariableAccess extends VariableAccess, @token_identifier {
   final override LocalVariableAccess::Range range;
@@ -88,6 +135,12 @@ class LocalVariableAccess extends VariableAccess, @token_identifier {
   }
 }
 
+/** An access to a local variable where the value is updated. */
+class LocalVariableWriteAccess extends LocalVariableAccess, VariableWriteAccess { }
+
+/** An access to a local variable where the value is read. */
+class LocalVariableReadAccess extends LocalVariableAccess, VariableReadAccess { }
+
 /** An access to a local variable. */
 class GlobalVariableAccess extends VariableAccess, @token_global_variable {
   final override GlobalVariableAccess::Range range;
@@ -97,3 +150,9 @@ class GlobalVariableAccess extends VariableAccess, @token_global_variable {
 
   final override string getAPrimaryQlClass() { result = "GlobalVariableAccess" }
 }
+
+/** An access to a global variable where the value is updated. */
+class GlobalVariableWriteAccess extends GlobalVariableAccess, VariableWriteAccess { }
+
+/** An access to a global variable where the value is read. */
+class GlobalVariableReadAccess extends GlobalVariableAccess, VariableReadAccess { }

ql/src/codeql_ruby/ast/internal/Pattern.qll

@@ -3,43 +3,49 @@ private import TreeSitter
 private import codeql_ruby.ast.internal.Variable
 private import codeql.Locations
 
-private predicate tuplePatternNode(Generated::AstNode n, boolean parameter) {
-  n instanceof Generated::DestructuredParameter and
-  parameter = true
-  or
-  n instanceof Generated::DestructuredLeftAssignment and
-  parameter = false
-  or
-  n instanceof Generated::LeftAssignmentList and
-  parameter = false
-  or
-  tuplePatternNode(n.getParent(), parameter)
-}
-
-private predicate patternNode(Generated::AstNode n, boolean parameter) {
-  tuplePatternNode(n, parameter)
-  or
-  parameter = true and
-  n = any(Callable c).getAParameter()
-  or
-  parameter = false and
-  n in [
-      any(Generated::Assignment assign).getLeft(),
-      any(Generated::OperatorAssignment assign).getLeft(),
-      any(Generated::ExceptionVariable exceptionVariable).getChild(),
-      any(Generated::For for).getPattern()
-    ]
-}
-
 /**
- * Holds if a variable is assigned at `i`. `parameter` indicates whether it is
- * an implicit parameter assignment.
+ * Holds if `n` is in the left-hand-side of an explicit assignment `assignment`.
  */
-predicate assignment(Generated::Identifier i, boolean parameter) { patternNode(i, parameter) }
+predicate explicitAssignmentNode(Generated::AstNode n, Generated::AstNode assignment) {
+  n = assignment.(Generated::Assignment).getLeft()
+  or
+  n = assignment.(Generated::OperatorAssignment).getLeft()
+  or
+  exists(Generated::AstNode parent |
+    parent = n.getParent() and
+    explicitAssignmentNode(parent, assignment)
+  |
+    parent instanceof Generated::DestructuredLeftAssignment
+    or
+    parent instanceof Generated::LeftAssignmentList
+  )
+}
+
+/** Holds if `n` is inside an implicit assignment. */
+predicate implicitAssignmentNode(Generated::AstNode n) {
+  n = any(Generated::ExceptionVariable ev).getChild()
+  or
+  n = any(Generated::For for).getPattern()
+  or
+  implicitAssignmentNode(n.getParent())
+}
+
+/** Holds if `n` is inside a parameter. */
+predicate implicitParameterAssignmentNode(Generated::AstNode n, Callable c) {
+  n = c.getAParameter()
+  or
+  implicitParameterAssignmentNode(n.getParent().(Generated::DestructuredParameter), c)
+}
 
 module Pattern {
   abstract class Range extends AstNode {
-    Range() { patternNode(this, _) }
+    Range() {
+      explicitAssignmentNode(this, _)
+      or
+      implicitAssignmentNode(this)
+      or
+      implicitParameterAssignmentNode(this, _)
+    }
 
     abstract Variable getAVariable();
   }

ql/src/codeql_ruby/ast/internal/Variable.qll

@@ -17,8 +17,7 @@ private VariableScope enclosingScope(Generated::AstNode node) {
 private predicate parameterAssignment(
   CallableScope::Range scope, string name, Generated::Identifier i
 ) {
-  assignment(i, true) and
-  scope = enclosingScope(i) and
+  implicitParameterAssignmentNode(i, scope.getScopeElement()) and
   name = i.getValue()
 }
 
@@ -50,7 +49,7 @@ private predicate scopeDefinesParameterVariable(
 
 /** Holds if `name` is assigned in `scope` at `i`. */
 private predicate scopeAssigns(VariableScope scope, string name, Generated::Identifier i) {
-  assignment(i, false) and
+  (explicitAssignmentNode(i, _) or implicitAssignmentNode(i)) and
   name = i.getValue() and
   scope = enclosingScope(i)
 }
@@ -143,6 +142,22 @@ private module Cached {
       )
     )
   }
+
+  private class Access extends Generated::Token {
+    Access() { access(this, _) or this instanceof Generated::GlobalVariable }
+  }
+
+  cached
+  predicate explicitWriteAccess(Access access, Generated::AstNode assignment) {
+    explicitAssignmentNode(access, assignment)
+  }
+
+  cached
+  predicate implicitWriteAccess(Access access) {
+    implicitAssignmentNode(access)
+    or
+    scopeDefinesParameterVariable(_, _, access)
+  }
 }
 
 import Cached