From 87940a649e3b4f757905015de95225d57ec2f27c Mon Sep 17 00:00:00 2001
From: Tim Foley <tfoleyNV@users.noreply.github.com>
Date: Fri, 24 Jul 2020 18:12:41 -0700
Subject: Fix bugs related to mutating implementations of interface methods
 (#1461)

There are two main bug fixes here:

* We were failing to diagnose when code calls a `[mutating]` method on a value that doesn't support mutation (that is an r-value instead of an l-value).

* We had a bug in the synthesis logic for interface requirements where we used the *result* type of the requirement in place of each of the *parameter* types.

The second bug made synthesis often produce incorrect signatures with `void` parameters.

The first bug meant that even though a `[mutating]` method should not be able to satisfy a non-`[mutating]` method (and we had code to enforce this for the "exact match" case), when we go on to try and synthesize a non-`[mutating]` method that satisfies the requirement by delegating to the user-written one, it would end up succeeding, because nothing was stopping a non-`[mutating]` method from calling a `[mutating]` one.

In each case this code adds a fix and a test case to confirm it.
---
 source/slang/slang-check-decl.cpp     |  9 +++++--
 source/slang/slang-check-overload.cpp | 49 ++++++++++++++++++++++++++++++++---
 source/slang/slang-diagnostic-defs.h  |  1 +
 3 files changed, 54 insertions(+), 5 deletions(-)

(limited to 'source')

diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp
index 2879d7fa2..3cc0fd0f5 100644
--- a/source/slang/slang-check-decl.cpp
+++ b/source/slang/slang-check-decl.cpp
@@ -1576,7 +1576,7 @@ namespace Slang
             //
             auto synParamDecl = m_astBuilder->create<ParamDecl>();
             synParamDecl->nameAndLoc = paramDeclRef.getDecl()->nameAndLoc;
-            synParamDecl->type.type = resultType;
+            synParamDecl->type.type = paramType;
 
             // We need to add the parameter as a child declaration of
             // the method we are building.
@@ -1599,7 +1599,12 @@ namespace Slang
         // if any.
         //
         ThisExpr* synThis = nullptr;
-        if( !requiredMemberDeclRef.getDecl()->hasModifier<HLSLStaticModifier>() )
+        if( requiredMemberDeclRef.getDecl()->hasModifier<HLSLStaticModifier>() )
+        {
+            auto synStaticModifier = m_astBuilder->create<HLSLStaticModifier>();
+            synFuncDecl->modifiers.first = synStaticModifier;
+        }
+        else
         {
             // For a non-`static` requirement, we need a `this` parameter.
             //
diff --git a/source/slang/slang-check-overload.cpp b/source/slang/slang-check-overload.cpp
index 69baf9f75..0a1294965 100644
--- a/source/slang/slang-check-overload.cpp
+++ b/source/slang/slang-check-overload.cpp
@@ -358,11 +358,54 @@ namespace Slang
         return true;
     }
 
+    bool isEffectivelyMutating(CallableDecl* decl)
+    {
+        if(decl->hasModifier<MutatingAttribute>())
+            return true;
+
+        if(decl->hasModifier<NonmutatingAttribute>())
+            return false;
+
+        if(as<SetterDecl>(decl))
+            return true;
+
+        return false;
+    }
+
     bool SemanticsVisitor::TryCheckOverloadCandidateDirections(
-        OverloadResolveContext&		/*context*/,
-        OverloadCandidate const&	/*candidate*/)
+        OverloadResolveContext&     context,
+        OverloadCandidate const&    candidate)
     {
-        // TODO(tfoley): check `in` and `out` markers, as needed.
+        if(candidate.flavor != OverloadCandidate::Flavor::Func)
+            return true;
+
+        auto funcDeclRef = candidate.item.declRef.as<CallableDecl>();
+        SLANG_ASSERT(funcDeclRef);
+
+        // Note: This operation was originally introduced as
+        // a place to add checking around l-value-ness of arguments
+        // and parameters, but currently that checking is being
+        // done in other places.
+        //
+        // For now we will only use this step to check the
+        // mutability of the `this` parameter where necessary.
+        //
+        if(!isEffectivelyStatic(funcDeclRef.getDecl()))
+        {
+            if(isEffectivelyMutating(funcDeclRef.getDecl()))
+            {
+                if(context.baseExpr && !context.baseExpr->type.isLeftValue)
+                {
+                    if(context.mode == OverloadResolveContext::Mode::ForReal)
+                    {
+                        getSink()->diagnose(context.loc, Diagnostics::mutatingMethodOnImmutableValue, funcDeclRef.getName());
+                        maybeDiagnoseThisNotLValue(context.baseExpr);
+                    }
+                    return false;
+                }
+            }
+        }
+
         return true;
     }
 
diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
index 4ad449b54..4ef2aa7be 100644
--- a/source/slang/slang-diagnostic-defs.h
+++ b/source/slang/slang-diagnostic-defs.h
@@ -248,6 +248,7 @@ DIAGNOSTIC(30041, Error, bitOperationNonIntegral, "bit operation: operand must b
 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 an immutable parameter by default in Slang; apply the `[mutating]` attribute to the function declaration to opt in to a mutable `this`")
+DIAGNOSTIC(30050, Error,  mutatingMethodOnImmutableValue, "mutating method '$0' cannot be called on an immutable value")
 
 DIAGNOSTIC(30051, Error, invalidValueForArgument, "invalid value for argument '$0'")
 DIAGNOSTIC(30052, Error, invalidSwizzleExpr, "invalid swizzle pattern '$0' on type '$1'")
-- 
cgit v1.2.3