CodeQL: Flag implicit compare-then-assign in branch conditions

Implicit compare-then-assign in branch conditions is buggy since
developers often mean assign-then-compare, but sometimes actually mean
compare-then-assign. GCC's -Wparentheses was originally meant to catch
assignment in place of comparison, requiring an extra set of parentheses
to turn this off. This had the happy coincidence of making developers
explicit about assign-then-compare vs compare-then-assign.

An outer level of extra parentheses will inhibit -Wparentheses warnings.
This often results in assign-then-compare being made explicit, but
instead of turning `if (x = foo() < 0)` into `if ((x = foo()) < 0)`, a
developer might write `if ((x = foo() < 0))`, which turns off the
warning, without fixing the problem. This happened in openzfs/zfs#18874.
There are other potential variations, such as `if ((x = (foo()) < 0))`,
which also suppresses GCC's warning, but fails to actually do anything
since the intended explicit parentheses to specify compare-then-assign
are around the right operand of the boolean operator, rather than around
the boolean operator, yet we have the additional parentheses needed to
silence GCC's -Wparentheses. In the `if ((x = (foo()) < 0))` case, the
intent was to make compare-then-assign explicit, and a typo caused it to
fail to become explicit. That is not a bug, but it makes it unclear what
the developer intended, which is problematic in itself.

This probably merits a bug report to GCC requesting a more intelligent
diagnostic that will treat compare-then-assign differently from
assignment in a branch condition. However, that is a slow process, this
has already bitten us once and with CodeQL, we can add our own check to
the PR process so that we catch other instances of this issue during
review, rather than some time later.

Given that assign-then-compare in branch conditions requires that
parentheses be added in such a way that the compiler AST no longer
contains an implicit compare-then-assign, we only need to check for an
implicit compare-then-assign in order to implement this check. Although
the likelihood of compound assignment being present in this bug pattern
is low, the same logic follows, so the check also will catch this
pattern on compound assignment. This check handles conditions in if,
while, do, for, ?:, && and ||. switch statements are intentionally
ignored, since using assign-then-compare in a switch statement would
turn the switch statement into a if-else. That is pointless, so allowing
an implicit compare-then-assign in switch statements is problem-free.
Coincidentally, GCC's -Wparentheses does not apply to switch statements
either. Finally, this considers all comparison operators, rather than
just the < operator used in the examples in this commit message.

The CodeQL check was written by Grok 4.5 Build Beta after several
iterations of prompt engineering and follow-up prompts to give it
corrections. It has also been subjected to a test suite of 19 true
positives and 17 true negatives to verify its behavior. It successfully
detected all true positives and fails to detect any true negatives. It
has also been applied not only to the OpenZFS codebase, but also the
Linux kernel and curl codebases, where it had zero detections. Related
queries in CodeQL were also run against the test suite, but had zero
detections. The query appears to be a well made query that has a very
high signal-to-noise ratio. It might be worth submitting to upstream
CodeQL for inclusion, but I would rather add it to our own repository so
we can begin benefiting from it today.

Assisted-by: Grok 4.5 Build Beta
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes #18899

.github/codeql-cpp.yml

@@ -3,3 +3,4 @@ name: "Custom CodeQL Analysis"
 queries:
   - uses: ./.github/codeql/custom-queries/cpp/deprecatedFunctionUsage.ql
   - uses: ./.github/codeql/custom-queries/cpp/dslDatasetHoldReleMismatch.ql
+  - uses: ./.github/codeql/custom-queries/cpp/AssignmentOfComparisonAsCondition.ql

.github/codeql/custom-queries/cpp/AssignmentOfComparisonAsCondition.ql

@@ -0,0 +1,96 @@
+/**
+ * @name Ambiguous assignment-of-comparison in a condition
+ * @description Finds any assignment (`=`, `+=`, `-=`, `|=`, …) that appears in
+ *              a branching condition and whose rvalue is an unparenthesized
+ *              comparison.
+ *
+ *              Comparison binds tighter than assignment, so
+ *                `if ((x = foo() < 0))`  /  `if ((x += foo() < 0))`
+ *              assigns the boolean result of the comparison. Programmers often
+ *              meant
+ *                `if ((x = foo()) < 0)`  /  `if ((x += foo()) < 0)`
+ *              (assign/update then compare). GCC `-Wparentheses` is silenced by
+ *              the outer parentheses in both forms, so the compiler does not
+ *              catch the mistake (see openzfs/zfs#18874).
+ *
+ *              Rule (intentionally simple):
+ *                assignment in a branch condition
+ *                AND rvalue is a comparison
+ *                AND that comparison is not parenthesized.
+ *
+ *              That is enough:
+ *                - Explicit assign-then-compare `((x = foo()) < 0)` has no
+ *                  comparison as the assignment's rvalue.
+ *                - Explicit compare-then-assign `((x = (foo() < 0)))` has a
+ *                  parenthesized comparison as the rvalue.
+ *                - Parentheses that only wrap a subexpression of the comparison
+ *                  (e.g. `(foo())`) do not count as parenthesizing the
+ *                  comparison itself.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id cpp/ambiguous-assignment-of-comparison-in-condition
+ * @tags reliability
+ *       correctness
+ *       external/cwe/cwe-783
+ *       external/cwe/cwe-480
+ */
+
+import cpp
+
+/**
+ * The condition expression of a branching construct
+ * (if / while / do-while / for / ternary `?:`).
+ */
+Expr branchCondition() {
+  result = any(IfStmt s).getCondition()
+  or
+  result = any(WhileStmt s).getCondition()
+  or
+  result = any(DoStmt s).getCondition()
+  or
+  result = any(ForStmt s).getCondition()
+  or
+  result = any(ConditionalExpr c).getCondition()
+}
+
+/**
+ * Holds if `e` is the branch condition, or appears anywhere under it
+ * (including under `&&` / `||` / `!`, and through parentheses/conversions).
+ */
+predicate inBranchCondition(Expr e) {
+  e = branchCondition()
+  or
+  // Structural child of something already in a branch condition
+  exists(Expr parent |
+    parent.getAChild() = e and
+    inBranchCondition(parent)
+  )
+  or
+  // ParenthesisExpr and other conversions hang off getConversion(), not getAChild()
+  exists(Expr base |
+    base.getConversion+() = e and
+    inBranchCondition(base)
+  )
+  or
+  exists(Expr conv |
+    e.getConversion+() = conv and
+    inBranchCondition(conv)
+  )
+}
+
+from Assignment a, ComparisonOperation cmp
+where
+  // Core rule: assignment in a branch condition whose rvalue is an
+  // unparenthesized comparison. Covers `=` and all compound forms (`+=`, …)
+  // because Assignment is the common base class.
+  a.getRValue() = cmp and
+  not cmp.isParenthesised() and
+  inBranchCondition(a) and
+  // Skip AST from templates that were never instantiated (incomplete / non-code).
+  not a.isFromUninstantiatedTemplate(_)
+select a,
+  "Assignment (`" + a.getOperator() +
+    "`) in a branch condition has an unparenthesized comparison as its rvalue; " +
+    "likely meant `((x " + a.getOperator() + " expr) op val)` rather than " +
+    "`(x " + a.getOperator() + " expr op val)`."