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

[ryao]

Motivation and Context

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 #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 explicitly 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.

Description

We add a CodeQL check that will detect recurrences of the buggy code pattern that caused #18874.

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.

How Has This Been Tested?

It has 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.

It is unclear if the test suite should be included in the tree. The test suite was made by Grok 4.5 Build Beta based on prompts I made giving it several examples of variations, with explanations for how even more examples could be made. The test suite is visible in github/codeql#22286.

Types of Changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[behlendorf]

Yeah, we can add this and see what it catches.