Add assignment-in-expression.ql

Author: Marcono1234

codeql-custom-queries-java/queries/error-prone/assignment-in-expression.ql

@@ -0,0 +1,34 @@
+/**
+ * Finds assignments which appear as part of another expression, i.e. the
+ * assignment result is not discarded. This decreases readability and might
+ * also indicate a bug in case the intention was to write `==` but by
+ * accident only `=` was written.
+ * E.g.:
+ * ```java
+ * String result;
+ * doSomething(result = getResult());
+ * ```
+ * Should instead be written as:
+ * ```java
+ * String result = getResult();
+ * doSomething(result);
+ * `` 
+ */
+
+import java
+import lib.Expressions
+
+from Assignment assign
+where
+    // Result is not discarded
+    not assign instanceof StmtExpr
+    // Ignore if assignment appears in loop condition, e.g. `while ((r = read()) != -1)`
+    and not any(LoopStmt l).getCondition() = assign.getParent+()
+    // Ignore chained assignments in the form `a = b = 1`
+    and not (
+        assign instanceof AssignExpr
+        and any(AssignExpr e).getRhs() = assign
+    )
+    // Ignore assignment in return, this is still somewhat readable, e.g. `return f = getResult();`
+    and not any(ReturnStmt r).getResult() = assign
+select assign, "Should write assignment as separate statement for better readability"

codeql-custom-queries-java/tests/error-prone/assignment-in-expression/Test.java

@@ -0,0 +1,53 @@
+class Test {
+    int f;
+    int g = f = getInt(); // This is allowed
+    int h = 1 + (f = getInt()); // This should be detected
+    Exception e;
+
+    int getInt() {
+        return 1;
+    }
+
+    void sink(int i) { }
+    
+    int test() {
+        sink(f = 1);
+
+        int i;
+        // Only permit multi assignment as StmtExpr
+        sink(f = i = 1);
+        f |= i |= 1; // This is not a normal multi assignment, should be detected
+
+        if (true) {
+            // This is pretty exotic and should be detected
+            throw e = new Exception();
+        }
+
+        // Assignment in init and update of for loop should be detected
+        for (int j = 1 + (i = getInt());; sink(f = getInt())) {
+        }
+
+        // Should be detected; not a simple assignment as return result
+        return 1 + (f = getInt());
+    }
+
+    int testCorrect() {
+        int i = 1;
+
+        // Multi assignment is allowed
+        f = i = 2;
+
+        // Usage in loop is allowed
+        while ((i = getInt()) != -1) {
+        }
+        for (; (i = getInt()) != -1;) {
+        }
+        do {} while ((i = getInt()) != -1);
+
+        // Unary assignments should not be detected
+        sink(i++);
+
+        // Should not be detected, still quite readable and used by some libraries
+        return f = getInt();
+    }
+}

codeql-custom-queries-java/tests/error-prone/assignment-in-expression/assignment-in-expression.expected

@@ -0,0 +1,8 @@
+| Test.java:4:18:4:29 | ...=... | Should write assignment as separate statement for better readability |
+| Test.java:14:14:14:18 | ...=... | Should write assignment as separate statement for better readability |
+| Test.java:18:14:18:22 | ...=... | Should write assignment as separate statement for better readability |
+| Test.java:19:14:19:19 | ...\|=... | Should write assignment as separate statement for better readability |
+| Test.java:23:19:23:37 | ...=... | Should write assignment as separate statement for better readability |
+| Test.java:27:27:27:38 | ...=... | Should write assignment as separate statement for better readability |
+| Test.java:27:48:27:59 | ...=... | Should write assignment as separate statement for better readability |
+| Test.java:31:21:31:32 | ...=... | Should write assignment as separate statement for better readability |

codeql-custom-queries-java/tests/error-prone/assignment-in-expression/assignment-in-expression.qlref

@@ -0,0 +1 @@
+./error-prone/assignment-in-expression.ql