From 0e6a1a27fe4bbbfc56c5c71ad8e2385faa9ae2f1 Mon Sep 17 00:00:00 2001
From: MikePopoloski <mike@popoloski.com>
Date: Sun, 26 May 2024 09:12:22 -0400
Subject: [PATCH] Be better about preserving the types of packed structs and
 unions used in binary operators

---
 .../ast/expressions/OperatorExpressions.cpp   | 13 +++++++-----
 tests/unittests/ast/ExpressionTests.cpp       |  2 +-
 tests/unittests/ast/WarningTests.cpp          | 21 +++++++++++++++++++
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/source/ast/expressions/OperatorExpressions.cpp b/source/ast/expressions/OperatorExpressions.cpp
index 8022a02d5..ac09c4392 100644
--- a/source/ast/expressions/OperatorExpressions.cpp
+++ b/source/ast/expressions/OperatorExpressions.cpp
@@ -114,6 +114,14 @@ const Type* Expression::binaryOperatorType(Compilation& compilation, const Type*
     if (!lt->isNumeric() || !rt->isNumeric())
         return &compilation.getErrorType();
 
+    // If both sides are the same type just use that type.
+    // NOTE: This specifically ignores the forceFourState option for enums,
+    // as that better matches expectations. This area of the LRM is underspecified.
+    if (lt->isMatching(*rt)) {
+        if (!forceFourState || lt->isFourState() || lt->isEnum())
+            return lt;
+    }
+
     // Figure out what the result type of an arithmetic binary operator should be. The rules are:
     // - If either operand is real, the result is real
     // - Otherwise, if either operand is shortreal, the result is shortreal
@@ -131,11 +139,6 @@ const Type* Expression::binaryOperatorType(Compilation& compilation, const Type*
             result = &compilation.getShortRealType();
         }
     }
-    else if (lt->isEnum() && rt->isEnum() && lt->isMatching(*rt)) {
-        // If both sides are the same enum type, preserve that in the output type.
-        // NOTE: This specifically ignores the forceFourState option.
-        return lt;
-    }
     else {
         bitwidth_t width = std::max(lt->getBitWidth(), rt->getBitWidth());
 
diff --git a/tests/unittests/ast/ExpressionTests.cpp b/tests/unittests/ast/ExpressionTests.cpp
index 609762039..7ac4eb98f 100644
--- a/tests/unittests/ast/ExpressionTests.cpp
+++ b/tests/unittests/ast/ExpressionTests.cpp
@@ -244,7 +244,7 @@ TEST_CASE("Expression types") {
     CHECK(typeof("su2 == su2") == "bit");
     CHECK(typeof("EVAL1 + 5") == "int");
     CHECK(typeof("up + 5") == "logic[31:0]");
-    CHECK(typeof("up + up") == "logic[1:0]");
+    CHECK(typeof("up + up") == "union packed{logic[1:0] a;bit[0:1] b;}u$1");
 
     // Unpacked arrays
     declare("bit [7:0] arr1 [2];");
diff --git a/tests/unittests/ast/WarningTests.cpp b/tests/unittests/ast/WarningTests.cpp
index c4a9c35e5..10f3ca7b6 100644
--- a/tests/unittests/ast/WarningTests.cpp
+++ b/tests/unittests/ast/WarningTests.cpp
@@ -944,3 +944,24 @@ endmodule
     CHECK(diags[4].code == diag::SignCompare);
     CHECK(diags[5].code == diag::BitwiseOpMismatch);
 }
+
+TEST_CASE("Binary operator with struct type preserves the type") {
+    auto tree = SyntaxTree::fromText(R"(
+module top;
+  typedef struct packed {
+      logic [1:0] a;
+      logic [1:0] b;
+  } s_t;
+
+  s_t a, b, c, d;
+
+  always_comb begin
+      d = a |  b | c;
+  end
+endmodule
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+    NO_COMPILATION_ERRORS;
+}