Allow [mutating] methods on existential values (#2245)

The problematic case is when an `interface` has a `[mutating]` method:

    interface ICounter
    {
        [mutating] void increment();
    }

and code tries to invoke that method on a value of existential type:

    ICounter c = ...;
    c.increment();

We know that the existential value `c` is conceptually a tuple of:

* A concrete type `X`
* A witness that `X : ICounter`
* A value `v` of type `X`

We simply want to invoke `increment()` on the `v` part, using the `X : ICounter` witness table.
The catch that the compiler faces is that the variable `c` is mutable, so we need to be careful that we "snapshot" its value (the tuple `X, X:ICounter, v`) at a single point.
The snapshotting behavior is important when invoking a method that involves `This` or associated types in its signature, so we cannot get rid of it.

The snapshotting we do relies on the idea of a `LetExpr` AST node, which cannot be written in the input syntax.
A `LetExpr` introduces a variable binding (with an initial-value expression) and then evaluates a body expression in the context of that binding.
For a call site like `c.increment()` the front-end makes an intermediate copy of `c` and then "opens" that immutable value to get at the elements of the tuple `X`, `X : ICounter`, `v`.
The resulting AST after checking looks something like:

    ICounter c = ...;
    (let tmp = c in extractExistentialValue(tmp)).increment();

In that form it is more clear why the attempt to call `increment()` fails:

1. The binding `tmp` sure looks immutable
2. There is no logic in the compiler to make `extractExistentialValue(x)` be an l-value if `x` is
3. There is seemingly no logic to write back from `tmp` to `c` when the operation completes

Let us walk through those problems in order.

Item (1) turns out to be a bit of a non-issue.
Despite the way that I've written out `let` expressions above, the logic in `moveTemp()` in the compiler actually introduces a *mutable* binding.

Item (2) can be fixed for the purposes of semantic checking by modifying `openExistential()`.
Simplistically, we make the overall expression be an l-value if the operand is.

Item (3) is handled at the level of AST->IR lowering. Each kind of expression that can form an l-value needs to have a way to represent the "location" of that l-value in the `LoweredValInfo` type.
This change adds a case to handle the `extractExistentialVal` operation, by tracking both the extract value (of concrete type) and the underlying l-value (of existential type).

Where all of this comes crashing against reality a bit is that the scoping I've drawn for the `let` expressions above kind of doesn't work once we look at types.
The basic problem is that the *type* of the `(let tmp = c in ...)` expression is the concrete type `X` that was extracted from the existential.
That type can conceptually be written as `ExtractExistentialType(tmp)` which, notably, references `tmp`.
That means that we end up with AST expression nodes that reference the variable `tmp` *outside* of its scope.
Furthermore, those references to `tmp` can end up being lowered to IR *before* we have lowered the `let ...` expression itself.

Fixing the scoping issue turns out to be a major undertaking.
The first (and more obvious) issue is needing to address the scoping problem.
The solution I implemented includes a bit of refactoring to make all the `SemanticsVisitor` types better able to pass around the contextual scope-dependent state that might be needed during semantic checking, but really only adds a single piece of state.

The semantic-checking state used for checking expressions is bottlenecked so that there will (or at least *should*) always be an explicit representation of a "scope" that surrounds a complete expression (as opposed to a sub-expression).
When a `LetExpr` needs to be introduced, it is added to a pending list on the active scope, rather than being added locally.
Once the complete expression is checked, the resulting expression is wrapped up in the pending `LetExpr`s so that their scope is as broad as possible.

Technically this solution doesn't cover all cases. For example:

    interface ICell { associatedtype Content; Content getContent(); }
    ...
    ICell cell = ...;
    let content = cell.getContent();

In this case the type of `content` refers to the binding introduced by a `LetExpr` in the initial-value expression.
I am leaving such issues as a piece of future work, in the hopes that we can get at least a partial fix for the problem in place.
A future fix probably nees to extend the scoping even wider (e.g., by unwrapping the `LetExpr`s from the initial-value expression and turning them into distinct temporaries).

The second piece of the fix is that we need a way for the modified value of the extracted existential to be "written back" to the original location.

Well...

We are actually being a little slippery here, based on some logic in the compiler codebase that I guess Just Works.
When AST->IR lowering encounters a `LetExpr` that binds an l-value to a name, it actually ends up binding that name more or less as a *reference* to that l-value.
At this point the `let`-ness of `LetExpr` is very much in doubt: the binding can be mutable, and it can even be an *alias* of some location?!?

In any case, the result is that the AST->IR codegen logic implicitly handles the "write-back" because the `let`-bound temporary is actually an alias for the original location.
A more complete future fix might need to introduce a distinct case in `LoweredValInfo` to handle the case of copy of a mutable temporary.

1 files changed, 34 insertions, 40 deletions

diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp
index 6c414b292..6d579e1fb 100644
--- a/source/slang/slang-check-decl.cpp
+++ b/source/slang/slang-check-decl.cpp
@@ -21,8 +21,8 @@ namespace Slang
         : public SemanticsDeclVisitorBase
         , public DeclVisitor<SemanticsDeclModifiersVisitor>
     {
-        SemanticsDeclModifiersVisitor(SharedSemanticsContext* shared)
-            : SemanticsDeclVisitorBase(shared)
+        SemanticsDeclModifiersVisitor(SemanticsContext const& outer)
+            : SemanticsDeclVisitorBase(outer)
         {}
 
         void visitDeclGroup(DeclGroup*) {}
@@ -37,8 +37,8 @@ namespace Slang
         : public SemanticsDeclVisitorBase
         , public DeclVisitor<SemanticsDeclHeaderVisitor>
     {
-        SemanticsDeclHeaderVisitor(SharedSemanticsContext* shared)
-            : SemanticsDeclVisitorBase(shared)
+        SemanticsDeclHeaderVisitor(SemanticsContext const& outer)
+            : SemanticsDeclVisitorBase(outer)
         {}
 
         void visitDecl(Decl*) {}
@@ -106,8 +106,8 @@ namespace Slang
         : public SemanticsDeclVisitorBase
         , public DeclVisitor<SemanticsDeclRedeclarationVisitor>
     {
-        SemanticsDeclRedeclarationVisitor(SharedSemanticsContext* shared)
-            : SemanticsDeclVisitorBase(shared)
+        SemanticsDeclRedeclarationVisitor(SemanticsContext const& outer)
+            : SemanticsDeclVisitorBase(outer)
         {}
 
         void visitDecl(Decl*) {}
@@ -127,8 +127,8 @@ namespace Slang
         : public SemanticsDeclVisitorBase
         , public DeclVisitor<SemanticsDeclBasesVisitor>
     {
-        SemanticsDeclBasesVisitor(SharedSemanticsContext* shared)
-            : SemanticsDeclVisitorBase(shared)
+        SemanticsDeclBasesVisitor(SemanticsContext const& outer)
+            : SemanticsDeclVisitorBase(outer)
         {}
 
         void visitDecl(Decl*) {}
@@ -161,8 +161,8 @@ namespace Slang
         : public SemanticsDeclVisitorBase
         , public DeclVisitor<SemanticsDeclBodyVisitor>
     {
-        SemanticsDeclBodyVisitor(SharedSemanticsContext* shared)
-            : SemanticsDeclVisitorBase(shared)
+        SemanticsDeclBodyVisitor(SemanticsContext const& outer)
+            : SemanticsDeclVisitorBase(outer)
         {}
 
         void visitDecl(Decl*) {}
@@ -668,7 +668,7 @@ namespace Slang
         /// This call does *not* handle updating the state of `decl`; the
         /// caller takes responsibility for doing so.
         ///
-    static void _dispatchDeclCheckingVisitor(Decl* decl, DeclCheckState state, SharedSemanticsContext* shared);
+    static void _dispatchDeclCheckingVisitor(Decl* decl, DeclCheckState state, SemanticsContext const& shared);
 
     // Make sure a declaration has been checked, so we can refer to it.
     // Note that this may lead to us recursively invoking checking,
@@ -729,7 +729,12 @@ namespace Slang
 
             // We now dispatch an appropriate visitor based on `nextState`.
             //
-            _dispatchDeclCheckingVisitor(decl, nextState, getShared());
+            // Note that we always dispatch the visitor in a "fresh" semantic-checking
+            // context, so that the state at the point where a declaration is *referenced*
+            // cannot affect the state in which the declaration is *checked*.
+            //
+            SemanticsContext subContext(getShared());
+            _dispatchDeclCheckingVisitor(decl, nextState, subContext);
 
             // In the common case, the visitor will have done the necessary
             // checking, but will *not* have updated the `checkState` on
@@ -1177,8 +1182,8 @@ namespace Slang
         : public SemanticsDeclVisitorBase
         , public DeclVisitor<SemanticsDeclConformancesVisitor>
     {
-        SemanticsDeclConformancesVisitor(SharedSemanticsContext* shared)
-            : SemanticsDeclVisitorBase(shared)
+        SemanticsDeclConformancesVisitor(SemanticsContext const& outer)
+            : SemanticsDeclVisitorBase(outer)
         {}
 
         void visitDecl(Decl*) {}
@@ -2088,31 +2093,24 @@ namespace Slang
         // to the user as some kind of overload-resolution failure.
         //
         // In order to protect the user from whatever errors might
-        // occur, we will swap out the current diagnostic sink for
-        // a temporary one.
+        // occur, we will perform the checking in the context of
+        // a temporary diagnostic sink.
         //
-        DiagnosticSink* savedSink = m_shared->m_sink;
-        DiagnosticSink tempSink(savedSink->getSourceManager(), nullptr);
-        m_shared->m_sink = &tempSink;
+        DiagnosticSink tempSink(getSourceManager(), nullptr);
+        SemanticsVisitor subVisitor(withSink(&tempSink));
 
         // With our temporary diagnostic sink soaking up any messages
         // from overload resolution, we can now try to resolve
         // the call to see what happens.
         //
-        auto checkedCall = ResolveInvoke(synCall);
+        auto checkedCall = subVisitor.ResolveInvoke(synCall);
 
         // Of course, it is possible that the call went through fine,
         // but the result isn't of the type we expect/require,
         // so we also need to coerce the result of the call to
         // the expected type.
         //
-        auto coercedCall = coerce(resultType, checkedCall);
-
-        // Once we are done making our semantic checks, we can
-        // restore the original sink, so that subsequent operations
-        // report diagnostics as usual.
-        //
-        m_shared->m_sink = savedSink;
+        auto coercedCall = subVisitor.coerce(resultType, checkedCall);
 
         // If our overload resolution or type coercion failed,
         // then we have not been able to synthesize a witness
@@ -2380,9 +2378,8 @@ namespace Slang
             // `SemanticsVisitor` so that code can push/pop the emission
             // of diagnostics more easily.
             //
-            DiagnosticSink* savedSink = m_shared->m_sink;
-            DiagnosticSink tempSink(savedSink->getSourceManager(), nullptr);
-            m_shared->m_sink = &tempSink;
+            DiagnosticSink tempSink(getSourceManager(), nullptr);
+            SemanticsVisitor subVisitor(withSink(&tempSink));
 
             // We start by constructing an expression that represents
             // `this.name` where `name` is the name of the required
@@ -2415,7 +2412,7 @@ namespace Slang
             // general-purpose language features is unlikely to be as efficient
             // as special-case logic.
             //
-            auto synMemberRef = createLookupResultExpr(
+            auto synMemberRef = subVisitor.createLookupResultExpr(
                 requiredMemberDeclRef.getName(),
                 lookupResult,
                 synThis,
@@ -2434,7 +2431,7 @@ namespace Slang
                 // which involves coercing the member access `this.name` to
                 // the expected type of the property.
                 //
-                auto coercedMemberRef = coerce(propertyType, synMemberRef);
+                auto coercedMemberRef = subVisitor.coerce(propertyType, synMemberRef);
                 auto synReturn = m_astBuilder->create<ReturnStmt>();
                 synReturn->expression = coercedMemberRef;
 
@@ -2461,7 +2458,7 @@ namespace Slang
                 synAssign->left = synMemberRef;
                 synAssign->right = synArgs[0];
 
-                auto synCheckedAssign = checkAssignWithCheckedOperands(synAssign);
+                auto synCheckedAssign = subVisitor.checkAssignWithCheckedOperands(synAssign);
 
                 auto synExprStmt = m_astBuilder->create<ExpressionStmt>();
                 synExprStmt->expression = synCheckedAssign;
@@ -2477,10 +2474,8 @@ namespace Slang
                 return false;
             }
 
-            // We restore the semantic checking state that was in place before
-            // we checked the synthesized accessor body, and then bail out
-            // if we ran into any errors (meaning that the synthesized accessor
-            // is not usable).
+            // We bail out if we ran into any errors (meaning that the synthesized
+            // accessor is not usable).
             //
             // TODO: If there were *warnings* emitted to the sink, it would probably
             // be good to show those warnings to the user, since they might indicate
@@ -2488,7 +2483,6 @@ namespace Slang
             // satisfying an `int` property requirement, but the user would probably
             // want to be warned when they do such a thing.
             //
-            m_shared->m_sink = savedSink;
             if(tempSink.getErrorCount() != 0)
                 return false;
 
@@ -5046,7 +5040,7 @@ namespace Slang
             name,
             decl->moduleNameAndLoc.loc,
             getSink(),
-            m_shared->m_environmentModules);
+            getShared()->m_environmentModules);
 
         // If we didn't find a matching module, then bail out
         if (!importedModule)
@@ -5324,7 +5318,7 @@ namespace Slang
     }
 
 
-    static void _dispatchDeclCheckingVisitor(Decl* decl, DeclCheckState state, SharedSemanticsContext* shared)
+    static void _dispatchDeclCheckingVisitor(Decl* decl, DeclCheckState state, SemanticsContext const& shared)
     {
         switch(state)
         {