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

Author: csyonghe

external/slang-rhi

@@ -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

source/slang/slang-ast-support-types.cpp

@@ -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
 {

source/slang/slang-ast-support-types.h

@@ -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)

source/slang/slang-capability.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,

source/slang/slang-check-decl.cpp

@@ -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 @@ struct SemanticsVisitor : public SemanticsContext
         /// declaration)
         ContainerDecl* parentDecl;
 
+        // An inner diagnostic sink to store diagnostics about why requirement synthesis failed.
+        DiagnosticSink innerSink;
+
         Dictionary<DeclRef<InterfaceDecl>, RefPtr<WitnessTable>> mapInterfaceToWitnessTable;
     };
 

source/slang/slang-check-impl.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")

source/slang/slang-diagnostic-defs.h

@@ -1,7 +1,6 @@
 // mutating-impl-of-non-mutating-req.slang
 
-//DIAGNOSTIC_TEST:SIMPLE:-target hlsl -entry main
-
+//DIAGNOSTIC_TEST:SIMPLE(filecheck=CHECK):-target hlsl -entry main
 interface IThing
 {
     int processValue(int inValue);
@@ -10,7 +9,8 @@ interface IThing
 struct Counter : IThing
 {
     int state;
-
+    
+    // CHECK: ([[# @LINE+1]]): error 38105:
     [mutating] int processValue(int inValue)
     {
         int result = state;

tests/diagnostics/interfaces/mutating-impl-of-non-mutating-req.slang

@@ -0,0 +1,37 @@
+//TEST:SIMPLE(filecheck=CHECK): -target spirv
+
+public interface ITest {
+    public void testIn(int a);
+    public void testOut(out int b);
+};
+
+public struct TestImpl : ITest {
+    // CHECK: ([[# @LINE + 1]]): error 38105
+    public void testIn(out int a) {
+        a = 5;
+    }
+    // CHECK: ([[# @LINE + 1]]): error 38105
+    public void testOut(int b) {
+        b = 6;
+    }
+}
+
+RWStructuredBuffer<int> output;
+
+void doSomething<T>(T data) where T : ITest {
+    int a = 516;
+    data.testIn(a);
+    int b = 687;
+    data.testOut(b);
+
+    output[0] = a;
+    output[1] = b;
+}
+
+[shader("compute")]
+[numthreads(1,1,1)]
+void computeMain()
+{
+    TestImpl data;
+    doSomething(data);
+}

tests/language-feature/interfaces/argument-direction-mismatch.slang

@@ -0,0 +1,29 @@
+//TEST:COMPARE_COMPUTE(filecheck-buffer=CHECK): -output-using-type
+
+public interface ITest {
+    public int testDir(inout int a);
+};
+
+public struct TestImpl : ITest {
+    public int testDir(inout int a) {
+        int oldA = a;
+        a = 5;
+        return a;
+ }
+}
+
+//TEST_INPUT:ubuffer(data=[0 0 0 0], stride=4):out,name=output
+RWStructuredBuffer<int> output;
+
+public struct Test : ITest = TestImpl;
+
+[shader("compute")]
+[numthreads(1,1,1)]
+void computeMain()
+{
+    Test data;
+    int a = 516;
+    int b = data.testDir(a);
+    // CHECK: 5
+    output[0] = b;
+}