Add diagnostic to prevent defining unsized variables. (shader-slang#4168)

* Add diagnostic to prevent defining unsized static variables.

* Fix tests.

* Add more tests.

* Fix to allow defining variables of link-time size.

* update diagnostic message.

* Fix tests.

* Simplify code.

Author: csyonghe

source/slang/slang-ast-decl.h

@@ -137,7 +137,8 @@ enum class TypeTag
 {
     None = 0,
     Unsized = 1,
-    Incomplete = 2
+    Incomplete = 2,
+    LinkTimeSized = 4,
 };
 
 // Declaration of a type that represents some sort of aggregate

source/slang/slang-check-conformance.cpp

@@ -274,7 +274,8 @@ namespace Slang
             }
             else if (auto intVal = arrayType->getElementCount())
             {
-                sized = !intVal->isLinkTimeVal();
+                sized = true;
+                typeTag = (TypeTag)((int)typeTag | (int)TypeTag::LinkTimeSized);
             }
             if (!sized)
                 typeTag = (TypeTag)((int)typeTag | (int)TypeTag::Unsized);

source/slang/slang-check-decl.cpp

@@ -2040,11 +2040,14 @@ namespace Slang
             }
         }
 
-        if (auto parentDecl = as<AggTypeDecl>(getParentDecl(varDecl)))
+        TypeTag varTypeTags = getTypeTags(varDecl->getType());
+        auto parentDecl = as<AggTypeDecl>(getParentDecl(varDecl));
+        if (parentDecl)
         {
-            auto typeTags = getTypeTags(varDecl->getType());
-            parentDecl->addTag(typeTags);
-            if ((int)typeTags & (int)TypeTag::Unsized)
+            parentDecl->addTag(varTypeTags);
+            auto unsizedMask = (int)TypeTag::Unsized;
+            bool isUnknownSize = (((int)varTypeTags & unsizedMask) != 0);
+            if (isUnknownSize)
             {
                 // Unsized decl must appear as the last member of the struct.
                 for (auto memberIdx = parentDecl->members.getCount() - 1; memberIdx >= 0; memberIdx--)
@@ -2064,7 +2067,17 @@ namespace Slang
                 }
             }
         }
-        
+        bool isGlobalOrLocalVar = !isGlobalShaderParameter(varDecl) && !as<ParamDecl>(varDecl) &&
+            (!parentDecl || isEffectivelyStatic(varDecl));
+        if (isGlobalOrLocalVar)
+        {
+            bool isUnsized = (((int)varTypeTags & (int)TypeTag::Unsized) != 0);
+            if (isUnsized)
+            {
+                getSink()->diagnose(varDecl, Diagnostics::varCannotBeUnsized);
+            }
+        }
+
         if (auto elementType = getConstantBufferElementType(varDecl->getType()))
         {
             if (doesTypeHaveTag(elementType, TypeTag::Incomplete))

source/slang/slang-check-overload.cpp

@@ -263,6 +263,14 @@ namespace Slang
         //
         bool success = true;
 
+        auto maybeReportGeneralError = [&]()
+        {
+            if (context.mode != OverloadResolveContext::Mode::JustTrying)
+            {
+                getSink()->diagnose(context.loc, Diagnostics::cannotSpecializeGeneric, candidate.item.declRef);
+            }
+        };
+
         Index aa = 0;
         for (auto memberRef : getMembers(m_astBuilder, genericDeclRef))
         {
@@ -287,7 +295,10 @@ namespace Slang
                         // or this reference is ill-formed.
                         auto substType = typeParamRef.substitute(m_astBuilder, typeParamRef.getDecl()->initType.type);
                         if (!substType)
+                        {
+                            maybeReportGeneralError();
                             return false;
+                        }
                         checkedArgs.add(substType);
                         continue;
                     }
@@ -355,7 +366,10 @@ namespace Slang
                         ConstantFoldingCircularityInfo newCircularityInfo(valParamRef.getDecl(), nullptr);
                         auto defaultVal = tryConstantFoldExpr(valParamRef.substitute(m_astBuilder, valParamRef.getDecl()->initExpr), ConstantFoldingKind::CompileTime, &newCircularityInfo);
                         if (!defaultVal)
+                        {
+                            maybeReportGeneralError();
                             return false;
+                        }
                         checkedArgs.add(defaultVal);
                         continue;
                     }
@@ -475,8 +489,9 @@ namespace Slang
             break;
 
         case OverloadCandidate::Flavor::Generic:
-            return TryCheckGenericOverloadCandidateTypes(context, candidate);
-
+            {
+                return TryCheckGenericOverloadCandidateTypes(context, candidate);
+            }
         default:
             SLANG_UNEXPECTED("unknown flavor of overload candidate");
             break;

source/slang/slang-diagnostic-defs.h

@@ -325,7 +325,10 @@ DIAGNOSTIC(30066, Error, classCanOnlyBeInitializedWithNew, "a class can only be
 DIAGNOSTIC(30067, Error, mutatingMethodOnFunctionInputParameterError, "mutating method '$0' called on `in` parameter '$1'; changes will not be visible to caller. copy the parameter into a local variable if this behavior is intended")
 DIAGNOSTIC(30068, Warning, mutatingMethodOnFunctionInputParameterWarning, "mutating method '$0' called on `in` parameter '$1'; changes will not be visible to caller. copy the parameter into a local variable if this behavior is intended")
 
-DIAGNOSTIC(30070, Error, unsizedMemberMustAppearLast, "unsized member can only appear as the last member in a composite type.")
+DIAGNOSTIC(30070, Error, unsizedMemberMustAppearLast, "member with unknown size at compile time can only appear as the last member in a composite type.")
+DIAGNOSTIC(30071, Error, varCannotBeUnsized, "cannot instantiate a variable of unsized type.")
+
+DIAGNOSTIC(30075, Error, cannotSpecializeGeneric, "cannot specialize generic '$0' with the provided arguments.")
 
 DIAGNOSTIC(30100, Error, staticRefToNonStaticMember, "type '$0' cannot be used to refer to non-static member '$1'")
 DIAGNOSTIC(30101, Error, cannotDereferenceType, "cannot dereference type '$0', do you mean to use '.'?")

source/slang/slang-lower-to-ir.cpp

@@ -7782,6 +7782,33 @@ struct DeclLoweringVisitor : DeclVisitor<DeclLoweringVisitor, LoweredValInfo>
         return false;
     }
 
+    struct NestedContext
+    {
+        IRGenEnv        subEnvStorage;
+        IRBuilder       subBuilderStorage;
+        IRGenContext    subContextStorage;
+
+        NestedContext(DeclLoweringVisitor* outer)
+            : subBuilderStorage(*outer->getBuilder())
+            , subContextStorage(*outer->context)
+        {
+            auto outerContext = outer->context;
+
+            subEnvStorage.outer = outerContext->env;
+
+            subContextStorage.irBuilder = &subBuilderStorage;
+            subContextStorage.env = &subEnvStorage;
+
+            subContextStorage.thisType = outerContext->thisType;
+            subContextStorage.thisTypeWitness = outerContext->thisTypeWitness;
+
+            subContextStorage.returnDestination = LoweredValInfo();
+        }
+
+        IRBuilder* getBuilder() { return &subBuilderStorage; }
+        IRGenContext* getContext() { return &subContextStorage; }
+    };
+
     LoweredValInfo lowerGlobalShaderParam(VarDecl* decl)
     {
         IRType* paramType = lowerType(context, decl->getType());
@@ -7938,86 +7965,66 @@ struct DeclLoweringVisitor : DeclVisitor<DeclLoweringVisitor, LoweredValInfo>
             return lowerGlobalConstantDecl(decl);
         }
 
-        IRType* varType = lowerType(context, decl->getType());
+        NestedContext nested(this);
+        auto subBuilder = nested.getBuilder();
+        auto subContext = nested.getContext();
 
-        auto builder = getBuilder();
+        IRGeneric* outerGeneric = nullptr;
+
+        // If we are static, then we need to insert the declaration before the parent.
+        // This tries to match the behavior of previous `lowerFunctionStaticConstVarDecl` functionality
+        if (isFunctionStaticVarDecl(decl))
+        {
+            // We need to insert the constant at a level above
+            // the function being emitted. This will usually
+            // be the global scope, but it might be an outer
+            // generic if we are lowering a generic function.
+            subBuilder->setInsertBefore(subBuilder->getFunc());
+        }
+        else if (!isFunctionVarDecl(decl))
+        {
+            outerGeneric = emitOuterGenerics(subContext, decl, decl);
+        }
+
+        IRType* varType = lowerType(subContext, decl->getType());
 
         // TODO(JS): Do we create something derived from IRGlobalVar? Or do we use 
         // a decoration to identify an *actual* global?
 
-        IRGlobalValueWithCode* irGlobal = builder->createGlobalVar(varType);
-        LoweredValInfo globalVal = LoweredValInfo::ptr(irGlobal);
+        IRGlobalValueWithCode* irGlobal = subBuilder->createGlobalVar(varType);
 
-        addLinkageDecoration(context, irGlobal, decl);
-        addNameHint(context, irGlobal, decl);
+        addLinkageDecoration(subContext, irGlobal, decl);
+        addNameHint(subContext, irGlobal, decl);
 
-        maybeSetRate(context, irGlobal, decl);
+        maybeSetRate(subContext, irGlobal, decl);
 
-        addVarDecorations(context, irGlobal, decl);
-        maybeAddDebugLocationDecoration(context, irGlobal);
+        addVarDecorations(subContext, irGlobal, decl);
+        maybeAddDebugLocationDecoration(subContext, irGlobal);
 
         if (decl)
         {
-            builder->addHighLevelDeclDecoration(irGlobal, decl);
+            subBuilder->addHighLevelDeclDecoration(irGlobal, decl);
         }
 
-        // A global variable's SSA value is a *pointer* to
-        // the underlying storage.
-        context->setGlobalValue(decl, globalVal);
-
         if( auto initExpr = decl->initExpr )
         {
-            IRBuilder subBuilderStorage = *getBuilder();
-            IRBuilder* subBuilder = &subBuilderStorage;
-
             subBuilder->setInsertInto(irGlobal);
 
-            IRGenContext subContextStorage = *context;
-            IRGenContext* subContext = &subContextStorage;
-
-            subContext->irBuilder = subBuilder;
-
-            // TODO: set up a parent IR decl to put the instructions into
-
             IRBlock* entryBlock = subBuilder->emitBlock();
             subBuilder->setInsertInto(entryBlock);
 
             LoweredValInfo initVal = lowerLValueExpr(subContext, initExpr);
             subContext->irBuilder->emitReturn(getSimpleVal(subContext, initVal));
         }
 
-        irGlobal->moveToEnd();
+        // A global variable's SSA value is a *pointer* to
+        // the underlying storage.
+        auto loweredValue = LoweredValInfo::ptr(finishOuterGenerics(subBuilder, irGlobal, outerGeneric));
+        context->setGlobalValue(decl, loweredValue);
 
-        return globalVal;
+        return loweredValue;
     }
 
-    struct NestedContext
-    {
-        IRGenEnv        subEnvStorage;
-        IRBuilder       subBuilderStorage;
-        IRGenContext    subContextStorage;
-
-        NestedContext(DeclLoweringVisitor* outer)
-            : subBuilderStorage(*outer->getBuilder())
-            , subContextStorage(*outer->context)
-        {
-            auto outerContext = outer->context;
-
-            subEnvStorage.outer = outerContext->env;
-
-            subContextStorage.irBuilder = &subBuilderStorage;
-            subContextStorage.env = &subEnvStorage;
-
-            subContextStorage.thisType = outerContext->thisType;
-            subContextStorage.thisTypeWitness = outerContext->thisTypeWitness;
-
-            subContextStorage.returnDestination = LoweredValInfo();
-        }
-
-        IRBuilder* getBuilder() { return &subBuilderStorage; }
-        IRGenContext* getContext() { return &subContextStorage; }
-    };
-
     LoweredValInfo lowerFunctionStaticConstVarDecl(
         VarDeclBase* decl)
     {
@@ -10235,10 +10242,10 @@ bool canDeclLowerToAGeneric(Decl* decl)
     // a generic that returns a type (a simple type-level function).
     if(as<TypeDefDecl>(decl)) return true;
 
-    // If we have a variable declaration that is *static* and *const* we can lower to a generic
+    // A static member variable declaration can be lowered into a generic.
     if (auto varDecl = as<VarDecl>(decl))
     {
-        if (varDecl->hasModifier<HLSLStaticModifier>() && varDecl->hasModifier<ConstModifier>())
+        if (varDecl->hasModifier<HLSLStaticModifier>())
         {
             return !isFunctionVarDecl(varDecl);
         }
@@ -10360,7 +10367,9 @@ LoweredValInfo emitDeclRef(
         // We can only really specialize things that map to single values.
         // It would be an error if we got a non-`None` value that
         // wasn't somehow a single value.
-        auto irGenericVal = getSimpleVal(context, genericVal);
+        genericVal = materialize(context, genericVal);
+        auto irGenericVal = genericVal.val;
+        SLANG_ASSERT(irGenericVal);
 
         // We have the IR value for the generic we'd like to specialize,
         // and now we need to get the value for the arguments.
@@ -10392,8 +10401,16 @@ LoweredValInfo emitDeclRef(
             irGenericVal,
             irArgs.getCount(),
             irArgs.getBuffer());
-
-        return LoweredValInfo::simple(irSpecializedVal);
+        switch (genericVal.flavor)
+        {
+        case LoweredValInfo::Flavor::Simple:
+            return LoweredValInfo::simple(irSpecializedVal);
+        case LoweredValInfo::Flavor::Ptr:
+            return LoweredValInfo::ptr(irSpecializedVal);
+        default:
+            SLANG_UNEXPECTED("unhandled lowered value flavor");
+            UNREACHABLE_RETURN(LoweredValInfo());
+        }
     }
     else if(auto thisTypeSubst = as<LookupDeclRef>(subst))
     {

source/slang/slang-syntax.cpp

@@ -25,6 +25,8 @@ void printDiagnosticArg(StringBuilder& sb, Decl* decl)
 
 void printDiagnosticArg(StringBuilder& sb, DeclRefBase* declRefBase)
 {
+    if (!declRefBase)
+        return;
     printDiagnosticArg(sb, declRefBase->getDecl());
 }
 

tests/bugs/gh-4150.slang

@@ -0,0 +1,49 @@
+//TEST:SIMPLE(filecheck=CHECK1):-target spirv -DERROR1
+//TEST:SIMPLE(filecheck=CHECK2):-target spirv -DERROR2
+extern static const int constValue = 1;
+
+struct RWTex<T : __BuiltinRealType, let N : uint>
+{
+#ifdef ERROR2
+    // expect error: cannot define static member with unsized type.
+    // CHECK2:([[# @LINE+1]]): error 30071
+    static [[vk::binding(0, 0)]] vector<T,N> rwtable[];
+#else
+    static [[vk::binding(0, 0)]] vector<T,N> rwtable[4];
+#endif
+    static vector<T, N> get(uint image_index) { return rwtable[image_index]; }
+}    
+
+struct Push
+{
+    uint image_id;
+};
+[[vk::push_constant]] Push p;
+RWStructuredBuffer<float3> output;
+[shader("compute")]
+[numthreads(8, 8, 1)]
+void main(uint3 pixel_i : SV_DispatchThreadID)
+{
+    output[0] =
+#ifdef ERROR1
+        // expect error: trying to specialize RWTex, which has two arguments, with only one argument.
+        // CHECK1:([[# @LINE+1]]): error 30075
+        RWTex<float3>::get(p.image_id);
+#else
+        RWTex<float, 3>::get(p.image_id);
+#endif
+    //CHECK1:([[# @LINE+1]]): error 30071
+    static float sa1[];
+
+    //CHECK1:([[# @LINE+1]]): error 30071
+    float sa2[];
+
+    //CHECK1-NOT:([[# @LINE+1]]): error
+    static float sa3[] = { 1, 2, 3 }; // should be ok.
+
+    //CHECK1-NOT:([[# @LINE+1]]): error
+    float sa4[] = { 1, 2, 3 }; // should be ok.
+
+    //CHECK1-NOT:([[# @LINE+1]]): error
+    float sa5[constValue]; // should be ok.
+}

tests/diagnostics/unsized.slang

@@ -5,6 +5,7 @@ extern static const int size = 1;
 struct V
 {
     // CHECK-DAG: ([[# @LINE+1]]): error 30070
+    int c[];
     int b[size];
     int a[];
 }