From 455830ed4282c9f0de7b73c78186355d3bd7e251 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Wed, 19 Jul 2023 11:51:39 -0700 Subject: [PATCH] Improved error messages for situations where a call expression targets an overloaded function or method and there are no matches. Added some heuristics to pick the "best" overload function for the error message. Previously, pyright used the last overload, but this sometimes led to confusing errors. This addresses #5525. (#5541) Co-authored-by: Eric Traut --- .../src/analyzer/typeEvaluator.ts | 47 ++++++++++++++----- .../src/tests/typeEvaluator2.test.ts | 2 +- 2 files changed, 35 insertions(+), 14 deletions(-) diff --git a/packages/pyright-internal/src/analyzer/typeEvaluator.ts b/packages/pyright-internal/src/analyzer/typeEvaluator.ts index b3ac45fc1..c081f30d3 100644 --- a/packages/pyright-internal/src/analyzer/typeEvaluator.ts +++ b/packages/pyright-internal/src/analyzer/typeEvaluator.ts @@ -339,6 +339,12 @@ interface MatchArgsToParamsResult { // A higher relevance means that it should be considered // first, before lower relevance overloads. relevance: number; + + // A score that indicates how well the overload matches with + // supplied arguments. Used to pick the "best" for purposes + // of error reporting when no matches are found. The higher + // the score, the worse the match. + argumentMatchScore: number; } export interface MemberAccessTypeResult { @@ -8239,7 +8245,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions // If there were multiple possible matches, evaluate the type as // Unknown, but include the "possible types" to allow for completion // suggestions. - if (!isDefinitiveMatchFound) { + if (!isDefinitiveMatchFound && possibleMatchResults.length > 0) { possibleMatchResults = filterOverloadMatchesForAnyArgs(possibleMatchResults); // Did the filtering produce a single result? If so, we're done. @@ -8532,22 +8538,26 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions return { argumentErrors: true, isTypeIncomplete: false, overloadsUsedForCall: [] }; } - // Create a helper function that evaluates the overload that matches - // the arg/param lists. - function evaluateUsingLastMatchingOverload(skipUnknownArgCheck: boolean) { - // Find the match with the largest overload index (i.e. the last overload - // that was in the overload list). - const lastMatch = filteredMatchResults.reduce((previous, current) => { - return current.overloadIndex > previous.overloadIndex ? current : previous; + // Create a helper function that evaluates the overload that best + // matches the arg/param lists. + function evaluateUsingBestMatchingOverload(skipUnknownArgCheck: boolean) { + // Find the match with the smallest argument match score. If there + // are more than one with the same score, use the one with the + // largest index. Later overloads tend to be more general. + const bestMatch = filteredMatchResults.reduce((previous, current) => { + if (current.argumentMatchScore === previous.argumentMatchScore) { + return current.overloadIndex > previous.overloadIndex ? current : previous; + } + return current.argumentMatchScore < previous.argumentMatchScore ? current : previous; }); const effectiveTypeVarContext = typeVarContext ?? new TypeVarContext(); - effectiveTypeVarContext.addSolveForScope(getTypeVarScopeIds(lastMatch.overload)); + effectiveTypeVarContext.addSolveForScope(getTypeVarScopeIds(bestMatch.overload)); effectiveTypeVarContext.unlock(); return validateFunctionArgumentTypesWithContext( errorNode, - lastMatch, + bestMatch, effectiveTypeVarContext, skipUnknownArgCheck, inferenceContext @@ -8558,7 +8568,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions // use the normal type matching mechanism because it is faster and // will provide a clearer error message. if (filteredMatchResults.length === 1) { - return evaluateUsingLastMatchingOverload(/* skipUnknownArgCheck */ false); + return evaluateUsingBestMatchingOverload(/* skipUnknownArgCheck */ false); } let expandedArgTypes: (Type | undefined)[][] | undefined = [argList.map((arg) => undefined)]; @@ -8618,7 +8628,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions // in speculative mode because it's very expensive, and we're going to // suppress the diagnostic anyway. if (!isDiagnosticSuppressedForNode(errorNode) && !isTypeIncomplete) { - const result = evaluateUsingLastMatchingOverload(/* skipUnknownArgCheck */ true); + const result = evaluateUsingBestMatchingOverload(/* skipUnknownArgCheck */ true); // Replace the result with an unknown type since we don't know // what overload should have been used. @@ -10477,6 +10487,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions paramSpecArgList, activeParam, relevance, + argumentMatchScore: 0, }; } @@ -10651,6 +10662,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions const type = matchResults.overload; let isTypeIncomplete = matchResults.isTypeIncomplete; let argumentErrors = false; + let argumentMatchScore = 0; let specializedInitSelfType: Type | undefined; let anyOrUnknownArgument: UnknownType | AnyType | undefined; const typeCondition = getTypeCondition(type); @@ -10775,7 +10787,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions let condition: TypeCondition[] = []; const argResults: ArgResult[] = []; - matchResults.argParams.forEach((argParam) => { + matchResults.argParams.forEach((argParam, argParamIndex) => { const argResult = validateArgType( argParam, typeVarContext, @@ -10791,6 +10803,11 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions if (!argResult.isCompatible) { argumentErrors = true; + + // Add the inverse index so earlier parameters represent larger errors. + // This will help the heuristics in the overload error paths to pick the + // most likely intended overload if none of them match. + argumentMatchScore += 1 + (matchResults.argParams.length - argParamIndex); } if (argResult.isTypeIncomplete) { @@ -10834,6 +10851,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions ) ) { argumentErrors = true; + argumentMatchScore += 1; } } else if (type.details.paramSpec) { if (!sawParamSpecArgs || !sawParamSpecKwargs) { @@ -10846,6 +10864,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions ); } argumentErrors = true; + argumentMatchScore += 1; } } @@ -10956,6 +10975,8 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions specializedInitSelfType = applySolvedTypeVars(specializedInitSelfType, typeVarContext); } + matchResults.argumentMatchScore = argumentMatchScore; + return { argumentErrors, argResults, diff --git a/packages/pyright-internal/src/tests/typeEvaluator2.test.ts b/packages/pyright-internal/src/tests/typeEvaluator2.test.ts index 2ddd1a075..7aebf6319 100644 --- a/packages/pyright-internal/src/tests/typeEvaluator2.test.ts +++ b/packages/pyright-internal/src/tests/typeEvaluator2.test.ts @@ -75,7 +75,7 @@ test('Assignment1', () => { test('Assignment2', () => { const analysisResults = TestUtils.typeAnalyzeSampleFiles(['assignment2.py']); - TestUtils.validateResults(analysisResults, 3); + TestUtils.validateResults(analysisResults, 2); }); test('Assignment3', () => {