Merge pull request github#18164 from Napalys/napalys/ql-validate-predicate-get-returns

Add query to ensure predicates starting with 'get' return a value

Author: Napalys

ql/ql/src/queries/style/ValidatePredicateGetReturns.ql

@@ -0,0 +1,37 @@
+/**
+ * @name Predicates starting with "get" or "as" should return a value
+ * @description Checks if predicates that start with "get" or "as" actually return a value.
+ * @kind problem
+ * @problem.severity warning
+ * @id ql/predicates-get-should-return-value
+ * @tags correctness
+ *       maintainability
+ * @precision high
+ */
+
+import ql
+import codeql_ql.ast.Ast
+
+/**
+ * Identifies predicates whose names start with "get", "as" followed by an uppercase letter.
+ * This ensures that only predicates like "getValue" are matched, excluding names like "getter".
+ */
+predicate isGetPredicate(Predicate pred, string prefix) {
+  prefix = pred.getName().regexpCapture("(get|as)[A-Z].*", 1)
+}
+
+/**
+ * Checks if a predicate has a return type. This is phrased negatively to not flag unresolved aliases.
+ */
+predicate hasNoReturnType(Predicate pred) {
+  not exists(pred.getReturnTypeExpr()) and
+  not pred.(ClasslessPredicate).getAlias() instanceof PredicateExpr
+  or
+  hasNoReturnType(pred.(ClasslessPredicate).getAlias().(PredicateExpr).getResolvedPredicate())
+}
+
+from Predicate pred, string prefix
+where
+  isGetPredicate(pred, prefix) and
+  hasNoReturnType(pred)
+select pred, "This predicate starts with '" + prefix + "' but does not return a value."

ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected

@@ -0,0 +1,6 @@
+| test.qll:4:11:4:18 | ClasslessPredicate getValue | This predicate starts with 'get' but does not return a value. |
+| test.qll:25:11:25:28 | ClasslessPredicate getImplementation2 | This predicate starts with 'get' but does not return a value. |
+| test.qll:28:11:28:19 | ClasslessPredicate getAlias2 | This predicate starts with 'get' but does not return a value. |
+| test.qll:31:11:31:17 | ClasslessPredicate asValue | This predicate starts with 'as' but does not return a value. |
+| test.qll:48:11:48:19 | ClasslessPredicate getAlias4 | This predicate starts with 'get' but does not return a value. |
+| test.qll:61:11:61:22 | ClasslessPredicate getDistance2 | This predicate starts with 'get' but does not return a value. |

ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.qlref

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

ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll

@@ -0,0 +1,67 @@
+import ql
+
+// NOT OK -- Predicate starts with "get" but does not return a value
+predicate getValue() { none() }
+
+// OK -- starts with get and returns a value
+string getData() { result = "data" }
+
+// OK -- starts with get but followed by a lowercase letter, probably should be ignored
+predicate getterFunction() { none() }
+
+// OK -- starts with get and returns a value
+string getImplementation() { result = "implementation" }
+
+// OK -- is an alias
+predicate getAlias = getImplementation/0;
+
+// OK -- Starts with "get" but followed by a lowercase letter, probably be ignored
+predicate getvalue() { none() }
+
+// OK -- Does not start with "get", should be ignored
+predicate retrieveValue() { none() }
+
+// NOT OK -- starts with get and does not return value
+predicate getImplementation2() { none() }
+
+// NOT OK -- is an alias for a predicate which does not have a return value
+predicate getAlias2 = getImplementation2/0;
+
+// NOT OK -- starts with as and does not return value
+predicate asValue() { none() }
+
+// OK -- starts with as but followed by a lowercase letter, probably should be ignored
+predicate assessment() { none() }
+
+// OK -- starts with as and returns a value
+string asString() { result = "string" }
+
+// OK -- starts with get and returns a value
+HiddenType getInjectableCompositeActionNode() {
+  exists(HiddenType hidden | result = hidden.toString())
+}
+
+// OK
+predicate implementation4() { none() }
+
+// NOT OK -- is an alias
+predicate getAlias4 = implementation4/0;
+
+// OK -- is an alias
+predicate alias5 = implementation4/0;
+
+int root() { none() }
+
+predicate edge(int x, int y) { none() }
+
+// OK -- Higher-order predicate
+int getDistance(int x) = shortestDistances(root/0, edge/2)(_, x, result)
+
+// NOT OK -- Higher-order predicate that does not return a value even though has 'get' in the name
+predicate getDistance2(int x, int y) = shortestDistances(root/0, edge/2)(_, x, y)
+
+// OK
+predicate unresolvedAlias = unresolved/0;
+
+// OK -- unresolved alias
+predicate getUnresolvedAlias = unresolved/0;