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

6 files changed, 150 insertions, 7 deletions

diff --git a/source/slang/slang-ast-support-types.cpp b/source/slang/slang-ast-support-types.cpp
index a06fa2b88..d59b6b286 100644
--- a/source/slang/slang-ast-support-types.cpp
+++ b/source/slang/slang-ast-support-types.cpp
@@ -70,4 +70,29 @@ UnownedStringSlice getHigherOrderOperatorName(HigherOrderInvokeExpr* expr)
     return UnownedStringSlice();
 }
 
+void printDiagnosticArg(StringBuilder& sb, ParameterDirection direction)
+{
+    switch (direction)
+    {
+    case kParameterDirection_In:
+        sb << "in";
+        break;
+    case kParameterDirection_Out:
+        sb << "out";
+        break;
+    case kParameterDirection_Ref:
+        sb << "ref";
+        break;
+    case kParameterDirection_InOut:
+        sb << "inout";
+        break;
+    case kParameterDirection_ConstRef:
+        sb << "constref";
+        break;
+    default:
+        sb << "(" << int(direction) << ")";
+        break;
+    }
+}
+
 } // namespace Slang
diff --git a/source/slang/slang-ast-support-types.h b/source/slang/slang-ast-support-types.h
index 4bbd51f93..21ca27a90 100644
--- a/source/slang/slang-ast-support-types.h
+++ b/source/slang/slang-ast-support-types.h
@@ -1634,6 +1634,8 @@ enum ParameterDirection
     kParameterDirection_ConstRef, ///< By-const-reference
 };
 
+void printDiagnosticArg(StringBuilder& sb, ParameterDirection direction);
+
 /// The kind of a builtin interface requirement that can be automatically synthesized.
 enum class BuiltinRequirementKind
 {
diff --git a/source/slang/slang-capability.cpp b/source/slang/slang-capability.cpp
index f56e246dd..7c4e34d32 100644
--- a/source/slang/slang-capability.cpp
+++ b/source/slang/slang-capability.cpp
@@ -1165,7 +1165,6 @@ void printDiagnosticArg(StringBuilder& sb, List<CapabilityAtom>& list)
     printDiagnosticArg(sb, set.newSetWithoutImpliedAtoms());
 }
 
-
 #ifdef UNIT_TEST_CAPABILITIES
 
 #define CHECK_CAPS(inData) SLANG_ASSERT(inData > 0)
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,
diff --git a/source/slang/slang-check-impl.h b/source/slang/slang-check-impl.h
index 45e899132..f464f9298 100644
--- a/source/slang/slang-check-impl.h
+++ b/source/slang/slang-check-impl.h
@@ -1143,6 +1143,12 @@ struct OuterScopeContextRAII
         context,                                           \
         decl->ownedScope ? decl->ownedScope : context->getOuterScope())
 
+struct RequirementSynthesisResult
+{
+    bool suceeded = false;
+    operator bool() const { return suceeded; }
+};
+
 struct SemanticsVisitor : public SemanticsContext
 {
     typedef SemanticsContext Super;
@@ -1742,6 +1748,9 @@ public:
         /// declaration)
         ContainerDecl* parentDecl;
 
+        // An inner diagnostic sink to store diagnostics about why requirement synthesis failed.
+        DiagnosticSink innerSink;
+
         Dictionary<DeclRef<InterfaceDecl>, RefPtr<WitnessTable>> mapInterfaceToWitnessTable;
     };
 
diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
index a2975e88f..fde04b09d 100644
--- a/source/slang/slang-diagnostic-defs.h
+++ b/source/slang/slang-diagnostic-defs.h
@@ -70,6 +70,25 @@ DIAGNOSTIC(
     Note,
     seeDeclarationOfInterfaceRequirement,
     "see interface requirement declaration of '$0'")
+
+DIAGNOSTIC(
+    -1,
+    Note,
+    genericSignatureDoesNotMatchRequirement,
+    "generic signature of '$0' does not match interface requirement.")
+
+DIAGNOSTIC(
+    -1,
+    Note,
+    cannotResolveOverloadForMethodRequirement,
+    "none of the overloads of '$0' match the interface requirement.")
+
+DIAGNOSTIC(
+    -1,
+    Note,
+    parameterDirectionDoesNotMatchRequirement,
+    "parameter '$0' is '$1' in the implementing member, but the interface requires '$2'.")
+
 // An alternate wording of the above note, emphasing the position rather than content of the
 // declaration.
 DIAGNOSTIC(-1, Note, declaredHere, "declared here")