AST: improve AST for special parameters

ql/src/codeql_ruby/ast/Variable.qll

@@ -122,7 +122,7 @@ class VariableAccess extends Expr {
    *
    * both `a` and `b` are write accesses belonging to the same assignment.
    */
-  predicate isExplicitWrite(AstNode assignment) { explicitWriteAccess(this, assignment) }
+  predicate isExplicitWrite(AstNode assignment) { range.isExplicitWrite(assignment) }
 
   /**
    * Holds if this access is a write access belonging to an implicit assignment.
@@ -139,7 +139,7 @@ class VariableAccess extends Expr {
    * the access to `elements` in the parameter list is an implicit assignment,
    * as is the first access to `e`.
    */
-  predicate isImplicitWrite() { implicitWriteAccess(this) }
+  predicate isImplicitWrite() { range.isImplicitWrite() }
 }
 
 /** An access to a variable where the value is updated. */
@@ -161,13 +161,13 @@ class VariableReadAccess extends VariableAccess {
 }
 
 /** An access to a local variable. */
-class LocalVariableAccess extends VariableAccess, @token_identifier {
+class LocalVariableAccess extends VariableAccess, LocalVariableAccess::LocalVariableRange {
   final override LocalVariableAccess::Range range;
 
   final override LocalVariable getVariable() { result = range.getVariable() }
 
   final override string getAPrimaryQlClass() {
-    not this instanceof SimpleParameter and result = "LocalVariableAccess"
+    not this instanceof NamedParameter and result = "LocalVariableAccess"
   }
 
   /**

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

@@ -111,6 +111,14 @@ module KeywordParameter {
     final override string toString() { result = this.getName() }
 
     final override string getName() { result = generated.getName().getValue() }
+
+    final override Location getLocation() { result = generated.getName().getLocation() }
+
+    final override predicate child(string label, AstNode::Range child) {
+      NamedParameter::Range.super.child(label, child)
+      or
+      label = "getDefaultValue" and child = getDefaultValue()
+    }
   }
 }
 
@@ -127,6 +135,14 @@ module OptionalParameter {
     final override string toString() { result = this.getName() }
 
     final override string getName() { result = generated.getName().getValue() }
+
+    final override Location getLocation() { result = generated.getName().getLocation() }
+
+    final override predicate child(string label, AstNode::Range child) {
+      NamedParameter::Range.super.child(label, child)
+      or
+      label = "getDefaultValue" and child = getDefaultValue()
+    }
   }
 }
 

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

@@ -116,6 +116,23 @@ private predicate strictlyBefore(Location one, Location two) {
   one.getStartLine() = two.getStartLine() and one.getStartColumn() < two.getStartColumn()
 }
 
+private Generated::AstNode getNodeForIdentifier(Generated::Identifier id) {
+  exists(Generated::AstNode parent | parent = id.getParent() |
+    if
+      parent instanceof Generated::BlockParameter
+      or
+      parent instanceof Generated::SplatParameter
+      or
+      parent instanceof Generated::HashSplatParameter
+      or
+      parent instanceof Generated::KeywordParameter
+      or
+      parent instanceof Generated::OptionalParameter
+    then result = parent
+    else result = id
+  )
+}
+
 cached
 private module Cached {
   /** Gets the enclosing scope for `node`. */
@@ -518,7 +535,7 @@ module LocalVariable {
 
     final override VariableScope getDeclaringScope() { result = scope }
 
-    final VariableAccess getDefiningAccess() { result = i }
+    final VariableAccess getDefiningAccess() { result = getNodeForIdentifier(i) }
   }
 }
 
@@ -577,28 +594,46 @@ module VariableAccess {
   abstract class Range extends Expr::Range {
     abstract Variable getVariable();
 
-    final override string toString() { result = this.getVariable().getName() }
+    final predicate isExplicitWrite(AstNode assignment) {
+      exists(Generated::Identifier i | this = getNodeForIdentifier(i) |
+        explicitWriteAccess(i, assignment)
+      )
+      or
+      not this = getNodeForIdentifier(_) and explicitWriteAccess(this, assignment)
+    }
 
-    override predicate child(string label, AstNode::Range child) { none() }
+    final predicate isImplicitWrite() {
+      exists(Generated::Identifier i | this = getNodeForIdentifier(i) | implicitWriteAccess(i))
+      or
+      not this = getNodeForIdentifier(_) and implicitWriteAccess(this)
+    }
   }
 }
 
 module LocalVariableAccess {
-  class Range extends VariableAccess::Range, @token_identifier {
-    override Generated::Identifier generated;
+  class LocalVariableRange =
+    @token_identifier or @splat_parameter or @keyword_parameter or @optional_parameter or
+        @hash_splat_parameter or @block_parameter;
+
+  class Range extends VariableAccess::Range, LocalVariableRange {
     LocalVariable variable;
 
     Range() {
-      access(this, variable) and
-      (
-        explicitWriteAccess(this, _)
-        or
-        implicitWriteAccess(this)
-        or
-        vcall(this)
+      exists(Generated::Identifier id |
+        this = getNodeForIdentifier(id) and
+        access(id, variable) and
+        (
+          explicitWriteAccess(id, _)
+          or
+          implicitWriteAccess(id)
+          or
+          vcall(id)
+        )
       )
     }
 
+    override string toString() { result = generated.(Generated::Identifier).getValue() }
+
     final override LocalVariable getVariable() { result = variable }
   }
 }
@@ -610,6 +645,8 @@ module GlobalVariableAccess {
     Range() { this.(Generated::GlobalVariable).getValue() = variable.getName() }
 
     final override GlobalVariable getVariable() { result = variable }
+
+    override string toString() { result = generated.(Generated::GlobalVariable).getValue() }
   }
 }
 
@@ -620,6 +657,8 @@ module InstanceVariableAccess {
     Range() { instanceVariableAccess(this, variable) }
 
     final override InstanceVariable getVariable() { result = variable }
+
+    override string toString() { result = generated.(Generated::InstanceVariable).getValue() }
   }
 }
 
@@ -630,5 +669,7 @@ module ClassVariableAccess {
     Range() { classVariableAccess(this, variable) }
 
     final override ClassVariable getVariable() { result = variable }
+
+    override string toString() { result = generated.(Generated::ClassVariable).getValue() }
   }
 }

ql/src/codeql_ruby/controlflow/internal/Completion.qll

@@ -218,7 +218,7 @@ private predicate inMatchingContext(AstNode n) {
     w.getPattern(_).getChild() = n
   )
   or
-  n = any(Trees::DefaultValueParameterTree t | t.hasDefaultValue()).getIdentifier()
+  n = any(Trees::DefaultValueParameterTree t | t.hasDefaultValue())
 }
 
 /**

ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll

@@ -431,9 +431,7 @@ module Trees {
     }
   }
 
-  private class BlockParameterTree extends ComplexParameterTree, BlockParameter {
-    final override Identifier getIdentifier() { result = this.getName() }
-  }
+  private class BlockParameterTree extends LeafTree, BlockParameter { }
 
   private class CaseTree extends PreOrderTree, Case {
     final override predicate propagatesAbnormal(AstNode child) {
@@ -494,47 +492,32 @@ module Trees {
 
   private class ComplexTree extends LeafTree, Complex { }
 
-  abstract private class ComplexParameterTree extends ControlFlowTree {
-    abstract Identifier getIdentifier();
-
-    final override predicate propagatesAbnormal(AstNode child) { none() }
-
-    final override predicate first(AstNode first) { first(this.getIdentifier(), first) }
-
-    final override predicate last(AstNode last, Completion c) {
-      last(this.getIdentifier(), last, c)
-    }
-
-    final override predicate succ(AstNode pred, AstNode succ, Completion c) { none() }
-  }
-
   private class ConstantTree extends LeafTree, Constant { }
 
   /** A parameter that may have a default value. */
-  abstract class DefaultValueParameterTree extends ControlFlowTree {
+  abstract class DefaultValueParameterTree extends PreOrderTree {
     abstract AstNode getDefaultValue();
 
-    abstract Identifier getIdentifier();
-
     predicate hasDefaultValue() { exists(this.getDefaultValue()) }
 
     final override predicate propagatesAbnormal(AstNode child) { child = this.getDefaultValue() }
 
-    final override predicate first(AstNode first) { first(this.getIdentifier(), first) }
-
     final override predicate last(AstNode last, Completion c) {
-      last(this.getIdentifier(), last, c) and
-      c.(MatchingCompletion).getValue() = true
-      or
-      last(this.getDefaultValue(), last, c)
-      or
-      last(this.getIdentifier(), last, c) and
-      not this.hasDefaultValue() and
+      last(this.getDefaultValue(), last, c) and
       c instanceof NormalCompletion
+      or
+      last = this and
+      (
+        not this.hasDefaultValue() and
+        c instanceof SimpleCompletion
+        or
+        this.hasDefaultValue() and
+        c.(MatchingCompletion).getValue() = true
+      )
     }
 
     final override predicate succ(AstNode pred, AstNode succ, Completion c) {
-      last(this.getIdentifier(), pred, c) and
+      pred = this and
       first(this.getDefaultValue(), succ) and
       c.(MatchingCompletion).getValue() = false
     }
@@ -675,9 +658,7 @@ module Trees {
 
   private class GlobalVariableTree extends LeafTree, GlobalVariable { }
 
-  private class HashSplatParameterTree extends ComplexParameterTree, HashSplatParameter {
-    final override Identifier getIdentifier() { result = this.getName() }
-  }
+  private class HashSplatParameterTree extends LeafTree, HashSplatParameter { }
 
   private HeredocBody heredoc(HeredocBeginning start) {
     exists(int i, File f |
@@ -734,8 +715,6 @@ module Trees {
 
   private class KeywordParameterTree extends DefaultValueParameterTree, KeywordParameter {
     final override AstNode getDefaultValue() { result = this.getValue() }
-
-    final override Identifier getIdentifier() { result = this.getName() }
   }
 
   class LambdaTree extends LeafTree, Lambda {
@@ -834,8 +813,6 @@ module Trees {
 
   private class OptionalParameterTree extends DefaultValueParameterTree, OptionalParameter {
     final override AstNode getDefaultValue() { result = this.getValue() }
-
-    final override Identifier getIdentifier() { result = this.getName() }
   }
 
   private class RationalTree extends LeafTree, Rational { }
@@ -1196,9 +1173,7 @@ module Trees {
     }
   }
 
-  private class SplatParameterTree extends ComplexParameterTree, SplatParameter {
-    final override Identifier getIdentifier() { result = this.getName() }
-  }
+  private class SplatParameterTree extends LeafTree, SplatParameter { }
 
   private class SuperTree extends LeafTree, Super { }