From 6b6aeb10c7e9a3b221ea65f1c64ddb1efbf97244 Mon Sep 17 00:00:00 2001
From: Tom Hvitved <hvitved@github.com>
Date: Wed, 19 May 2021 14:33:52 +0200
Subject: [PATCH] Improve performance of `internal/Module.qll`

---
 ql/src/codeql_ruby/ast/internal/Module.qll    | 53 ++++++++++++++-----
 .../library-tests/modules/ancestors.expected  |  6 +--
 .../modules/superclasses.expected             |  6 +--
 3 files changed, 46 insertions(+), 19 deletions(-)

diff --git a/ql/src/codeql_ruby/ast/internal/Module.qll b/ql/src/codeql_ruby/ast/internal/Module.qll
index 1a0b56f961d..3e21e43c879 100644
--- a/ql/src/codeql_ruby/ast/internal/Module.qll
+++ b/ql/src/codeql_ruby/ast/internal/Module.qll
@@ -143,6 +143,14 @@ private ModuleBase enclosing(ModuleBase m, int level) {
   result = enclosing(m.getEnclosingModule(), level - 1)
 }
 
+pragma[noinline]
+private Namespace enclosingNameSpaceConstantReadAccess(
+  ConstantReadAccess c, int priority, string name
+) {
+  result = enclosing(c.getEnclosingModule(), priority) and
+  name = c.getName()
+}
+
 /**
  * Resolve constant read access (typically a scope expression) to a qualified name. The
  * `priority` value indicates the precedence of the solution with respect to the lookup order.
@@ -154,26 +162,35 @@ private ModuleBase enclosing(ModuleBase m, int level) {
 private string resolveScopeExpr(ConstantReadAccess c, int priority) {
   c.hasGlobalScope() and result = c.getName() and priority = 0
   or
-  result = qualifiedModuleName(resolveScopeExpr(c.getScopeExpr(), priority), c.getName())
+  exists(string name |
+    result = qualifiedModuleName(resolveScopeExprConstantReadAccess(c, priority, name), name)
+  )
   or
   not exists(c.getScopeExpr()) and
   not c.hasGlobalScope() and
   (
-    exists(Namespace n |
-      result = qualifiedModuleName(constantDefinition0(n), c.getName()) and
-      n = enclosing(c.getEnclosingModule(), priority)
+    exists(string name |
+      exists(Namespace n |
+        n = enclosingNameSpaceConstantReadAccess(c, priority, name) and
+        result = qualifiedModuleName(constantDefinition0(n), name)
+      )
+      or
+      result =
+        qualifiedModuleName(ancestors(qualifiedModuleNameConstantReadAccess(c, name),
+            priority - maxDepth()), name)
     )
     or
-    result =
-      qualifiedModuleName(ancestors(qualifiedModuleName(c.getEnclosingModule()),
-          priority - maxDepth()), c.getName())
-    or
-    result = c.getName() and
     priority = maxDepth() + 4 and
-    qualifiedModuleName(c.getEnclosingModule()) != "BasicObject"
+    qualifiedModuleNameConstantReadAccess(c, result) != "BasicObject"
   )
 }
 
+pragma[noinline]
+private string resolveScopeExprConstantReadAccess(ConstantReadAccess c, int priority, string name) {
+  result = resolveScopeExpr(c.getScopeExpr(), priority) and
+  name = c.getName()
+}
+
 bindingset[qualifier, name]
 private string scopeAppend(string qualifier, string name) {
   if qualifier = "Object" then result = name else result = qualifier + "::" + name
@@ -185,6 +202,18 @@ private string qualifiedModuleName(ModuleBase m) {
   result = constantDefinition0(m)
 }
 
+pragma[noinline]
+private string qualifiedModuleNameConstantWriteAccess(ConstantWriteAccess c, string name) {
+  result = qualifiedModuleName(c.getEnclosingModule()) and
+  name = c.getName()
+}
+
+pragma[noinline]
+private string qualifiedModuleNameConstantReadAccess(ConstantReadAccess c, string name) {
+  result = qualifiedModuleName(c.getEnclosingModule()) and
+  name = c.getName()
+}
+
 /**
  * Get a qualified name for a constant definition. May return multiple qualified
  * names because we over-approximate when resolving scope resolutions and ignore
@@ -198,9 +227,7 @@ private string constantDefinition0(ConstantWriteAccess c) {
   or
   not exists(c.getScopeExpr()) and
   not c.hasGlobalScope() and
-  exists(ModuleBase enclosing | enclosing = c.getEnclosingModule() |
-    result = scopeAppend(qualifiedModuleName(enclosing), c.getName())
-  )
+  exists(string name | result = scopeAppend(qualifiedModuleNameConstantWriteAccess(c, name), name))
 }
 
 /**
diff --git a/ql/test/library-tests/modules/ancestors.expected b/ql/test/library-tests/modules/ancestors.expected
index 4d00bf4dd04..f727ce332e8 100644
--- a/ql/test/library-tests/modules/ancestors.expected
+++ b/ql/test/library-tests/modules/ancestors.expected
@@ -60,6 +60,9 @@ modules.rb:
 #   30| Foo::ClassInAnotherDefinitionOfFoo
 #-----| super -> Object
 
+#  116| XX::YY
+#-----| super -> YY
+
 #   65| Test::Foo1
 
 #   70| Test::Foo2
@@ -68,9 +71,6 @@ modules.rb:
 
 #   84| Other::Foo1
 
-#  116| XX::YY
-#-----| super -> YY
-
 #    6| Foo::Bar::ClassInFooBar
 #-----| super -> Object
 
diff --git a/ql/test/library-tests/modules/superclasses.expected b/ql/test/library-tests/modules/superclasses.expected
index 38c127ff92b..c1e93aaed9f 100644
--- a/ql/test/library-tests/modules/superclasses.expected
+++ b/ql/test/library-tests/modules/superclasses.expected
@@ -55,6 +55,9 @@ modules.rb:
 #   30| Foo::ClassInAnotherDefinitionOfFoo
 #-----|  -> Object
 
+#  116| XX::YY
+#-----|  -> YY
+
 #   65| Test::Foo1
 
 #   70| Test::Foo2
@@ -63,9 +66,6 @@ modules.rb:
 
 #   84| Other::Foo1
 
-#  116| XX::YY
-#-----|  -> YY
-
 #    6| Foo::Bar::ClassInFooBar
 #-----|  -> Object