From 2bc2126a1efcb9b94459c984ee79260c15cb9475 Mon Sep 17 00:00:00 2001
From: MikePopoloski <mike@popoloski.com>
Date: Sun, 24 Nov 2024 19:16:49 -0500
Subject: [PATCH] Add Wcase-not-wildcard and Wcasez-with-x warnings

---
 scripts/diagnostics.txt                       |  2 ++
 scripts/warning_docs.txt                      | 30 ++++++++++++++++
 .../ast/statements/ConditionalStatements.cpp  | 20 +++++++++++
 tests/unittests/ast/WarningTests.cpp          | 36 +++++++++++++++++--
 4 files changed, 85 insertions(+), 3 deletions(-)

diff --git a/scripts/diagnostics.txt b/scripts/diagnostics.txt
index ca43516eb..e108c26d9 100644
--- a/scripts/diagnostics.txt
+++ b/scripts/diagnostics.txt
@@ -874,6 +874,8 @@ warning case-enum CaseEnum "enumeration {} not handled in case statement"
 warning case-enum-explicit CaseEnumExplicit "enumeration {} not explicitly handled in case statement"
 warning case-dup CaseDup "'{}' statement contains duplicate items for value '{}' (the second one will never be matched)"
 warning case-overlap CaseOverlap "'{}' statement contains overlapping items"
+warning case-not-wildcard CaseNotWildcard "'case' statement item has unknown bits which don't work as wildcards; should this be a 'casex' or 'casez' statement?"
+warning casez-with-x CaseZWithX "'casez' statement item has 'x' bits which don't work as wildcards; should this be a 'casex' statement?"
 warning seq-no-match SeqNoMatch "sequence can never be matched"
 warning event-const EventExpressionConstant "edge expression is constant"
 warning empty-stmt EmptyStatement "extra ';' has no effect"
diff --git a/scripts/warning_docs.txt b/scripts/warning_docs.txt
index f793792ac..5e876312b 100644
--- a/scripts/warning_docs.txt
+++ b/scripts/warning_docs.txt
@@ -1594,3 +1594,33 @@ module m;
     end
 endmodule
 ```
+
+-Wcase-not-wildcard
+A normal case statement has items with integer literals that contain X or Z bits.
+These bits don't count as wildcards, so the designer may have meant for this to
+be a 'casex' or 'casez' statement instead.
+```
+module m;
+    logic [2:0] a;
+    initial begin
+        case (a)
+            3'b10?:;
+        endcase
+    end
+endmodule
+```
+
+-Wcasez-with-x
+A 'casez' statement has items with integer literals that contain X bits.
+These bits don't count as wildcards, so the designer may have meant for this to
+be a 'casex' statement instead.
+```
+module m;
+    logic [2:0] a;
+    initial begin
+        casez (a)
+            3'b10x:;
+        endcase
+    end
+endmodule
+```
diff --git a/source/ast/statements/ConditionalStatements.cpp b/source/ast/statements/ConditionalStatements.cpp
index 2b5d7e0c3..78265885c 100644
--- a/source/ast/statements/ConditionalStatements.cpp
+++ b/source/ast/statements/ConditionalStatements.cpp
@@ -395,6 +395,26 @@ Statement& CaseStatement::fromSyntax(Compilation& compilation, const CaseStateme
                     }
                 }
             }
+
+            if (condition == CaseStatementCondition::Normal ||
+                condition == CaseStatementCondition::WildcardJustZ) {
+                // If we're not in a wildcard case we should warn
+                // about integer literal items that have unknown bits.
+                // Similarly, if we're in a wildcard case with just Zs
+                // we should warn if we see Xs.
+                auto& unwrapped = item->unwrapImplicitConversions();
+                if (unwrapped.kind == ExpressionKind::IntegerLiteral) {
+                    auto& lit = unwrapped.as<IntegerLiteral>();
+                    if (condition == CaseStatementCondition::Normal &&
+                        lit.getValue().hasUnknown()) {
+                        context.addDiag(diag::CaseNotWildcard, item->sourceRange);
+                    }
+                    else if (condition == CaseStatementCondition::WildcardJustZ &&
+                             lit.getValue().countXs() > 0) {
+                        context.addDiag(diag::CaseZWithX, item->sourceRange);
+                    }
+                }
+            }
         }
     }
 
diff --git a/tests/unittests/ast/WarningTests.cpp b/tests/unittests/ast/WarningTests.cpp
index 5615b5970..b6fbf7305 100644
--- a/tests/unittests/ast/WarningTests.cpp
+++ b/tests/unittests/ast/WarningTests.cpp
@@ -1241,10 +1241,40 @@ endmodule
     compilation.addSyntaxTree(tree);
 
     auto& diags = compilation.getAllDiagnostics();
-    REQUIRE(diags.size() == 5);
+    REQUIRE(diags.size() == 7);
     CHECK(diags[0].code == diag::CaseOverlap);
     CHECK(diags[1].code == diag::CaseOverlap);
     CHECK(diags[2].code == diag::CaseOverlap);
-    CHECK(diags[3].code == diag::CaseOverlap);
-    CHECK(diags[4].code == diag::CaseOverlap);
+    CHECK(diags[3].code == diag::CaseZWithX);
+    CHECK(diags[4].code == diag::CaseZWithX);
+    CHECK(diags[5].code == diag::CaseOverlap);
+    CHECK(diags[6].code == diag::CaseOverlap);
+}
+
+TEST_CASE("Case items with unknowns that are not wildcards") {
+    auto tree = SyntaxTree::fromText(R"(
+module m;
+    logic [3:0] a;
+    initial begin
+        case (a)
+            4'b?10:;
+            4'bx10:;
+            default;
+        endcase
+        casez (a)
+            4'bx10:;
+            default;
+        endcase
+    end
+endmodule
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+
+    auto& diags = compilation.getAllDiagnostics();
+    REQUIRE(diags.size() == 3);
+    CHECK(diags[0].code == diag::CaseNotWildcard);
+    CHECK(diags[1].code == diag::CaseNotWildcard);
+    CHECK(diags[2].code == diag::CaseZWithX);
 }