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>

11 files changed, 73 insertions, 50 deletions

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.