Check mismatching method parameter direction against interface declaration. (#5964)

1 files changed, 95 insertions, 6 deletions

diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp
index 3fea267ee..c4d64c668 100644
--- a/source/slang/slang-check-decl.cpp
+++ b/source/slang/slang-check-decl.cpp
@@ -3412,7 +3412,9 @@ bool SemanticsVisitor::doesSignatureMatchRequirement(
     {
         auto requiredParam = requiredParams[paramIndex];
         auto satisfyingParam = satisfyingParams[paramIndex];
-
+        if (getParameterDirection(requiredParam.getDecl()) !=
+            getParameterDirection(satisfyingParam.getDecl()))
+            return false;
         auto requiredParamType = getType(m_astBuilder, requiredParam);
         auto satisfyingParamType = getType(m_astBuilder, satisfyingParam);
 
@@ -4394,14 +4396,14 @@ void SemanticsVisitor::addRequiredParamsToSynthesizedDecl(
     //
     for (auto paramDeclRef : getParameters(m_astBuilder, requirement))
     {
-        auto paramType = getType(m_astBuilder, paramDeclRef);
+        auto paramType = QualType(getType(m_astBuilder, paramDeclRef));
 
         // For each parameter of the requirement, we create a matching
         // parameter (same name and type) for the synthesized method.
         //
         auto synParamDecl = m_astBuilder->create<ParamDecl>();
         synParamDecl->nameAndLoc = paramDeclRef.getDecl()->nameAndLoc;
-        synParamDecl->type.type = paramType;
+        synParamDecl->type.type = paramType.type;
 
         // We need to add the parameter as a child declaration of
         // the method we are building.
@@ -4410,6 +4412,7 @@ void SemanticsVisitor::addRequiredParamsToSynthesizedDecl(
         synthesized->members.add(synParamDecl);
 
         // Add modifiers
+        paramType.isLeftValue = true;
         for (auto modifier : paramDeclRef.getDecl()->modifiers)
         {
             if (as<NoDiffModifier>(modifier))
@@ -4426,6 +4429,8 @@ void SemanticsVisitor::addRequiredParamsToSynthesizedDecl(
                     (Modifier*)m_astBuilder->createByNodeType(modifier->astNodeType);
                 clonedModifier->keywordName = modifier->keywordName;
                 addModifier(synParamDecl, clonedModifier);
+                if (as<ConstRefModifier>(modifier))
+                    paramType.isLeftValue = false;
             }
         }
 
@@ -4445,6 +4450,7 @@ void SemanticsVisitor::addRequiredParamsToSynthesizedDecl(
                 synMemberExpr->base = synArg;
                 synMemberExpr->elementIndices.add((uint32_t)i);
                 synMemberExpr->type = elementType;
+                synMemberExpr->type.isLeftValue = paramType.isLeftValue;
                 synArgs.add(synMemberExpr);
             }
         }
@@ -4572,6 +4578,22 @@ static bool isWrapperTypeDecl(Decl* decl)
     return false;
 }
 
+// Is it allowed to have an interface method parameter whose direction is `reqDir`, and an
+// implementing method parameter whose direction is `implDir`?
+//
+static bool matchParamDirection(ParameterDirection implDir, ParameterDirection reqDir)
+{
+    // If the parameter directions match exactly, then we are good.
+    if (implDir == reqDir)
+        return true;
+    // Otherwise, we only allow the cases where reqDir is `InOut` and implDir is `In` or `Out`.
+    if (implDir == kParameterDirection_In && reqDir == kParameterDirection_InOut)
+        return true;
+    if (implDir == kParameterDirection_Out && reqDir == kParameterDirection_InOut)
+        return true;
+    return false;
+}
+
 bool SemanticsVisitor::trySynthesizeMethodRequirementWitness(
     ConformanceCheckingContext* context,
     LookupResult const& lookupResult,
@@ -4749,7 +4771,13 @@ bool SemanticsVisitor::trySynthesizeMethodRequirementWitness(
         // If checking the generic app failed, we can't synthesize the witness.
         //
         if (tempSink.getErrorCount() != 0)
+        {
+            context->innerSink.diagnose(
+                SourceLoc(),
+                Diagnostics::genericSignatureDoesNotMatchRequirement,
+                baseOverloadedExpr->name);
             return false;
+        }
     }
 
     // We now have the reference to the overload group we plan to call,
@@ -4791,11 +4819,67 @@ bool SemanticsVisitor::trySynthesizeMethodRequirementWitness(
     // diagnose a generic "failed to satisfying requirement" error.
     //
     if (tempSink.getErrorCount() != 0)
+    {
+        context->innerSink.diagnose(
+            SourceLoc(),
+            Diagnostics::cannotResolveOverloadForMethodRequirement,
+            baseOverloadedExpr->name);
         return false;
+    }
 
-    // If we were able to type-check the call, then we should
-    // be able to finish construction of a suitable witness.
+    // If we were able to type-check the call, we also need to make
+    // sure that the resolved callee member has consistent parameter
+    // direction as the requirement method.
+    //
+    // For example, if there is a requirement:
+    // ```
+    // interface IFoo { void method(out int x); }
+    // ```
+    // and a type:
+    // ```
+    // struct X : IFoo { void method(int x) { ... } }
+    // ```
+    // After we synthesize:
+    // ```
+    // void X::synthesized_method(out int x) { this.method(x); }
+    // ```
+    // The synthesized method will pass all type check just fine,
+    // but we don't want to allow this method to be used as a witness
+    // for the requirement due to inconsistent parameter direction.
+    // So let's check for this now.
     //
+    if (auto checkedInvoke = as<InvokeExpr>(checkedCall))
+    {
+        if (auto declRefExpr = as<DeclRefExpr>(checkedInvoke->functionExpr))
+        {
+            if (auto callee = as<CallableDecl>(declRefExpr->declRef))
+            {
+                auto synParams = synFuncDecl->getParameters();
+                auto calleeParams = callee.getDecl()->getParameters();
+                auto synParamIter = synParams.begin();
+                auto calleeParamIter = calleeParams.begin();
+                for (; synParamIter != synParams.end() && calleeParamIter != calleeParams.end();
+                     ++synParamIter, ++calleeParamIter)
+                {
+                    auto synParam = *synParamIter;
+                    auto calleeParam = *calleeParamIter;
+                    if (!matchParamDirection(
+                            getParameterDirection(calleeParam),
+                            getParameterDirection(synParam)))
+                    {
+                        context->innerSink.diagnose(
+                            calleeParam,
+                            Diagnostics::parameterDirectionDoesNotMatchRequirement,
+                            calleeParam,
+                            getParameterDirection(calleeParam),
+                            getParameterDirection(synParam));
+                        return false;
+                    }
+                }
+            }
+        }
+    }
+
     // We've already created the outer declaration (including its
     // parameters), and the inner expression, so the main work
     // that is left is defining the body of the new function,
@@ -5293,7 +5377,7 @@ bool SemanticsVisitor::trySynthesizeWrapperTypePropertyRequirementWitness(
         base->name = getName("inner");
         propertyRef->baseExpression = base;
         innerProperty = innerAccessorDeclRef.getParent();
-        propertyRef->name = getParentDecl(innerAccessorDeclRef.getDecl())->getName();
+        propertyRef->name = requiredMemberDeclRef.getName();
         auto checkedPropertyRefExpr = CheckExpr(propertyRef);
 
         if (as<GetterDecl>(requiredAccessorDeclRef))
@@ -6608,6 +6692,7 @@ bool SemanticsVisitor::findWitnessForInterfaceRequirement(
     // a wrapper type (struct Foo:IFoo=FooImpl), and we will synthesize
     // wrappers that redirects the call into the inner element.
     //
+    context->innerSink.reset();
     if (trySynthesizeRequirementWitness(context, lookupResult, requiredMemberDeclRef, witnessTable))
     {
         return true;
@@ -6638,6 +6723,10 @@ bool SemanticsVisitor::findWitnessForInterfaceRequirement(
             subType,
             requiredMemberDeclRef);
     }
+    if (context->innerSink.outputBuffer.getLength())
+    {
+        getSink()->diagnoseRaw(Severity::Note, context->innerSink.outputBuffer.getUnownedSlice());
+    }
     getSink()->diagnose(
         requiredMemberDeclRef,
         Diagnostics::seeDeclarationOfInterfaceRequirement,