Address PR review: add orderBy/andMoreText defaults, convert to query test, update docs

Author: Copilot

.gitignore

@@ -323,6 +323,35 @@ If you wish to perform a path search such as the above, but without reporting pr
 use the `Qtil::GraphPathSearch` module instead, which provides an efficient search algorithm
 without producing a `@kind path-problem` query.
 
+**LimitResults**: A module for capping the number of related entities reported per finding while
+informing the user how many were omitted. This is useful for queries that find multiple related
+entities per finding (such as fields, parameters, or call sites) where listing all of them would
+produce too many results.
+
+Only `problem` and `message` must be implemented. `placeholderString`, `orderBy`, `maxResults`, and
+`andMoreText` have sensible defaults. The instantiated module's `problems` query predicate is
+automatically part of the query output:
+
+```ql
+module MyConfig implements Qtil::LimitResultsConfigSig<IncompleteInitialization, Field> {
+  predicate problem(IncompleteInitialization init, Field f) { f = init.getField() }
+
+  bindingset[remaining]
+  string message(IncompleteInitialization init, Field f, string remaining) {
+    result = init.getKindStr() + " does not initialize non-static data member $@" + remaining + "."
+  }
+}
+
+module Results = Qtil::LimitResults<IncompleteInitialization, Field, MyConfig>;
+
+// Results::problems is automatically part of the query output — no from/where/select needed.
+```
+
+The `problems` query predicate yields `(finding, message, entity, entityStr)` tuples. At most
+`maxResults()` entities (default: 3) are reported per finding, ranked by `orderBy()` (default:
+`entity.toString()`). When some are omitted, `andMoreText(n)` (default: `" (and N more)"`) is
+appended to the message.
+
 ### Inheritance
 
 **Instance**: A module to make `instanceof` inheritance easier in CodeQL, by writing

README.md

@@ -4,14 +4,15 @@
  *
  * This is useful for queries that find multiple related entities per finding (such as fields,
  * parameters, or call sites) but where listing all of them would be too noisy. Instead, only the
- * top `N` entities are reported (ordered by `placeholderString()`), and the message notes how many
- * were omitted.
+ * top `N` entities are reported (ordered by `orderBy()`), and the message notes how many were
+ * omitted.
  *
  * ## Usage
  *
  * Implement the `LimitResultsConfigSig` signature and instantiate the `LimitResults` module. Only
- * `problem` and `message` are required — `placeholderString` and `maxResults` have sensible
- * defaults:
+ * `problem` and `message` are required — `placeholderString`, `orderBy`, `maxResults`, and
+ * `andMoreText` have sensible defaults. The instantiated module's `problems` query predicate is
+ * automatically part of the query output without any `from`/`where`/`select` boilerplate:
  *
  * ```ql
  * module MyConfig implements LimitResultsConfigSig<MyFinding, MyEntity> {
@@ -21,37 +22,28 @@
  *
  *   bindingset[remaining]
  *   string message(MyFinding finding, MyEntity entity, string remaining) {
- *     result = "Finding $@ has entity $@" + remaining + "."
+ *     result = "Finding " + finding.getName() + " has entity $@" + remaining + "."
  *   }
  * }
  *
  * module Results = LimitResults<MyFinding, MyEntity, MyConfig>;
  * ```
- *
- * The instantiated module exposes a `problems` query predicate that can be used directly as
- * the output of a query without any `from`/`where`/`select` boilerplate:
- *
- * ```ql
- * module Results = LimitResults<MyFinding, MyEntity, MyConfig>;
- * // The query predicate Results::problems(...) is automatically part of the query output.
- * ```
  */
 
 private import qtil.parameterization.SignatureTypes
 
 /**
  * A signature for configuring the `LimitResults` module.
  *
- * Only `problem` and `message` must be implemented. The predicates `placeholderString` and
- * `maxResults` have defaults and may be overridden.
+ * Only `problem` and `message` must be implemented. The predicates `placeholderString`,
+ * `orderBy`, `maxResults`, and `andMoreText` have defaults and may be overridden.
  */
 signature module LimitResultsConfigSig<FiniteStringableType Finding, FiniteStringableType Entity> {
   /**
    * The relationship between findings and their associated entities.
    *
    * Defines which entities are relevant to a given finding. All entities satisfying this predicate
-   * will be counted, but only the top `maxResults()` (ordered by `placeholderString()`) will be
-   * reported.
+   * will be counted, but only the top `maxResults()` (ordered by `orderBy()`) will be reported.
    */
   predicate problem(Finding finding, Entity entity);
 
@@ -66,25 +58,46 @@ signature module LimitResultsConfigSig<FiniteStringableType Finding, FiniteStrin
   string message(Finding finding, Entity entity, string remaining);
 
   /**
-   * The display string for an entity, also used as the ordering key (ascending).
+   * The display string for an entity.
    *
-   * Entities with smaller placeholder strings are reported first. When the total count exceeds
-   * `maxResults()`, only the first `maxResults()` entities by this ordering are reported.
+   * Used as the `entityStr` column in the `problems` query predicate output. Also used as the
+   * default ordering key — see `orderBy`.
    *
    * Defaults to `entity.toString()`.
    */
   default string placeholderString(Entity entity) { result = entity.toString() }
 
+  /**
+   * The key to use when ordering entities within a finding (ascending).
+   *
+   * Entities with smaller order keys are reported first. When the total count exceeds
+   * `maxResults()`, only the first `maxResults()` entities by this ordering are reported.
+   *
+   * Defaults to `placeholderString(entity)`.
+   */
+  default string orderBy(Entity entity) { result = placeholderString(entity) }
+
   /**
    * The maximum number of entities to report per finding.
    *
    * When the total number of entities for a finding exceeds this value, only the first
-   * `maxResults()` entities (by `placeholderString()`) are reported, and the message includes a
-   * `" (and N more)"` suffix indicating the number of omitted entities.
+   * `maxResults()` entities (by `orderBy()`) are reported, and the message includes the
+   * `andMoreText()` suffix indicating the number of omitted entities.
    *
    * Defaults to `3`.
    */
   default int maxResults() { result = 3 }
+
+  /**
+   * The suffix appended to the message when `n` entities are omitted.
+   *
+   * The parameter `n` is the number of omitted entities (i.e. `total - maxResults()`). Override
+   * this to customise the "and N more" text, for example to use a different locale.
+   *
+   * Defaults to `" (and N more)"`.
+   */
+  bindingset[n]
+  default string andMoreText(int n) { result = " (and " + n + " more)" }
 }
 
 /**
@@ -118,22 +131,23 @@ module LimitResults<FiniteStringableType Finding, FiniteStringableType Entity, L
    *
    * At most `Config::maxResults()` entities are reported per finding. They are selected by ranking
    * all entities satisfying `Config::problem(finding, entity)` in ascending order of
-   * `Config::placeholderString(entity)`, and taking those with rank <= `Config::maxResults()`.
+   * `Config::orderBy(entity)`, and taking those with rank <= `Config::maxResults()`.
    *
    * The `message` is produced by `Config::message(finding, entity, remaining)`, where `remaining`
-   * is `" (and N more)"` if the total exceeds `Config::maxResults()`, or `""` otherwise.
+   * is `Config::andMoreText(n)` (with `n = total - maxResults()`) if the total exceeds
+   * `Config::maxResults()`, or `""` otherwise.
    */
   predicate hasLimitedResult(Finding finding, Entity entity, string message) {
     exists(int total, int ranked, string remaining |
       total = count(Entity e | Config::problem(finding, e)) and
       entity =
         rank[ranked](Entity e | Config::problem(finding, e) |
-          e order by Config::placeholderString(e)
+          e order by Config::orderBy(e)
         ) and
       ranked <= Config::maxResults() and
       (
         total > Config::maxResults() and
-        remaining = " (and " + (total - Config::maxResults()) + " more)"
+        remaining = Config::andMoreText(total - Config::maxResults())
         or
         total <= Config::maxResults() and remaining = ""
       ) and

src/qtil/results/LimitResults.qll

@@ -1 +1,7 @@
-| All 8 tests passed. |
+| BugA | BugA has field X | BugA.X | BugA.X |
+| BugB | BugB has field A | BugB.A | BugB.A |
+| BugB | BugB has field B | BugB.B | BugB.B |
+| BugB | BugB has field C | BugB.C | BugB.C |
+| BugC | BugC has field A (and 2 more) | BugC.A | BugC.A |
+| BugC | BugC has field B (and 2 more) | BugC.B | BugC.B |
+| BugC | BugC has field C (and 2 more) | BugC.C | BugC.C |

test/qtil/results/LimitResultsTest.expected

@@ -1,5 +1,4 @@
 import qtil.results.LimitResults
-import qtil.testing.Qnit
 import Bugs
 
 module TestConfig implements LimitResultsConfigSig<Bug, BugField> {
@@ -13,134 +12,6 @@ module TestConfig implements LimitResultsConfigSig<Bug, BugField> {
 
 module Results = LimitResults<Bug, BugField, TestConfig>;
 
-/** BugA has 1 field (X), which is fewer than maxResults=3 (default), so all results are shown. */
-class TestBugAResultCount extends Test, Case {
-  override predicate run(Qnit test) {
-    if count(BugField f, string msg | Results::hasLimitedResult(any(Bug b | b = TBugA()), f, msg)) =
-      1
-    then test.pass("BugA: 1 result shown (fewer than maxResults)")
-    else test.fail("BugA: wrong result count")
-  }
-}
-
-/** BugA shows field X with no suffix since total <= maxResults. */
-class TestBugANoSuffix extends Test, Case {
-  override predicate run(Qnit test) {
-    if
-      exists(BugField f, string msg |
-        Results::hasLimitedResult(any(Bug b | b = TBugA()), f, msg) and
-        f.getFieldName() = "X" and
-        msg = "BugA has field X"
-      )
-    then test.pass("BugA: correct message with no suffix")
-    else test.fail("BugA: message incorrect")
-  }
-}
-
-/** BugB has 3 fields (A, B, C), exactly maxResults=3 (default), so all results are shown. */
-class TestBugBResultCount extends Test, Case {
-  override predicate run(Qnit test) {
-    if count(BugField f, string msg | Results::hasLimitedResult(any(Bug b | b = TBugB()), f, msg)) =
-      3
-    then test.pass("BugB: 3 results shown (exactly maxResults)")
-    else test.fail("BugB: wrong result count")
-  }
-}
-
-/** BugB shows all 3 fields with no suffix since total <= maxResults. */
-class TestBugBNoSuffix extends Test, Case {
-  override predicate run(Qnit test) {
-    if
-      forall(string fieldName |
-        fieldName = ["A", "B", "C"] and
-        exists(BugField f |
-          f.getFieldName() = fieldName and
-          f.getBugName() = "BugB"
-        )
-      |
-        exists(BugField f, string msg |
-          Results::hasLimitedResult(any(Bug b | b = TBugB()), f, msg) and
-          f.getFieldName() = fieldName and
-          msg = "BugB has field " + fieldName
-        )
-      )
-    then test.pass("BugB: all 3 results shown with no suffix")
-    else test.fail("BugB: some results have wrong message or are missing")
-  }
+query predicate problems(Bug bug, string msg, BugField field, string fieldStr) {
+  Results::problems(bug, msg, field, fieldStr)
 }
-
-/** BugC has 5 fields (A, B, C, D, E), which exceeds maxResults=3 (default), so only 3 are shown. */
-class TestBugCResultCount extends Test, Case {
-  override predicate run(Qnit test) {
-    if count(BugField f, string msg | Results::hasLimitedResult(any(Bug b | b = TBugC()), f, msg)) =
-      3
-    then test.pass("BugC: 3 results shown (capped at maxResults)")
-    else test.fail("BugC: wrong result count")
-  }
-}
-
-/**
- * BugC shows only the first 3 fields alphabetically (A, B, C), not D or E, using the default
- * placeholderString ordering (toString() = "BugC.A", "BugC.B", ...).
- */
-class TestBugCTopRanked extends Test, Case {
-  override predicate run(Qnit test) {
-    if
-      forall(string fieldName | fieldName = ["A", "B", "C"] |
-        exists(BugField f |
-          Results::hasLimitedResult(any(Bug b | b = TBugC()), f, _) and
-          f.getFieldName() = fieldName and
-          f.getBugName() = "BugC"
-        )
-      ) and
-      not exists(BugField f |
-        Results::hasLimitedResult(any(Bug b | b = TBugC()), f, _) and
-        f.getFieldName() = ["D", "E"] and
-        f.getBugName() = "BugC"
-      )
-    then test.pass("BugC: top 3 alphabetical fields shown, D and E omitted")
-    else test.fail("BugC: wrong fields shown")
-  }
-}
-
-/** BugC shows a suffix " (and 2 more)" since 5 - 3 = 2 entities are omitted. */
-class TestBugCSuffix extends Test, Case {
-  override predicate run(Qnit test) {
-    if
-      forall(BugField f, string msg |
-        Results::hasLimitedResult(any(Bug b | b = TBugC()), f, msg)
-      |
-        msg.matches("% (and 2 more)")
-      )
-    then test.pass("BugC: all shown results have ' (and 2 more)' suffix")
-    else test.fail("BugC: some results have wrong suffix")
-  }
-}
-
-/**
- * The `problems` query predicate returns (finding, msg, entity, entityStr) tuples, where
- * entityStr is the default placeholderString (toString()).
- */
-class TestProblemsQueryPredicate extends Test, Case {
-  override predicate run(Qnit test) {
-    if
-      // BugC: 3 problems shown, each entityStr = BugField.toString() = "BugC.<field>"
-      count(Bug b, string msg, BugField f, string fstr |
-        Results::problems(b, msg, f, fstr) and b = TBugC()
-      ) = 3 and
-      forall(Bug b, string msg, BugField f, string fstr |
-        Results::problems(b, msg, f, fstr) and b = TBugC()
-      |
-        fstr = "BugC." + f.getFieldName()
-      ) and
-      // BugA: 1 problem, entityStr = "BugA.X"
-      exists(Bug b, string msg, BugField f, string fstr |
-        Results::problems(b, msg, f, fstr) and
-        b = TBugA() and
-        fstr = "BugA.X"
-      )
-    then test.pass("problems query predicate returns correct results with entityStr = toString()")
-    else test.fail("problems query predicate returned unexpected results")
-  }
-}
-