C: Implicit compare-then-assign in branch conditions should be flagged

[ryao]

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.

A CodeQL check to detect this condition will likely be merged into openzfs/zfs soon:

openzfs/zfs#18899

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. In specific, all of these queries failed to detect a single test case in the test suite:

• cpp/assign-where-compare-meant
• cpp/compare-where-assign-meant
• cpp/comparison-precedence
• cpp/operator-precedence-logic-error-when-use-bool-type
• cpp/operator-precedence-logic-error-when-use-bitwise-logical-operations

It is unclear whether I should include the test suite in my OpenZFS PR, but I would be happy to include it in a CodeQL PR if upstream is willing to take it.

For reference, the test suite is simply this:

/* Synthetic fixtures for ambiguous assignment-of-comparison query.
 * TP = true positive (must flag). TN = true negative (must not flag).
 */

int foo(void);
int bar(void);
void *getp(void);

/* ========== TRUE POSITIVES (ambiguous / GCC-silent bugs) ========== */

/* TP1: classic PR-style bug — outer parens silence GCC */
int tp1(void) {
    int x;
    if ((x = foo() < 0))
        return x;
    return 0;
}

/* TP2: while form */
int tp2(void) {
    int x;
    while ((x = foo() != 0))
        return x;
    return 0;
}

/* TP3: no outer parens — GCC also warns; still the same pattern */
int tp3(void) {
    int x;
    if (x = foo() < 0)
        return x;
    return 0;
}

/* TP4: for condition */
int tp4(void) {
    int x;
    for (; (x = bar() <= 0); )
        return x;
    return 0;
}

/* TP5: && chain — assignment-of-comparison is an operand */
int tp5(void) {
    int x, y = 1;
    if ((x = foo() < 0) && y)
        return x;
    return 0;
}

/* TP6: ternary */
int tp6(void) {
    int x;
    return (x = foo() > 0) ? x : 0;
}

/* TP7: pointer equality on RHS, not parenthesized as comparison */
int tp7(void) {
    int x;
    if ((x = getp() == 0))
        return x;
    return 0;
}

/* TP8: (foo()) only parenthesizes the call, not the comparison — still ambiguous */
int tp8(void) {
    int x;
    if ((x = (foo()) < 0))
        return x;
    return 0;
}

/* TP9: do-while */
int tp9(void) {
    int x;
    do {
        x = 1;
    } while ((x = foo() < 0));
    return x;
}

/* TP10: || chain */
int tp10(void) {
    int x, y = 0;
    if (y || (x = foo() > 0))
        return x;
    return 0;
}

/* TP11: compound += with unparenthesized comparison */
int tp11(void) {
    int x = 0;
    if ((x += foo() < 0))
        return x;
    return 0;
}

/* TP12: compound -= */
int tp12(void) {
    int x = 0;
    while ((x -= bar() != 0))
        return x;
    return 0;
}

/* TP13: compound |= */
int tp13(void) {
    int x = 0;
    if ((x |= foo() > 0))
        return x;
    return 0;
}

/* TP14: compound &= in && chain */
int tp14(void) {
    int x = ~0, y = 1;
    if ((x &= foo() < 0) && y)
        return x;
    return 0;
}

/* TP15: assignment of unparenthesized comparison nested under outer comparison
 * (still ambiguous what was assigned; simplified rule flags this) */
int tp15(void) {
    int x;
    if ((x = foo() < 0) == 1)
        return x;
    return 0;
}

/* TP16: compound *= */
int tp16(void) {
    int x = 1;
    if ((x *= foo() != 0))
        return x;
    return 0;
}

/* TP17: compound ^= in ternary */
int tp17(void) {
    int x = 0;
    return (x ^= foo() < 0) ? x : 0;
}

/* TP18: compound <<= */
int tp18(void) {
    int x = 1;
    if ((x <<= bar() > 0))
        return x;
    return 0;
}

/* ========== TRUE NEGATIVES (explicit decision) ========== */

/* TN1: explicit assign-then-compare */
int tn1(void) {
    int x;
    if ((x = foo()) < 0)
        return x;
    return 0;
}

/* TN2: explicit assign then != */
int tn2(void) {
    int x;
    while ((x = foo()) != 0)
        return x;
    return 0;
}

/* TN3: intentional assign of non-comparison, parenthesized */
int tn3(void) {
    int x;
    if ((x = foo()))
        return x;
    return 0;
}

/* TN4: explicit compare-then-assign — parens around the comparison */
int tn4_explicit_bool_assign(void) {
    int flag;
    if ((flag = (foo() < 0)))
        return flag;
    return 0;
}

/* TN5: extra redundant parens around assign-then-compare — still explicit */
int tn5(void) {
    int x;
    if (((x = foo()) < 0))
        return x;
    return 0;
}

/* TN6: assign constant used as condition — different pattern, not ours */
int tn6(void) {
    int x;
    if ((x = 0))
        return x;
    return 0;
}

/* TN7: plain comparison */
int tn7(void) {
    int x = foo();
    if (x < 0)
        return x;
    return 0;
}

/* TN8: explicit compare-then-assign without outer double-paren style */
int tn8(void) {
    int x;
    if (x = (foo() < 0))  /* GCC may warn about assign as truth; still explicit intent */
        return x;
    return 0;
}

/* TN9: assign then compare with >= */
int tn9(void) {
    int x;
    for (; (x = bar()) >= 0; )
        return x;
    return 0;
}

/* TN10: logical and of two explicit assign-then-compares */
int tn10(void) {
    int x, y;
    if ((x = foo()) < 0 && (y = bar()) < 0)
        return x + y;
    return 0;
}

/* TN11: compound += then compare (explicit assign-then-compare) */
int tn11(void) {
    int x = 0;
    if ((x += foo()) < 0)
        return x;
    return 0;
}

/* TN12: compound += of parenthesized comparison (explicit compare-then-assign) */
int tn12(void) {
    int x = 0;
    if ((x += (foo() < 0)))
        return x;
    return 0;
}

/* TN13: compound &= of non-comparison */
int tn13(void) {
    int x = ~0;
    if ((x &= foo()))
        return x;
    return 0;
}

/* TN14: compound |= then compare */
int tn14(void) {
    int x = 0;
    while ((x |= bar()) != 0)
        return x;
    return 0;
}

/* TN15: compound -= of parenthesized comparison */
int tn15(void) {
    int x = 0;
    if ((x -= (foo() != 0)))
        return x;
    return 0;
}

/* TN16: compound <<= then compare */
int tn16(void) {
    int x = 1;
    if ((x <<= bar()) > 0)
        return x;
    return 0;
}

/* TP19: bug pattern inside a macro expansion — must still be reported */
#define BAD_CHECK(x) if (((x) = foo() < 0)) return (x)
int tp19_macro(void) {
    int x;
    BAD_CHECK(x);
    return 0;
}

/* TN17: explicit assign-then-compare inside a macro — must not report */
#define GOOD_CHECK(x) if (((x) = foo()) < 0) return (x)
int tn17_macro(void) {
    int x;
    GOOD_CHECK(x);
    return 0;
}

Is this something that should go into a PR, or would upstream prefer to handle this its own way?