Add Wpacked-array-conv

scripts/diagnostics.txt

@@ -748,6 +748,7 @@ warning width-trunc WidthTruncate "implicit conversion truncates from {} to {} b
 warning port-width-expand PortWidthExpand "implicit conversion of port connection expands from {} to {} bits"
 warning port-width-trunc PortWidthTruncate "implicit conversion of port connection truncates from {} to {} bits"
 warning implicit-conv ImplicitConvert "implicit conversion from {} to {}"
+warning packed-array-conv PackedArrayConv "implicit conversion from {} to {}"
 warning nonblocking-final NonblockingInFinal "nonblocking assignments in final blocks have no effect"
 warning port-coercion InputPortCoercion "input net port '{}' coerced to 'inout'"
 warning port-coercion OutputPortCoercion "output net port '{}' coerced to 'inout'"
@@ -1140,7 +1141,8 @@ group default = { real-underflow real-overflow vector-overflow int-overflow unco
                   raw-protect-eof protected-envelope specify-param dup-timing-path invalid-pulsestyle
                   negative-timing-limit bad-procedural-force duplicate-defparam implicit-port-type-mismatch
                   split-distweight-op dpi-pure-task multibit-edge unknown-sys-name unknown-library
-                  dup-config-rule unused-config-cell unused-config-instance specify-condition-expr }
+                  dup-config-rule unused-config-cell unused-config-instance specify-condition-expr
+                  packed-array-conv }
 
 group extra = { empty-member empty-stmt dup-import pointless-void-cast case-gen-none case-gen-dup
                 unused-result format-real ignored-slice task-ignored width-trunc dup-attr event-const
@@ -1153,7 +1155,7 @@ group pedantic = { empty-pattern implicit-net-port nonstandard-escape-code nonst
 
 group conversion = { width-trunc width-expand port-width-trunc port-width-expand implicit-conv
                      constant-conversion sign-conversion float-bool-conv int-bool-conv float-int-conv
-                     int-float-conv float-widen float-narrow }
+                     int-float-conv float-widen float-narrow packed-array-conv }
 
 group unused = { unused-def unused-net unused-implicit-net unused-variable undriven-net unassigned-variable
                  unused-but-set-net unused-but-set-variable unused-port undriven-port unused-argument

scripts/warning_docs.txt

@@ -1379,3 +1379,16 @@ primitive d_edge_ff (q, clock, data);
     endtable
 endprimitive
 ```
+
+-Wpacked-array-conv
+Indicates an implicit conversion between types with differing
+packed array dimensions, even if their overall bit width is the same.
+This will not warn for types with a single packed dimension; at least
+one of the types must have multiple declared dimensions.
+```
+module m;
+    logic [3:1][1:0] a;
+    logic [5:0] b;
+    initial b = a;
+endmodule
+```

source/ast/expressions/AssignmentExpressions.cpp

@@ -158,9 +158,13 @@ Expression* Expression::tryConnectPortArray(const ASTContext& context, const Typ
     // bit ranges from it -- the range select expression works on the declared
     // range of the packed array so a multidimensional wouldn't work correctly
     // without this conversion.
-    result = &ConversionExpression::makeImplicit(
-        context, comp.getType(*instPortWidth, result->type->getIntegralFlags()),
-        ConversionKind::Implicit, *result, nullptr, {});
+    //
+    // We set the UnevaluatedBranch flag here so that we don't get any warnings
+    // related to implicit conversions.
+    auto& vecType = comp.getType(*instPortWidth, result->type->getIntegralFlags());
+    result = &ConversionExpression::makeImplicit(context.resetFlags(ASTFlags::UnevaluatedBranch),
+                                                 vecType, ConversionKind::Implicit, *result,
+                                                 nullptr, {});
 
     // We have enough bits to assign each port on each instance, so now we just need
     // to pick the right ones. The spec says we start with all right hand indices

source/ast/expressions/ConversionExpression.cpp

@@ -11,6 +11,7 @@
 #include "slang/ast/Bitstream.h"
 #include "slang/ast/Compilation.h"
 #include "slang/ast/EvalContext.h"
+#include "slang/ast/expressions/LiteralExpressions.h"
 #include "slang/ast/expressions/MiscExpressions.h"
 #include "slang/ast/expressions/OperatorExpressions.h"
 #include "slang/ast/symbols/InstanceSymbols.h"
@@ -88,6 +89,21 @@ bool isUnionMemberType(const Type& left, const Type& right) {
     return false;
 }
 
+bool hasSameDims(const Type& left, const Type& right) {
+    if (left.getFixedRange().width() != right.getFixedRange().width())
+        return false;
+
+    auto le = left.getArrayElementType();
+    auto re = right.getArrayElementType();
+    if (bool(le) != bool(re))
+        return false;
+
+    if (!le)
+        return true;
+
+    return hasSameDims(*le, *re);
+}
+
 void checkImplicitConversions(const ASTContext& context, const Type& sourceType,
                               const Type& targetType, const Expression& op,
                               const Expression* parentExpr, SourceRange operatorRange,
@@ -97,6 +113,31 @@ void checkImplicitConversions(const ASTContext& context, const Type& sourceType,
                t.kind == SymbolKind::EnumType;
     };
 
+    auto isMultiDimArray = [](const Type& t) {
+        return t.kind == SymbolKind::PackedArrayType && t.getArrayElementType()->getBitWidth() > 1;
+    };
+
+    auto parentIsComparison = [&] {
+        if (parentExpr && parentExpr->kind == ExpressionKind::BinaryOp) {
+            switch (parentExpr->as<BinaryExpression>().op) {
+                case BinaryOperator::Equality:
+                case BinaryOperator::Inequality:
+                case BinaryOperator::CaseEquality:
+                case BinaryOperator::CaseInequality:
+                case BinaryOperator::GreaterThanEqual:
+                case BinaryOperator::GreaterThan:
+                case BinaryOperator::LessThanEqual:
+                case BinaryOperator::LessThan:
+                case BinaryOperator::WildcardEquality:
+                case BinaryOperator::WildcardInequality:
+                    return true;
+                default:
+                    break;
+            }
+        }
+        return false;
+    };
+
     auto addDiag = [&](DiagCode code) -> Diagnostic& {
         auto& diag = context.addDiag(code, op.sourceRange);
         if (operatorRange.start())
@@ -127,6 +168,30 @@ void checkImplicitConversions(const ASTContext& context, const Type& sourceType,
             return;
         }
 
+        // Warn for conversions between packed arrays of differing
+        // dimension counts or sizes.
+        if (isMultiDimArray(lt) || isMultiDimArray(rt) && !hasSameDims(lt, rt)) {
+            // Avoid warning for assignments or comparisons with 0 or '0, '1.
+            auto isZeroOrUnsized = [](const Expression& e) {
+                auto expr = &e.unwrapImplicitConversions();
+                if (expr->kind == ExpressionKind::ConditionalOp) {
+                    if (auto known = expr->as<ConditionalExpression>().knownSide())
+                        expr = known;
+                }
+
+                return expr->kind == ExpressionKind::UnbasedUnsizedIntegerLiteral ||
+                       (expr->kind == ExpressionKind::IntegerLiteral &&
+                        bool(expr->as<IntegerLiteral>().getValue() == 0));
+            };
+
+            if (!isZeroOrUnsized(op) &&
+                (!parentIsComparison() ||
+                 !isZeroOrUnsized(parentExpr->as<BinaryExpression>().right()))) {
+                addDiag(diag::PackedArrayConv) << sourceType << targetType;
+                return;
+            }
+        }
+
         // Check to rule out false positives: try to eval as a constant.
         // We'll ignore any constants, because they will get their own more
         // fine grained warning during eval.
@@ -136,27 +201,7 @@ void checkImplicitConversions(const ASTContext& context, const Type& sourceType,
         // Warn for sign conversions.
         if (lt.isSigned() != rt.isSigned()) {
             // Comparisons get their own warning elsewhere.
-            bool isComparison = false;
-            if (parentExpr && parentExpr->kind == ExpressionKind::BinaryOp) {
-                switch (parentExpr->as<BinaryExpression>().op) {
-                    case BinaryOperator::Equality:
-                    case BinaryOperator::Inequality:
-                    case BinaryOperator::CaseEquality:
-                    case BinaryOperator::CaseInequality:
-                    case BinaryOperator::GreaterThanEqual:
-                    case BinaryOperator::GreaterThan:
-                    case BinaryOperator::LessThanEqual:
-                    case BinaryOperator::LessThan:
-                    case BinaryOperator::WildcardEquality:
-                    case BinaryOperator::WildcardInequality:
-                        isComparison = true;
-                        break;
-                    default:
-                        break;
-                }
-            }
-
-            if (!isComparison)
+            if (!parentIsComparison())
                 addDiag(diag::SignConversion) << sourceType << targetType;
         }
 

tests/unittests/ast/ClassTests.cpp

@@ -1918,7 +1918,7 @@ class D;
     bit [3:0][2:1] B [5:1][4];
     constraint c1 {
         foreach (A[i, j, k]) A[i][j][k] inside {2,4,8,16};
-        foreach (B[q, r, , s]) B[q][r] inside {1,2,3};
+        foreach (B[q, r, , s]) 32'(B[q][r]) inside {1,2,3};
     }
 
     randc int b;

tests/unittests/ast/ExpressionTests.cpp

@@ -783,13 +783,17 @@ endmodule
     compilation.addSyntaxTree(tree);
 
     auto& diags = compilation.getAllDiagnostics();
-    REQUIRE(diags.size() == 6);
+    REQUIRE(diags.size() == 10);
     CHECK(diags[0].code == diag::IndexOOB);
     CHECK(diags[1].code == diag::IndexOOB);
-    CHECK(diags[2].code == diag::RangeOOB);
+    CHECK(diags[2].code == diag::PackedArrayConv);
     CHECK(diags[3].code == diag::RangeOOB);
-    CHECK(diags[4].code == diag::RangeOOB);
+    CHECK(diags[4].code == diag::PackedArrayConv);
     CHECK(diags[5].code == diag::RangeOOB);
+    CHECK(diags[6].code == diag::PackedArrayConv);
+    CHECK(diags[7].code == diag::RangeOOB);
+    CHECK(diags[8].code == diag::PackedArrayConv);
+    CHECK(diags[9].code == diag::RangeOOB);
 }
 
 TEST_CASE("Empty concat error") {
@@ -2827,7 +2831,7 @@ endmodule
 TEST_CASE("Index oob tryEval regress GH #602") {
     auto tree = SyntaxTree::fromText(R"(
 module top #(
-    parameter [2:0][4:0] IDX_MAP = {5'd1, 5'd3, 5'd4}
+    parameter [2:0][4:0] IDX_MAP = '{5'd1, 5'd3, 5'd4}
 );
     logic [4:0] sig;
 

tests/unittests/ast/TypeTests.cpp

@@ -947,7 +947,7 @@ module m;
     union packed { logic [3:0] a; bit [1:4] b; } [4:1] u;
     enum { A, B, C } [4:1] e;
 
-    initial e = A;
+    initial e[1] = A;
 endmodule
 )");