Add basic support for [mutating] methods (#667)

By default, when writing a "method" (aka "member function") in Slang, the `this` parameter is implicitly an `in` parameter. So this:

```hlsl
struct Foo
{
    int state;
    int getState() { return state; }
    void setState(int s) { state = s; }
};
```

is desugared into something like this:

```hlsl
struct Foo { int state };
int Foo_getState(Foo this) { return this.state; }

// BAD:
void Foo_setState(Foo this, int s) { this.state = s; }
```

That "setter" doesn't really do what was intended. It modifies a local copy of type `Foo`, because `in` parameters in HLSL represent by-value copy-in semantics, and are mutable in the body the function. Slang was updated to give a static error on the original code to catch this kind of mistake (so that `this` parameters are unlike ordinary function parameters, and no longer mutable).

Of course, sometimes users *want* a mutable `this` parameter. Rather than make a mutable `this` the default (there are arguments both for and against this), this change adds a new attribute `[mutating]` that can be put on a method (member function) to indicate that its `this` parameter should be an `in out` parameter:

```hlsl
[mutating] void setState(int s) { state = s; }
```

The above will translate to, more or less:

```hlsl
void Foo_setState(inout Foo this, int s) { this.state = s; }
```

One added detail is that `[mutating]` can also be used on interface requirements, with the same semantics. A `[mutating]` requirement can be satisfied with a `[mutating]` or non-`[mutating]` method, while a non-`[mutating]` requirement can't be satisfied with a `[mutating]` method (the call sites would not expect mutation to happen).

The design of `[mutating]` here is heavily influenced by the equivalent `mutating` keyword in Swift.

Notes on the implementation:

* Adding the new attribute was straightforward using the existing support, but I had to change around where attributes get checked in the overall sequencing of static checks, because attributes were being checked *after* function bodies, but with this change I need to look at semantically-checked attributes to determine the mutability of `this`

* The check to restrict it so that `[mutating]` methods cannot satisfy non-`[mutating]` requirements was easy to add, but it points out the fact that there is a huge TODO comment where the actual checking of method *signatures* is supposed to happen. That is a bug waiting to bite users and needs to be fixed!

* While we had special-case logic to detect attempts to modify state accessed through an immutable `this` (e.g., `this.state = s`), that logic didn't trigger when the mutation happened through a function/operator call (e.g., `this.state += s`), so this change factors out the validation logic for that case and calls through to it from both the assignment and `out` argument cases.

* The error message for the special-case check was updated to note that the user could apply `[mutating]` to their function declaration to get rid of the error.

* The semantic checking logic for an explicit `this` expression was already walking up through the scopes (created during parsing) and looking for a scope that represents an outer type declaration that `this` might be referring to. We simply extend it to note when it passes through the scope for a function or similar declaration (`FunctionDeclBase`) and check for the `[mutating]` attribute. If the attribute is seen, it returns a mutable `this` expression, and otherwise leaves it immutable.

* The IR lowering logic then needed to be updated so that when adding an IR-level parameter to represent `this`, it gives it the appropriate "direction" based on the attributes of the function declaration being lowered. The rest of the IR logic works as-is, because it will treat `this` just like an other parameter (whether it is `in` or `inout`).

* This biggest chunk of work was the "implicit `this`" case, because ordinary name lookup may resolve an expression like `state` into `this.state`, so that the `this` expression comes out of "thin air." To handle this case, I extended the structure of the "breadcrumbs" that come along with a lookup result (the breadcrumbs are used for any case where a single identifier like `state` needs to be embellished to a more complex expression as a result of lookup), so that it can identify whether a `Breadcrumb::Kind::This` node comes from a `[mutating]` context or not. Similar to the logic for an explicit `this`, we handle this by noting when we pass through a `FunctionDeclBase` when moving up through scopes, and look for the `[mutating]` attribute on it. The rest of the work was just plumbing the additional state through.

8 files changed, 145 insertions, 64 deletions

diff --git a/source/slang/check.cpp b/source/slang/check.cpp
index f0e7be32b..985c5c0b7 100644
--- a/source/slang/check.cpp
+++ b/source/slang/check.cpp
@@ -475,11 +475,13 @@ namespace Slang
         }
 
         RefPtr<Expr> createImplicitThisMemberExpr(
-            Type*       type,
-            SourceLoc   loc)
+            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;
         }
@@ -528,14 +530,16 @@ namespace Slang
                         {
                             bb = createImplicitThisMemberExpr(
                                 GetTargetType(extensionDeclRef),
-                                loc);
+                                loc,
+                                breadcrumb->thisParameterMode);
                         }
                         else
                         {
                             auto type = DeclRefType::Create(getSession(), breadcrumb->declRef);
                             bb = createImplicitThisMemberExpr(
                                 type,
-                                loc);
+                                loc,
+                                breadcrumb->thisParameterMode);
                         }
                     }
                     break;
@@ -779,6 +783,10 @@ namespace Slang
                 decl->SetCheckState(DeclCheckState::CheckingHeader);
             }
 
+            // Check the modifiers on the declaration first, in case
+            // semantics of the body itself will depend on them.
+            checkModifiers(decl);
+
             // Use visitor pattern to dispatch to correct case
             dispatchDecl(decl);
 
@@ -2197,12 +2205,6 @@ namespace Slang
                     EnusreAllDeclsRec(d);
                 }
 
-                // Do any semantic checking required on modifiers?
-                for (auto d : programNode->Members)
-                {
-                    checkModifiers(d.Ptr());
-                }
-
                 if (pass == 0)
                 {
                     // now we can check all interface conformances
@@ -2236,6 +2238,14 @@ namespace Slang
             DeclRef<CallableDecl>   requiredMemberDeclRef,
             RefPtr<WitnessTable>    witnessTable)
         {
+            if(satisfyingMemberDeclRef.getDecl()->HasModifier<MutatingAttribute>()
+                && !requiredMemberDeclRef.getDecl()->HasModifier<MutatingAttribute>())
+            {
+                // A `[mutating]` method can't satisfy a non-`[mutating]` requirement,
+                // but vice-versa is okay.
+                return false;
+            }
+
             // TODO: actually implement matching here. For now we'll
             // just pretend that things are satisfied in order to make progress..
             witnessTable->requirementDictionary.Add(
@@ -4599,6 +4609,44 @@ namespace Slang
 
         //
 
+            /// Given an immutable `expr` used as an l-value emit a special diagnostic if it was derived from `this`.
+        void maybeDiagnoseThisNotLValue(Expr* expr)
+        {
+            // We will try to handle expressions of the form:
+            //
+            //      e ::= "this"
+            //          | e . name
+            //          | e [ expr ]
+            //
+            // We will unwrap the `e.name` and `e[expr]` cases in a loop.
+            RefPtr<Expr> e = expr;
+            for(;;)
+            {
+                if(auto memberExpr = e.As<MemberExpr>())
+                {
+                    e = memberExpr->BaseExpression;
+                }
+                else if(auto subscriptExpr = e.As<IndexExpr>())
+                {
+                    e = subscriptExpr->BaseExpression;
+                }
+                else
+                {
+                    break;
+                }
+            }
+            //
+            // Now we check to see if we have a `this` expression,
+            // and if it is immutable.
+            if(auto thisExpr = e.As<ThisExpr>())
+            {
+                if(!thisExpr->type.IsLeftValue)
+                {
+                    getSink()->diagnose(thisExpr, Diagnostics::thisIsImmutableByDefault);
+                }
+            }
+        }
+
         RefPtr<Expr> visitAssignExpr(AssignExpr* expr)
         {
             expr->left = CheckExpr(expr->left);
@@ -4622,40 +4670,7 @@ namespace Slang
                     // is immutable. We can give the user a bit more context into
                     // what is going on.
                     //
-                    // We will try to handle expressions of the form:
-                    //
-                    //      e ::= "this"
-                    //          | e . name
-                    //          | e [ expr ]
-                    //
-                    // We will unwrap the `e.name` and `e[expr]` cases in a loop.
-                    RefPtr<Expr> e = expr->left;
-                    for(;;)
-                    {
-                        if(auto memberExpr = e.As<MemberExpr>())
-                        {
-                            e = memberExpr->BaseExpression;
-                        }
-                        else if(auto subscriptExpr = e.As<IndexExpr>())
-                        {
-                            e = subscriptExpr->BaseExpression;
-                        }
-                        else
-                        {
-                            break;
-                        }
-                    }
-                    //
-                    // Now we check to see if we have a `this` expression,
-                    // and if it is immutable.
-                    if(auto thisExpr = e.As<ThisExpr>())
-                    {
-                        if(!thisExpr->type.IsLeftValue)
-                        {
-                            getSink()->diagnose(thisExpr, Diagnostics::thisIsImmutableByDefault);
-                        }
-                    }
-
+                    maybeDiagnoseThisNotLValue(expr->left);
                 }
             }
             expr->type = type;
@@ -6298,7 +6313,10 @@ namespace Slang
 
             LookupResultItem ctorItem;
             ctorItem.declRef = ctorDeclRef;
-            ctorItem.breadcrumbs = new LookupResultItem::Breadcrumb(LookupResultItem::Breadcrumb::Kind::Member, typeItem.declRef, typeItem.breadcrumbs);
+            ctorItem.breadcrumbs = new LookupResultItem::Breadcrumb(
+                LookupResultItem::Breadcrumb::Kind::Member,
+                typeItem.declRef,
+                typeItem.breadcrumbs);
 
             OverloadCandidate candidate;
             candidate.flavor = OverloadCandidate::Flavor::Func;
@@ -7585,6 +7603,8 @@ namespace Slang
                                             implicitCastExpr->Arguments[0]->type,
                                             implicitCastExpr->type);
                                     }
+
+                                    maybeDiagnoseThisNotLValue(argExpr);
                                 }
                             }
                             else
@@ -8132,6 +8152,9 @@ namespace Slang
         // expression.
         RefPtr<Expr> visitThisExpr(ThisExpr* expr)
         {
+            // A `this` expression will default to immutable.
+            expr->type.IsLeftValue = false;
+
             // We will do an upwards search starting in the current
             // scope, looking for a surrounding type (or `extension`)
             // declaration that could be the referrant of the expression.
@@ -8139,14 +8162,22 @@ namespace Slang
             while (scope)
             {
                 auto containerDecl = scope->containerDecl;
-                if (auto aggTypeDecl = containerDecl->As<AggTypeDecl>())
+
+                if( auto funcDeclBase = containerDecl->As<FunctionDeclBase>() )
+                {
+                    if( funcDeclBase->HasModifier<MutatingAttribute>() )
+                    {
+                        expr->type.IsLeftValue = true;
+                    }
+                }
+                else if (auto aggTypeDecl = containerDecl->As<AggTypeDecl>())
                 {
                     checkDecl(aggTypeDecl);
 
                     // Okay, we are using `this` in the context of an
                     // aggregate type, so the expression should be
                     // of the corresponding type.
-                    expr->type = DeclRefType::Create(
+                    expr->type.type = DeclRefType::Create(
                         getSession(),
                         makeDeclRef(aggTypeDecl));
                     return expr;
@@ -8165,7 +8196,7 @@ namespace Slang
                     // if there are multiple extensions in scope that add
                     // members with the same name...
                     //
-                    expr->type = QualType(extensionDecl->targetType.type);
+                    expr->type.type = extensionDecl->targetType.type;
                     return expr;
                 }
 
diff --git a/source/slang/core.meta.slang b/source/slang/core.meta.slang
index 8626a832a..b1e6e0fd6 100644
--- a/source/slang/core.meta.slang
+++ b/source/slang/core.meta.slang
@@ -1235,3 +1235,6 @@ attribute_syntax [__vulkanRayPayload] : VulkanRayPayloadAttribute;
 
 __attributeTarget(VarDeclBase)
 attribute_syntax [__vulkanHitAttributes] : VulkanHitAttributesAttribute;
+
+__attributeTarget(FunctionDeclBase)
+attribute_syntax [mutating] : MutatingAttribute;
diff --git a/source/slang/core.meta.slang.h b/source/slang/core.meta.slang.h
index 854d114bf..f0d61fa12 100644
--- a/source/slang/core.meta.slang.h
+++ b/source/slang/core.meta.slang.h
@@ -1253,3 +1253,6 @@ SLANG_RAW("attribute_syntax [__vulkanRayPayload] : VulkanRayPayloadAttribute;\n"
 SLANG_RAW("\n")
 SLANG_RAW("__attributeTarget(VarDeclBase)\n")
 SLANG_RAW("attribute_syntax [__vulkanHitAttributes] : VulkanHitAttributesAttribute;\n")
+SLANG_RAW("\n")
+SLANG_RAW("__attributeTarget(FunctionDeclBase)\n")
+SLANG_RAW("attribute_syntax [mutating] : MutatingAttribute;\n")
diff --git a/source/slang/diagnostic-defs.h b/source/slang/diagnostic-defs.h
index 8a9773ba9..1c27dde0e 100644
--- a/source/slang/diagnostic-defs.h
+++ b/source/slang/diagnostic-defs.h
@@ -204,7 +204,7 @@ DIAGNOSTIC(30035, Error, componentOverloadTypeMismatch, "'$0': type of overloade
 DIAGNOSTIC(30041, Error, bitOperationNonIntegral, "bit operation: operand must be integral type.")
 DIAGNOSTIC(30047, Error, argumentExpectedLValue, "argument passed to parameter '$0' must be l-value.")
 DIAGNOSTIC(30048, Note,  implicitCastUsedAsLValue, "argument was implicitly cast from '$0' to '$1', and Slang does not support using an implicit cast as an l-value")
-DIAGNOSTIC(30049, Note,  thisIsImmutableByDefault, "a 'this' parameter is currently immutable in Slang")
+DIAGNOSTIC(30049, Note,  thisIsImmutableByDefault, "a 'this' parameter is an immutable parameter by default in Slang; apply the `[mutating]` attribute to the function declaration to opt in to a mutable `this`")
 
 DIAGNOSTIC(30051, Error, invalidValueForArgument, "invalid value for argument '$0'")
 DIAGNOSTIC(30052, Error, invalidSwizzleExpr, "invalid swizzle pattern '$0' on type '$1'")
diff --git a/source/slang/lookup.cpp b/source/slang/lookup.cpp
index 2735bc6ba..e4ae3c8bb 100644
--- a/source/slang/lookup.cpp
+++ b/source/slang/lookup.cpp
@@ -20,6 +20,7 @@ DeclRef<ExtensionDecl> ApplyExtensionToType(
 struct BreadcrumbInfo
 {
     LookupResultItem::Breadcrumb::Kind kind;
+    LookupResultItem::Breadcrumb::ThisParameterMode thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::Default;
     DeclRef<Decl> declRef;
     BreadcrumbInfo* prev = nullptr;
 };
@@ -161,7 +162,8 @@ LookupResultItem CreateLookupResultItem(
         breadcrumbs = new LookupResultItem::Breadcrumb(
             bb->kind,
             bb->declRef,
-            breadcrumbs);
+            breadcrumbs,
+            bb->thisParameterMode);
     }
     item.breadcrumbs = breadcrumbs;
     return item;
@@ -433,6 +435,8 @@ void DoLookupImpl(
     LookupRequest const&    request,
     LookupResult&           result)
 {
+    auto thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::Default;
+
     auto scope      = request.scope;
     auto endScope   = request.endScope;
     for (;scope != endScope; scope = scope->parent)
@@ -464,6 +468,7 @@ void DoLookupImpl(
             if (auto aggTypeDeclRef = containerDeclRef.As<AggTypeDeclBase>())
             {
                 breadcrumb.kind = LookupResultItem::Breadcrumb::Kind::This;
+                breadcrumb.thisParameterMode = thisParameterMode;
                 breadcrumb.declRef = aggTypeDeclRef;
                 breadcrumb.prev = nullptr;
 
@@ -488,6 +493,18 @@ void DoLookupImpl(
             DoLocalLookupImpl(
                 session,
                 name, containerDeclRef, request, result, breadcrumbs);
+
+            if( auto funcDeclRef = containerDeclRef.As<FunctionDeclBase>() )
+            {
+                if( funcDeclRef.getDecl()->HasModifier<MutatingAttribute>() )
+                {
+                    thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::Mutating;
+                }
+                else
+                {
+                    thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::Default;
+                }
+            }
         }
 
         if (result.isValid())
diff --git a/source/slang/lower-to-ir.cpp b/source/slang/lower-to-ir.cpp
index 7e22c4694..86cf63b25 100644
--- a/source/slang/lower-to-ir.cpp
+++ b/source/slang/lower-to-ir.cpp
@@ -4494,19 +4494,10 @@ struct DeclLoweringVisitor : DeclVisitor<DeclLoweringVisitor, LoweredValInfo>
     // parameter for the enclosing type:
     //
     void addThisParameter(
+        ParameterDirection  direction,
         Type*               type,
         ParameterLists*     ioParameterLists)
     {
-        // For now we make any `this` parameter default to `in`. Eventually
-        // we should add a way for the user to opt in to mutability of `this`
-        // in cases where they really want it.
-        //
-        // Note: an alternative here might be to have the built-in types like
-        // `Texture2D` actually use `class` declarations and say that the
-        // `this` parameter for a class type is always `in`, while `struct`
-        // types can default to `in out`.
-        ParameterDirection direction = kParameterDirection_In;
-
         ParameterInfo info;
         info.type = type;
         info.decl = nullptr;
@@ -4516,6 +4507,7 @@ struct DeclLoweringVisitor : DeclVisitor<DeclLoweringVisitor, LoweredValInfo>
         ioParameterLists->params.Add(info);
     }
     void addThisParameter(
+        ParameterDirection  direction,
         AggTypeDecl*        typeDecl,
         ParameterLists*     ioParameterLists)
     {
@@ -4526,6 +4518,7 @@ struct DeclLoweringVisitor : DeclVisitor<DeclLoweringVisitor, LoweredValInfo>
         auto declRef = createDefaultSpecializedDeclRef(typeDecl);
         RefPtr<Type> type = DeclRefType::Create(context->getSession(), declRef);
         addThisParameter(
+            direction,
             type,
             ioParameterLists);
     }
@@ -4558,13 +4551,26 @@ struct DeclLoweringVisitor : DeclVisitor<DeclLoweringVisitor, LoweredValInfo>
             // parameter corresponding to the outer declaration.
             if( innerMode != kParameterListCollectMode_Static )
             {
+                // For now we make any `this` parameter default to `in`.
+                //
+                ParameterDirection direction = kParameterDirection_In;
+                //
+                // Applications can opt in to a mutable `this` parameter,
+                // by applying the `[mutating]` attribute to their
+                // declaration.
+                //
+                if( decl->HasModifier<MutatingAttribute>() )
+                {
+                    direction = kParameterDirection_InOut;
+                }
+
                 if( auto aggTypeDecl = dynamic_cast<AggTypeDecl*>(parentDecl) )
                 {
-                    addThisParameter(aggTypeDecl, ioParameterLists);
+                    addThisParameter(direction, aggTypeDecl, ioParameterLists);
                 }
                 else if( auto extensionDecl = dynamic_cast<ExtensionDecl*>(parentDecl) )
                 {
-                    addThisParameter(extensionDecl->targetType, ioParameterLists);
+                    addThisParameter(direction, extensionDecl->targetType, ioParameterLists);
                 }
             }
         }
diff --git a/source/slang/modifier-defs.h b/source/slang/modifier-defs.h
index 35bd38e2e..dc4e99cdd 100644
--- a/source/slang/modifier-defs.h
+++ b/source/slang/modifier-defs.h
@@ -399,6 +399,12 @@ SIMPLE_SYNTAX_CLASS(VulkanRayPayloadAttribute, Attribute)
 // intersection shader to pass hit attribute information.
 SIMPLE_SYNTAX_CLASS(VulkanHitAttributesAttribute, Attribute)
 
+// A `[mutating]` attribute, which indicates that a member
+// function is allowed to modify things through its `this`
+// argument.
+//
+SIMPLE_SYNTAX_CLASS(MutatingAttribute, Attribute)
+
 // HLSL modifiers for geometry shader input topology
 SIMPLE_SYNTAX_CLASS(HLSLGeometryShaderInputPrimitiveTypeModifier, Modifier)
 SIMPLE_SYNTAX_CLASS(HLSLPointModifier        , HLSLGeometryShaderInputPrimitiveTypeModifier)
diff --git a/source/slang/syntax.h b/source/slang/syntax.h
index 24660b7fd..019740a5e 100644
--- a/source/slang/syntax.h
+++ b/source/slang/syntax.h
@@ -864,7 +864,7 @@ namespace Slang
         class Breadcrumb : public RefObject
         {
         public:
-            enum class Kind
+            enum class Kind : uint8_t
             {
                 // The lookup process looked "through" an in-scope
                 // declaration to the fields inside of it, so that
@@ -895,6 +895,16 @@ namespace Slang
             // The kind of lookup step that was performed
             Kind kind;
 
+            // For the `Kind::This` case, is the `this` parameter
+            // mutable or not?
+            enum class ThisParameterMode : uint8_t
+            {
+                Default,
+                Mutating,
+            };
+            ThisParameterMode thisParameterMode = ThisParameterMode::Default;
+
+
             // As needed, a reference to the declaration that faciliated
             // the lookup step.
             //
@@ -910,8 +920,13 @@ namespace Slang
             // arrive at a final value.
             RefPtr<Breadcrumb> next;
 
-            Breadcrumb(Kind kind, DeclRef<Decl> declRef, RefPtr<Breadcrumb> next)
+            Breadcrumb(
+                Kind                kind,
+                DeclRef<Decl>       declRef,
+                RefPtr<Breadcrumb>  next,
+                ThisParameterMode   thisParameterMode = ThisParameterMode::Default)
                 : kind(kind)
+                , thisParameterMode(thisParameterMode)
                 , declRef(declRef)
                 , next(next)
             {}