From 2d23a962766a97cbb11bcee5483a66aec923da49 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Thu, 24 Jul 2025 00:47:26 -0700 Subject: Fix confusing error messages for interface return type mismatches (#7854) * Initial plan * Add improved diagnostic for interface return type mismatches Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> * Complete fix for interface return type mismatch error reporting Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> * Move diagnostic to synthesis phase for better interface return type mismatch errors Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> * Remove extraneous test file and update .gitignore Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> * Add diagnostic test for interface return type mismatch and apply formatting Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> * Address feedback: restore whitespace and use filecheck for diagnostic test Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> * Fix logic error in return type mismatch detection Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> * Remove unnecessary flag by using out parameter for diagnostic tracking Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> * Refactor witness synthesis failure reporting to use structured approach Replace ad-hoc `outSpecificDiagnosticEmitted` parameter with `WitnessSynthesisFailureReason` enum and `MethodWitnessSynthesisFailureDetails` struct as requested in code review. This provides: - Clear taxonomy of failure reasons (General, MethodResultTypeMismatch, MethodParameterMismatch) - Centralized diagnostic emission in findWitnessForInterfaceRequirement - Better extensibility for future failure types - Improved maintainability by removing state tracking flags The return type mismatch diagnostic continues to work correctly, showing error 38106 with precise location information. Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> * Remove unused MethodParameterMismatch enum and duplicate code Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> * Remove redundant requiredMethod field from MethodWitnessSynthesisFailureDetails Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> * Address feedback: add outFailureDetails guard and remove unnecessary hasReturnTypeError variable Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> * Fix regression: restore original diagnostic message for mutating method mismatch Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> * Fix. * Fix. * Remove `innerSink`. * Print candidates considered for interface match upon error. * Fix tests. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> Co-authored-by: Yong He --- source/slang/slang-check-decl.cpp | 136 ++++++++++++++++++++++++----------- source/slang/slang-check-impl.h | 58 ++++++++++----- source/slang/slang-diagnostic-defs.h | 34 ++++----- 3 files changed, 151 insertions(+), 77 deletions(-) (limited to 'source') diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp index cebe1bb00..4ebad6392 100644 --- a/source/slang/slang-check-decl.cpp +++ b/source/slang/slang-check-decl.cpp @@ -5330,7 +5330,8 @@ bool SemanticsVisitor::trySynthesizeMethodRequirementWitness( ConformanceCheckingContext* context, LookupResult const& lookupResult, DeclRef requiredMemberDeclRef, - RefPtr witnessTable) + RefPtr witnessTable, + MethodWitnessSynthesisFailureDetails* outFailureDetails) { // The situation here is that the context of an inheritance // declaration didn't provide an exact match for a required @@ -5512,10 +5513,8 @@ bool SemanticsVisitor::trySynthesizeMethodRequirementWitness( // if (tempSink.getErrorCount() != 0) { - context->innerSink.diagnose( - SourceLoc(), - Diagnostics::genericSignatureDoesNotMatchRequirement, - baseOverloadedExpr->name); + if (outFailureDetails) + outFailureDetails->reason = WitnessSynthesisFailureReason::GenericSignatureMismatch; return false; } } @@ -5539,31 +5538,42 @@ bool SemanticsVisitor::trySynthesizeMethodRequirementWitness( // so we also need to coerce the result of the call to // the expected type. // - auto coercedCall = subVisitor.coerce(CoercionSite::Return, resultType, checkedCall, getSink()); + auto coercedCall = subVisitor.coerce(CoercionSite::Return, resultType, checkedCall, &tempSink); // If our overload resolution or type coercion failed, // then we have not been able to synthesize a witness // for the requirement. // - // TODO: We might want to detect *why* overload resolution - // or type coercion failed, and report errors accordingly. - // - // More detailed diagnostics could help users understand - // what they did wrong, e.g.: - // - // * "We tried to use `foo(int)` but the interface requires `foo(String)` - // - // * "You have two methods that can apply as `bar()` and we couldn't tell which one you meant - // - // For now we just bail out here and rely on the caller to - // diagnose a generic "failed to satisfying requirement" error. + // Check if this was specifically a return type coercion failure + // and provide a more specific diagnostic. // if (tempSink.getErrorCount() != 0) { - context->innerSink.diagnose( - SourceLoc(), - Diagnostics::cannotResolveOverloadForMethodRequirement, - baseOverloadedExpr->name); + if (outFailureDetails) + outFailureDetails->reason = WitnessSynthesisFailureReason::General; + + // Check if the failure was due to return type coercion + if (!IsErrorExpr(checkedCall) && outFailureDetails) + { + // The call resolved - check if it's a return type mismatch + auto actualReturnType = checkedCall->type; + if (!actualReturnType->equals(resultType)) + { + // Find the actual implementation method that was called + if (auto invokeExpr = as(checkedCall)) + { + if (auto declRefExpr = as(invokeExpr->functionExpr)) + { + // Store failure details instead of emitting diagnostic immediately + outFailureDetails->reason = + WitnessSynthesisFailureReason::MethodResultTypeMismatch; + outFailureDetails->candidateMethod = declRefExpr->declRef; + outFailureDetails->actualType = actualReturnType; + outFailureDetails->expectedType = resultType; + } + } + } + } return false; } @@ -5607,12 +5617,15 @@ bool SemanticsVisitor::trySynthesizeMethodRequirementWitness( getParameterDirection(calleeParam), getParameterDirection(synParam))) { - context->innerSink.diagnose( - calleeParam, - Diagnostics::parameterDirectionDoesNotMatchRequirement, - calleeParam, - getParameterDirection(calleeParam), - getParameterDirection(synParam)); + if (outFailureDetails) + { + outFailureDetails->reason = + WitnessSynthesisFailureReason::ParameterDirMismatch; + outFailureDetails->candidateMethod = declRefExpr->declRef; + outFailureDetails->actualDir = getParameterDirection(calleeParam); + outFailureDetails->expectedDir = getParameterDirection(synParam); + outFailureDetails->paramDecl = calleeParam; + } return false; } } @@ -6683,7 +6696,8 @@ bool SemanticsVisitor::trySynthesizeRequirementWitness( ConformanceCheckingContext* context, LookupResult const& lookupResult, DeclRef requiredMemberDeclRef, - RefPtr witnessTable) + RefPtr witnessTable, + MethodWitnessSynthesisFailureDetails* outFailureDetails) { SLANG_UNUSED(lookupResult); SLANG_UNUSED(requiredMemberDeclRef); @@ -6696,7 +6710,8 @@ bool SemanticsVisitor::trySynthesizeRequirementWitness( context, lookupResult, requiredFuncDeclRef, - witnessTable)) + witnessTable, + outFailureDetails)) return true; if (auto builtinAttr = @@ -7514,8 +7529,13 @@ 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)) + MethodWitnessSynthesisFailureDetails failureDetails = {}; + if (trySynthesizeRequirementWitness( + context, + lookupResult, + requiredMemberDeclRef, + witnessTable, + &failureDetails)) { return true; } @@ -7536,24 +7556,54 @@ bool SemanticsVisitor::findWitnessForInterfaceRequirement( // and if nothing is found we print the candidates that made it // furthest in checking. // - if (!lookupResult.isOverloaded() && lookupResult.isValid()) + // Based on the failure reason, emit specific diagnostics + if (failureDetails.reason == WitnessSynthesisFailureReason::MethodResultTypeMismatch) { + // Emit specific return type mismatch diagnostic getSink()->diagnose( - lookupResult.item.declRef, - Diagnostics::memberDoesNotMatchRequirementSignature, - lookupResult.item.declRef); + failureDetails.candidateMethod, + Diagnostics::memberReturnTypeMismatch, + failureDetails.candidateMethod, + failureDetails.actualType, + failureDetails.expectedType); } - else + else if (failureDetails.reason == WitnessSynthesisFailureReason::ParameterDirMismatch) { getSink()->diagnose( - inheritanceDecl, - Diagnostics::typeDoesntImplementInterfaceRequirement, - subType, - requiredMemberDeclRef); + failureDetails.paramDecl, + Diagnostics::parameterDirectionDoesNotMatchRequirement, + failureDetails.paramDecl, + failureDetails.actualDir, + failureDetails.expectedDir); } - if (context->innerSink.outputBuffer.getLength()) + else if (failureDetails.reason == WitnessSynthesisFailureReason::GenericSignatureMismatch) + { + getSink()->diagnose( + SourceLoc(), + Diagnostics::genericSignatureDoesNotMatchRequirement, + requiredMemberDeclRef.getDecl()->getName()); + } + else { - getSink()->diagnoseRaw(Severity::Note, context->innerSink.outputBuffer.getUnownedSlice()); + // General failure - use existing logic + if (!lookupResult.isOverloaded() && lookupResult.isValid()) + { + getSink()->diagnose( + lookupResult.item.declRef, + Diagnostics::memberDoesNotMatchRequirementSignature, + lookupResult.item.declRef); + } + else + { + getSink()->diagnose( + inheritanceDecl, + Diagnostics::typeDoesntImplementInterfaceRequirement, + subType, + requiredMemberDeclRef); + + for (auto& item : lookupResult) + getSink()->diagnose(item.declRef, Diagnostics::seeOverloadConsidered, item.declRef); + } } getSink()->diagnose( requiredMemberDeclRef, diff --git a/source/slang/slang-check-impl.h b/source/slang/slang-check-impl.h index 81dca7312..3145c9454 100644 --- a/source/slang/slang-check-impl.h +++ b/source/slang/slang-check-impl.h @@ -1814,11 +1814,6 @@ public: void checkModifiers(ModifiableSyntaxNode* syntaxNode); void checkVisibility(Decl* decl); - bool doesSignatureMatchRequirement( - DeclRef satisfyingMemberDeclRef, - DeclRef requiredMemberDeclRef, - RefPtr witnessTable); - bool doesAccessorMatchRequirement( DeclRef satisfyingMemberDeclRef, DeclRef requiredMemberDeclRef); @@ -1859,10 +1854,6 @@ public: // // If it does, then inserts a witness into `witnessTable` // and returns `true`, otherwise returns `false` - bool doesMemberSatisfyRequirement( - DeclRef memberDeclRef, - DeclRef requiredMemberDeclRef, - RefPtr witnessTable); // State used while checking if a declaration (either a type declaration // or an extension of that type) conforms to the interfaces it claims @@ -1879,12 +1870,42 @@ public: /// declaration) ContainerDecl* parentDecl; - // An inner diagnostic sink to store diagnostics about why requirement synthesis failed. - DiagnosticSink innerSink; - Dictionary, RefPtr> mapInterfaceToWitnessTable; }; + /// Reasons why witness synthesis can fail + enum class WitnessSynthesisFailureReason + { + General, // Generic failure (default) + MethodResultTypeMismatch, // Method return type doesn't match interface requirement + ParameterDirMismatch, // Parameter direction mismatch (e.g., `in` vs `out`) + GenericSignatureMismatch, // Generic signature mismatch (e.g., number of generic parameters) + }; + + /// Details about method witness synthesis failure + struct MethodWitnessSynthesisFailureDetails + { + WitnessSynthesisFailureReason reason = WitnessSynthesisFailureReason::General; + DeclRef candidateMethod; // The method that was considered but failed + Type* actualType = nullptr; // For type mismatches: the actual type found + Type* expectedType = nullptr; // For type mismatches: the expected type + ParameterDirection actualDir = + kParameterDirection_In; // For direction mismatches: the actual direction + ParameterDirection expectedDir = + kParameterDirection_In; // For direction mismatches: the expected direction + ParamDecl* paramDecl = nullptr; // For direction mismatches: the parameter declaration + }; + + bool doesSignatureMatchRequirement( + DeclRef satisfyingMemberDeclRef, + DeclRef requiredMemberDeclRef, + RefPtr witnessTable); + + bool doesMemberSatisfyRequirement( + DeclRef memberDeclRef, + DeclRef requiredMemberDeclRef, + RefPtr witnessTable); + void addModifiersToSynthesizedDecl( ConformanceCheckingContext* context, DeclRef requirement, @@ -1937,12 +1958,14 @@ public: /// `lookupResult`. /// /// On success, installs the syntethesized method in `witnessTable` and returns `true`. - /// Otherwise, returns `false`. + /// Otherwise, returns `false` and sets `outFailureDetails` with information about why + /// synthesis failed. bool trySynthesizeMethodRequirementWitness( ConformanceCheckingContext* context, LookupResult const& lookupResult, DeclRef requiredMemberDeclRef, - RefPtr witnessTable); + RefPtr witnessTable, + MethodWitnessSynthesisFailureDetails* outFailureDetails = nullptr); bool trySynthesizeConstructorRequirementWitness( ConformanceCheckingContext* context, @@ -1994,13 +2017,14 @@ public: /// `lookupResult`. /// /// On success, installs the syntethesized declaration in `witnessTable` and returns `true`. - /// Otherwise, returns `false`. + /// Otherwise, returns `false` and sets `outFailureDetails` with information about why + /// synthesis failed. bool trySynthesizeRequirementWitness( ConformanceCheckingContext* context, LookupResult const& lookupResult, DeclRef requiredMemberDeclRef, - RefPtr witnessTable); - + RefPtr witnessTable, + MethodWitnessSynthesisFailureDetails* outFailureDetails = nullptr); enum SynthesisPattern { diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h index bab9a0fb8..07433df5c 100644 --- a/source/slang/slang-diagnostic-defs.h +++ b/source/slang/slang-diagnostic-defs.h @@ -72,23 +72,7 @@ DIAGNOSTIC( 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'.") +DIAGNOSTIC(-1, Note, seeOverloadConsidered, "see overloads considered: '$0'.") // An alternate wording of the above note, emphasing the position rather than content of the // declaration. @@ -2076,6 +2060,22 @@ DIAGNOSTIC( Error, memberDoesNotMatchRequirementSignature, "member '$0' does not match interface requirement.") +DIAGNOSTIC( + 38106, + Error, + memberReturnTypeMismatch, + "member '$0' return type '$1' does not match interface requirement return type '$2'.") +DIAGNOSTIC( + 38107, + Error, + genericSignatureDoesNotMatchRequirement, + "generic signature of '$0' does not match interface requirement.") +DIAGNOSTIC( + 38108, + Error, + parameterDirectionDoesNotMatchRequirement, + "parameter '$0' direction '$1' does not match interface requirement '$2'.") + DIAGNOSTIC( 38101, Error, -- cgit v1.2.3