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.

1 files changed, 79 insertions, 48 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;
                 }