Fix spurious width warnings for conditional operators within an expli…

…cit cast
MikePopoloski · Jan 30, 2025 · 1908099 · 1908099
1 parent 2e39ffe
commit 1908099
Show file tree

Hide file tree

Showing 9 changed files with 41 additions and 16 deletions.
diff --git a/include/slang/ast/expressions/LiteralExpressions.h b/include/slang/ast/expressions/LiteralExpressions.h
@@ -100,7 +100,8 @@ class SLANG_EXPORT UnbasedUnsizedIntegerLiteral : public Expression {
     SVInt getValue() const;
 
     ConstantValue evalImpl(EvalContext& context) const;
-    bool propagateType(const ASTContext& context, const Type& newType, SourceRange opRange);
+    bool propagateType(const ASTContext& context, const Type& newType, SourceRange opRange,
+                       ConversionKind conversionKind);
     std::optional<bitwidth_t> getEffectiveWidthImpl() const;
     EffectiveSign getEffectiveSignImpl(bool isForConversion) const;
 

diff --git a/include/slang/ast/expressions/MiscExpressions.h b/include/slang/ast/expressions/MiscExpressions.h
@@ -306,7 +306,8 @@ class SLANG_EXPORT MinTypMaxExpression : public Expression {
     Expression& selected() { return *selected_; }
 
     ConstantValue evalImpl(EvalContext& context) const;
-    bool propagateType(const ASTContext& context, const Type& newType, SourceRange opRange);
+    bool propagateType(const ASTContext& context, const Type& newType, SourceRange opRange,
+                       ConversionKind conversionKind);
     std::optional<bitwidth_t> getEffectiveWidthImpl() const;
     EffectiveSign getEffectiveSignImpl(bool isForConversion) const;
 

diff --git a/include/slang/ast/expressions/OperatorExpressions.h b/include/slang/ast/expressions/OperatorExpressions.h
@@ -37,7 +37,8 @@ class SLANG_EXPORT UnaryExpression : public Expression {
     Expression& operand() { return *operand_; }
 
     ConstantValue evalImpl(EvalContext& context) const;
-    bool propagateType(const ASTContext& context, const Type& newType, SourceRange opRange);
+    bool propagateType(const ASTContext& context, const Type& newType, SourceRange opRange,
+                       ConversionKind conversionKind);
     std::optional<bitwidth_t> getEffectiveWidthImpl() const;
     EffectiveSign getEffectiveSignImpl(bool isForConversion) const;
 
@@ -93,7 +94,8 @@ class SLANG_EXPORT BinaryExpression : public Expression {
     Expression& right() { return *right_; }
 
     ConstantValue evalImpl(EvalContext& context) const;
-    bool propagateType(const ASTContext& context, const Type& newType, SourceRange opRange);
+    bool propagateType(const ASTContext& context, const Type& newType, SourceRange opRange,
+                       ConversionKind conversionKind);
     std::optional<bitwidth_t> getEffectiveWidthImpl() const;
     EffectiveSign getEffectiveSignImpl(bool isForConversion) const;
 
@@ -158,7 +160,8 @@ class SLANG_EXPORT ConditionalExpression : public Expression {
     Expression& right() { return *right_; }
 
     ConstantValue evalImpl(EvalContext& context) const;
-    bool propagateType(const ASTContext& context, const Type& newType, SourceRange opRange);
+    bool propagateType(const ASTContext& context, const Type& newType, SourceRange opRange,
+                       ConversionKind conversionKind);
     std::optional<bitwidth_t> getEffectiveWidthImpl() const;
     EffectiveSign getEffectiveSignImpl(bool isForConversion) const;
 
@@ -394,7 +397,8 @@ class SLANG_EXPORT ValueRangeExpression : public Expression {
     Expression& right() { return *right_; }
 
     ConstantValue evalImpl(EvalContext& context) const;
-    bool propagateType(const ASTContext& context, const Type& newType, SourceRange opRange);
+    bool propagateType(const ASTContext& context, const Type& newType, SourceRange opRange,
+                       ConversionKind conversionKind);
 
     ConstantValue checkInside(EvalContext& context, const ConstantValue& val) const;
 

diff --git a/source/ast/Expression.cpp b/source/ast/Expression.cpp
@@ -160,7 +160,7 @@ struct Expression::PropagationVisitor {
         // check if the conversion should be pushed further down the tree. Otherwise we
         // should insert the implicit conversion here.
         bool needConversion = !newType.isEquivalent(*expr.type);
-        if constexpr (requires { expr.propagateType(context, newType, opRange); }) {
+        if constexpr (requires { expr.propagateType(context, newType, opRange, conversionKind); }) {
             if ((newType.isFloating() && expr.type->isFloating()) ||
                 (newType.isIntegral() && expr.type->isIntegral()) || newType.isString() ||
                 expr.kind == ExpressionKind::ValueRange) {
@@ -174,7 +174,7 @@ struct Expression::PropagationVisitor {
                     updateRange(expr);
                 }
 
-                if (expr.propagateType(context, newType, opRange)) {
+                if (expr.propagateType(context, newType, opRange, conversionKind)) {
                     // We propagated the type successfully so we don't need a conversion.
                     // We should however clear out any constant value that may have been
                     // stored here, since it may no longer be valid given the new type.

diff --git a/source/ast/expressions/ConversionExpression.cpp b/source/ast/expressions/ConversionExpression.cpp
@@ -303,6 +303,8 @@ static bool actuallyNeededCast(const Type& type, const Expression& operand) {
             return actuallyNeededCast(type, operand.as<MinTypMaxExpression>().selected());
         case ExpressionKind::ConditionalOp: {
             auto& cond = operand.as<ConditionalExpression>();
+            if (!type.isEquivalent(*cond.left().type) || !type.isEquivalent(*cond.right().type))
+                return true;
             return actuallyNeededCast(type, cond.left()) || actuallyNeededCast(type, cond.right());
         }
         default:
@@ -403,7 +405,8 @@ Expression& ConversionExpression::fromSyntax(Compilation& comp, const CastExpres
     };
 
     if (type->isMatching(*operand->type) && !isGenvar() && !isDifferentTypedef() &&
-        ((assignmentTarget && assignmentTarget->isMatching(*type)) ||
+        ((assignmentTarget && assignmentTarget->isMatching(*type) &&
+          operand->kind != ExpressionKind::ConditionalOp) ||
          !actuallyNeededCast(*type, *operand))) {
         context.addDiag(diag::UselessCast, syntax.apostrophe.location())
             << *operand->type << targetExpr.sourceRange << operand->sourceRange;

diff --git a/source/ast/expressions/LiteralExpressions.cpp b/source/ast/expressions/LiteralExpressions.cpp
@@ -166,7 +166,7 @@ Expression& UnbasedUnsizedIntegerLiteral::fromSyntax(Compilation& compilation,
 }
 
 bool UnbasedUnsizedIntegerLiteral::propagateType(const ASTContext&, const Type& newType,
-                                                 SourceRange) {
+                                                 SourceRange, ConversionKind) {
     SLANG_ASSERT(newType.isIntegral());
     SLANG_ASSERT(newType.getBitWidth());
 

diff --git a/source/ast/expressions/MiscExpressions.cpp b/source/ast/expressions/MiscExpressions.cpp
@@ -1340,7 +1340,7 @@ Expression& MinTypMaxExpression::fromSyntax(Compilation& compilation,
 }
 
 bool MinTypMaxExpression::propagateType(const ASTContext& context, const Type& newType,
-                                        SourceRange opRange) {
+                                        SourceRange opRange, ConversionKind) {
     // Only the selected expression gets a propagated type.
     type = &newType;
     contextDetermined(context, selected_, this, newType, opRange);

diff --git a/source/ast/expressions/OperatorExpressions.cpp b/source/ast/expressions/OperatorExpressions.cpp
@@ -329,7 +329,7 @@ Expression& UnaryExpression::fromSyntax(Compilation& compilation,
 }
 
 bool UnaryExpression::propagateType(const ASTContext& context, const Type& newType,
-                                    SourceRange propRange) {
+                                    SourceRange propRange, ConversionKind) {
     switch (op) {
         case UnaryOperator::Plus:
         case UnaryOperator::Minus:
@@ -1113,7 +1113,7 @@ void BinaryExpression::analyzeOpTypes(const Type& clt, const Type& crt, const Ty
 }
 
 bool BinaryExpression::propagateType(const ASTContext& context, const Type& newType,
-                                     SourceRange propRange) {
+                                     SourceRange propRange, ConversionKind) {
     switch (op) {
         case BinaryOperator::Add:
         case BinaryOperator::Subtract:
@@ -1460,7 +1460,7 @@ Expression& ConditionalExpression::fromSyntax(Compilation& comp,
 }
 
 bool ConditionalExpression::propagateType(const ASTContext& context, const Type& newType,
-                                          SourceRange opRange) {
+                                          SourceRange opRange, ConversionKind conversionKind) {
     const bool parentTypeEquiv = type->isEquivalent(newType);
     type = &newType;
 
@@ -1477,7 +1477,8 @@ bool ConditionalExpression::propagateType(const ASTContext& context, const Type&
                             std::optional<bitwidth_t> otherEffectiveWidth) {
         // This is a propagated conversion but we'd like to see width-expand
         // warnings anyway so we'll manually do the check conversion check here.
-        if (!flags.has(ASTFlags::UnevaluatedBranch)) {
+        if (!flags.has(ASTFlags::UnevaluatedBranch) &&
+            conversionKind <= ConversionKind::Propagated) {
             // If the parent type was already equivalent to what's being propagated,
             // then the only conversions we might be doing are "self induced", in the
             // sense that one branch is propagating its type to the other side.
@@ -2423,7 +2424,7 @@ Expression& ValueRangeExpression::fromSyntax(Compilation& comp,
 }
 
 bool ValueRangeExpression::propagateType(const ASTContext& context, const Type& newType,
-                                         SourceRange opRange) {
+                                         SourceRange opRange, ConversionKind) {
     contextDetermined(context, left_, this, newType, opRange);
     if (rangeKind == ValueRangeKind::Simple)
         contextDetermined(context, right_, this, newType, opRange);

diff --git a/tests/unittests/ast/WarningTests.cpp b/tests/unittests/ast/WarningTests.cpp
@@ -1358,6 +1358,21 @@ module test2;
     c = cond ? d : 0;
   end
 endmodule
+
+module test3;
+    localparam int unsigned SIZE = 16;
+
+    logic a;
+    logic [7:0] b, c;
+    logic [SIZE-1:0] d, e, f, g;
+
+    always_comb begin
+        d = SIZE'( a ? b : c);
+        e = SIZE'(b);
+        f = a ? SIZE'(b) : SIZE'(c);
+        g = SIZE'( (a == 0) ? f : c);
+    end
+endmodule
 )");
 
     Compilation compilation;