Merge pull request #12804 from asgerf/rb/api-graphs-cached

Ruby: restrict join order of API graph predicates

Author: asgerf

ruby/ql/lib/codeql/ruby/ApiGraphs.qll

@@ -99,9 +99,7 @@ module API {
      */
     pragma[inline]
     DataFlow::Node getAValueReachableFromSource() {
-      exists(DataFlow::LocalSourceNode src | Impl::use(this, src) |
-        Impl::trackUseNode(src).flowsTo(result)
-      )
+      result = getAValueReachableFromSourceInline(this)
     }
 
     /**
@@ -121,7 +119,10 @@ module API {
      * end
      * ```
      */
-    DataFlow::LocalSourceNode asSource() { Impl::use(this, result) }
+    pragma[inline]
+    DataFlow::LocalSourceNode asSource() {
+      result = pragma[only_bind_out](this).(Node::Internal).asSourceInternal()
+    }
 
     /**
      * Gets a data-flow node where this value leaves the current codebase and flows into an
@@ -167,6 +168,7 @@ module API {
     /**
      * Gets a call to a method on the receiver represented by this API component.
      */
+    pragma[inline]
     DataFlow::CallNode getAMethodCall(string method) { result = this.getReturn(method).asSource() }
 
     /**
@@ -177,15 +179,20 @@ module API {
      * - A submodule of a module
      * - An attribute of an object
      */
-    bindingset[m]
-    bindingset[result]
-    Node getMember(string m) { result = this.getASuccessor(Label::member(m)) }
+    pragma[inline]
+    Node getMember(string m) {
+      result = pragma[only_bind_out](this).(Node::Internal).getMemberInternal(m)
+    }
 
     /**
      * Gets a node representing a member of this API component where the name of the member may
      * or may not be known statically.
      */
-    Node getAMember() { result = this.getASuccessor(Label::member(_)) }
+    cached
+    Node getAMember() {
+      Impl::forceCachingInSameStage() and
+      result = this.getASuccessor(Label::member(_))
+    }
 
     /**
      * Gets a node representing an instance of this API component, that is, an object whose
@@ -198,41 +205,54 @@ module API {
      * This predicate may have multiple results when there are multiple constructor calls invoking this API component.
      * Consider using `getAnInstantiation()` if there is a need to distinguish between individual constructor calls.
      */
+    pragma[inline]
     Node getInstance() { result = this.getASubclass().getReturn("new") }
 
     /**
      * Gets a node representing a call to `method` on the receiver represented by this node.
      */
+    pragma[inline]
     MethodAccessNode getMethod(string method) {
-      result = this.getASubclass().getASuccessor(Label::method(method))
+      result = pragma[only_bind_out](this).(Node::Internal).getMethodInternal(method)
     }
 
     /**
      * Gets a node representing the result of this call.
      */
-    Node getReturn() { result = this.getASuccessor(Label::return()) }
+    pragma[inline]
+    Node getReturn() { result = pragma[only_bind_out](this).(Node::Internal).getReturnInternal() }
 
     /**
      * Gets a node representing the result of calling a method on the receiver represented by this node.
      */
+    pragma[inline]
     Node getReturn(string method) { result = this.getMethod(method).getReturn() }
 
     /** Gets an API node representing the `n`th positional parameter. */
-    pragma[nomagic]
-    Node getParameter(int n) { result = this.getASuccessor(Label::parameter(n)) }
+    cached
+    Node getParameter(int n) {
+      Impl::forceCachingInSameStage() and
+      result = this.getASuccessor(Label::parameter(n))
+    }
 
     /** Gets an API node representing the given keyword parameter. */
-    pragma[nomagic]
+    cached
     Node getKeywordParameter(string name) {
+      Impl::forceCachingInSameStage() and
       result = this.getASuccessor(Label::keywordParameter(name))
     }
 
     /** Gets an API node representing the block parameter. */
-    Node getBlock() { result = this.getASuccessor(Label::blockParameter()) }
+    cached
+    Node getBlock() {
+      Impl::forceCachingInSameStage() and
+      result = this.getASuccessor(Label::blockParameter())
+    }
 
     /**
      * Gets a `new` call to the function represented by this API component.
      */
+    pragma[inline]
     DataFlow::ExprNode getAnInstantiation() { result = this.getInstance().asSource() }
 
     /**
@@ -255,12 +275,17 @@ module API {
      * ```
      * In the example above, `getMember("A").getAnImmediateSubclass()` will return uses of `B` only.
      */
-    Node getAnImmediateSubclass() { result = this.getASuccessor(Label::subclass()) }
+    cached
+    Node getAnImmediateSubclass() {
+      Impl::forceCachingInSameStage() and result = this.getASuccessor(Label::subclass())
+    }
 
     /**
      * Gets a node representing the `content` stored on the base object.
      */
+    cached
     Node getContent(DataFlow::Content content) {
+      Impl::forceCachingInSameStage() and
       result = this.getASuccessor(Label::content(content))
     }
 
@@ -274,10 +299,16 @@ module API {
     }
 
     /** Gets a node representing the instance field of the given `name`, which must include the `@` character. */
-    Node getField(string name) { result = this.getContent(DataFlowPrivate::TFieldContent(name)) }
+    cached
+    Node getField(string name) {
+      Impl::forceCachingInSameStage() and
+      result = this.getContent(DataFlowPrivate::TFieldContent(name))
+    }
 
     /** Gets a node representing an element of this collection (known or unknown). */
+    cached
     Node getAnElement() {
+      Impl::forceCachingInSameStage() and
       result = this.getContents(any(DataFlow::ContentSet set | set.isAnyElement()))
     }
 
@@ -337,7 +368,7 @@ module API {
     /**
      * Gets a textual representation of this element.
      */
-    abstract string toString();
+    string toString() { none() }
 
     /**
      * Gets a path of the given `length` from the root to this node.
@@ -363,6 +394,65 @@ module API {
     int getDepth() { result = Impl::distanceFromRoot(this) }
   }
 
+  /** Companion module to the `Node` class. */
+  module Node {
+    /**
+     * INTERNAL USE ONLY.
+     *
+     * An API node, with some internal predicates exposed.
+     */
+    class Internal extends Node {
+      /**
+       * INTERNAL USE ONLY.
+       *
+       * Same as `asSource()` but without join-order hints.
+       */
+      cached
+      DataFlow::LocalSourceNode asSourceInternal() {
+        Impl::forceCachingInSameStage() and
+        Impl::use(this, result)
+      }
+
+      /**
+       * Same as `getMember` but without join-order hints.
+       */
+      cached
+      Node getMemberInternal(string m) {
+        Impl::forceCachingInSameStage() and
+        result = this.getASuccessor(Label::member(m))
+      }
+
+      /**
+       * Same as `getMethod` but without join-order hints.
+       */
+      cached
+      MethodAccessNode getMethodInternal(string method) {
+        Impl::forceCachingInSameStage() and
+        result = this.getASubclass().getASuccessor(Label::method(method))
+      }
+
+      /**
+       * INTERNAL USE ONLY.
+       *
+       * Same as `getReturn()` but without join-order hints.
+       */
+      cached
+      Node getReturnInternal() {
+        Impl::forceCachingInSameStage() and result = this.getASuccessor(Label::return())
+      }
+    }
+  }
+
+  bindingset[node]
+  pragma[inline_late]
+  private DataFlow::Node getAValueReachableFromSourceInline(Node node) {
+    exists(DataFlow::LocalSourceNode src, DataFlow::LocalSourceNode dst |
+      Impl::use(node, pragma[only_bind_into](src)) and
+      pragma[only_bind_into](dst) = Impl::trackUseNode(src) and
+      dst.flowsTo(result)
+    )
+  }
+
   /** The root node of an API graph. */
   class Root extends Node, Impl::MkRoot {
     override string toString() { result = "root" }
@@ -443,7 +533,10 @@ module API {
    * you should use `.getMember` on the parent module/class. For example, for nodes corresponding to the class `Gem::Version`,
    * use `getTopLevelMember("Gem").getMember("Version")`.
    */
-  Node getTopLevelMember(string m) { result = root().getMember(m) }
+  cached
+  Node getTopLevelMember(string m) {
+    Impl::forceCachingInSameStage() and result = root().(Node::Internal).getMemberInternal(m)
+  }
 
   /**
    * Provides the actual implementation of API graphs, cached for performance.
@@ -469,6 +562,36 @@ module API {
    */
   cached
   private module Impl {
+    cached
+    predicate forceCachingInSameStage() { any() }
+
+    cached
+    predicate forceCachingBackref() {
+      1 = 1
+      or
+      exists(getTopLevelMember(_))
+      or
+      exists(
+        any(Node n)
+            .(Node::Internal)
+            .getMemberInternal("foo")
+            .getAMember()
+            .(Node::Internal)
+            .getMethodInternal("foo")
+            .(Node::Internal)
+            .getReturnInternal()
+            .getParameter(0)
+            .getKeywordParameter("foo")
+            .getBlock()
+            .getAnImmediateSubclass()
+            .getContent(_)
+            .getField(_)
+            .getAnElement()
+            .(Node::Internal)
+            .asSourceInternal()
+      )
+    }
+
     cached
     newtype TApiNode =
       /** The root of the API graph. */

ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll

@@ -40,7 +40,7 @@ private API::Node activeRecordClassApiNode() {
       // In Rails applications `ApplicationRecord` typically extends `ActiveRecord::Base`, but we
       // treat it separately in case the `ApplicationRecord` definition is not in the database.
       API::getTopLevelMember("ApplicationRecord")
-    ].getASubclass()
+    ]
 }
 
 /**
@@ -58,7 +58,7 @@ private API::Node activeRecordClassApiNode() {
 class ActiveRecordModelClass extends ClassDeclaration {
   ActiveRecordModelClass() {
     this.getSuperclassExpr() =
-      activeRecordClassApiNode().getAValueReachableFromSource().asExpr().getExpr()
+      activeRecordClassApiNode().getASubclass().getAValueReachableFromSource().asExpr().getExpr()
   }
 
   // Gets the class declaration for this class and all of its super classes

ruby/ql/lib/codeql/ruby/frameworks/ActiveResource.qll

@@ -17,8 +17,9 @@ module ActiveResource {
   /**
    * An ActiveResource model class. This is any (transitive) subclass of ActiveResource.
    */
+  pragma[nomagic]
   private API::Node modelApiNode() {
-    result = API::getTopLevelMember("ActiveResource").getMember("Base").getASubclass+()
+    result = API::getTopLevelMember("ActiveResource").getMember("Base").getASubclass()
   }
 
   /**

ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll

@@ -85,7 +85,6 @@ module ActiveStorage {
           // Class methods
           API::getTopLevelMember("ActiveStorage")
               .getMember("Blob")
-              .getASubclass()
               .getAMethodCall(["create_after_unfurling!", "create_and_upload!"]),
           // Instance methods
           any(BlobInstance i, DataFlow::CallNode c |

ruby/ql/lib/codeql/ruby/frameworks/GraphQL.qll

@@ -42,7 +42,7 @@ private class GraphqlRelayClassicMutationClass extends ClassDeclaration {
     this.getSuperclassExpr() =
       graphQlSchema()
           .getMember("RelayClassicMutation")
-          .getASubclass*()
+          .getASubclass()
           .getAValueReachableFromSource()
           .asExpr()
           .getExpr()

ruby/ql/lib/codeql/ruby/frameworks/Twirp.qll

@@ -18,8 +18,7 @@ module Twirp {
    */
   class ServiceInstantiation extends DataFlow::CallNode {
     ServiceInstantiation() {
-      this =
-        API::getTopLevelMember("Twirp").getMember("Service").getASubclass().getAnInstantiation()
+      this = API::getTopLevelMember("Twirp").getMember("Service").getAnInstantiation()
     }
 
     /**
@@ -62,7 +61,7 @@ module Twirp {
    */
   class ClientInstantiation extends DataFlow::CallNode {
     ClientInstantiation() {
-      this = API::getTopLevelMember("Twirp").getMember("Client").getASubclass().getAnInstantiation()
+      this = API::getTopLevelMember("Twirp").getMember("Client").getAnInstantiation()
     }
   }
 

ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModelsSpecific.qll

@@ -113,16 +113,18 @@ API::Node getExtraNodeFromType(string type) {
   |
     suffix = "!" and
     (
-      result.asSource() = constRef
+      result.(API::Node::Internal).asSourceInternal() = constRef
       or
-      result.asSource() = constRef.getADescendentModule().getAnOwnModuleSelf()
+      result.(API::Node::Internal).asSourceInternal() =
+        constRef.getADescendentModule().getAnOwnModuleSelf()
     )
     or
     suffix = "" and
     (
-      result.asSource() = constRef.getAMethodCall("new")
+      result.(API::Node::Internal).asSourceInternal() = constRef.getAMethodCall("new")
       or
-      result.asSource() = constRef.getADescendentModule().getAnInstanceSelf()
+      result.(API::Node::Internal).asSourceInternal() =
+        constRef.getADescendentModule().getAnInstanceSelf()
     )
   )
   or