Update template specialization assert handling

Amend the fix for DXASSERTS in template specialization by incorporating
changes from review comments.

Author: Tim Corringham

tools/clang/lib/Sema/SemaHLSL.cpp

@@ -830,12 +830,10 @@ GetOrCreateTemplateSpecialization(ASTContext &context, Sema &sema,
     if (specializationDecl->getInstantiatedFrom().isNull()) {
       // InstantiateClassTemplateSpecialization returns true if it finds an
       // error.
-      bool errorFound = sema.InstantiateClassTemplateSpecialization(
-          NoLoc, specializationDecl,
-          TemplateSpecializationKind::TSK_ImplicitInstantiation, true);
-      // Template specialization is suppressed if a fatal error has already been
-      // registered so don't assert in such cases.
-      DXVERIFY_NOMSG(sema.Diags.hasFatalErrorOccurred() || !errorFound);
+      if (sema.InstantiateClassTemplateSpecialization(
+              NoLoc, specializationDecl,
+              TemplateSpecializationKind::TSK_ImplicitInstantiation, true))
+        return QualType();
     }
     return context.getTemplateSpecializationType(
         TemplateName(templateDecl), templateArgs.data(), templateArgs.size(),
@@ -846,19 +844,20 @@ GetOrCreateTemplateSpecialization(ASTContext &context, Sema &sema,
       context, TagDecl::TagKind::TTK_Class, currentDeclContext, NoLoc, NoLoc,
       templateDecl, templateArgsForDecl.data(), templateArgsForDecl.size(),
       nullptr);
-  // InstantiateClassTemplateSpecialization returns true if it finds an error.
-  bool errorFound = sema.InstantiateClassTemplateSpecialization(
-      NoLoc, specializationDecl,
-      TemplateSpecializationKind::TSK_ImplicitInstantiation, true);
-  // Template specialization is suppressed if a fatal error has already been
-  // registered so don't assert in such cases.
-  DXVERIFY_NOMSG(sema.Diags.hasFatalErrorOccurred() || !errorFound);
+  // template specialization isn't performed if a fatal error has occurred
+  if (!sema.Diags.hasFatalErrorOccurred()) {
+    // InstantiateClassTemplateSpecialization returns true if it finds an error.
+    [[maybe_unused]] bool errorFound =
+        sema.InstantiateClassTemplateSpecialization(
+            NoLoc, specializationDecl,
+            TemplateSpecializationKind::TSK_ImplicitInstantiation, true);
+    assert(!errorFound && "template specialization failed");
+  }
   templateDecl->AddSpecialization(specializationDecl, InsertPos);
   specializationDecl->setImplicit(true);
-
   QualType canonType = context.getTypeDeclType(specializationDecl);
-  DXASSERT(isa<RecordType>(canonType),
-           "type of non-dependent specialization is not a RecordType");
+  assert(isa<RecordType>(canonType) &&
+         "type of non-dependent specialization is not a RecordType");
   TemplateArgumentListInfo templateArgumentList(NoLoc, NoLoc);
   TemplateArgumentLocInfo NoTemplateArgumentLocInfo;
   for (unsigned i = 0; i < templateArgs.size(); i++) {
@@ -893,18 +892,17 @@ static QualType GetOrCreateMatrixSpecialization(
       context, *sema, matrixTemplateDecl,
       ArrayRef<TemplateArgument>(templateArgs));
 
-#ifndef NDEBUG
-  // Verify that we can read the field member from the template record.
-  DXASSERT(matrixSpecializationType->getAsCXXRecordDecl(),
+  if (!matrixSpecializationType.isNull() &&
+      !sema->Diags.hasFatalErrorOccurred()) {
+    assert(matrixSpecializationType->getAsCXXRecordDecl() &&
            "type of non-dependent specialization is not a RecordType");
-  DeclContext::lookup_result lookupResult =
-      matrixSpecializationType->getAsCXXRecordDecl()->lookup(
-          DeclarationName(&context.Idents.get(StringRef("h"))));
-  // Template specialization is suppressed if a fatal error has been registered
-  // so only assert if lookup failed for some other reason.
-  DXASSERT(sema->Diags.hasFatalErrorOccurred() || !lookupResult.empty(),
+    // Verify that we can read the field member from the template record.
+    [[maybe_unused]] DeclContext::lookup_result lookupResult =
+        matrixSpecializationType->getAsCXXRecordDecl()->lookup(
+            DeclarationName(&context.Idents.get(StringRef("h"))));
+    assert(!lookupResult.empty() &&
            "otherwise matrix handle cannot be looked up");
-#endif
+  }
 
   return matrixSpecializationType;
 }
@@ -930,18 +928,17 @@ GetOrCreateVectorSpecialization(ASTContext &context, Sema *sema,
       context, *sema, vectorTemplateDecl,
       ArrayRef<TemplateArgument>(templateArgs));
 
-#ifndef NDEBUG
-  // Verify that we can read the field member from the template record.
-  DXASSERT(vectorSpecializationType->getAsCXXRecordDecl(),
+  if (!vectorSpecializationType.isNull() &&
+      !sema->Diags.hasFatalErrorOccurred()) {
+    assert(vectorSpecializationType->getAsCXXRecordDecl() &&
            "type of non-dependent specialization is not a RecordType");
-  DeclContext::lookup_result lookupResult =
-      vectorSpecializationType->getAsCXXRecordDecl()->lookup(
-          DeclarationName(&context.Idents.get(StringRef("h"))));
-  // Template specialization is suppressed if a fatal error has been registered
-  // so only assert if lookup failed for some other reason.
-  DXASSERT(sema->Diags.hasFatalErrorOccurred() || !lookupResult.empty(),
+    // Verify that we can read the field member from the template record.
+    [[maybe_unused]] DeclContext::lookup_result lookupResult =
+        vectorSpecializationType->getAsCXXRecordDecl()->lookup(
+            DeclarationName(&context.Idents.get(StringRef("h"))));
+    assert(!lookupResult.empty() &&
            "otherwise vector handle cannot be looked up");
-#endif
+  }
 
   return vectorSpecializationType;
 }
@@ -960,18 +957,16 @@ GetOrCreateNodeOutputRecordSpecialization(ASTContext &context, Sema *sema,
   QualType specializationType = GetOrCreateTemplateSpecialization(
       context, *sema, templateDecl, ArrayRef<TemplateArgument>(templateArgs));
 
-#ifdef DBG
-  // Verify that we can read the field member from the template record.
-  DXASSERT(specializationType->getAsCXXRecordDecl(),
+  if (!specializationType.isNull() && !sema->Diags.hasFatalErrorOccurred()) {
+    assert(specializationType->getAsCXXRecordDecl() &&
            "type of non-dependent specialization is not a RecordType");
-  DeclContext::lookup_result lookupResult =
-      specializationType->getAsCXXRecordDecl()->lookup(
-          DeclarationName(&context.Idents.get(StringRef("h"))));
-  // Template specialization is suppressed if a fatal error has been registered
-  // so only assert if lookup failed for some other reason.
-  DXASSERT(sema->Diags.hasFatalErrorOccurred() || !lookupResult.empty(),
+    // Verify that we can read the field member from the template record.
+    [[maybe_unused]] DeclContext::lookup_result lookupResult =
+        specializationType->getAsCXXRecordDecl()->lookup(
+            DeclarationName(&context.Idents.get(StringRef("h"))));
+    assert(!lookupResult.empty() &&
            "otherwise *NodeOutputRecords handle cannot be looked up");
-#endif
+  }
 
   return specializationType;
 }
@@ -5105,12 +5100,13 @@ class HLSLExternalSource : public ExternalSemaSource {
           // This is a bit of a special case we need to handle. Because the
           // buffer types don't use their template parameter in a way that would
           // force instantiation, we need to force specialization here.
-          GetOrCreateTemplateSpecialization(
+          QualType Ty = GetOrCreateTemplateSpecialization(
               *m_context, *m_sema,
               cast<ClassTemplateDecl>(
                   TST->getTemplateName().getAsTemplateDecl()),
               llvm::ArrayRef<TemplateArgument>(TST->getArgs(),
                                                TST->getNumArgs()));
+          return Ty.isNull();
         }
         if (const RecordType *recordType = argType->getAs<RecordType>()) {
           if (!recordType->getDecl()->isCompleteDefinition()) {

tools/clang/test/DXC/specialization_with_fatal_error.hlsl

@@ -2,9 +2,9 @@
 
 // Clang suppresses template specialization if a fatal error has been
 // registered (this reduces the risk of a cascade of secondary errors).
-// However, DXC DXASSERTs if a template specialization fails - which
-// prevents the error diagnostic being generated.
-// We check here that a DXASSERT is no longer raised if a fatal error
+// However, DXC asserted if a template specialization failed - which
+// prevented the error diagnostic being generated.
+// We check here that an assert is no longer raised if a fatal error
 // has been registered, and that the error diagnostic is generated.
 
 float a;
@@ -16,7 +16,7 @@ float a;
 void b() {};
 
 int3 c(int X) {
-  // DXASSERT was triggered if include file a.h doesn't exist, and the error 
-  // diagnostic was not produced.
-  return X.xxx; 
+  // an assert was triggered for the expression below when include file a.h
+  // doesn't exist, and the error diagnostic expected above was not produced.
+  return X.xxx;
 }