Merge pull request #7098 from github/ruby/desugar-for-1

Ruby: Desugar `for` loops as calls to `each`

Author: hvitved

csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowGraph.qll

@@ -25,7 +25,7 @@ module ControlFlow {
    * Only nodes that can be reached from the callable entry point are included in
    * the CFG.
    */
-  class Node extends TNode {
+  class Node extends TCfgNode {
     /** Gets a textual representation of this control flow node. */
     string toString() { none() }
 

csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImplShared.qll

@@ -788,7 +788,7 @@ private module Cached {
    * The control flow graph is pruned for unreachable nodes.
    */
   cached
-  newtype TNode =
+  newtype TCfgNode =
     TEntryNode(CfgScope scope) { succEntrySplits(scope, _, _, _) } or
     TAnnotatedExitNode(CfgScope scope, boolean normal) {
       exists(Reachability::SameSplitsBlock b, SuccessorType t | b.isReachable(_) |
@@ -807,7 +807,7 @@ private module Cached {
 
   /** Gets a successor node of a given flow type, if any. */
   cached
-  TNode getASuccessor(TNode pred, SuccessorType t) {
+  TCfgNode getASuccessor(TCfgNode pred, SuccessorType t) {
     // Callable entry node -> callable body
     exists(ControlFlowElement succElement, Splits succSplits, CfgScope scope |
       result = TElementNode(succElement, succSplits) and

ruby/ql/lib/codeql/ruby/AST.qll

@@ -18,6 +18,27 @@ private import ast.internal.Scope
 private import ast.internal.Synthesis
 private import ast.internal.TreeSitter
 
+cached
+private module Cached {
+  cached
+  ModuleBase getEnclosingModule(Scope s) {
+    result = s
+    or
+    not s instanceof ModuleBase and result = getEnclosingModule(s.getOuterScope())
+  }
+
+  cached
+  MethodBase getEnclosingMethod(Scope s) {
+    result = s
+    or
+    not s instanceof MethodBase and
+    not s instanceof ModuleBase and
+    result = getEnclosingMethod(s.getOuterScope())
+  }
+}
+
+private import Cached
+
 /**
  * A node in the abstract syntax tree. This class is the base class for all Ruby
  * program elements.
@@ -39,20 +60,10 @@ class AstNode extends TAstNode {
   final string getPrimaryQlClasses() { result = concat(this.getAPrimaryQlClass(), ",") }
 
   /** Gets the enclosing module, if any. */
-  ModuleBase getEnclosingModule() {
-    exists(Scope::Range s |
-      s = scopeOf(toGeneratedInclSynth(this)) and
-      toGeneratedInclSynth(result) = s.getEnclosingModule()
-    )
-  }
+  ModuleBase getEnclosingModule() { result = getEnclosingModule(scopeOfInclSynth(this)) }
 
   /** Gets the enclosing method, if any. */
-  MethodBase getEnclosingMethod() {
-    exists(Scope::Range s |
-      s = scopeOf(toGeneratedInclSynth(this)) and
-      toGeneratedInclSynth(result) = s.getEnclosingMethod()
-    )
-  }
+  MethodBase getEnclosingMethod() { result = getEnclosingMethod(scopeOfInclSynth(this)) }
 
   /** Gets a textual representation of this node. */
   cached

ruby/ql/lib/codeql/ruby/ast/Control.qll

@@ -583,7 +583,7 @@ class ForExpr extends Loop, TForExpr {
   final override string getAPrimaryQlClass() { result = "ForExpr" }
 
   /** Gets the body of this `for` loop. */
-  final override Stmt getBody() { toGenerated(result) = g.getBody() }
+  final override StmtSequence getBody() { toGenerated(result) = g.getBody() }
 
   /** Gets the pattern representing the iteration argument. */
   final Pattern getPattern() { toGenerated(result) = g.getPattern() }

ruby/ql/lib/codeql/ruby/ast/Expr.qll

@@ -1,6 +1,7 @@
 private import codeql.ruby.AST
 private import codeql.ruby.CFG
 private import internal.AST
+private import internal.Expr
 private import internal.TreeSitter
 
 /**
@@ -91,90 +92,19 @@ class StmtSequence extends Expr, TStmtSequence {
   }
 }
 
-private class StmtSequenceSynth extends StmtSequence, TStmtSequenceSynth {
-  final override Stmt getStmt(int n) { synthChild(this, n, result) }
-
-  final override string toString() { result = "..." }
-}
-
-private class Then extends StmtSequence, TThen {
-  private Ruby::Then g;
-
-  Then() { this = TThen(g) }
-
-  override Stmt getStmt(int n) { toGenerated(result) = g.getChild(n) }
-
-  final override string toString() { result = "then ..." }
-}
-
-private class Else extends StmtSequence, TElse {
-  private Ruby::Else g;
-
-  Else() { this = TElse(g) }
-
-  override Stmt getStmt(int n) { toGenerated(result) = g.getChild(n) }
-
-  final override string toString() { result = "else ..." }
-}
-
-private class Do extends StmtSequence, TDo {
-  private Ruby::Do g;
-
-  Do() { this = TDo(g) }
-
-  override Stmt getStmt(int n) { toGenerated(result) = g.getChild(n) }
-
-  final override string toString() { result = "do ..." }
-}
-
-private class Ensure extends StmtSequence, TEnsure {
-  private Ruby::Ensure g;
-
-  Ensure() { this = TEnsure(g) }
-
-  override Stmt getStmt(int n) { toGenerated(result) = g.getChild(n) }
-
-  final override string toString() { result = "ensure ..." }
-}
-
 /**
  * A sequence of statements representing the body of a method, class, module,
  * or do-block. That is, any body that may also include rescue/ensure/else
  * statements.
  */
 class BodyStmt extends StmtSequence, TBodyStmt {
-  // Not defined by dispatch, as it should not be exposed
-  private Ruby::AstNode getChild(int i) {
-    result = any(Ruby::Method g | this = TMethod(g)).getChild(i)
-    or
-    result = any(Ruby::SingletonMethod g | this = TSingletonMethod(g)).getChild(i)
-    or
-    exists(Ruby::Lambda g | this = TLambda(g) |
-      result = g.getBody().(Ruby::DoBlock).getChild(i) or
-      result = g.getBody().(Ruby::Block).getChild(i)
-    )
-    or
-    result = any(Ruby::DoBlock g | this = TDoBlock(g)).getChild(i)
-    or
-    result = any(Ruby::Program g | this = TToplevel(g)).getChild(i) and
-    not result instanceof Ruby::BeginBlock
-    or
-    result = any(Ruby::Class g | this = TClassDeclaration(g)).getChild(i)
-    or
-    result = any(Ruby::SingletonClass g | this = TSingletonClass(g)).getChild(i)
-    or
-    result = any(Ruby::Module g | this = TModuleDeclaration(g)).getChild(i)
-    or
-    result = any(Ruby::Begin g | this = TBeginExpr(g)).getChild(i)
-  }
-
   final override Stmt getStmt(int n) {
-    result =
-      rank[n + 1](AstNode node, int i |
-        toGenerated(node) = this.getChild(i) and
-        not node instanceof Else and
-        not node instanceof RescueClause and
-        not node instanceof Ensure
+    toGenerated(result) =
+      rank[n + 1](Ruby::AstNode node, int i |
+        node = getBodyStmtChild(this, i) and
+        not node instanceof Ruby::Else and
+        not node instanceof Ruby::Rescue and
+        not node instanceof Ruby::Ensure
       |
         node order by i
       )
@@ -183,17 +113,25 @@ class BodyStmt extends StmtSequence, TBodyStmt {
   /** Gets the `n`th rescue clause in this block. */
   final RescueClause getRescue(int n) {
     result =
-      rank[n + 1](RescueClause node, int i | toGenerated(node) = this.getChild(i) | node order by i)
+      rank[n + 1](RescueClause node, int i |
+        toGenerated(node) = getBodyStmtChild(this, i)
+      |
+        node order by i
+      )
   }
 
   /** Gets a rescue clause in this block. */
   final RescueClause getARescue() { result = this.getRescue(_) }
 
   /** Gets the `else` clause in this block, if any. */
-  final StmtSequence getElse() { result = unique(Else s | toGenerated(s) = getChild(_)) }
+  final StmtSequence getElse() {
+    result = unique(Else s | toGenerated(s) = getBodyStmtChild(this, _))
+  }
 
   /** Gets the `ensure` clause in this block, if any. */
-  final StmtSequence getEnsure() { result = unique(Ensure s | toGenerated(s) = getChild(_)) }
+  final StmtSequence getEnsure() {
+    result = unique(Ensure s | toGenerated(s) = getBodyStmtChild(this, _))
+  }
 
   final predicate hasEnsure() { exists(this.getEnsure()) }
 

ruby/ql/lib/codeql/ruby/ast/Method.qll

@@ -2,6 +2,7 @@ private import codeql.ruby.AST
 private import codeql.ruby.controlflow.ControlFlowGraph
 private import internal.AST
 private import internal.TreeSitter
+private import internal.Method
 
 /** A callable. */
 class Callable extends StmtSequence, Expr, Scope, TCallable {
@@ -212,16 +213,6 @@ class DoBlock extends Block, BodyStmt, TDoBlock {
  * ```
  */
 class BraceBlock extends Block, TBraceBlock {
-  private Ruby::Block g;
-
-  BraceBlock() { this = TBraceBlock(g) }
-
-  final override Parameter getParameter(int n) {
-    toGenerated(result) = g.getParameters().getChild(n)
-  }
-
-  final override Stmt getStmt(int i) { toGenerated(result) = g.getChild(i) }
-
   final override string toString() { result = "{ ... }" }
 
   final override string getAPrimaryQlClass() { result = "BraceBlock" }

ruby/ql/lib/codeql/ruby/ast/Parameter.qll

@@ -57,7 +57,11 @@ class NamedParameter extends Parameter, TNamedParameter {
   final VariableAccess getAnAccess() { result = this.getVariable().getAnAccess() }
 
   /** Gets the access that defines the underlying local variable. */
-  final VariableAccess getDefiningAccess() { result = this.getVariable().getDefiningAccess() }
+  final VariableAccess getDefiningAccess() {
+    result = this.getVariable().getDefiningAccess()
+    or
+    result = this.(SimpleParameterSynthImpl).getDefininingAccess()
+  }
 
   override AstNode getAChild(string pred) {
     result = super.getAChild(pred)
@@ -68,14 +72,12 @@ class NamedParameter extends Parameter, TNamedParameter {
 }
 
 /** A simple (normal) parameter. */
-class SimpleParameter extends NamedParameter, PatternParameter, VariablePattern, TSimpleParameter {
-  private Ruby::Identifier g;
-
-  SimpleParameter() { this = TSimpleParameter(g) }
+class SimpleParameter extends NamedParameter, PatternParameter, VariablePattern, TSimpleParameter instanceof SimpleParameterImpl {
+  final override string getName() { result = SimpleParameterImpl.super.getNameImpl() }
 
-  final override string getName() { result = g.getValue() }
-
-  final override LocalVariable getVariable() { result = TLocalVariableReal(_, _, g) }
+  final override LocalVariable getVariable() {
+    result = SimpleParameterImpl.super.getVariableImpl()
+  }
 
   final override LocalVariable getAVariable() { result = this.getVariable() }
 

ruby/ql/lib/codeql/ruby/ast/Pattern.qll

@@ -4,6 +4,7 @@ private import internal.AST
 private import internal.Pattern
 private import internal.TreeSitter
 private import internal.Variable
+private import internal.Parameter
 
 /** A pattern. */
 class Pattern extends AstNode {
@@ -15,6 +16,8 @@ class Pattern extends AstNode {
     implicitParameterAssignmentNode(toGenerated(this), _)
     or
     this = getSynthChild(any(AssignExpr ae), 0)
+    or
+    this instanceof SimpleParameterImpl
   }
 
   /** Gets a variable used in (or introduced by) this pattern. */

ruby/ql/lib/codeql/ruby/ast/Scope.qll

@@ -3,22 +3,20 @@ private import internal.AST
 private import internal.Scope
 private import internal.TreeSitter
 
-class Scope extends AstNode, TScopeType {
-  private Scope::Range range;
+/**
+ * A variable scope. This is either a top-level (file), a module, a class,
+ * or a callable.
+ */
+class Scope extends AstNode, TScopeType instanceof ScopeImpl {
+  /** Gets the outer scope, if any. */
+  Scope getOuterScope() { result = super.getOuterScopeImpl() }
 
-  Scope() { range = toGenerated(this) }
+  /** Gets a variable declared in this scope. */
+  Variable getAVariable() { result = super.getAVariableImpl() }
 
-  /** Gets the scope in which this scope is nested, if any. */
-  Scope getOuterScope() { toGenerated(result) = range.getOuterScope() }
-
-  /** Gets a variable that is declared in this scope. */
-  final Variable getAVariable() { result.getDeclaringScope() = this }
-
-  /** Gets the variable declared in this scope with the given name, if any. */
-  final Variable getVariable(string name) {
-    result = this.getAVariable() and
-    result.getName() = name
-  }
+  /** Gets the variable named `name` declared in this scope. */
+  Variable getVariable(string name) { result = super.getVariableImpl(name) }
 }
 
+/** A scope in which a `self` variable exists. */
 class SelfScope extends Scope, TSelfScopeType { }

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

@@ -5,6 +5,7 @@ private import codeql.Locations
 private import internal.AST
 private import internal.TreeSitter
 private import internal.Variable
+private import internal.Parameter
 
 /** A variable declared in a scope. */
 class Variable instanceof VariableImpl {
@@ -50,7 +51,7 @@ class LocalVariable extends Variable, TLocalVariable {
    *
    * `x` is a captured variable, whereas `y` is not.
    */
-  predicate isCaptured() { this.getAnAccess().isCapturedAccess() }
+  final predicate isCaptured() { this.getAnAccess().isCapturedAccess() }
 }
 
 /** A global variable. */
@@ -110,7 +111,11 @@ class VariableAccess extends Expr instanceof VariableAccessImpl {
    * the access to `elements` in the parameter list is an implicit assignment,
    * as is the first access to `e`.
    */
-  predicate isImplicitWrite() { implicitWriteAccess(toGenerated(this)) }
+  predicate isImplicitWrite() {
+    implicitWriteAccess(toGenerated(this))
+    or
+    this = any(SimpleParameterSynthImpl p).getDefininingAccess()
+  }
 
   final override string toString() { result = VariableAccessImpl.super.toString() }
 }
@@ -147,7 +152,7 @@ class LocalVariableAccess extends VariableAccess instanceof LocalVariableAccessI
    * the access to `x` in the first `puts x` is a captured access, while
    * the access to `x` in the second `puts x` is not.
    */
-  predicate isCapturedAccess() { isCapturedAccess(this) }
+  final predicate isCapturedAccess() { isCapturedAccess(this) }
 }
 
 /** An access to a local variable where the value is updated. */
@@ -195,10 +200,4 @@ class SelfVariableAccess extends LocalVariableAccess instanceof SelfVariableAcce
 }
 
 /** An access to the `self` variable where the value is read. */
-class SelfVariableReadAccess extends SelfVariableAccess, VariableReadAccess {
-  // We override the definition in `LocalVariableAccess` because it gives the
-  // wrong result for synthesised `self` variables.
-  override predicate isCapturedAccess() {
-    this.getVariable().getDeclaringScope() != this.getCfgScope()
-  }
-}
+class SelfVariableReadAccess extends SelfVariableAccess, VariableReadAccess { }