Better diagnostics when compilation is aborted (#517)

* Improve messages when compilation is aborted.

Make sure to include the information from any `Slang::Exception` that was thrown, so that the poor user can at least point us at our own message string from an assertion failure.

This doesn't provide them line-number information in their code or the Slang codebase, so there is still work to be done in making the compiler more friendly about this stuff.

* When aborting compilation, try to note what source location we were working on

This is handled by having exception handlers on the stack at key bottleneck points in semantic checking and IR generation, which can then emit a diagnostic to note what we were working on when things failed.

This is not intended to be an indiciation to the user that their code is at fault for a compiler crash (it is always our fault), but might give them a chance to work around whatever bug is blocking them.

5 files changed, 133 insertions, 21 deletions

diff --git a/source/slang/check.cpp b/source/slang/check.cpp
index acf473998..5fd8be2d5 100644
--- a/source/slang/check.cpp
+++ b/source/slang/check.cpp
@@ -455,6 +455,51 @@ namespace Slang
                 innerDeclRef);
         }
 
+        // This routine is a bottleneck for all declaration checking,
+        // so that we can add some quality-of-life features for users
+        // in cases where the compiler crashes
+        void dispatchDecl(DeclBase* decl)
+        {
+            try
+            {
+                DeclVisitor::dispatch(decl);
+            }
+            // Don't emit any context message for an explicit `AbortCompilationException`
+            // because it should only happen when an error is already emitted.
+            catch(AbortCompilationException&) { throw; }
+            catch(...)
+            {
+                getCompileRequest()->noteInternalErrorLoc(decl->loc);
+                throw;
+            }
+        }
+        void dispatchStmt(Stmt* stmt)
+        {
+            try
+            {
+                StmtVisitor::dispatch(stmt);
+            }
+            catch(AbortCompilationException&) { throw; }
+            catch(...)
+            {
+                getCompileRequest()->noteInternalErrorLoc(stmt->loc);
+                throw;
+            }
+        }
+        void dispatchExpr(Expr* expr)
+        {
+            try
+            {
+                ExprVisitor::dispatch(expr);
+            }
+            catch(AbortCompilationException&) { throw; }
+            catch(...)
+            {
+                getCompileRequest()->noteInternalErrorLoc(expr->loc);
+                throw;
+            }
+        }
+
         // Make sure a declaration has been checked, so we can refer to it.
         // Note that this may lead to us recursively invoking checking,
         // so this may not be the best way to handle things.
@@ -499,7 +544,7 @@ namespace Slang
             }
 
             // Use visitor pattern to dispatch to correct case
-            DeclVisitor::dispatch(decl);
+            dispatchDecl(decl);
 
             if(state > decl->checkState)
             {
@@ -2492,7 +2537,7 @@ namespace Slang
         {
             for (auto decl : declGroup->decls)
             {
-                DeclVisitor::dispatch(decl);
+                dispatchDecl(decl);
             }
         }
 
@@ -2549,7 +2594,7 @@ namespace Slang
         void checkStmt(Stmt* stmt)
         {
             if (!stmt) return;
-            StmtVisitor::dispatch(stmt);
+            dispatchStmt(stmt);
             checkModifiers(stmt);
         }
 
@@ -3052,7 +3097,7 @@ namespace Slang
             // 2. `EnsureDecl()` is specialized for `Decl*` instead of `DeclBase*`
             // and trying to special case `DeclGroup*` here feels silly.
             //
-            DeclVisitor::dispatch(stmt->decl);
+            dispatchDecl(stmt->decl);
         }
 
         void visitBlockStmt(BlockStmt* stmt)
diff --git a/source/slang/compiler.h b/source/slang/compiler.h
index 703991e36..4cda366f0 100644
--- a/source/slang/compiler.h
+++ b/source/slang/compiler.h
@@ -423,6 +423,12 @@ namespace Slang
             sourceManager = sm;
             mSink.sourceManager = sm;
         }
+
+            /// During propagation of an exception for an internal
+            /// error, note that this source location was involved
+        void noteInternalErrorLoc(SourceLoc const& loc);
+
+        int internalErrorLocsNoted = 0;
     };
 
     void generateOutput(
diff --git a/source/slang/diagnostic-defs.h b/source/slang/diagnostic-defs.h
index a3c51d67e..c77e75817 100644
--- a/source/slang/diagnostic-defs.h
+++ b/source/slang/diagnostic-defs.h
@@ -341,9 +341,11 @@ DIAGNOSTIC(51092, Error, stageDoesntHaveInputWorld, "'$0' doesn't appear to have
 
 // 99999 - Internal compiler errors, and not-yet-classified diagnostics.
 
-DIAGNOSTIC(99999, Internal, unimplemented, "unimplemented: $0")
-DIAGNOSTIC(99999, Internal, unexpected, "unexpected: $0")
-DIAGNOSTIC(99999, Internal, internalCompilerError, "internal compiler error")
-DIAGNOSTIC(99999, Error, compilationAborted, "compilation aborted due to internal error");
+DIAGNOSTIC(99999, Internal, unimplemented, "unimplemented feature in Slang compiler: $0")
+DIAGNOSTIC(99999, Internal, unexpected, "unexpected condition encountered in Slang compiler: $0")
+DIAGNOSTIC(99999, Internal, internalCompilerError, "Slang internal compiler error")
+DIAGNOSTIC(99999, Error, compilationAborted, "Slang compilation aborted due to internal error");
+DIAGNOSTIC(99999, Error, compilationAbortedDueToException, "Slang compilation aborted due to an exception of $0: $1");
+DIAGNOSTIC(99999, Note, noteLocationOfInternalError, "the Slang compiler threw an exception while working on code near this location");
 
 #undef DIAGNOSTIC
diff --git a/source/slang/lower-to-ir.cpp b/source/slang/lower-to-ir.cpp
index e5cf19a10..fffd8acf3 100644
--- a/source/slang/lower-to-ir.cpp
+++ b/source/slang/lower-to-ir.cpp
@@ -355,6 +355,11 @@ struct IRGenContext
     {
         return shared->compileRequest->mSession;
     }
+
+    CompileRequest* getCompileRequest()
+    {
+        return shared->compileRequest;
+    }
 };
 
 void setGlobalValue(SharedIRGenContext* sharedContext, Decl* decl, LoweredValInfo value)
@@ -2680,7 +2685,19 @@ void lowerStmt(
 
     StmtLoweringVisitor visitor;
     visitor.context = context;
-    return visitor.dispatch(stmt);
+
+    try
+    {
+        visitor.dispatch(stmt);
+    }
+    // Don't emit any context message for an explicit `AbortCompilationException`
+    // because it should only happen when an error is already emitted.
+    catch(AbortCompilationException&) { throw; }
+    catch(...)
+    {
+        context->getCompileRequest()->noteInternalErrorLoc(stmt->loc);
+        throw;
+    }
 }
 
 static LoweredValInfo maybeMoveMutableTemp(
@@ -4380,7 +4397,21 @@ LoweredValInfo lowerDecl(
 
     DeclLoweringVisitor visitor;
     visitor.context = &subContext;
-    return visitor.dispatch(decl);
+
+
+
+    try
+    {
+        return visitor.dispatch(decl);
+    }
+    // Don't emit any context message for an explicit `AbortCompilationException`
+    // because it should only happen when an error is already emitted.
+    catch(AbortCompilationException&) { throw; }
+    catch(...)
+    {
+        context->getCompileRequest()->noteInternalErrorLoc(decl->loc);
+        throw;
+    }
 }
 
 // Ensure that a version of the given declaration has been emitted to the IR
diff --git a/source/slang/slang.cpp b/source/slang/slang.cpp
index dbd48753b..9e740d5f5 100644
--- a/source/slang/slang.cpp
+++ b/source/slang/slang.cpp
@@ -9,6 +9,9 @@
 #include "syntax-visitors.h"
 #include "../slang/type-layout.h"
 
+// Used to print exception type names in internal-compiler-error messages
+#include <typeinfo>
+
 #ifdef _WIN32
 #define WIN32_LEAN_AND_MEAN
 #define NOMINMAX
@@ -724,6 +727,23 @@ Decl * CompileRequest::lookupGlobalDecl(Name * name)
     return resultDecl;
 }
 
+void CompileRequest::noteInternalErrorLoc(SourceLoc const& loc)
+{
+    // Don't consider invalid source locations.
+    if(!loc.isValid())
+        return;
+
+    // If this is the first source location being noted,
+    // then emit a message to help the user isolate what
+    // code might have confused the compiler.
+    if(internalErrorLocsNoted == 0)
+    {
+        mSink.diagnose(loc, Diagnostics::noteLocationOfInternalError);
+    }
+    internalErrorLocsNoted++;
+}
+
+
 RefPtr<ModuleDecl> findOrImportModule(
     CompileRequest*     request,
     Name*               name,
@@ -1111,27 +1131,35 @@ SLANG_API int spCompile(
     //
     // TODO: Consider supporting Windows "Structured Exception Handling"
     // so that we can also recover from a wider class of crashes.
+    int anyErrors = 1;
     try
     {
-        int anyErrors = req->executeActions();
-        return anyErrors;
+        anyErrors = req->executeActions();
     }
     catch (Slang::AbortCompilationException&)
     {
-        // This should only be thrown if we already emitted a fatal error
-        // message, so there is no reason to print something else.
-        //
-        // We still need to copy the diagnostic output into the variable
-        // that the user will query via the API.
-        req->mDiagnosticOutput = req->mSink.outputBuffer.ProduceString();
-        return 1;
+        // This situation indicates a fatal (but not necesarily internal) error
+        // that forced compilation to terminate. There should already have been
+        // a diagnositc produced, so we don't need to add one here.
+    }
+    catch (Slang::Exception& e)
+    {
+        // The compiler failed due to an internal error that was detected.
+        // We will print out information on the exception to help out the user
+        // in either filing a bug, or locating what in their code created
+        // a problem.
+        req->mSink.diagnose(Slang::SourceLoc(), Slang::Diagnostics::compilationAbortedDueToException, typeid(e).name(), e.Message);
     }
     catch (...)
     {
+        // The compiler failed due to some exception that wasn't a sublass of
+        // `Exception`, so something really fishy is going on. We want to
+        // let the user know that we messed up, so they know to blame Slang
+        // and not some other component in their system.
         req->mSink.diagnose(Slang::SourceLoc(), Slang::Diagnostics::compilationAborted);
-        req->mDiagnosticOutput = req->mSink.outputBuffer.ProduceString();
-        return 1;
     }
+    req->mDiagnosticOutput = req->mSink.outputBuffer.ProduceString();
+    return anyErrors;
 #else
     // When debugging, we probably don't want to filter out any errors, since
     // we are probably trying to root-cause and *fix* those errors.