JavaScript: Refactor AMD/CommonJS path expression analysis to avoid bad magic.

javascript/ql/src/semmle/javascript/AMD.qll

@@ -170,20 +170,19 @@ class AMDModuleDefinition extends CallExpr {
   }
 }
 
-/** A path expression appearing in the list of dependencies of an AMD module. */
-private class AMDDependencyPath extends PathExprInModule, ConstantString {
-  AMDDependencyPath() {
-    exists(AMDModuleDefinition amd | this.getParentExpr*() = amd.getDependencies().getAnElement())
+/** An AMD dependency, considered as a path expression. */
+private class AmdDependencyPath extends PathExprCandidate {
+  AmdDependencyPath() {
+    exists(AMDModuleDefinition amd |
+      this = amd.getDependencies().getAnElement() or
+      this = amd.getARequireCall().getAnArgument()
+    )
   }
-
-  override string getValue() { result = this.(ConstantString).getStringValue() }
 }
 
-/** A path expression appearing in a `require` call in an AMD module. */
-private class AMDRequirePath extends PathExprInModule, ConstantString {
-  AMDRequirePath() {
-    exists(AMDModuleDefinition amd | this.getParentExpr*() = amd.getARequireCall().getAnArgument())
-  }
+/** A constant path element appearing in an AMD dependency expression. */
+private class ConstantAmdDependencyPathElement extends PathExprInModule, ConstantString {
+  ConstantAmdDependencyPathElement() { this = any(AmdDependencyPath amd).getAPart() }
 
   override string getValue() { result = this.(ConstantString).getStringValue() }
 }

javascript/ql/src/semmle/javascript/NodeJS.qll

@@ -239,22 +239,22 @@ class Require extends CallExpr, Import {
   }
 }
 
-/** A literal path expression appearing in a `require` import. */
-private class LiteralRequiredPath extends PathExprInModule, ConstantString {
-  LiteralRequiredPath() { exists(Require req | this.getParentExpr*() = req.getArgument(0)) }
-
-  override string getValue() { result = this.getStringValue() }
-}
-
-/** A literal path expression appearing in a call to `require.resolve`. */
-private class LiteralRequireResolvePath extends PathExprInModule, ConstantString {
-  LiteralRequireResolvePath() {
+/** An argument to `require` or `require.resolve`, considered as a path expression. */
+private class RequirePath extends PathExprCandidate {
+  RequirePath() {
+    this = any(Require req).getArgument(0)
+    or
     exists(RequireVariable req, MethodCallExpr reqres |
       reqres.getReceiver() = req.getAnAccess() and
       reqres.getMethodName() = "resolve" and
-      this.getParentExpr*() = reqres.getArgument(0)
+      this = reqres.getArgument(0)
     )
   }
+}
+
+/** A constant path element appearing in a call to `require` or `require.resolve`. */
+private class ConstantRequirePathElement extends PathExprInModule, ConstantString {
+  ConstantRequirePathElement() { this = any(RequirePath rp).getAPart() }
 
   override string getValue() { result = this.getStringValue() }
 }

javascript/ql/src/semmle/javascript/Paths.qll

@@ -236,3 +236,20 @@ private class ConcatPath extends PathExpr {
     result = this.(AddExpr).getAnOperand().(PathExpr).getSearchRoot(priority)
   }
 }
+
+/**
+ * An expression that appears in a syntactic position where it may represent a path.
+ *
+ * Examples include arguments to the CommonJS `require` function or AMD dependency arguments.
+ */
+abstract class PathExprCandidate extends Expr {
+  /**
+   * Gets an expression that is nested inside this expression.
+   *
+   * Equivalent to `getAChildExpr*()`, but useful to enforce a better join order (in spite of
+   * what the optimizer thinks, there are generally far fewer `PathExprCandidate`s than
+   * `ConstantString`s).
+   */
+  pragma[nomagic]
+  Expr getAPart() { result = this or result = getAPart().getAChildExpr() }
+}