Fix erroneous error claiming variable is being used before its declaration (#2958)

* Simplify type of diagnoseImpl

* Show source line for Note diagnostics, opting out of this where appropriate

* Make declared after use diagnostic clearer

* Fix erroneous error claiming variable is being used before its declaration

Closes https://github.com/shader-slang/slang/issues/2936

* Fix build on msvc

---------

Co-authored-by: jsmall-nvidia <jsmall@nvidia.com>

13 files changed, 95 insertions, 82 deletions

diff --git a/source/compiler-core/slang-diagnostic-sink.cpp b/source/compiler-core/slang-diagnostic-sink.cpp
index f360d3cc6..197e9bb64 100644
--- a/source/compiler-core/slang-diagnostic-sink.cpp
+++ b/source/compiler-core/slang-diagnostic-sink.cpp
@@ -75,7 +75,7 @@ SourceLoc getDiagnosticPos(Token const& token)
 }
 
 // Take the format string for a diagnostic message, along with its arguments, and turn it into a
-static void formatDiagnosticMessage(StringBuilder& sb, char const* format, int argCount, DiagnosticArg const* const* args)
+static void formatDiagnosticMessage(StringBuilder& sb, char const* format, int argCount, DiagnosticArg const* args)
 {
     char const* spanBegin = format;
     for(;;)
@@ -116,8 +116,8 @@ static void formatDiagnosticMessage(StringBuilder& sb, char const* format, int a
                 }
                 else
                 {
-                    DiagnosticArg const* arg = args[index];
-                    arg->printFunc(sb, arg->data);
+                    DiagnosticArg const& arg = args[index];
+                    arg.printFunc(sb, arg.data);
                 }
             }
             break;
@@ -459,9 +459,7 @@ static void formatDiagnostic(
         _tokenLengthNoteDiagnostic(sink, sourceView, sourceLoc, sb);
     }
 
-    // We don't don't output source line information if this is a 'note' as a note is extra information for one
-    // of the other main severity types, and so the information should already be output on the initial line
-    if (sourceView && sink->isFlagSet(DiagnosticSink::Flag::SourceLocationLine) && diagnostic.severity != Severity::Note)
+    if (sourceView && sink->isFlagSet(DiagnosticSink::Flag::SourceLocationLine) && diagnostic.loc.isValid())
     {
         _sourceLocationNoteDiagnostic(sink, sourceView, sourceLoc, sb);
     }
@@ -600,7 +598,7 @@ Severity DiagnosticSink::getEffectiveMessageSeverity(DiagnosticInfo const& info)
     return effectiveSeverity;
 }
 
-void DiagnosticSink::diagnoseImpl(SourceLoc const& pos, DiagnosticInfo info, int argCount, DiagnosticArg const* const* args)
+void DiagnosticSink::diagnoseImpl(SourceLoc const& pos, DiagnosticInfo info, int argCount, DiagnosticArg const* args)
 {
     // Override the severity in the 'info' structure to pass it further into formatDiagnostics
     info.severity = getEffectiveMessageSeverity(info);
diff --git a/source/compiler-core/slang-diagnostic-sink.h b/source/compiler-core/slang-diagnostic-sink.h
index ebd43b456..c150de900 100644
--- a/source/compiler-core/slang-diagnostic-sink.h
+++ b/source/compiler-core/slang-diagnostic-sink.h
@@ -162,39 +162,31 @@ public:
         /// Get the total amount of errors that have taken place on this DiagnosticSink
     SLANG_FORCE_INLINE int getErrorCount() { return m_errorCount; }
 
-    void diagnoseDispatch(SourceLoc const& pos, DiagnosticInfo const& info)
-    {
-        diagnoseImpl(pos, info, 0, nullptr);
-    }
-
-    void diagnoseDispatch(SourceLoc const& pos, DiagnosticInfo const& info, DiagnosticArg const& arg0)
+    template<typename P, typename... Args>
+    void diagnose(P const& pos, DiagnosticInfo const& info, Args const&... args )
     {
-        DiagnosticArg const* args[] = { &arg0 };
-        diagnoseImpl(pos, info, 1, args);
+        DiagnosticArg as[] = { DiagnosticArg(args)... };
+        diagnoseImpl(getDiagnosticPos(pos), info, sizeof...(args), as);
     }
 
-    void diagnoseDispatch(SourceLoc const& pos, DiagnosticInfo const& info, DiagnosticArg const& arg0, DiagnosticArg const& arg1)
+    template<typename P>
+    void diagnose(P const& pos, DiagnosticInfo const& info)
     {
-        DiagnosticArg const* args[] = { &arg0, &arg1 };
-        diagnoseImpl(pos, info, 2, args);
+        // MSVC gets upset with the zero sized array above, so overload that case here
+        diagnoseImpl(getDiagnosticPos(pos), info, 0, nullptr);
     }
 
-    void diagnoseDispatch(SourceLoc const& pos, DiagnosticInfo const& info, DiagnosticArg const& arg0, DiagnosticArg const& arg1, DiagnosticArg const& arg2)
+    // Useful for notes on existing diagnostics, where it would be redundant to display the same line again.
+    // (Ideally we would print the error/warning and notes in one call...)
+    template<typename P, typename... Args>
+    void diagnoseWithoutSourceView(P const& pos, DiagnosticInfo const& info, Args const&... args )
     {
-        DiagnosticArg const* args[] = { &arg0, &arg1, &arg2 };
-        diagnoseImpl(pos, info, 3, args);
-    }
+        const auto fs = this->getFlags();
+        this->resetFlag(Flag::SourceLocationLine);
 
-    void diagnoseDispatch(SourceLoc const& pos, DiagnosticInfo const& info, DiagnosticArg const& arg0, DiagnosticArg const& arg1, DiagnosticArg const& arg2, DiagnosticArg const& arg3)
-    {
-        DiagnosticArg const* args[] = { &arg0, &arg1, &arg2, &arg3 };
-        diagnoseImpl(pos, info, 4, args);
-    }
+        diagnose(pos, info, args...);
 
-    template<typename P, typename... Args>
-    void diagnose(P const& pos, DiagnosticInfo const& info, Args const&... args )
-    {
-        diagnoseDispatch(getDiagnosticPos(pos), info, args...);
+        this->setFlags(fs);
     }
 
         // Add a diagnostic with raw text
@@ -267,7 +259,7 @@ public:
     ISlangWriter* writer = nullptr;
 
 protected:
-    void diagnoseImpl(SourceLoc const& pos, DiagnosticInfo info, int argCount, DiagnosticArg const* const* args);
+    void diagnoseImpl(SourceLoc const& pos, DiagnosticInfo info, int argCount, DiagnosticArg const* args);
     void diagnoseImpl(DiagnosticInfo const& info, const UnownedStringSlice& formattedMessage);
 
     Severity getEffectiveMessageSeverity(DiagnosticInfo const& info);
diff --git a/source/slang/slang-ast-decl.cpp b/source/slang/slang-ast-decl.cpp
index 261378b9a..2f1c7c47e 100644
--- a/source/slang/slang-ast-decl.cpp
+++ b/source/slang/slang-ast-decl.cpp
@@ -102,4 +102,20 @@ void ContainerDecl::buildMemberDictionary()
     SLANG_ASSERT(isMemberDictionaryValid());
 }
 
+bool isLocalVar(const Decl* decl)
+{
+    const auto varDecl = as<VarDecl>(decl);
+    if(!varDecl)
+        return false;
+    const Decl* pp = varDecl->parentDecl;
+    if(as<ScopeDecl>(pp))
+        return true;
+    while(auto genericDecl = as<GenericDecl>(pp))
+        pp = genericDecl->inner;
+    if(as<FunctionDeclBase>(pp))
+        return true;
+
+    return false;
+}
+
 } // namespace Slang
diff --git a/source/slang/slang-ast-decl.h b/source/slang/slang-ast-decl.h
index e75660c7b..93e5a19ad 100644
--- a/source/slang/slang-ast-decl.h
+++ b/source/slang/slang-ast-decl.h
@@ -552,4 +552,6 @@ class BackwardDerivativeRequirementDecl : public DerivativeRequirementDecl
 bool isInterfaceRequirement(Decl* decl);
 InterfaceDecl* findParentInterfaceDecl(Decl* decl);
 
+bool isLocalVar(const Decl* decl);
+
 } // namespace Slang
diff --git a/source/slang/slang-ast-support-types.h b/source/slang/slang-ast-support-types.h
index b6e98bbd4..d3cb4e55d 100644
--- a/source/slang/slang-ast-support-types.h
+++ b/source/slang/slang-ast-support-types.h
@@ -21,6 +21,7 @@
 #include "slang-ref-object-reflect.h"
 
 #include <assert.h>
+#include <type_traits>
 
 namespace Slang
 {
@@ -1181,7 +1182,17 @@ namespace Slang
         IgnoreBaseInterfaces = 1 << 0,
         Completion = 1 << 1, ///< Lookup all applicable decls for code completion suggestions
         NoDeref = 1 << 2,
+        ConsiderAllLocalNamesInScope = 1 << 3,
+        ///^ Normally we rely on the checking state of local names to determine
+        /// if they have been declared. If the scopes are currently
+        /// "under-construction" and not being checked, then it's safe to
+        /// consider all names we've inserted so far. This is used when
+        /// checking to see if a keyword is shadowed.
     };
+    inline LookupOptions operator&(LookupOptions a, LookupOptions b)
+    {
+        return (LookupOptions)((std::underlying_type_t<LookupOptions>)a & (std::underlying_type_t<LookupOptions>)b);
+    }
 
     class SerialRefObject;
 
@@ -1421,7 +1432,8 @@ namespace Slang
         LookupMask          mask        = LookupMask::Default;
         LookupOptions       options     = LookupOptions::None;
 
-        bool isCompletionRequest() const { return ((int)options & (int)LookupOptions::Completion) != 0; }
+        bool isCompletionRequest() const { return (options & LookupOptions::Completion) != LookupOptions::None; }
+        bool shouldConsiderAllLocalNames() const { return (options & LookupOptions::ConsiderAllLocalNamesInScope) != LookupOptions::None; }
     };
 
     struct WitnessTable;
diff --git a/source/slang/slang-check-conversion.cpp b/source/slang/slang-check-conversion.cpp
index 1afbaf6e8..17b55a2cb 100644
--- a/source/slang/slang-check-conversion.cpp
+++ b/source/slang/slang-check-conversion.cpp
@@ -1115,7 +1115,7 @@ namespace Slang
                 if (cost >= kConversionCost_Explicit)
                 {
                     getSink()->diagnose(fromExpr, Diagnostics::typeMismatch, toType, fromType);
-                    getSink()->diagnose(
+                    getSink()->diagnoseWithoutSourceView(
                         fromExpr, Diagnostics::noteExplicitConversionPossible, fromType, toType);
                 }
                 else if (cost >= kConversionCost_Default)
diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp
index e2d6b797a..46b968279 100644
--- a/source/slang/slang-check-decl.cpp
+++ b/source/slang/slang-check-decl.cpp
@@ -395,20 +395,12 @@ namespace Slang
         return true;
     }
 
-    static bool _isLocalVar(VarDeclBase* varDecl)
+    [[maybe_unused]]
+    static bool _isUncheckedLocalVar(const Decl* decl)
     {
-        auto pp = varDecl->parentDecl;
-
-        if(as<ScopeDecl>(pp))
-            return true;
-
-        if(auto genericDecl = as<GenericDecl>(pp))
-            pp = genericDecl;
-
-        if(as<FuncDecl>(pp))
-            return true;
-
-        return false;
+        auto checkStateExt = decl->checkState;
+        auto isUnchecked = checkStateExt.getState() == DeclCheckState::Unchecked || checkStateExt.isBeingChecked();
+        return isUnchecked && isLocalVar(decl);
     }
 
     // Get the type to use when referencing a declaration
@@ -422,35 +414,12 @@ namespace Slang
     {
         if( sema )
         {
-            // Hack: if we are somehow referencing a local variable declaration
-            // before the line of code that defines it, then we need to diagnose
-            // an error.
-            //
-            // TODO: The right answer is that lookup should have been performed in
-            // the scope that was in place *before* the variable was declared, but
-            // this is a quick fix that at least alerts the user to how we are
-            // interpreting their code.
-            //
-            // We detect the problematic case by looking for an attempt to reference
-            // a local variable declaration when it is unchecked, or in the process
-            // of being checked (the latter case catches a local variable that refers
-            // to itself in its initial-value expression).
-            //
-            auto checkStateExt = declRef.getDecl()->checkState;
-            if( checkStateExt.getState() == DeclCheckState::Unchecked
-                || checkStateExt.isBeingChecked() )
-            {
-                if(auto varDecl = as<VarDecl>(declRef.getDecl()))
-                {
-                    if(_isLocalVar(varDecl))
-                    {
-                        sema->getSink()->diagnose(varDecl, Diagnostics::localVariableUsedBeforeDeclared, varDecl);
-                        return QualType(astBuilder->getErrorType());
-                    }
-                }
-            }
+            // If this is a local variable which hasn't been checked yet then
+            // it's probably a declare-after-use which has incorrectly got
+            // through declref resolution.
+            SLANG_ASSERT(!_isUncheckedLocalVar(declRef.getDecl()));
 
-            // Once we've rules out the case of referencing a local declaration
+            // Once we've ruled out the case of referencing a local declaration
             // before it has been checked, we will go ahead and ensure that
             // semantic checking has been performed on the chosen declaration,
             // at least up to the point where we can query its type.
diff --git a/source/slang/slang-check-expr.cpp b/source/slang/slang-check-expr.cpp
index aa3ca889d..11b560d93 100644
--- a/source/slang/slang-check-expr.cpp
+++ b/source/slang/slang-check-expr.cpp
@@ -1909,7 +1909,7 @@ namespace Slang
         {
             if(!thisExpr->type.isLeftValue)
             {
-                getSink()->diagnose(thisExpr, Diagnostics::thisIsImmutableByDefault);
+                getSink()->diagnoseWithoutSourceView(thisExpr, Diagnostics::thisIsImmutableByDefault);
             }
         }
     }
@@ -2130,7 +2130,7 @@ namespace Slang
                                             diagnostic = &Diagnostics::implicitCastUsedAsLValue;
                                         }
 
-                                        getSink()->diagnose(
+                                        getSink()->diagnoseWithoutSourceView(
                                             argExpr,
                                             *diagnostic,
                                             implicitCastExpr->arguments[pp]->type,
diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
index 922348d61..8f6526d9d 100644
--- a/source/slang/slang-diagnostic-defs.h
+++ b/source/slang/slang-diagnostic-defs.h
@@ -41,6 +41,8 @@ DIAGNOSTIC(-1, Note, seeRequirementDeclaration, "see requirement declaration")
 DIAGNOSTIC(-1, Note, doYouForgetToMakeComponentAccessible, "do you forget to make component '$0' acessible from '$1' (missing public qualifier)?")
 
 DIAGNOSTIC(-1, Note, seeDeclarationOf, "see declaration of '$0'")
+// An alternate wording of the above note, emphasing the position rather than content of the declaration.
+DIAGNOSTIC(-1, Note, declaredHere, "declared here")
 DIAGNOSTIC(-1, Note, seeOtherDeclarationOf, "see other declaration of '$0'")
 DIAGNOSTIC(-1, Note, seePreviousDeclarationOf, "see previous declaration of '$0'")
 DIAGNOSTIC(-1, Note, includeOutput, "include $0")
diff --git a/source/slang/slang-ir-witness-table-wrapper.cpp b/source/slang/slang-ir-witness-table-wrapper.cpp
index 68fdf5b60..d6cfda808 100644
--- a/source/slang/slang-ir-witness-table-wrapper.cpp
+++ b/source/slang/slang-ir-witness-table-wrapper.cpp
@@ -190,7 +190,7 @@ namespace Slang
             if (!sharedContext->doesTypeFitInAnyValue(concreteType, interfaceType, &typeSize, &sizeLimit))
             {
                 sharedContext->sink->diagnose(concreteType, Diagnostics::typeDoesNotFitAnyValueSize, concreteType);
-                sharedContext->sink->diagnose(concreteType, Diagnostics::typeAndLimit, concreteType, typeSize, sizeLimit);
+                sharedContext->sink->diagnoseWithoutSourceView(concreteType, Diagnostics::typeAndLimit, concreteType, typeSize, sizeLimit);
                 return;
             }
 
diff --git a/source/slang/slang-lookup.cpp b/source/slang/slang-lookup.cpp
index 0c4279761..46977b71d 100644
--- a/source/slang/slang-lookup.cpp
+++ b/source/slang/slang-lookup.cpp
@@ -139,6 +139,13 @@ static void _lookUpMembersInValue(
     LookupResult&	        ioResult,
     BreadcrumbInfo*	        breadcrumbs);
 
+static bool _isUncheckedLocalVar(const Decl* decl)
+{
+    auto checkStateExt = decl->checkState;
+    auto isUnchecked = checkStateExt.getState() == DeclCheckState::Unchecked || checkStateExt.isBeingChecked();
+    return isUnchecked && isLocalVar(decl);
+}
+
     /// Look up direct members (those declared in `containerDeclRef` itself, as well
     /// as transitively through any direct members that are marked "transparent."
     ///
@@ -162,6 +169,8 @@ static void _lookUpDirectAndTransparentMembers(
         // return all the members that are available.
         for (auto member : containerDecl->members)
         {
+            if(!request.shouldConsiderAllLocalNames() && _isUncheckedLocalVar(member))
+                continue;
             if (!DeclPassesLookupMask(member, request.mask))
                 continue;
             AddToLookupResult(
@@ -182,6 +191,12 @@ static void _lookUpDirectAndTransparentMembers(
         // type declarations.
         for (auto m = firstDecl; m; m = m->nextInContainerWithSameName)
         {
+            // Skip this declaration if we are checking and this hasn't been
+            // checked yet. Because we traverse block statements in order, if
+            // it's unchecked or being checked then it isn't declared yet.
+            if(!request.shouldConsiderAllLocalNames() && _isUncheckedLocalVar(m))
+                continue;
+
             if (!DeclPassesLookupMask(m, request.mask))
                 continue;
 
@@ -948,10 +963,14 @@ LookupResult lookUp(
     SemanticsVisitor*   semantics,
     Name*               name,
     Scope*              scope,
-    LookupMask          mask)
+    LookupMask          mask,
+    bool                considerAllLocalNamesInScope)
 {
     LookupResult result;
-    LookupRequest request = initLookupRequest(semantics, name, mask, LookupOptions::None, scope);
+    const auto options = considerAllLocalNamesInScope
+        ? LookupOptions::ConsiderAllLocalNamesInScope
+        : LookupOptions::None;
+    LookupRequest request = initLookupRequest(semantics, name, mask, options, scope);
     _lookUpInScopes(astBuilder, name, request, result);
     return result;
 }
diff --git a/source/slang/slang-lookup.h b/source/slang/slang-lookup.h
index 7a9346498..69374c024 100644
--- a/source/slang/slang-lookup.h
+++ b/source/slang/slang-lookup.h
@@ -18,7 +18,8 @@ LookupResult lookUp(
     SemanticsVisitor*   semantics,
     Name*               name,
     Scope*              scope,
-    LookupMask          mask = LookupMask::Default);
+    LookupMask          mask = LookupMask::Default,
+    bool                considerAllLocalNamesInScope = false);
 
 // Perform member lookup in the context of a type
 LookupResult lookUpMember(
diff --git a/source/slang/slang-parser.cpp b/source/slang/slang-parser.cpp
index 5f9d5fbc4..b8310451c 100644
--- a/source/slang/slang-parser.cpp
+++ b/source/slang/slang-parser.cpp
@@ -1021,7 +1021,9 @@ namespace Slang
             parser->astBuilder,
             nullptr, // no semantics visitor available yet
             name,
-            parser->currentScope);
+            parser->currentScope,
+            LookupMask::Default,
+            true);
 
         // If we didn't find anything, or the result was overloaded,
         // then we aren't going to be able to extract a single decl.