From 986e1cb597ccce863fa238d637623f3dd854b2c9 Mon Sep 17 00:00:00 2001
From: Napalys <napalys@github.com>
Date: Fri, 29 Nov 2024 14:33:40 +0100
Subject: [PATCH] Add ValidatePredicateGetReturns query and tests

---
 .../style/ValidatePredicateGetReturns.ql      | 40 +++++++++++++++++++
 .../ValidatePredicateGetReturns.expected      |  2 +
 .../ValidatePredicateGetReturns.qlref         |  1 +
 .../ValidatePredicateGetReturns/test.qll      | 28 +++++++++++++
 4 files changed, 71 insertions(+)
 create mode 100644 ql/ql/src/queries/style/ValidatePredicateGetReturns.ql
 create mode 100644 ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected
 create mode 100644 ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.qlref
 create mode 100644 ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll

diff --git a/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql
new file mode 100644
index 00000000000..2a3de1cfb3b
--- /dev/null
+++ b/ql/ql/src/queries/style/ValidatePredicateGetReturns.ql
@@ -0,0 +1,40 @@
+/**
+ * @name Predicates starting with "get" should return a value
+ * @description Checks if predicates that start with "get" 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" followed by an uppercase letter.
+ * This ensures that only predicates like "getValue" are matched, excluding names like "getter".
+ */
+predicate isGetPredicate(Predicate pred) { pred.getName().regexpMatch("get[A-Z].*") }
+
+/**
+ * Checks if a predicate has a return type.
+ */
+predicate hasReturnType(Predicate pred) {
+  exists(Type returnType | pred.getReturnType() = returnType)
+}
+
+/**
+ * Checks if a predicate is an alias using getAlias().
+ */
+predicate isAlias(Predicate pred) {
+  pred instanceof ClasslessPredicate and exists(pred.(ClasslessPredicate).getAlias())
+}
+
+from Predicate pred
+where
+  isGetPredicate(pred) and
+  not hasReturnType(pred) and
+  not isAlias(pred)
+select pred, "This predicate starts with 'get' but does not return a value."
diff --git a/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected
new file mode 100644
index 00000000000..b78fe32cc75
--- /dev/null
+++ b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.expected
@@ -0,0 +1,2 @@
+| 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. |
diff --git a/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.qlref b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.qlref
new file mode 100644
index 00000000000..e116f69d6b2
--- /dev/null
+++ b/ql/ql/test/queries/style/ValidatePredicateGetReturns/ValidatePredicateGetReturns.qlref
@@ -0,0 +1 @@
+queries/style/ValidatePredicateGetReturns.ql
diff --git a/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll b/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll
new file mode 100644
index 00000000000..b62b0e2dbaf
--- /dev/null
+++ b/ql/ql/test/queries/style/ValidatePredicateGetReturns/test.qll
@@ -0,0 +1,28 @@
+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() }
+
+// OK -- is an alias
+predicate getAlias2 = getImplementation2/0;