Merge pull request #9593 from erik-krogh/param2

QL: followup fixes to parameterized modules

.github/workflows/ql-for-ql-build.yml

@@ -50,9 +50,6 @@ jobs:
           path: ${{ runner.temp }}/query-pack.zip
 
   extractors:
-    strategy:
-      fail-fast: false
-
     runs-on: ubuntu-latest
 
     steps:
@@ -195,9 +192,36 @@ jobs:
           category: "ql-for-ql-${{ matrix.folder }}"
       - name: Copy sarif file to CWD
         run: cp ../results/ql.sarif ./${{ matrix.folder }}.sarif
+      - name: Fixup the $scema in sarif  # Until https://github.com/microsoft/sarif-vscode-extension/pull/436/ is part in a stable release
+        run: |
+          sed -i 's/\$schema.*/\$schema": "https:\/\/raw.githubusercontent.com\/oasis-tcs\/sarif-spec\/master\/Schemata\/sarif-schema-2.1.0",/' ${{ matrix.folder }}.sarif 
       - name: Sarif as artifact
         uses: actions/upload-artifact@v3
         with:
           name: ${{ matrix.folder }}.sarif
           path: ${{ matrix.folder }}.sarif
 
+  combine:
+    runs-on: ubuntu-latest
+    needs:
+      - analyze
+
+    steps:
+      - uses: actions/checkout@v3
+      - name: Make a folder for artifacts. 
+        run: mkdir -p results
+      - name: Download all sarif files
+        uses: actions/download-artifact@v3
+        with: 
+            path: results
+      - uses: actions/setup-node@v3
+        with:
+            node-version: 16
+      - name: Combine all sarif files
+        run: | 
+          node ./ql/scripts/merge-sarif.js results/**/*.sarif combined.sarif
+      - name: Upload combined sarif file
+        uses: actions/upload-artifact@v3
+        with:
+            name: combined.sarif
+            path: combined.sarif

.github/workflows/ql-for-ql-dataset_measure.yml

@@ -36,7 +36,7 @@ jobs:
             ql/target
           key: ${{ runner.os }}-qltest-cargo-${{ hashFiles('ql/**/Cargo.lock') }}
       - name: Build Extractor
-        run: cd ql; env "PATH=$PATH:`dirname ${CODEQL}`" ./create-extractor-pack.sh
+        run: cd ql; env "PATH=$PATH:`dirname ${CODEQL}`" ./scripts/create-extractor-pack.sh
         env:
           CODEQL: ${{ steps.find-codeql.outputs.codeql-path }}
       - name: Checkout ${{ matrix.repo }}

.github/workflows/ql-for-ql-tests.yml

@@ -36,7 +36,7 @@ jobs:
         run: |
           cd ql;
           codeqlpath=$(dirname ${{ steps.find-codeql.outputs.codeql-path }});
-          env "PATH=$PATH:$codeqlpath" ./create-extractor-pack.sh
+          env "PATH=$PATH:$codeqlpath" ./scripts/create-extractor-pack.sh
       - name: Run QL tests
         run: |
           "${CODEQL}" test run --check-databases --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition --search-path "${{ github.workspace }}/ql/extractor-pack" --consistency-queries ql/ql/consistency-queries ql/ql/test

javascript/ql/src/Expressions/TypoDatabase.qll

@@ -5793,6 +5793,8 @@ predicate typos(string wrong, string right) {
   or
   wrong = "paramters" and right = "parameters"
   or
+  wrong = "parametarized" and right = "parameterized"
+  or
   wrong = "paranthesis" and right = "parenthesis"
   or
   wrong = "paraphenalia" and right = "paraphernalia"

ql/README.md

@@ -21,14 +21,14 @@ cargo build --release
 The generated `ql/src/ql.dbscheme` and `ql/src/codeql_ql/ast/internal/TreeSitter.qll` files are included in the repository, but they can be re-generated as follows:
 
 ```bash
-./create-extractor-pack.sh
+./scripts/create-extractor-pack.sh
 ```
 
 ## Building a CodeQL database for a QL program
 
 First, get an extractor pack:
 
-Run `./create-extractor-pack.sh` (Linux/Mac) or `.\create-extractor-pack.ps1` (Windows PowerShell) and the pack will be created in the `extractor-pack` directory.
+Run `./scripts/create-extractor-pack.sh` (Linux/Mac) or `.\scripts\create-extractor-pack.ps1` (Windows PowerShell) and the pack will be created in the `extractor-pack` directory.
 
 Then run
 

ql/ql/src/codeql_ql/ast/Ast.qll

@@ -688,7 +688,7 @@ class TypeExpr extends TType, TypeRef {
     // resolve type
     resolveTypeExpr(this, result)
     or
-    // if it resolves to a module
+    // if it resolves to a module,
     exists(FileOrModule mod | resolveModuleRef(this, mod) | result = mod.toType())
   }
 
@@ -1181,7 +1181,7 @@ class Import extends TImport, ModuleMember, TypeRef {
    */
   string getImportString() {
     exists(string selec |
-      not exists(getSelectionName(_)) and selec = ""
+      not exists(this.getSelectionName(_)) and selec = ""
       or
       selec =
         "::" + strictconcat(int i, string q | q = this.getSelectionName(i) | q, "::" order by i)
@@ -2231,9 +2231,7 @@ class ModuleExpr extends TModuleExpr, TypeRef {
     or
     not exists(me.getName()) and result = me.getChild().(QL::SimpleId).getValue()
     or
-    exists(QL::ModuleInstantiation instantiation | instantiation.getParent() = me |
-      result = instantiation.getName().getChild().getValue()
-    )
+    result = me.getAFieldOrChild().(QL::ModuleInstantiation).getName().getChild().getValue()
   }
 
   /**
@@ -2268,9 +2266,7 @@ class ModuleExpr extends TModuleExpr, TypeRef {
    * The result is either a `PredicateExpr` or a `TypeExpr`.
    */
   SignatureExpr getArgument(int i) {
-    exists(QL::ModuleInstantiation instantiation | instantiation.getParent() = me |
-      result.toQL() = instantiation.getChild(i)
-    )
+    result.toQL() = me.getAFieldOrChild().(QL::ModuleInstantiation).getChild(i)
   }
 }
 

ql/ql/src/codeql_ql/ast/internal/Module.qll

@@ -218,6 +218,20 @@ private module Cached {
     not exists(me.(ModuleExpr).getQualifier()) and
     exists(ContainerOrModule enclosing, string name | resolveModuleRefHelper(me, enclosing, name) |
       definesModule(enclosing, name, m, _)
+    ) and
+    (
+      not me instanceof TypeExpr
+      or
+      // remove some spurious results that can happen with `TypeExpr`
+      me instanceof TypeExpr and
+      m instanceof Module_ and // TypeExpr can only resolve to a Module, and only in some scenarios
+      (
+        // only possible if this is inside a moduleInstantiation.
+        me = any(ModuleExpr mod).getArgument(_).asType()
+        or
+        // or if it's a parameter to a parameterized module
+        me = any(SignatureExpr sig, Module mod | mod.hasParameter(_, _, sig) | sig).asType()
+      )
     )
     or
     exists(FileOrModule mid |
@@ -229,7 +243,8 @@ private module Cached {
   pragma[noinline]
   private predicate resolveModuleRefHelper(TypeRef me, ContainerOrModule enclosing, string name) {
     enclosing = getEnclosingModule(me).getEnclosing*() and
-    name = [me.(ModuleExpr).getName(), me.(TypeExpr).getClassName()]
+    name = [me.(ModuleExpr).getName(), me.(TypeExpr).getClassName()] and
+    (not me instanceof ModuleExpr or not enclosing instanceof Folder_) // module expressions are not imports, so they can't resolve to a file (which is contained in a folder).
   }
 }
 
@@ -332,7 +347,19 @@ module ModConsistency {
    *  }
    */
 
-  query predicate noName(Module mod) { not exists(mod.getName()) }
+  query predicate noName(AstNode mod) {
+    mod instanceof Module and
+    not exists(mod.(Module).getName())
+    or
+    mod instanceof ModuleExpr and
+    not exists(mod.(ModuleExpr).getName())
+  }
 
-  query predicate nonUniqueName(Module mod) { count(mod.getName()) >= 2 }
+  query predicate nonUniqueName(AstNode mod) {
+    mod instanceof Module and
+    count(mod.(Module).getName()) >= 2
+    or
+    mod instanceof ModuleExpr and
+    count(mod.(ModuleExpr).getName()) >= 2
+  }
 }

ql/ql/src/codeql_ql/ast/internal/Predicate.qll

@@ -31,7 +31,8 @@ private predicate definesPredicate(
       name = alias.getName() and
       resolvePredicateExpr(alias.getAlias(), p) and
       public = getPublicBool(alias) and
-      arity = alias.getArity()
+      arity = alias.getArity() and
+      arity = p.getArity()
     )
   )
 }
@@ -48,15 +49,18 @@ private module Cached {
       m = pe.getQualifier().getResolvedModule() and
       public = true
     |
-      definesPredicate(m, pe.getName(), p.getArity(), p, public)
+      definesPredicate(m, pe.getName(), p.getArity(), p, public) and
+      p.getArity() = pe.getArity()
     )
   }
 
   private predicate resolvePredicateCall(PredicateCall pc, PredicateOrBuiltin p) {
+    // calls to class methods
     exists(Class c, ClassType t |
       c = pc.getParent*() and
       t = c.getType() and
-      p = t.getClassPredicate(pc.getPredicateName(), pc.getNumberOfArguments())
+      p = t.getClassPredicate(pc.getPredicateName(), pc.getNumberOfArguments()) and
+      not exists(pc.getQualifier()) // no module qualifier, because then it's not a call to a class method.
     )
     or
     exists(FileOrModule m, boolean public |
@@ -85,22 +89,47 @@ private module Cached {
     )
   }
 
+  /**
+   *  Holds if `mc` is a `this.method()` call to a predicate defined in the same class.
+   * helps avoid spuriously resolving to predicates in super-classes.
+   */
+  private predicate resolveSelfClassCalls(MemberCall mc, PredicateOrBuiltin p) {
+    exists(Class c |
+      mc.getBase() instanceof ThisAccess and
+      c = mc.getEnclosingPredicate().getParent() and
+      p = c.getClassPredicate(mc.getMemberName()) and
+      p.getArity() = mc.getNumberOfArguments()
+    )
+  }
+
   private predicate resolveMemberCall(MemberCall mc, PredicateOrBuiltin p) {
+    resolveSelfClassCalls(mc, p)
+    or
+    not resolveSelfClassCalls(mc, _) and
     exists(Type t |
       t = mc.getBase().getType() and
       p = t.getClassPredicate(mc.getMemberName(), mc.getNumberOfArguments())
     )
     or
     // super calls - and `this.method()` calls in charpreds. (Basically: in charpreds there is no difference between super and this.)
-    exists(AstNode sup, ClassType type, Type supertype |
+    exists(AstNode sup, Type supertype |
       sup instanceof Super
       or
       sup.(ThisAccess).getEnclosingPredicate() instanceof CharPred
     |
       mc.getBase() = sup and
-      sup.getEnclosingPredicate().getParent().(Class).getType() = type and
-      supertype in [type.getASuperType(), type.getAnInstanceofType()] and
-      p = supertype.getClassPredicate(mc.getMemberName(), mc.getNumberOfArguments())
+      p = supertype.getClassPredicate(mc.getMemberName(), mc.getNumberOfArguments()) and
+      (
+        // super.method()
+        not exists(mc.getSuperType()) and
+        exists(ClassType type |
+          sup.getEnclosingPredicate().getParent().(Class).getType() = type and
+          supertype in [type.getASuperType(), type.getAnInstanceofType()]
+        )
+        or
+        // Class.super.method()
+        supertype = mc.getSuperType().getResolvedType()
+      )
     )
   }
 
@@ -172,24 +201,29 @@ module PredConsistency {
   }
 
   query predicate multipleResolvePredicateExpr(PredicateExpr pe, int c, ClasslessPredicate p) {
-    c = strictcount(ClasslessPredicate p0 | resolvePredicateExpr(pe, p0)) and
+    c =
+      strictcount(ClasslessPredicate p0 |
+        resolvePredicateExpr(pe, p0) and
+        // aliases are expected to resolve to multiple.
+        not exists(p0.getAlias())
+      ) and
     c > 1 and
     resolvePredicateExpr(pe, p)
   }
-  // This can happen with parametarized modules
-  /*
-   * query predicate multipleResolveCall(Call call, int c, PredicateOrBuiltin p) {
-   *    c =
-   *      strictcount(PredicateOrBuiltin p0 |
-   *        resolveCall(call, p0) and
-   *        // aliases are expected to resolve to multiple.
-   *        not exists(p0.(ClasslessPredicate).getAlias()) and
-   *        // overridden predicates may have multiple targets
-   *        not p0.(ClassPredicate).isOverride()
-   *      ) and
-   *    c > 1 and
-   *    resolveCall(call, p)
-   *  }
-   */
 
+  query predicate multipleResolveCall(Call call, int c, PredicateOrBuiltin p) {
+    c =
+      strictcount(PredicateOrBuiltin p0 |
+        resolveCall(call, p0) and
+        // aliases are expected to resolve to multiple.
+        not exists(p0.(ClasslessPredicate).getAlias()) and
+        // overridden predicates may have multiple targets
+        not p0.(ClassPredicate).isOverride() and
+        not p0 instanceof Relation // <- DB relations resolve to both a relation and a predicate.
+      ) and
+    c > 1 and
+    resolveCall(call, p) and
+    // parameterized modules are expected to resolve to multiple.
+    not exists(Predicate sig | not exists(sig.getBody()) and resolveCall(call, sig))
   }
+}

ql/ql/src/codeql_ql/dataflow/DataFlow.qll

@@ -204,7 +204,7 @@ class SuperNode extends LocalFlow::TSuperNode {
   Node getANode() { LocalFlow::getRepr(result) = repr }
 
   /** Gets an AST node from any of the nodes in this super node. */
-  AstNode asAstNode() { result = getANode().asAstNode() }
+  AstNode asAstNode() { result = this.getANode().asAstNode() }
 
   /**
    * Gets a single node from this super node.
@@ -214,23 +214,25 @@ class SuperNode extends LocalFlow::TSuperNode {
    * - An `AstNodeNode` is preferred over other nodes.
    * - A node occurring earlier is preferred over one occurring later.
    */
-  Node getArbitraryRepr() { result = min(Node n | n = getANode() | n order by getInternalId(n)) }
+  Node getArbitraryRepr() {
+    result = min(Node n | n = this.getANode() | n order by getInternalId(n))
+  }
 
   /**
    * Gets the predicate containing all nodes that are part of this super node.
    */
-  Predicate getEnclosingPredicate() { result = getANode().getEnclosingPredicate() }
+  Predicate getEnclosingPredicate() { result = this.getANode().getEnclosingPredicate() }
 
   /** Gets a string representation of this super node. */
   string toString() {
     exists(int c |
-      c = strictcount(getANode()) and
-      result = "Super node of " + c + " nodes in " + getEnclosingPredicate().getName()
+      c = strictcount(this.getANode()) and
+      result = "Super node of " + c + " nodes in " + this.getEnclosingPredicate().getName()
     )
   }
 
   /** Gets the location of an arbitrary node in this super node. */
-  Location getLocation() { result = getArbitraryRepr().getLocation() }
+  Location getLocation() { result = this.getArbitraryRepr().getLocation() }
 
   /** Gets any member call whose receiver is in the same super node. */
   MemberCall getALocalMemberCall() { superNode(result.getBase()) = this }
@@ -287,7 +289,7 @@ class SuperNode extends LocalFlow::TSuperNode {
   cached
   private string getAStringValue(Tracker t) {
     t.start() and
-    result = asAstNode().(String).getValue()
+    result = this.asAstNode().(String).getValue()
     or
     exists(SuperNode pred, Tracker t2 |
       this = pred.track(t2, t) and

ql/ql/src/codeql_ql/style/TypoDatabase.qll

@@ -5793,6 +5793,8 @@ predicate typos(string wrong, string right) {
   or
   wrong = "paramters" and right = "parameters"
   or
+  wrong = "parametarized" and right = "parameterized"
+  or
   wrong = "paranthesis" and right = "parenthesis"
   or
   wrong = "paraphenalia" and right = "paraphernalia"

ql/ql/src/queries/diagnostics/EmptyConsistencies.ql

@@ -22,6 +22,11 @@ where
   or
   PredConsistency::noResolvePredicateExpr(node) and msg = "PredConsistency::noResolvePredicateExpr"
   or
+  PredConsistency::multipleResolveCall(node, _, _) and msg = "PredConsistency::multipleResolveCall"
+  or
+  PredConsistency::multipleResolvePredicateExpr(node, _, _) and
+  msg = "PredConsistency::multipleResolvePredicateExpr"
+  or
   TypeConsistency::exprNoType(node) and msg = "TypeConsistency::exprNoType"
   or
   TypeConsistency::varDefNoType(node) and msg = "TypeConsistency::varDefNoType"

ql/ql/src/queries/reports/FrameworkCoverage.ql

@@ -18,11 +18,13 @@ class PackageImportCall extends PredicateCall {
   PackageImportCall() {
     this.getQualifier().getName() = ["API", "DataFlow"] and
     this.getPredicateName() = ["moduleImport", "moduleMember"] and
-    not isExcludedFile(getLocation().getFile())
+    not isExcludedFile(this.getLocation().getFile())
   }
 
   /** Gets the name of a package referenced by this call */
-  string getAPackageName() { result = DataFlow::superNode(getArgument(0)).getAStringValueNoCall() }
+  string getAPackageName() {
+    result = DataFlow::superNode(this.getArgument(0)).getAStringValueNoCall()
+  }
 }
 
 /** Gets a reference to `package` or any transitive member thereof. */

ql/ql/test/callgraph/MultiResolve.qll

@@ -0,0 +1,58 @@
+predicate foo(int a, int b) {
+  a = 2 and
+  b = 2
+}
+
+predicate foo(int a, int b, int c) {
+  a = 2 and
+  b = 2 and
+  c = 2
+}
+
+predicate myFoo = foo/2;
+
+predicate test(int i) { myFoo(i, i) } // <- should only resolve to the `foo` with 2 arguments (and the `myFoo` alias).
+
+module MyMod {
+  predicate bar() { any() }
+
+  class Bar extends int {
+    Bar() { this = 42 }
+
+    predicate bar() {
+      MyMod::bar() // <- should resolve to the module's predicate.
+    }
+  }
+}
+
+class Super1 extends int {
+  Super1() { this = 42 }
+
+  predicate foo() { any() }
+}
+
+class Super2 extends int {
+  Super2() { this = 42 }
+
+  predicate foo() { none() }
+}
+
+class Sub extends Super1, Super2 {
+  override predicate foo() { Super1.super.foo() } // <- should resolve to Super1::foo()
+}
+
+module Foo {
+  predicate foo() { any() }
+}
+
+predicate test() {
+  Foo::foo() // <- should resolve to `foo` from the module above, and not from the `Foo.qll` file.
+}
+
+class Sub2 extends Super1, Super2 {
+  override predicate foo() { Super2.super.foo() } // <- should resolve to Super2::foo()
+
+  predicate test() {
+    this.foo() // <- should resolve to only the above `foo` predicate, but currently it resolves to that, and all the overrides [INCONSISTENCY]
+  }
+}

ql/ql/test/callgraph/callgraph.expected

@@ -14,10 +14,15 @@ getTarget
 | Foo.qll:31:5:31:12 | PredicateCall | Foo.qll:24:13:24:18 | ClasslessPredicate alias0 |
 | Foo.qll:36:36:36:65 | MemberCall | file://:0:0:0:0 | replaceAll |
 | Foo.qll:38:39:38:67 | MemberCall | file://:0:0:0:0 | regexpCapture |
+| MultiResolve.qll:14:25:14:35 | PredicateCall | MultiResolve.qll:1:11:1:13 | ClasslessPredicate foo |
+| MultiResolve.qll:14:25:14:35 | PredicateCall | MultiResolve.qll:12:11:12:15 | ClasslessPredicate myFoo |
+| MultiResolve.qll:23:7:23:18 | PredicateCall | MultiResolve.qll:17:13:17:15 | ClasslessPredicate bar |
+| MultiResolve.qll:41:30:41:47 | MemberCall | MultiResolve.qll:31:13:31:15 | ClassPredicate foo |
+| MultiResolve.qll:49:3:49:12 | PredicateCall | MultiResolve.qll:45:13:45:15 | ClasslessPredicate foo |
+| MultiResolve.qll:53:30:53:47 | MemberCall | MultiResolve.qll:37:13:37:15 | ClassPredicate foo |
+| MultiResolve.qll:56:5:56:14 | MemberCall | MultiResolve.qll:53:22:53:24 | ClassPredicate foo |
 | Overrides.qll:8:30:8:39 | MemberCall | Overrides.qll:6:7:6:9 | ClassPredicate bar |
-| Overrides.qll:16:39:16:48 | MemberCall | Overrides.qll:6:7:6:9 | ClassPredicate bar |
 | Overrides.qll:16:39:16:48 | MemberCall | Overrides.qll:14:16:14:18 | ClassPredicate bar |
-| Overrides.qll:24:39:24:48 | MemberCall | Overrides.qll:6:7:6:9 | ClassPredicate bar |
 | Overrides.qll:24:39:24:48 | MemberCall | Overrides.qll:22:16:22:18 | ClassPredicate bar |
 | Overrides.qll:28:3:28:9 | MemberCall | Overrides.qll:6:7:6:9 | ClassPredicate bar |
 | Overrides.qll:29:3:29:10 | MemberCall | Overrides.qll:8:13:8:15 | ClassPredicate baz |
@@ -43,5 +48,6 @@ exprPredicate
 | Foo.qll:26:22:26:31 | predicate | Foo.qll:20:13:20:20 | ClasslessPredicate myThing2 |
 | Foo.qll:47:55:47:62 | predicate | Foo.qll:42:20:42:25 | NewTypeBranch MkRoot |
 | Foo.qll:47:65:47:70 | predicate | Foo.qll:44:19:44:22 | ClasslessPredicate edge |
+| MultiResolve.qll:12:19:12:23 | predicate | MultiResolve.qll:1:11:1:13 | ClasslessPredicate foo |
 | ParamModules.qll:4:18:4:25 | predicate | ParamModules.qll:2:23:2:28 | ClasslessPredicate fooSig |
 | ParamModules.qll:10:34:10:40 | predicate | ParamModules.qll:8:13:8:17 | ClasslessPredicate myFoo |

ql/ql/test/queries/style/RedundantCast/Foo.qll

@@ -0,0 +1,11 @@
+class Foo extends string {
+  Foo() { this = "Foo" }
+}
+
+predicate test(Foo f) { f.(Foo).toString() = "X" }
+
+predicate test2(Foo a, Foo b) { a.(Foo) = b }
+
+predicate called(Foo a) { a.toString() = "X" }
+
+predicate test3(string s) { called(s.(Foo)) }

ql/ql/test/queries/style/RedundantCast/RedundantCast.expected

@@ -0,0 +1,3 @@
+| Foo.qll:5:25:5:31 | InlineCast | Redundant cast to $@ | Foo.qll:5:28:5:30 | TypeExpr | Foo |
+| Foo.qll:7:33:7:39 | InlineCast | Redundant cast to $@ | Foo.qll:7:36:7:38 | TypeExpr | Foo |
+| Foo.qll:11:36:11:42 | InlineCast | Redundant cast to $@ | Foo.qll:11:39:11:41 | TypeExpr | Foo |

ql/ql/test/queries/style/RedundantCast/RedundantCast.qlref

@@ -0,0 +1 @@
+queries/style/RedundantCast.ql

ql/scripts/merge-sarif.js

@@ -0,0 +1,23 @@
+var fs = require("fs");
+
+// first a  list of files to merge, and the last argument is the output file.
+async function main(files) {
+  const inputs = files
+    .slice(0, -1)
+    .map((file) => fs.readFileSync(file))
+    .map((data) => JSON.parse(data));
+  const out = inputs[0]; // just arbitrarily take the first one
+  const outFile = files[files.length - 1];
+
+  const combinedResults = [];
+
+  for (const sarif of inputs) {
+    combinedResults.push(...sarif.runs[0].results);
+  }
+
+  out.runs[0].artifacts = []; // the indexes in these won't make sense, so I hope this works.
+  out.runs[0].results = combinedResults;
+
+  fs.writeFileSync(outFile, JSON.stringify(out, null, 2));
+}
+main(process.argv.splice(2));