Diagnose attempts to call instance methods from static methods (#1330)

Currently we fail to diagnose code that calls an instance method from a static method using implicit `this`, and instead crash during lowering of the AST to the IR.

This change introduces a bit more detail to the "this parameter mode" that is computed during lookup, so that it differentiates three cases. The existing two cases of a mutable `this` and immutable `this` remain, but we add a third case where the "this parameter mode" only allows for a reference to the `This` type.

When turning lookup "breadcrumb" information into actual expressions, we respect this setting to construct either a `This` or `this` expression.

In order to actually diagnose the incorrect reference, I had to add code around an existing `TODO` comment that noted how we should diagnose attempts to refer to instance members through a type. Enabling that diagnostic revealed a missing case needed by generics (including those in the stdlib) - a type-constraint member is always referenced statically.

Putting the diagnostic for a static reference to a non-static member in its new bottleneck location meant that some code higher up the call static that handles explicit static member references had to be tweaked to not produce double error messages.

This change includes a new diagnostic test to show that we now give an error message on code that makes this mistake, instead of crashing.

8 files changed, 167 insertions, 73 deletions

diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp
index d6d115859..13db80ee1 100644
--- a/source/slang/slang-check-decl.cpp
+++ b/source/slang/slang-check-decl.cpp
@@ -557,6 +557,9 @@ namespace Slang
         if(as<SimpleTypeDecl>(decl))
             return true;
 
+        if(as<TypeConstraintDecl>(decl))
+            return true;
+
         return false;
     }
 
diff --git a/source/slang/slang-check-expr.cpp b/source/slang/slang-check-expr.cpp
index a8114c8d6..8f2baed96 100644
--- a/source/slang/slang-check-expr.cpp
+++ b/source/slang/slang-check-expr.cpp
@@ -191,13 +191,33 @@ namespace Slang
             // actually names a type, because in that case we are doing
             // a static member reference.
             //
-            // TODO: Should we be checking if the member is static here?
-            // If it isn't, should we be automatically producing a "curried"
-            // form (e.g., for a member function, return a value usable
-            // for referencing it as a free function).
-            //
-            if (as<TypeType>(baseExpr->type))
+            if (auto typeType = as<TypeType>(baseExpr->type))
             {
+                // Before forming the reference, we will check if the
+                // member being referenced can even be used as a static
+                // member, and if not we will diagnose an error.
+                //
+                // TODO: It is conceptually possible to allow static
+                // references to many instance members, provided we
+                // change the exposed type/signature.
+                //
+                // E.g., if we have:
+                //
+                //      struct Test { float getVal() { ... } }
+                //
+                // Then a reference to `Test.getVal` could be allowed,
+                // and given a type of `(Test) -> float` to indicate
+                // that it is an "unbound" instance method.
+                //
+                if( !isDeclUsableAsStaticMember(declRef.getDecl()) )
+                {
+                    getSink()->diagnose(
+                        loc,
+                        Diagnostics::staticRefToNonStaticMember,
+                        typeType->type,
+                        declRef.GetName());
+                }
+
                 auto expr = new StaticMemberExpr();
                 expr->loc = loc;
                 expr->type = type;
@@ -280,18 +300,6 @@ namespace Slang
         return derefExpr;
     }
 
-    RefPtr<Expr> SemanticsVisitor::createImplicitThisMemberExpr(
-        Type*                                           type,
-        SourceLoc                                       loc,
-        LookupResultItem::Breadcrumb::ThisParameterMode thisParameterMode)
-    {
-        RefPtr<ThisExpr> expr = new ThisExpr();
-        expr->type = type;
-        expr->type.IsLeftValue = thisParameterMode == LookupResultItem::Breadcrumb::ThisParameterMode::Mutating;
-        expr->loc = loc;
-        return expr;
-    }
-
     RefPtr<Expr> SemanticsVisitor::ConstructLookupResultExpr(
         LookupResultItem const& item,
         RefPtr<Expr>            baseExpr,
@@ -330,22 +338,60 @@ namespace Slang
                     // at the start of a chain.
                     SLANG_ASSERT(bb == nullptr);
 
-                    // The member was looked up via a `this` expression,
-                    // so we need to create one here.
-                    if (auto extensionDeclRef = breadcrumb->declRef.as<ExtensionDecl>())
+                    // We will compute the type to use for `This` using
+                    // the same logic that a direct reference to `This`
+                    // uses.
+                    //
+                    auto thisType = calcThisType(breadcrumb->declRef);
+
+                    // Next we construct an appropriate expression to
+                    // stand in for the implicit `this` or `This` reference.
+                    //
+                    // The lookup process will have computed the appropriate
+                    // "mode" to use for the implicit `this` or `This`.
+                    //
+                    auto thisParameterMode = breadcrumb->thisParameterMode;
+                    if(thisParameterMode == LookupResultItem::Breadcrumb::ThisParameterMode::Type)
                     {
-                        bb = createImplicitThisMemberExpr(
-                            GetTargetType(extensionDeclRef),
-                            loc,
-                            breadcrumb->thisParameterMode);
+                        // If we are in a static context, then we do not
+                        // have implicit `this` expression, and the expression
+                        // we construct will need to start with the `This`
+                        // type.
+                        //
+                        // Because we are constrained to yield an expression
+                        // here, we must construct an expression that
+                        // references `This`, and the *type* of that expression
+                        // will be `typeof(This)`, which conceptually
+                        // `typeof(typeof(this))`
+                        //
+                        auto thisTypeType = getTypeType(thisType);
+
+                        auto typeExpr = new SharedTypeExpr();
+                        typeExpr->type.type = thisTypeType;
+                        typeExpr->base.type = thisType;
+
+                        bb = typeExpr;
                     }
                     else
                     {
-                        auto type = DeclRefType::Create(getSession(), breadcrumb->declRef);
-                        bb = createImplicitThisMemberExpr(
-                            type,
-                            loc,
-                            breadcrumb->thisParameterMode);
+                        // In a context where both static and instance members can
+                        // be referenced, we will construct a reference to `this`,
+                        // and then rely on downstream logic to ensure that a
+                        // refernece to `this.someStaticMember` will be translated
+                        // over to `This.someStaticMember`.
+                        //
+                        RefPtr<ThisExpr> expr = new ThisExpr();
+                        expr->type.type = thisType;
+                        expr->loc = loc;
+
+                        // Whether or not the implicit `this` is mutable depends
+                        // on the context in which it is used, and the lookup
+                        // logic will have computed an appropriate "mode" based
+                        // on the context during lookup.
+                        //
+                        expr->type.IsLeftValue = thisParameterMode == LookupResultItem::Breadcrumb::ThisParameterMode::MutableValue;
+
+                        bb = expr;
                     }
                 }
                 break;
@@ -1501,29 +1547,16 @@ namespace Slang
             // For now let's just be expedient and disallow all of that, because
             // we can always add it back in later.
 
-            if (!lookupResult.isOverloaded())
+            // If the lookup result is overloaded, then we want to filter
+            // it to just those candidates that can be referenced statically,
+            // and ignore any that would only be allowed as instance members.
+            //
+            if(lookupResult.isOverloaded())
             {
-                // The non-overloaded case is relatively easy. We just want
-                // to look at the member being referenced, and check if
-                // it is allowed in a `static` context:
+                // We track both the usable items, and whether or
+                // not there were any non-static items that need
+                // to be ignored.
                 //
-                if (!isUsableAsStaticMember(lookupResult.item))
-                {
-                    getSink()->diagnose(
-                        expr->loc,
-                        Diagnostics::staticRefToNonStaticMember,
-                        type,
-                        expr->name);
-                }
-            }
-            else
-            {
-                // The overloaded case is trickier, because we should first
-                // filter the list of candidates, because if there is anything
-                // that *is* usable in a static context, then we should assume
-                // the user just wants to reference that. We should only
-                // issue an error if *all* of the items that were discovered
-                // are non-static.
                 bool anyNonStatic = false;
                 List<LookupResultItem> staticItems;
                 for (auto item : lookupResult.items)
@@ -1558,6 +1591,7 @@ namespace Slang
                             Diagnostics::staticRefToNonStaticMember,
                             type,
                             expr->name);
+                        return CreateErrorExpr(expr);
                     }
                 }
                 // If there were no non-static items, then the `items`
diff --git a/source/slang/slang-check-impl.h b/source/slang/slang-check-impl.h
index df0080ec1..15bad1ec5 100644
--- a/source/slang/slang-check-impl.h
+++ b/source/slang/slang-check-impl.h
@@ -345,11 +345,6 @@ namespace Slang
             RefPtr<Expr>    base,
             SourceLoc       loc);
 
-        RefPtr<Expr> createImplicitThisMemberExpr(
-            Type*                                           type,
-            SourceLoc                                       loc,
-            LookupResultItem::Breadcrumb::ThisParameterMode thisParameterMode);
-
         RefPtr<Expr> ConstructLookupResultExpr(
             LookupResultItem const& item,
             RefPtr<Expr>            baseExpr,
diff --git a/source/slang/slang-check-overload.cpp b/source/slang/slang-check-overload.cpp
index e64fef4aa..e21258f09 100644
--- a/source/slang/slang-check-overload.cpp
+++ b/source/slang/slang-check-overload.cpp
@@ -445,9 +445,9 @@ namespace Slang
     }
 
     RefPtr<Expr> SemanticsVisitor::createGenericDeclRef(
-        RefPtr<Expr>            baseExpr,
-        RefPtr<Expr>            originalExpr,
-        RefPtr<GenericSubstitution>   subst)
+        RefPtr<Expr>                baseExpr,
+        RefPtr<Expr>                originalExpr,
+        RefPtr<GenericSubstitution> subst)
     {
         auto baseDeclRefExpr = as<DeclRefExpr>(baseExpr);
         if (!baseDeclRefExpr)
diff --git a/source/slang/slang-lookup.cpp b/source/slang/slang-lookup.cpp
index 5f2927ccb..7fefa1241 100644
--- a/source/slang/slang-lookup.cpp
+++ b/source/slang/slang-lookup.cpp
@@ -670,10 +670,10 @@ static void _lookUpInScopes(
             if (auto aggTypeDeclBaseRef = containerDeclRef.as<AggTypeDeclBase>())
             {
                 // When reconstructing the final expression for a result
-                // looked up through the tyep or extension, we will need
+                // looked up through the type or extension, we will need
                 // a `this` expression (or a `This` type expression) to
                 // mark the base of the member reference, so we create
-                // a "breadcrumb" here to trakc that fact.
+                // a "breadcrumb" here to track that fact.
                 //
                 BreadcrumbInfo breadcrumb;
                 breadcrumb.kind = LookupResultItem::Breadcrumb::Kind::This;
@@ -714,25 +714,57 @@ static void _lookUpInScopes(
 
             // Before we proceed up to the next outer scope to perform lookup
             // again, we need to consider what the current scope tells us
-            // about how to interpret uses of `this`. For example, if
-            // we are inside a `[mutating]` method, then the implicit `this`
-            // that we use for lookup should be an l-value.
+            // about how to interpret uses of implicit `this` or `This`. For
+            // example, if we are inside a `[mutating]` method, then the implicit
+            // `this` that we use for lookup should be an l-value.
+            //
+            // Similarly, if we look up a member in a type from the scope
+            // of some nested type, then there shouldn't be an implicit `this`
+            // expression for the outer type, but instead an implicit `This`.
             //
             if( containerDeclRef.is<ConstructorDecl>() )
             {
-                thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::Mutating;
+                // In the context of an `__init` declaration, the members of
+                // the surrounding type are accessible through a mutable `this`.
+                //
+                thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::MutableValue;
             }
             else if( auto funcDeclRef = containerDeclRef.as<FunctionDeclBase>() )
             {
-                if( funcDeclRef.getDecl()->HasModifier<MutatingAttribute>() )
+                // The implicit `this`/`This` for a function-like declaration
+                // depends on modifiers attached to the declaration.
+                //
+                if( funcDeclRef.getDecl()->HasModifier<HLSLStaticModifier>() )
                 {
-                    thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::Mutating;
+                    // A `static` method only has access to an implicit `This`,
+                    // and does not have a `this` expression available.
+                    //
+                    thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::Type;
+                }
+                else if( funcDeclRef.getDecl()->HasModifier<MutatingAttribute>() )
+                {
+                    // In a non-`static` method marked `[mutating]` there is
+                    // an implicit `this` parameter that is mutable.
+                    //
+                    thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::MutableValue;
                 }
                 else
                 {
-                    thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::Default;
+                    // In all other cases, there is an implicit `this` parameter
+                    // that is immutable.
+                    //
+                    thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::ImmutableValue;
                 }
             }
+            else if( containerDeclRef.as<AggTypeDeclBase>() )
+            {
+                // When lookup moves from a nested typed declaration to an
+                // outer scope, there is no ability to use an implicit `this`
+                // expression, and we have only the `This` type available.
+                //
+                thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::Type;
+            }
+            // TODO: What other cases need to be enumerated here?
         }
 
         if (result.isValid())
diff --git a/source/slang/slang-syntax.h b/source/slang/slang-syntax.h
index db9b7284b..ff5da913f 100644
--- a/source/slang/slang-syntax.h
+++ b/source/slang/slang-syntax.h
@@ -1112,16 +1112,19 @@ namespace Slang
             // The kind of lookup step that was performed
             Kind kind;
 
-            // For the `Kind::This` case, is the `this` parameter
-            // mutable or not?
+            // For the `Kind::This` case, what does the implicit
+            // `this` or `This` parameter refer to?
+            //
             enum class ThisParameterMode : uint8_t
             {
-                Default,
-                Mutating,
+                ImmutableValue, // An immutable `this` value
+                MutableValue,   // A mutable `this` value
+                Type,           // A `This` type
+
+                Default = ImmutableValue,
             };
             ThisParameterMode thisParameterMode = ThisParameterMode::Default;
 
-
             // As needed, a reference to the declaration that faciliated
             // the lookup step.
             //
diff --git a/tests/diagnostics/call-instance-from-static.slang b/tests/diagnostics/call-instance-from-static.slang
new file mode 100644
index 000000000..3cb8af502
--- /dev/null
+++ b/tests/diagnostics/call-instance-from-static.slang
@@ -0,0 +1,21 @@
+// call-instance-from-static.slang
+
+// Test that calling an instance method from a static method is an error.
+
+//DIAGNOSTIC_TEST:SIMPLE:-target hlsl -entry main -stage compute
+
+struct Test
+{
+	void instanceMethod()
+	{}
+
+	static void staticMethod()
+	{
+		instanceMethod();
+	}
+}
+
+void main()
+{
+	Test.staticMethod();
+}
\ No newline at end of file
diff --git a/tests/diagnostics/call-instance-from-static.slang.expected b/tests/diagnostics/call-instance-from-static.slang.expected
new file mode 100644
index 000000000..6be3f7cd5
--- /dev/null
+++ b/tests/diagnostics/call-instance-from-static.slang.expected
@@ -0,0 +1,6 @@
+result code = -1
+standard error = {
+tests/diagnostics/call-instance-from-static.slang(14): error 30100: type 'Test' cannot be used to refer to non-static member 'instanceMethod'
+}
+standard output = {
+}