Amend template specialization DXASSERT conditions

[tcorringham]

Clang suppresses template specialization if a fatal error has been reported in order to reduce the risk of a cascade of secondary error diagnostics.

However, DXC DXASSERTs if template specialization fails - even if that is due to an unrelated fatal error - which has the unintended result of hiding the fatal error and hence providing no indication of what the problem is.

The DXASSERT conditions have been amended so they are no longer raised if a fatal error has been registered.

Fixes #6615
Fixes #4875

[llvm-beanz]

This is all a grotesque misuse of asserts. We shouldn't be asserting for things that can actually fail.

My gut feeling here is that if we're going to spend the time fixing this we should actually refactor the code so that we propagate the error condition and gracefully exit early.

[tcorringham]

Asserts are used widely to detect exceptional error conditions that should never occur in general use - this style of programming has advantages and disadvantages, which could be debated at length (I'm not a fan...)

In this instance, it was expected that the HLSL template specialization can never fail (it is straightforward, and well exercised) - however, when a fatal error has already been registered template specialization is suppressed leading to the failure of the specialization, and thereby triggering the assert.

There could be an argument for changing the general approach of using asserts, and possibly replacing them with a fatal error - that should be possible for most, but I suspect there would be cases where continuing compilation to the point where error diagnostics would be generated wouldn't be possible.

In this specific case, the assert could easily be replaced by a fatal error - but the only expected trigger is when another fatal error has already been registered. Simply suppressing the assert in that case does allow the compilation to continue, and the fatal error is then generated. Adding a second fatal error to replace the assert seems unnecessary, though a fatal error could be generated if specialization failed due to some other reason.

Simply changing the assert condition does solve the immediate issue, but doesn't address the wider issue of the use of asserts. I'm open to suggestions of how to progress this...

[llvm-beanz]

I'm not asking you to fix DXC's misuse of asserts everywhere, just in the code that you're modifying in this PR already. I don't think any best-practices for C/C++ programming recommends using them in this manner, but more importantly they're not consistent with LLVM convention and style (which we've adopted for all future changes).

You've already had to update a chain of asserts across all the use sites, we could instead just clean them up. It isn't actually any more work.

Here's a patch that accomplishes the same thing you're doing here, but cleans up the code it touches along the way:

diff --git a/tools/clang/lib/Sema/SemaHLSL.cpp b/tools/clang/lib/Sema/SemaHLSL.cpp
index 4df32f9a659..7e9ba3f1334 100644
--- a/tools/clang/lib/Sema/SemaHLSL.cpp
+++ b/tools/clang/lib/Sema/SemaHLSL.cpp
@@ -869,11 +869,10 @@ GetOrCreateTemplateSpecialization(ASTContext &context, Sema &sema,
     if (specializationDecl->getInstantiatedFrom().isNull()) {
       // InstantiateClassTemplateSpecialization returns true if it finds an
       // error.
-      DXVERIFY_NOMSG(false ==
-                     sema.InstantiateClassTemplateSpecialization(
-                         NoLoc, specializationDecl,
-                         TemplateSpecializationKind::TSK_ImplicitInstantiation,
-                         true));
+      if (sema.InstantiateClassTemplateSpecialization(
+              NoLoc, specializationDecl,
+              TemplateSpecializationKind::TSK_ImplicitInstantiation, true))
+        return QualType();
     }
     return context.getTemplateSpecializationType(
         TemplateName(templateDecl), templateArgs.data(), templateArgs.size(),
@@ -930,16 +929,15 @@ 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()) {
+    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"))));
-  DXASSERT(!lookupResult.empty(),
+    [[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;
 }
@@ -965,16 +963,16 @@ 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()) {
+    // Verify that we can read the field member from the template record.
+    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"))));
-  DXASSERT(!lookupResult.empty(),
+    [[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;
 }
@@ -1021,16 +1019,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()) {
+    // Verify that we can read the field member from the template record.
+    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"))));
-  DXASSERT(!lookupResult.empty(),
+    [[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;
 }
@@ -5242,12 +5240,13 @@ public:
           // 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()) {

[tcorringham]

I have adopted the suggested approach - however, use of hasFatalErrorOccurred() is still required in some places to avoid conditions which trigger subsequent asserts.