Analyser: force parentheses for ambiguous operator combinations

chqrlie · bvdberg · commit 04ff3b99503e · 2025-06-26T09:39:54.000+02:00
* flag use of combined operators with unintuitive relative precedence
  as constraint violations (analyser errors)
* fix code like `if (buf.st_mode &amp; S_IFMT == S_IFREG)` that now requires
  parentheses:  `if ((buf.st_mode &amp; S_IFMT) == S_IFREG)`
* update tests
diff --git a/analyser/module_analyser_binop.c2 b/analyser/module_analyser_binop.c2
@@ -16,6 +16,7 @@
 module module_analyser;
 
 import ast local;
+import c2_prec local;
 import conversion_checker;
 import ctv_analyser;
 
@@ -400,6 +401,70 @@ const u8[elemsof(TypeKind)][elemsof(TypeKind)] BinOpConvComparision = {
     // Alias + Module are zero
 }
 
+const Prec[elemsof(BinaryOpcode)] Prec_table = {
+    [BinaryOpcode.Multiply]    = Multiplicative,
+    [BinaryOpcode.Divide]      = Multiplicative,
+    [BinaryOpcode.Remainder]   = Multiplicative,
+    [BinaryOpcode.Add]         = Additive,
+    [BinaryOpcode.Subtract]    = Additive,
+    [BinaryOpcode.ShiftLeft]   = Shift,
+    [BinaryOpcode.ShiftRight]  = Shift,
+    [BinaryOpcode.LessThan]    = Relational,
+    [BinaryOpcode.GreaterThan] = Relational,
+    [BinaryOpcode.LessEqual]   = Relational,
+    [BinaryOpcode.GreaterEqual] = Relational,
+    [BinaryOpcode.Equal]       = Relational,
+    [BinaryOpcode.NotEqual]    = Relational,
+    [BinaryOpcode.And]         = Bitwise,
+    [BinaryOpcode.Xor]         = Bitwise,
+    [BinaryOpcode.Or]          = Bitwise,
+    [BinaryOpcode.LAnd]        = LogicalAndOr,
+    [BinaryOpcode.LOr]         = LogicalAndOr,
+    [BinaryOpcode.Assign]      = Assignment,
+    [BinaryOpcode.MulAssign]   = Assignment,
+    [BinaryOpcode.DivAssign]   = Assignment,
+    [BinaryOpcode.RemAssign]   = Assignment,
+    [BinaryOpcode.AddAssign]   = Assignment,
+    [BinaryOpcode.SubAssign]   = Assignment,
+    [BinaryOpcode.ShlAssign]   = Assignment,
+    [BinaryOpcode.ShrAssign]   = Assignment,
+    [BinaryOpcode.AndAssign]   = Assignment,
+    [BinaryOpcode.XorAssign]   = Assignment,
+    [BinaryOpcode.OrAssign]    = Assignment,
+}
+
+const C_Prec[elemsof(BinaryOpcode)] C_Prec_table = {
+    [BinaryOpcode.Multiply]    = Multiplicative,
+    [BinaryOpcode.Divide]      = Multiplicative,
+    [BinaryOpcode.Remainder]   = Multiplicative,
+    [BinaryOpcode.Add]         = Additive,
+    [BinaryOpcode.Subtract]    = Additive,
+    [BinaryOpcode.ShiftLeft]   = Shift,
+    [BinaryOpcode.ShiftRight]  = Shift,
+    [BinaryOpcode.LessThan]    = Relative,
+    [BinaryOpcode.GreaterThan] = Relative,
+    [BinaryOpcode.LessEqual]   = Relative,
+    [BinaryOpcode.GreaterEqual] = Relative,
+    [BinaryOpcode.Equal]       = Equality,
+    [BinaryOpcode.NotEqual]    = Equality,
+    [BinaryOpcode.And]         = And,
+    [BinaryOpcode.Xor]         = Xor,
+    [BinaryOpcode.Or]          = Or,
+    [BinaryOpcode.LAnd]        = LogicalAnd,
+    [BinaryOpcode.LOr]         = LogicalOr,
+    [BinaryOpcode.Assign]      = Assignment,
+    [BinaryOpcode.MulAssign]   = Assignment,
+    [BinaryOpcode.DivAssign]   = Assignment,
+    [BinaryOpcode.RemAssign]   = Assignment,
+    [BinaryOpcode.AddAssign]   = Assignment,
+    [BinaryOpcode.SubAssign]   = Assignment,
+    [BinaryOpcode.ShlAssign]   = Assignment,
+    [BinaryOpcode.ShrAssign]   = Assignment,
+    [BinaryOpcode.AndAssign]   = Assignment,
+    [BinaryOpcode.XorAssign]   = Assignment,
+    [BinaryOpcode.OrAssign]    = Assignment,
+}
+
 fn QualType Analyser.checkBinopComparison(Analyser* ma, BinaryOperator* b, QualType lhs, QualType rhs) {
     QualType lcanon = lhs.getCanonicalType();
     QualType rcanon = rhs.getCanonicalType();
@@ -475,6 +540,16 @@ fn QualType Analyser.checkPointerFuncComparison(Analyser* ma, BinaryOperator* b,
     return QualType_Invalid;
 }
 
+fn bool incompatible_opcodes(BinaryOpcode op1, BinaryOpcode op2) {
+    C_Prec c_p1 = C_Prec_table[op1];
+    C_Prec c_p2 = C_Prec_table[op2];
+    Prec c2_p1 = Prec_table[op1];
+    Prec c2_p2 = Prec_table[op2];
+    if (c2_p1 == c2_p2) return c2_p1 == Relational || c_p1 != c_p2;
+    if (c2_p1 < c2_p2) return c_p1 >= c_p2;
+    return c_p1 <= c_p2;
+}
+
 fn QualType Analyser.analyseBinaryOperator(Analyser* ma, Expr** e_ptr) {
     Expr* e = *e_ptr;
     BinaryOperator* b = (BinaryOperator*)e;
@@ -510,6 +585,23 @@ fn QualType Analyser.analyseBinaryOperator(Analyser* ma, Expr** e_ptr) {
     Expr* lhs = b.getLHS();
     Expr* rhs = b.getRHS();
 
+    if (lhs.isBinaryOperator()) {
+        BinaryOpcode op1 = cast<BinaryOperator*>(lhs).getOpcode();
+        if (incompatible_opcodes(opcode, op1)) {
+            ma.error(lhs.getLoc(), "operators '%s' and '%s' do not combine without parentheses",
+                     opcode.str(), op1.str());
+            return QualType_Invalid;
+        }
+    }
+    if (rhs.isBinaryOperator()) {
+        BinaryOpcode op2 = cast<BinaryOperator*>(rhs).getOpcode();
+        if (incompatible_opcodes(opcode, op2)) {
+            ma.error(rhs.getLoc(), "operators '%s' and '%s' do not combine without parentheses",
+                     opcode.str(), op2.str());
+            return QualType_Invalid;
+        }
+    }
+
     if (!validBinOpKind(ltype) || ltype.isVoid()) {
         QualType tl = lhs.getType();
         ma.error(lhs.getLoc(), "invalid operand to binary expression '%s'", tl.diagName());
diff --git a/ast/binary_operator.c2 b/ast/binary_operator.c2
@@ -90,6 +90,10 @@ public fn bool BinaryOpcode.isComparison(BinaryOpcode opcode) {
     return opcode >= BinaryOpcode.LessThan && opcode <= BinaryOpcode.NotEqual;
 }
 
+public fn const char* BinaryOpcode.str(BinaryOpcode opcode) {
+    return binaryOpcode_names[opcode];
+}
+
 type BinaryOperatorBits struct {
     u32 : NumExprBits;
     u32 kind : 5;
diff --git a/ast/expr.c2 b/ast/expr.c2
@@ -237,7 +237,7 @@ public fn bool Expr.isTilde(const Expr* e) {
     return false;
 }
 
-fn bool Expr.isBinaryOperator(const Expr* e) {
+public fn bool Expr.isBinaryOperator(const Expr* e) {
     return e.getKind() == ExprKind.BinaryOperator;
 }
 
diff --git a/ast_utils/c2_prec.c2 b/ast_utils/c2_prec.c2
@@ -0,0 +1,52 @@
+/* Copyright 2022-2025 Bas van den Berg
+ * Copyright 2025 Charlie Gordon
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+module c2_prec;
+
+/// PrecedenceLevels - These have been altered from C99 to C2
+/// In particular, addition now comes after bitwise and shifts
+/// Bitwise is directly after shift and equality and relational have
+/// the same precedence.
+
+public type Prec enum u8 {
+    Unknown,        // Not binary operator.
+    Comma,          // ,
+    Assignment,     // =, *=, /=, %=, +=, -=, <<=, >>=, &=, ^=, |=
+    Conditional,    // ?
+    LogicalAndOr,   // &&, ||
+    Relational,     // ==, !=, >=, <=, >, <
+    Additive,       // -, +
+    Bitwise,        // ^, |, &
+    Shift,          // <<, >>
+    Multiplicative, // *, /, %
+}
+
+public type C_Prec enum u8 {
+    Unknown,        // Not binary operator.
+    Comma,          // ,
+    Assignment,     // =, *=, /=, %=, +=, -=, <<=, >>=, &=, ^=, |=
+    Conditional,    // ?
+    LogicalOr,      // ||
+    LogicalAnd,     // &&
+    Or,             // |
+    Xor,            // ^
+    And,            // &
+    Equality,       // !=, ==
+    Relative,       // >=, <=, >, <
+    Shift,          // <<, >>
+    Additive,       // -, +
+    Multiplicative, // *, /, %
+}
diff --git a/common/file/reader.c2 b/common/file/reader.c2
@@ -51,7 +51,7 @@ public fn bool Reader.open(Reader* file, const char* filename) {
         return false;
     }
 
-    if (statbuf.st_mode & S_IFMT != S_IFREG) {
+    if ((statbuf.st_mode & S_IFMT) != S_IFREG) {
         file.error = Err_not_a_file;
         close(fd);
         return false;
diff --git a/common/utils.c2 b/common/utils.c2
@@ -72,7 +72,7 @@ public fn bool findProjectDir(PathInfo* info) {
             }
         } else {
             // must be file, not dir
-            if (buf.st_mode & S_IFMT == S_IFREG) {
+            if ((buf.st_mode & S_IFMT) == S_IFREG) {
                 char* path_prefix = base_path + strlen(path);
                 if (*path_prefix == '/') path_prefix++;
                 if (info) {
diff --git a/parser/c2_parser_expr.c2 b/parser/c2_parser_expr.c2
@@ -17,6 +17,7 @@ module c2_parser;
 
 //import ast_builder local;   // BUG: should have to be imported
 import ast local;
+import c2_prec local;
 import constants;
 //import number_radix local;    // BUG: should have to be imported
 import token local;
@@ -25,24 +26,6 @@ import src_loc local;
 import ctype local;
 import string;
 
-/// PrecedenceLevels - These have been altered from C99 to C2
-/// In particular, addition now comes after bitwise and shifts
-/// Bitwise is directly after shift and equality and relational have
-/// the same precedence.
-
-type Prec enum u8 {
-    Unknown         = 0,    // Not binary operator.
-    Comma           = 1,    // ,
-    Assignment      = 2,    // =, *=, /=, %=, +=, -=, <<=, >>=, &=, ^=, |=
-    Conditional     = 3,    // ?
-    LogicalAndOr    = 4,    // &&, ||
-    Relational      = 5,    // ==, !=, >=, <=, >, <
-    Additive        = 6,    // -, +
-    Bitwise         = 7,    // ^, |, &
-    Shift           = 8,    // <<, >>
-    Multiplicative  = 9,    // *, /, %
-}
-
 fn Expr* Parser.parseExpr(Parser* p) {
     Expr* lhs = p.parseAssignmentExpression();
     if (lhs.isInitlistAssignment())
diff --git a/recipe.txt b/recipe.txt
@@ -172,7 +172,10 @@ executable c2c
     (ast)
     (yaml)
 
+    ast_utils/c2_prec.c2
+
     (common)
+
     common/build_file.c2
     common/bit_array.c2
     common/component_sorter.c2
diff --git a/test/c_generator/expr/check_shift.c2t b/test/c_generator/expr/check_shift.c2t
@@ -6,7 +6,7 @@
 module test;
 
 public fn i32 main(i32 argc, const char** argv) {
-    i32 a = 1 << 2 + 1;
+    i32 a = 1 << (2 + 1);
     i32 b = (1 << 2) + 1;
     i32 c = (1 >> 2) + 1;
     return 0;
@@ -16,7 +16,7 @@ public fn i32 main(i32 argc, const char** argv) {
 
 int32_t main(int32_t argc, const char** argv)
 {
-    int32_t a = ((1 << 2) + 1);
+    int32_t a = (1 << ((2 + 1)));
     int32_t b = (((1 << 2)) + 1);
     int32_t c = (((1 >> 2)) + 1);
 
diff --git a/test/c_generator/expr/combine_errors.c2 b/test/c_generator/expr/combine_errors.c2
@@ -0,0 +1,9 @@
+module test;
+
+i32 a = 1 << 2 + 1; // @error{operators '+' and '<<' do not combine without parentheses}
+i32 b = 1 << 2 - 1; // @error{operators '-' and '<<' do not combine without parentheses}
+i32 c = 1 >> 2 + 1; // @error{operators '+' and '>>' do not combine without parentheses}
+i32 d = 1 >> 2 - 1; // @error{operators '-' and '>>' do not combine without parentheses}
+i32 a1 = 1 ^ 2 | 3 & 4; // @error{operators '|' and '^' do not combine without parentheses}
+i32 a2 = 1 && 2 || 3 && 4 || 5; // @error{operators '||' and '&&' do not combine without parentheses}
+i32 a3 = 1 ^ 2 | 3 & 4 == 5 || 6 <= 5; // @error{operators '|' and '^' do not combine without parentheses}
diff --git a/test/c_generator/expr/prec_expr.c2t b/test/c_generator/expr/prec_expr.c2t
@@ -6,29 +6,35 @@
 module test;
 
 public fn i32 main(i32 argc, const char** argv) {
-    i32 a = 1 << 2 + 3;
-        a = 1 ^ 2 | 3 & 4;
-        a = 1 && 2 || 3 && 4 || 5;
-        a = 1 ^ 2 | 3 & 4 == 5 || 6 <= 5;
+    i32 a;
+    // a = 1 << 2 + 3;
+    // a = 1 << (2 + 3); // C
+    a = (1 << 2) + 3;    // C2
+
+    // a = 1 ^ 2 | 3 & 4;
+    // a = (1 ^ 2) | (3 & 4); // C
+    a = ((1 ^ 2) | 3) & 4;    // C2
+
+    // a = ((1 && 2) || (3 && 4)) || 5; // C
+    // a = 1 && 2 || 3 && 4 || 5;
+    a = (((1 && 2) || 3) && 4) || 5;    // C2
+
+    // a = 1 ^ 2 | 3 & 4 == 5 || 6 <= 5;
+    // a = ((1 ^ 2) | (3 & (4 == 5))) || (6 <= 5); // C
+    a = (((1 ^ 2) | 3) & 4) == 5 || 6 <= 5; // C2
+
     return 0;
 }
 
 // @expect{atleast, cgen/build.c}
 
 int32_t main(int32_t argc, const char** argv)
 {
-    int32_t a = ((1 << 2) + 3);
-//  int32_t a = (1 << (2 + 3)); C99
-
-    a = (((1 ^ 2) | 3) & 4);
-//  a = ((1 ^ 2) | (3 & 4)); C99
-
-    a = ((((1 && 2) || 3) && 4) || 5);
-//  a = (((1 && 2) || (3 && 4)) || 5); C99
-
-    a = (((((1 ^ 2) | 3) & 4) == 5) || (6 <= 5));
-//  a = (((1 ^ 2) | (3 & (4 == 5))) || (6 <= 5)); C99
-
+    int32_t a;
+    a = (((1 << 2)) + 3);
+    a = (((((1 ^ 2)) | 3)) & 4);
+    a = (((((((1 && 2)) || 3)) && 4)) || 5);
+    a = ((((((((1 ^ 2)) | 3)) & 4)) == 5) || (6 <= 5));
     return 0;
 }
 
diff --git a/test/expr/constant.c2 b/test/expr/constant.c2
@@ -27,19 +27,19 @@ static_assert(1 << 3, 8);
 static_assert(1 << 4 >> 1, 8);
 static_assert(4 + 2 * 2, 8);
 static_assert(4 + 8 / 2, 8);
-static_assert(4 + 1 << 2, 8);
+static_assert(4 + 1 << 2, 8); // @error{operators '+' and '<<' do not combine without parentheses}
 static_assert(true, 1);
 static_assert(4 || 4, 1);
 static_assert(4 && 4, 1);
-static_assert(4 || 4 && 4, 1);
+static_assert(4 || 4 && 4, 1); // @error{operators '&&' and '||' do not combine without parentheses}
 static_assert(+true, 1);
 static_assert(+(4 || 4), 1);
 static_assert(+(4 && 4), 1);
-static_assert(+(4 || 4 && 4), 1);
+static_assert(+(4 || 4 && 4), 1); // @error{operators '&&' and '||' do not combine without parentheses}
 static_assert(0+true, 1);
 static_assert(0+(4 || 4), 1);
 static_assert(0+(4 && 4), 1);
-static_assert(0+(4 || 4 && 4), 1);
+static_assert(0+(4 || 4 && 4), 1); // @error{operators '&&' and '||' do not combine without parentheses}
 static_assert(1 ? 8 : 3, 8);
 static_assert(1 < 2, 1);
 static_assert(1 > 0, 1);
diff --git a/test/literals/init_operators.c2 b/test/literals/init_operators.c2
@@ -16,7 +16,7 @@ fn i32 test2() {
 }
 
 fn void test3() {
-    i32 a = 1 + 3 * 4 << 1;
+    i32 a = 1 + 3 * 4 << 1; // @error{operators '+' and '<<' do not combine without parentheses}
     i32 b = 1 + 3 * 4 && 1;
 }
 
diff --git a/tools/tester/tester.c2 b/tools/tester/tester.c2
@@ -356,9 +356,9 @@ public fn i32 main(i32 argc, char** argv) {
         usize len = strlen(target);
         if (len > 1 && target[len - 1] == '/') target[--len] = '\0';
 
-        if (statbuf.st_mode & S_IFMT == S_IFREG) {
+        if ((statbuf.st_mode & S_IFMT) == S_IFREG) {
             handle_file(queue, target);
-        } else if (statbuf.st_mode & S_IFMT == S_IFDIR) {
+        } else if ((statbuf.st_mode & S_IFMT) == S_IFDIR) {
             handle_dir(queue, target);
         } else {
             print_error("argument must be a regular file a directory: %s", target);

Original file line number	Diff line number	Diff line change
`@@ -237,7 +237,7 @@ public fn bool Expr.isTilde(const Expr* e) {`
`237`	`237`	`return false;`
`238`	`238`	`}`
`239`	`239`
`240`		`-fn bool Expr.isBinaryOperator(const Expr* e) {`
	`240`	`+public fn bool Expr.isBinaryOperator(const Expr* e) {`
`241`	`241`	`return e.getKind() == ExprKind.BinaryOperator;`
`242`	`242`	`}`
`243`	`243`
Original file line number	Diff line number	Diff line change
`@@ -51,7 +51,7 @@ public fn bool Reader.open(Reader* file, const char* filename) {`
`51`	`51`	`return false;`
`52`	`52`	`}`
`53`	`53`
`54`		`- if (statbuf.st_mode & S_IFMT != S_IFREG) {`
	`54`	`+ if ((statbuf.st_mode & S_IFMT) != S_IFREG) {`
`55`	`55`	`file.error = Err_not_a_file;`
`56`	`56`	`close(fd);`
`57`	`57`	`return false;`
Original file line number	Diff line number	Diff line change
`@@ -72,7 +72,7 @@ public fn bool findProjectDir(PathInfo* info) {`
`72`	`72`	`}`
`73`	`73`	`} else {`
`74`	`74`	`// must be file, not dir`
`75`		`- if (buf.st_mode & S_IFMT == S_IFREG) {`
	`75`	`+ if ((buf.st_mode & S_IFMT) == S_IFREG) {`
`76`	`76`	`char* path_prefix = base_path + strlen(path);`
`77`	`77`	`if (*path_prefix == '/') path_prefix++;`
`78`	`78`	`if (info) {`