Merge pull request github#12292 from github/calumgrant/aggregate-domain

Query and tests for sum without domain

Author: MathiasVP

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

@@ -1808,7 +1808,7 @@ class FullAggregate extends TFullAggregate, Aggregate {
 
   /**
    * Gets the kind of aggregate.
-   * E.g. for `min(int i | foo(i))` the result is "foo".
+   * E.g. for `min(int i | foo(i))` the result is "min".
    */
   override string getKind() { result = kind }
 

ql/ql/src/queries/bugs/SumWithoutDomain.ql

@@ -0,0 +1,16 @@
+/**
+ * @name Sum is missing a domain
+ * @description An aggregate like 'sum' should work over a domain, otherwise duplicate values will not be counted.
+ * @kind problem
+ * @problem.severity error
+ * @id ql/sum-missing-domain
+ * @tags correctness
+ * @precision medium
+ */
+
+import ql
+
+from ExprAggregate agg
+where agg.getKind() = ["sum", "strictsum", "avg"]
+select agg,
+  "This " + agg.getKind() + " does not have a domain argument, so may produce surprising results."

ql/ql/test/queries/bugs/SumWithoutDomain/SumWithoutDomain.expected

@@ -0,0 +1 @@
+| Test.qll:3:12:3:25 | ExprAggregate[sum] | This sum does not have a domain argument, so may produce surprising results. |

ql/ql/test/queries/bugs/SumWithoutDomain/SumWithoutDomain.qlref

@@ -0,0 +1 @@
+queries/bugs/SumWithoutDomain.ql

ql/ql/test/queries/bugs/SumWithoutDomain/Test.qll

@@ -0,0 +1,7 @@
+// Result is 3 and not 4
+int foo() {
+  result = sum([1, 1, 2]) // <- Alert here
+}
+
+// Ok - false negative
+predicate bar() { sum(int x | x = [1, 1, 2] | x) = 3 }