Warning on lossy implicit casts. (#2367)

* Warning on bool to float conversion.

* Fix test cases.

* Improve.

* LanguageServer: don't show constant value for non constant variables.

* Fix tests.

* Fix warnings in tests.

Co-authored-by: Yong He <yhe@nvidia.com>

11 files changed, 222 insertions, 73 deletions

diff --git a/source/slang/slang-check-conversion.cpp b/source/slang/slang-check-conversion.cpp
index 4bcf07b2b..a3273f942 100644
--- a/source/slang/slang-check-conversion.cpp
+++ b/source/slang/slang-check-conversion.cpp
@@ -955,15 +955,39 @@ namespace Slang
             // but then emit a diagnostic when actually reifying
             // the result expression.
             //
-            if( cost >= kConversionCost_Explicit )
+            if (outToExpr)
             {
-                if( outToExpr )
+                if (cost >= kConversionCost_Explicit)
                 {
                     getSink()->diagnose(fromExpr, Diagnostics::typeMismatch, toType, fromType);
-                    getSink()->diagnose(fromExpr, Diagnostics::noteExplicitConversionPossible, fromType, toType);
+                    getSink()->diagnose(
+                        fromExpr, Diagnostics::noteExplicitConversionPossible, fromType, toType);
+                }
+                else if (cost >= kConversionCost_Default)
+                {
+                    // For general types of implicit conversions, we issue a warning, unless `fromExpr` is a known constant
+                    // and we know it won't cause a problem.
+                    bool shouldEmitGeneralWarning = true;
+                    if (isScalarIntegerType(toType))
+                    {
+                        if (auto intVal = tryFoldIntegerConstantExpression(fromExpr, nullptr))
+                        {
+                            if (auto val = as<ConstantIntVal>(intVal))
+                            {
+                                if (isIntValueInRangeOfType(val->value, toType))
+                                {
+                                    // OK.
+                                    shouldEmitGeneralWarning = false;
+                                }
+                            }
+                        }
+                    }
+                    if (shouldEmitGeneralWarning)
+                    {
+                        getSink()->diagnose(fromExpr, Diagnostics::unrecommendedImplicitConversion, fromType, toType);
+                    }
                 }
             }
-
             if(outCost)
                 *outCost = cost;
 
diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp
index f2a339643..abe7642fb 100644
--- a/source/slang/slang-check-decl.cpp
+++ b/source/slang/slang-check-decl.cpp
@@ -14,6 +14,8 @@
 
 #include "slang-syntax.h"
 
+#include <limits>
+
 namespace Slang
 {
         /// Visitor to transition declarations to `DeclCheckState::CheckedModifiers`
@@ -3487,7 +3489,36 @@ namespace Slang
         if(!basicType)
             return false;
 
-        return isIntegerBaseType(basicType->baseType);
+        return isIntegerBaseType(basicType->baseType) || basicType->baseType == BaseType::Bool;
+    }
+
+    bool SemanticsVisitor::isIntValueInRangeOfType(IntegerLiteralValue value, Type* type)
+    {
+        auto basicType = as<BasicExpressionType>(type);
+        if (!basicType)
+            return false;
+
+        switch (basicType->baseType)
+        {
+        case BaseType::UInt8:
+            return (value >= 0 && value <= std::numeric_limits<uint8_t>::max()) || (value == -1);
+        case BaseType::UInt16:
+            return (value >= 0 && value <= std::numeric_limits<uint16_t>::max()) || (value == -1);
+        case BaseType::UInt:
+            return (value >= 0 && value <= std::numeric_limits<uint32_t>::max()) || (value == -1);
+        case BaseType::UInt64:
+            return true;
+        case BaseType::Int8:
+            return value >= std::numeric_limits<int8_t>::min() && value <= std::numeric_limits<int8_t>::max();
+        case BaseType::Int16:
+            return value >= std::numeric_limits<int16_t>::min() && value <= std::numeric_limits<int16_t>::max();
+        case BaseType::Int:
+            return value >= std::numeric_limits<int32_t>::min() && value <= std::numeric_limits<int32_t>::max();
+        case BaseType::Int64:
+            return value >= std::numeric_limits<int64_t>::min() && value <= std::numeric_limits<int64_t>::max();
+        default:
+            return false;
+        }
     }
 
     void SemanticsVisitor::validateEnumTagType(Type* type, SourceLoc const& loc)
@@ -3772,7 +3803,7 @@ namespace Slang
             // We want to enforce that this is an integer constant
             // expression, but we don't actually care to retain
             // the value.
-            CheckIntegerConstantExpression(initExpr);
+            CheckIntegerConstantExpression(initExpr, IntegerConstantExpressionCoercionType::AnyInteger, nullptr);
 
             decl->tagExpr = initExpr;
         }
diff --git a/source/slang/slang-check-expr.cpp b/source/slang/slang-check-expr.cpp
index 2ac02b978..c96db6f3b 100644
--- a/source/slang/slang-check-expr.cpp
+++ b/source/slang/slang-check-expr.cpp
@@ -884,7 +884,8 @@ namespace Slang
 
         auto funcDeclRef = getDeclRef(m_astBuilder, funcDeclRefExpr);
         auto intrinsicMod = funcDeclRef.getDecl()->findModifier<IntrinsicOpModifier>();
-        if (!intrinsicMod)
+        auto implicitCast = funcDeclRef.getDecl()->findModifier<ImplicitConversionModifier>();
+        if (!intrinsicMod && !implicitCast)
         {
             // We can't constant fold anything that doesn't map to a builtin
             // operation right now.
@@ -942,54 +943,91 @@ namespace Slang
 
         // At this point, all the operands had simple integer values, so we are golden.
         IntegerLiteralValue resultValue = 0;
-        auto opName = funcDeclRef.getName();
-
-        // handle binary operators
-        if (opName == getName("-"))
+        // If this is an implicit cast, we can try to fold.
+        if (implicitCast)
         {
-            if (argCount == 1)
+            auto targetBasicType = as<BasicExpressionType>(invokeExpr.getExpr()->type.type);
+            if (!targetBasicType)
+                return nullptr;
+            switch (targetBasicType->baseType)
             {
-                resultValue = -constArgVals[0];
+            case BaseType::Bool:
+                resultValue = constArgVals[0] != 0;
+                break;
+            case BaseType::Int:
+            case BaseType::UInt:
+            case BaseType::UInt16:
+            case BaseType::Int16:
+            case BaseType::UInt8:
+            case BaseType::Int8:
+                resultValue = constArgVals[0];
+                break;
+            default:
+                return nullptr;
             }
-            else if (argCount == 2)
+        }
+        else
+        {
+            auto opName = funcDeclRef.getName();
+
+            // handle binary operators
+            if (opName == getName("-"))
             {
-                resultValue = constArgVals[0] - constArgVals[1];
+                if (argCount == 1)
+                {
+                    resultValue = -constArgVals[0];
+                }
+                else if (argCount == 2)
+                {
+                    resultValue = constArgVals[0] - constArgVals[1];
+                }
+            }
+            else if (opName == getName("!"))
+            {
+                resultValue = constArgVals[0] != 0;
+            }
+            else if (opName == getName("~"))
+            {
+                resultValue = ~constArgVals[0];
             }
-        }
 
-        // simple binary operators
+            // simple binary operators
 #define CASE(OP)                                                    \
-        else if(opName == getName(#OP)) do {                    \
-            if(argCount != 2) return nullptr;                   \
-            resultValue = constArgVals[0] OP constArgVals[1];   \
-        } while(0)
-
-        CASE(+); // TODO: this can also be unary...
-        CASE(*);
-        CASE(<<);
-        CASE(>>);
-        CASE(&);
-        CASE(|);
-        CASE(^);
+            else if(opName == getName(#OP)) do {                    \
+                if(argCount != 2) return nullptr;                   \
+                resultValue = constArgVals[0] OP constArgVals[1];   \
+            } while(0)
+
+            CASE(+); // TODO: this can also be unary...
+            CASE(*);
+            CASE(<<);
+            CASE(>>);
+            CASE(&);
+            CASE(|);
+            CASE(^);
+            CASE(!=);
+            CASE(==);
+            CASE(>=);
+            CASE(<=);
+            CASE(<);
+            CASE(>);
 #undef CASE
-
-        // binary operators with chance of divide-by-zero
-        // TODO: issue a suitable error in that case
+            // binary operators with chance of divide-by-zero
+            // TODO: issue a suitable error in that case
 #define CASE(OP)                                                    \
-        else if(opName == getName(#OP)) do {                    \
-            if(argCount != 2) return nullptr;                   \
-            if(!constArgVals[1]) return nullptr;                \
-            resultValue = constArgVals[0] OP constArgVals[1];   \
-        } while(0)
-
-        CASE(/);
-        CASE(%);
+            else if(opName == getName(#OP)) do {                    \
+                if(argCount != 2) return nullptr;                   \
+                if(!constArgVals[1]) return nullptr;                \
+                resultValue = constArgVals[0] OP constArgVals[1];   \
+            } while(0)
+            CASE(/);
+            CASE(%);
 #undef CASE
-
-        // TODO(tfoley): more cases
-        else
-        {
-            return nullptr;
+            // TODO(tfoley): more cases
+            else
+            {
+                return nullptr;
+            }
         }
 
         IntVal* result = m_astBuilder->create<ConstantIntVal>(resultValue);
@@ -1126,13 +1164,27 @@ namespace Slang
         return tryConstantFoldExpr(expr, circularityInfo);
     }
 
-    IntVal* SemanticsVisitor::CheckIntegerConstantExpression(Expr* inExpr, DiagnosticSink* sink)
+    IntVal* SemanticsVisitor::CheckIntegerConstantExpression(Expr* inExpr, IntegerConstantExpressionCoercionType coercionType, Type* expectedType, DiagnosticSink* sink)
     {
         // No need to issue further errors if the expression didn't even type-check.
         if(IsErrorExpr(inExpr)) return nullptr;
 
         // First coerce the expression to the expected type
-        auto expr = coerce(m_astBuilder->getIntType(),inExpr);
+        Expr* expr = nullptr;
+        switch (coercionType)
+        {
+        case IntegerConstantExpressionCoercionType::SpecificType:
+            expr = coerce(expectedType, inExpr);
+            break;
+        case IntegerConstantExpressionCoercionType::AnyInteger:
+            if (isScalarIntegerType(inExpr->type))
+                expr = inExpr;
+            else
+                expr = coerce(m_astBuilder->getIntType(), inExpr);
+            break;
+        default:
+            break;
+        }
 
         // No need to issue further errors if the type coercion failed.
         if(IsErrorExpr(expr)) return nullptr;
@@ -1145,9 +1197,9 @@ namespace Slang
         return result;
     }
 
-    IntVal* SemanticsVisitor::CheckIntegerConstantExpression(Expr* inExpr)
+    IntVal* SemanticsVisitor::CheckIntegerConstantExpression(Expr* inExpr, IntegerConstantExpressionCoercionType coercionType, Type* expectedType)
     {
-        return CheckIntegerConstantExpression(inExpr, getSink());
+        return CheckIntegerConstantExpression(inExpr, coercionType, expectedType, getSink());
     }
 
     IntVal* SemanticsVisitor::CheckEnumConstantExpression(Expr* expr)
@@ -1218,7 +1270,7 @@ namespace Slang
             IntVal* elementCount = nullptr;
             if (indexExpr)
             {
-                elementCount = CheckIntegerConstantExpression(indexExpr);
+                elementCount = CheckIntegerConstantExpression(indexExpr, IntegerConstantExpressionCoercionType::AnyInteger, nullptr);
             }
 
             auto elementType = CoerceToUsableType(TypeExp(baseExpr, baseTypeType->type));
diff --git a/source/slang/slang-check-impl.h b/source/slang/slang-check-impl.h
index 345652263..9a849384c 100644
--- a/source/slang/slang-check-impl.h
+++ b/source/slang/slang-check-impl.h
@@ -562,8 +562,8 @@ namespace Slang
 
         Type* ExtractGenericArgType(Expr* exp);
 
-        IntVal* ExtractGenericArgInteger(Expr* exp, DiagnosticSink* sink);
-        IntVal* ExtractGenericArgInteger(Expr* exp);
+        IntVal* ExtractGenericArgInteger(Expr* exp, Type* genericParamType, DiagnosticSink* sink);
+        IntVal* ExtractGenericArgInteger(Expr* exp, Type* genericParamType);
 
         Val* ExtractGenericArgVal(Expr* exp);
 
@@ -1035,6 +1035,8 @@ namespace Slang
             /// Is `type` a scalar integer type.
         bool isScalarIntegerType(Type* type);
 
+        bool isIntValueInRangeOfType(IntegerLiteralValue value, Type* type);
+
         // Validate that `type` is a suitable type to use
         // as the tag type for an `enum`
         void validateEnumTagType(Type* type, SourceLoc const& loc);
@@ -1135,8 +1137,13 @@ namespace Slang
             ConstantFoldingCircularityInfo* circularityInfo);
 
         // Enforce that an expression resolves to an integer constant, and get its value
-        IntVal* CheckIntegerConstantExpression(Expr* inExpr);
-        IntVal* CheckIntegerConstantExpression(Expr* inExpr, DiagnosticSink* sink);
+        enum class IntegerConstantExpressionCoercionType
+        {
+            SpecificType,
+            AnyInteger
+        };
+        IntVal* CheckIntegerConstantExpression(Expr* inExpr, IntegerConstantExpressionCoercionType coercionType, Type* expectedType);
+        IntVal* CheckIntegerConstantExpression(Expr* inExpr, IntegerConstantExpressionCoercionType coercionType, Type* expectedType, DiagnosticSink* sink);
 
         IntVal* CheckEnumConstantExpression(Expr* expr);
 
diff --git a/source/slang/slang-check-modifier.cpp b/source/slang/slang-check-modifier.cpp
index 16a1cae26..a7fb641a2 100644
--- a/source/slang/slang-check-modifier.cpp
+++ b/source/slang/slang-check-modifier.cpp
@@ -18,7 +18,7 @@ namespace Slang
         // First type-check the expression as normal
         expr = CheckExpr(expr);
 
-        auto intVal = CheckIntegerConstantExpression(expr);
+        auto intVal = CheckIntegerConstantExpression(expr, IntegerConstantExpressionCoercionType::AnyInteger, nullptr);
         if(!intVal)
             return nullptr;
 
diff --git a/source/slang/slang-check-overload.cpp b/source/slang/slang-check-overload.cpp
index 36c93a2e2..9109130e2 100644
--- a/source/slang/slang-check-overload.cpp
+++ b/source/slang/slang-check-overload.cpp
@@ -314,14 +314,10 @@ namespace Slang
                 // If we have an argument to work with, then we will
                 // try to extract its speicalization-time constant value.
                 //
-                // TODO: This is one of the places where we will need to
-                // generalize in order to support generic value parameters
-                // with types other than `int`.
-                //
                 Val* val = nullptr;
                 if( arg )
                 {
-                    val = ExtractGenericArgInteger(arg, context.mode == OverloadResolveContext::Mode::JustTrying ? nullptr : getSink());
+                    val = ExtractGenericArgInteger(arg, getType(m_astBuilder, valParamRef), context.mode == OverloadResolveContext::Mode::JustTrying ? nullptr : getSink());
                 }
 
                 // If any of the above checking steps fail and we don't
diff --git a/source/slang/slang-check-stmt.cpp b/source/slang/slang-check-stmt.cpp
index 57bb3b85a..2bf6cb830 100644
--- a/source/slang/slang-check-stmt.cpp
+++ b/source/slang/slang-check-stmt.cpp
@@ -140,7 +140,7 @@ namespace Slang
     Expr* SemanticsVisitor::checkExpressionAndExpectIntegerConstant(Expr* expr, IntVal** outIntVal)
     {
         expr = CheckExpr(expr);
-        auto intVal = CheckIntegerConstantExpression(expr);
+        auto intVal = CheckIntegerConstantExpression(expr, IntegerConstantExpressionCoercionType::AnyInteger, nullptr);
         if (outIntVal)
             *outIntVal = intVal;
         return expr;
diff --git a/source/slang/slang-check-type.cpp b/source/slang/slang-check-type.cpp
index 50bc55641..b8789dd76 100644
--- a/source/slang/slang-check-type.cpp
+++ b/source/slang/slang-check-type.cpp
@@ -119,9 +119,14 @@ namespace Slang
         return ExpectAType(exp);
     }
 
-    IntVal* SemanticsVisitor::ExtractGenericArgInteger(Expr* exp, DiagnosticSink* sink)
+    IntVal* SemanticsVisitor::ExtractGenericArgInteger(Expr* exp, Type* genericParamType, DiagnosticSink* sink)
     {
-        IntVal* val = CheckIntegerConstantExpression(exp, sink);
+        IntVal* val = CheckIntegerConstantExpression(
+            exp,
+            genericParamType ? IntegerConstantExpressionCoercionType::SpecificType
+                             : IntegerConstantExpressionCoercionType::AnyInteger,
+            genericParamType,
+            sink);
         if(val) return val;
 
         // If the argument expression could not be coerced to an integer
@@ -132,9 +137,9 @@ namespace Slang
         return val;
     }
 
-    IntVal* SemanticsVisitor::ExtractGenericArgInteger(Expr* exp)
+    IntVal* SemanticsVisitor::ExtractGenericArgInteger(Expr* exp, Type* genericParamType)
     {
-        return ExtractGenericArgInteger(exp, getSink());
+        return ExtractGenericArgInteger(exp, genericParamType, getSink());
     }
 
     Val* SemanticsVisitor::ExtractGenericArgVal(Expr* exp)
@@ -158,7 +163,7 @@ namespace Slang
             {
                 CheckExpr(exp);
             }
-            return ExtractGenericArgInteger(exp);
+            return ExtractGenericArgInteger(exp, nullptr);
         }
     }
 
@@ -275,7 +280,6 @@ namespace Slang
                         }
                         return false;
                     }
-
                     // TODO: this is one place where syntax should get cloned!
                     if (outProperType)
                         args.add(valParam->initExpr);
diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
index 0fa5d64a7..e44a880a2 100644
--- a/source/slang/slang-diagnostic-defs.h
+++ b/source/slang/slang-diagnostic-defs.h
@@ -293,7 +293,7 @@ DIAGNOSTIC(33071, Error, expectedAStringLiteral, "expected a string literal")
 
 DIAGNOSTIC(   -1, Note, noteExplicitConversionPossible, "explicit conversion from '$0' to '$1' is possible")
 DIAGNOSTIC(30080, Error, ambiguousConversion, "more than one implicit conversion exists from '$0' to '$1'")
-
+DIAGNOSTIC(30081, Warning, unrecommendedImplicitConversion, "implicit conversion from '$0' to '$1' is not recommended")
 DIAGNOSTIC(30090, Error, tryClauseMustApplyToInvokeExpr, "expression in a 'try' clause must be a call to a function or operator overload.")
 DIAGNOSTIC(30091, Error, tryInvokeCalleeShouldThrow, "'$0' called from a 'try' clause does not throw an error, make sure the callee is marked as 'throws'")
 DIAGNOSTIC(30092, Error, calleeOfTryCallMustBeFunc, "callee in a 'try' clause must be a function")
diff --git a/source/slang/slang-language-server.cpp b/source/slang/slang-language-server.cpp
index f209a4955..cb077cfdb 100644
--- a/source/slang/slang-language-server.cpp
+++ b/source/slang/slang-language-server.cpp
@@ -15,6 +15,7 @@
 #include "../compiler-core/slang-json-native.h"
 #include "../compiler-core/slang-json-rpc-connection.h"
 #include "../compiler-core/slang-language-server-protocol.h"
+#include "slang-check-impl.h"
 #include "slang-language-server.h"
 #include "slang-workspace-version.h"
 #include "slang-language-server-ast-lookup.h"
@@ -201,10 +202,19 @@ SlangResult LanguageServer::didOpenTextDocument(const DidOpenTextDocumentParams&
     return SLANG_OK;
 }
 
-String getDeclSignatureString(DeclRef<Decl> declRef, ASTBuilder* astBuilder)
+static bool isBoolType(Type* t)
+{
+    auto basicType = as<BasicExpressionType>(t);
+    if (!basicType)
+        return false;
+    return basicType->baseType == BaseType::Bool;
+}
+
+String getDeclSignatureString(DeclRef<Decl> declRef, WorkspaceVersion* version)
 {
     if (declRef.getDecl())
     {
+        auto astBuilder = version->linkage->getASTBuilder();
         ASTPrinter printer(
             astBuilder,
             ASTPrinter::OptionFlag::ParamNames | ASTPrinter::OptionFlag::NoInternalKeywords |
@@ -213,6 +223,9 @@ String getDeclSignatureString(DeclRef<Decl> declRef, ASTBuilder* astBuilder)
         if (auto varDecl = as<VarDeclBase>(declRef.getDecl()))
         {
             auto& sb = printer.getStringBuilder();
+            if (!varDecl->findModifier<ConstModifier>() && !as<LetDecl>(declRef.getDecl()))
+                return printer.getString();
+
             if (auto litExpr = as<LiteralExpr>(varDecl->initExpr))
             {
                 sb << " = " << litExpr->token.getContent();
@@ -224,6 +237,27 @@ String getDeclSignatureString(DeclRef<Decl> declRef, ASTBuilder* astBuilder)
                     sb << " = " << (isTypeDecl->constantVal->value ? "true" : "false");
                 }
             }
+            else if (varDecl->initExpr)
+            {
+                DiagnosticSink sink;
+                SharedSemanticsContext semanticContext(version->linkage, getModule(varDecl), &sink);
+                SemanticsVisitor semanticsVisitor(&semanticContext);
+                if (auto intVal = semanticsVisitor.tryFoldIntegerConstantExpression(varDecl->initExpr, nullptr))
+                {
+                    if (auto constantInt = as<ConstantIntVal>(intVal))
+                    {
+                        sb << " = ";
+                        if (isBoolType(varDecl->getType()))
+                        {
+                            sb << (constantInt->value ? "true" : "false");
+                        }
+                        else
+                        {
+                            sb << constantInt->value;
+                        }
+                    }
+                }
+            }
         }
         return printer.getString();
     }
@@ -421,7 +455,7 @@ SlangResult LanguageServer::hover(
         if (declRef.getDecl())
         {
             sb << "```\n"
-                << getDeclSignatureString(declRef, version->linkage->getASTBuilder())
+                << getDeclSignatureString(declRef, version)
                 << "\n```\n";
             
             _tryGetDocumentation(sb, version, declRef.getDecl());
@@ -777,7 +811,7 @@ SlangResult LanguageServer::completionResolve(
     if (itemId >= 0 && itemId < candidateItems.getCount())
     {
         auto declRef = candidateItems[itemId].declRef;
-        resolvedItem.detail = getDeclSignatureString(declRef, version->linkage->getASTBuilder());
+        resolvedItem.detail = getDeclSignatureString(declRef, version);
         StringBuilder docSB;
         _tryGetDocumentation(docSB, version, declRef.getDecl());
         resolvedItem.documentation.value = docSB.ProduceString();
diff --git a/source/slang/slang-stdlib.cpp b/source/slang/slang-stdlib.cpp
index 628a075e3..1ead48cb8 100644
--- a/source/slang/slang-stdlib.cpp
+++ b/source/slang/slang-stdlib.cpp
@@ -126,7 +126,7 @@ namespace Slang
             // If we are converting to a "larger" type, then
             // we are doing a lossless promotion, and otherwise
             // we are doing a demotion.
-            if( toInfo.conversionRank > fromInfo.conversionRank)
+            if (toInfo.conversionRank > fromInfo.conversionRank)
                 return kConversionCost_RankPromotion;
             else
                 return kConversionCost_GeneralConversion;
@@ -174,7 +174,8 @@ namespace Slang
         // types. This makes sense because we relaly want to prefer
         // conversion to `float` as the default.
         else if (toInfo.conversionKind == kBaseTypeConversionKind_Float
-            && toInfo.conversionRank >= kBaseTypeConversionRank_Int32)
+            && toInfo.conversionRank >= kBaseTypeConversionRank_Int32
+            && fromInfo.conversionRank >= kBaseTypeConversionRank_Int8)
         {
             return kConversionCost_IntegerToFloatConversion;
         }