Skip to content

Fix for Issue 56013 #56373

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix for Issue 56013 #56373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d1cfec5
original checker and new tests/result
craigphicks Nov 12, 2023
2317daa
fixed checker and test results
craigphicks Nov 14, 2023
77eab82
in checkExpression(...), reset links.resolvedSignature to global reso…
craigphicks Nov 14, 2023
4789f74
tests/cases/compiler/_co/_coXXX.ts
craigphicks Nov 27, 2023
2c4bc6d
cosmetic changes to checker.ts
craigphicks Nov 28, 2023
e36a090
dprint fmt checker.ts
craigphicks Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 97 additions & 61 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1487,6 +1487,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
var lastGetCombinedModifierFlagsNode: Declaration | undefined;
var lastGetCombinedModifierFlagsResult = ModifierFlags.None;

var chooseOverloadRecursionLevel = -1;
var chooseOverloadFlushNodesSignaturesReq: (Set<Node> | undefined)[] = [];

// for public members that accept a Node or one of its subtypes, we must guard against
// synthetic nodes created during transformations by calling `getParseTreeNode`.
// for most of these, we perform the guard only on `checker` to avoid any possible
Expand Down Expand Up @@ -34310,87 +34313,97 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function chooseOverload(candidates: Signature[], relation: Map<string, RelationComparisonResult>, isSingleNonGenericCandidate: boolean, signatureHelpTrailingComma = false) {
candidatesForArgumentError = undefined;
candidateForArgumentArityError = undefined;
candidateForTypeArgumentError = undefined;
chooseOverloadRecursionLevel++;
chooseOverloadFlushNodesSignaturesReq[chooseOverloadRecursionLevel] = undefined;
const result = chooseOverloadWorker();
Copy link
Author

@craigphicks craigphicks Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main body of choose overload is wrapped in an Immediately Invoked Function Expression to implement the chooseOverloadFlushNodesSignaturesReq stack without error. Calling the IIFE resulted in indentation which messes up the diff. Actually there is only a single line changed inside the IIFE.

Copy link
Member

@weswigham weswigham Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a style thing - extract the IIFE into a named function in the scope, such as chooseOverloadWorker instead; makes debugging easier later for it to be named (and is what we do elsewhere when we want to use patterns like this).

chooseOverloadFlushNodesSignaturesReq[chooseOverloadRecursionLevel] = undefined;
chooseOverloadRecursionLevel--;
return result;

if (isSingleNonGenericCandidate) {
const candidate = candidates[0];
if (some(typeArguments) || !hasCorrectArity(node, args, candidate, signatureHelpTrailingComma)) {
return undefined;
}
if (getSignatureApplicabilityError(node, args, candidate, relation, CheckMode.Normal, /*reportErrors*/ false, /*containingMessageChain*/ undefined)) {
candidatesForArgumentError = [candidate];
return undefined;
}
return candidate;
}
function chooseOverloadWorker() {
candidatesForArgumentError = undefined;
candidateForArgumentArityError = undefined;
candidateForTypeArgumentError = undefined;

for (let candidateIndex = 0; candidateIndex < candidates.length; candidateIndex++) {
const candidate = candidates[candidateIndex];
if (!hasCorrectTypeArgumentArity(candidate, typeArguments) || !hasCorrectArity(node, args, candidate, signatureHelpTrailingComma)) {
continue;
if (isSingleNonGenericCandidate) {
const candidate = candidates[0];
if (some(typeArguments) || !hasCorrectArity(node, args, candidate, signatureHelpTrailingComma)) {
return undefined;
}
if (getSignatureApplicabilityError(node, args, candidate, relation, CheckMode.Normal, /*reportErrors*/ false, /*containingMessageChain*/ undefined)) {
candidatesForArgumentError = [candidate];
return undefined;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the top of every loop after the first the request is set.

return candidate;
}

let checkCandidate: Signature;
let inferenceContext: InferenceContext | undefined;

if (candidate.typeParameters) {
let typeArgumentTypes: Type[] | undefined;
if (some(typeArguments)) {
typeArgumentTypes = checkTypeArguments(candidate, typeArguments, /*reportErrors*/ false);
if (!typeArgumentTypes) {
candidateForTypeArgumentError = candidate;
continue;
}
}
else {
inferenceContext = createInferenceContext(candidate.typeParameters, candidate, /*flags*/ isInJSFile(node) ? InferenceFlags.AnyDefault : InferenceFlags.None);
typeArgumentTypes = inferTypeArguments(node, candidate, args, argCheckMode | CheckMode.SkipGenericFunctions, inferenceContext);
argCheckMode |= inferenceContext.flags & InferenceFlags.SkippedGenericFunction ? CheckMode.SkipGenericFunctions : CheckMode.Normal;
}
checkCandidate = getSignatureInstantiation(candidate, typeArgumentTypes, isInJSFile(candidate.declaration), inferenceContext && inferenceContext.inferredTypeParameters);
// If the original signature has a generic rest type, instantiation may produce a
// signature with different arity and we need to perform another arity check.
if (getNonArrayRestType(candidate) && !hasCorrectArity(node, args, checkCandidate, signatureHelpTrailingComma)) {
candidateForArgumentArityError = checkCandidate;
for (let candidateIndex = 0; candidateIndex < candidates.length; candidateIndex++) {
if (candidateIndex > 0) chooseOverloadFlushNodesSignaturesReq[chooseOverloadRecursionLevel] = new Set();
const candidate = candidates[candidateIndex];
if (!hasCorrectTypeArgumentArity(candidate, typeArguments) || !hasCorrectArity(node, args, candidate, signatureHelpTrailingComma)) {
continue;
}
}
else {
checkCandidate = candidate;
}
if (getSignatureApplicabilityError(node, args, checkCandidate, relation, argCheckMode, /*reportErrors*/ false, /*containingMessageChain*/ undefined)) {
// Give preference to error candidates that have no rest parameters (as they are more specific)
(candidatesForArgumentError || (candidatesForArgumentError = [])).push(checkCandidate);
continue;
}
if (argCheckMode) {
// If one or more context sensitive arguments were excluded, we start including
// them now (and keeping do so for any subsequent candidates) and perform a second
// round of type inference and applicability checking for this particular candidate.
argCheckMode = CheckMode.Normal;
if (inferenceContext) {
const typeArgumentTypes = inferTypeArguments(node, candidate, args, argCheckMode, inferenceContext);
checkCandidate = getSignatureInstantiation(candidate, typeArgumentTypes, isInJSFile(candidate.declaration), inferenceContext.inferredTypeParameters);

let checkCandidate: Signature;
let inferenceContext: InferenceContext | undefined;

if (candidate.typeParameters) {
let typeArgumentTypes: Type[] | undefined;
if (some(typeArguments)) {
typeArgumentTypes = checkTypeArguments(candidate, typeArguments, /*reportErrors*/ false);
if (!typeArgumentTypes) {
candidateForTypeArgumentError = candidate;
continue;
}
}
else {
inferenceContext = createInferenceContext(candidate.typeParameters, candidate, /*flags*/ isInJSFile(node) ? InferenceFlags.AnyDefault : InferenceFlags.None);
typeArgumentTypes = inferTypeArguments(node, candidate, args, argCheckMode | CheckMode.SkipGenericFunctions, inferenceContext);
argCheckMode |= inferenceContext.flags & InferenceFlags.SkippedGenericFunction ? CheckMode.SkipGenericFunctions : CheckMode.Normal;
}
checkCandidate = getSignatureInstantiation(candidate, typeArgumentTypes, isInJSFile(candidate.declaration), inferenceContext && inferenceContext.inferredTypeParameters);
// If the original signature has a generic rest type, instantiation may produce a
// signature with different arity and we need to perform another arity check.
if (getNonArrayRestType(candidate) && !hasCorrectArity(node, args, checkCandidate, signatureHelpTrailingComma)) {
candidateForArgumentArityError = checkCandidate;
continue;
}
}
else {
checkCandidate = candidate;
}
if (getSignatureApplicabilityError(node, args, checkCandidate, relation, argCheckMode, /*reportErrors*/ false, /*containingMessageChain*/ undefined)) {
// Give preference to error candidates that have no rest parameters (as they are more specific)
(candidatesForArgumentError || (candidatesForArgumentError = [])).push(checkCandidate);
continue;
}
if (argCheckMode) {
// If one or more context sensitive arguments were excluded, we start including
// them now (and keeping do so for any subsequent candidates) and perform a second
// round of type inference and applicability checking for this particular candidate.
argCheckMode = CheckMode.Normal;
if (inferenceContext) {
const typeArgumentTypes = inferTypeArguments(node, candidate, args, argCheckMode, inferenceContext);
checkCandidate = getSignatureInstantiation(candidate, typeArgumentTypes, isInJSFile(candidate.declaration), inferenceContext.inferredTypeParameters);
// If the original signature has a generic rest type, instantiation may produce a
// signature with different arity and we need to perform another arity check.
if (getNonArrayRestType(candidate) && !hasCorrectArity(node, args, checkCandidate, signatureHelpTrailingComma)) {
candidateForArgumentArityError = checkCandidate;
continue;
}
}
if (getSignatureApplicabilityError(node, args, checkCandidate, relation, argCheckMode, /*reportErrors*/ false, /*containingMessageChain*/ undefined)) {
// Give preference to error candidates that have no rest parameters (as they are more specific)
(candidatesForArgumentError || (candidatesForArgumentError = [])).push(checkCandidate);
continue;
}
}
candidates[candidateIndex] = checkCandidate;
return checkCandidate;
}
candidates[candidateIndex] = checkCandidate;
return checkCandidate;
}

return undefined;
return undefined;
}
}
}

Expand Down Expand Up @@ -35151,6 +35164,13 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
links.resolvedSignature = resolvingSignature;
let result = resolveSignature(node, candidatesOutArray, checkMode || CheckMode.Normal);
/**
* The following is an invariant condition beneath the above call to resolveSignature:
* - links.resolvedSignature is either the global constant readonly value `resolvingSignature`
* - or a calculated signature.
*/
Debug.assert(links.resolvedSignature);

// When CheckMode.SkipGenericFunctions is set we use resolvingSignature to indicate that call
// resolution should be deferred.
if (result !== resolvingSignature) {
Expand Down Expand Up @@ -35361,6 +35381,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function checkDeprecatedSignature(signature: Signature, node: CallLikeExpression) {
Debug.assert(signature);
if (signature.flags & SignatureFlags.IsSignatureCandidateForOverloadFailure) return;
if (signature.declaration && signature.declaration.flags & NodeFlags.Deprecated) {
const suggestionNode = getDeprecatedSuggestionNode(node);
Expand Down Expand Up @@ -38932,6 +38953,21 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const saveCurrentNode = currentNode;
currentNode = node;
instantiationCount = 0;
if (node.kind === SyntaxKind.CallExpression && chooseOverloadRecursionLevel >= 0) {
let setOfNode: Set<Node> | undefined;
if (chooseOverloadRecursionLevel >= 0 && (setOfNode = chooseOverloadFlushNodesSignaturesReq[chooseOverloadRecursionLevel])) {
if (!setOfNode.has(node)) {
/**
* The following is an invariant condition beneath the above call to resolveSignature:
* - getNodeLinks(node).resolvedSignature is either the global constant readonly value `resolvingSignature`
* - or a calculated signature.
* Therefore resetting must set to `resolvingSignature` - it means "not yet calculated".
*/
getNodeLinks(node).resolvedSignature = resolvingSignature;
setOfNode.add(node);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised the perf of this is OK, but it's likely because this solution is incomplete - this only partially purges the cached nodes; the cached parameter symbol types on the parameter symbol links are still there! So while you'll get new signatures here, any parameters that get witnessed by multiple overload passes will still have their types locked down. Which isn't any worse than today, to be fair! It's a known issue with the signature resolution logic. IME, adding a full hierarchical cache to fully handle it and make speculative overload resolution totally unwindable was pretty bad for perf, but this approach, being partial, looks like it's OK. And if it still manages to fix a few usecases, that sounds pretty OK to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like, this feels odd, because it seems naive - it's basically resetting the resolved signature of every call expression we visit while looking at nested overload resolutions - in theory, only some calls should need to get reset - those which actually depend on the overload resolution in progress (which'd require a bit more tracing). I'd have figured over-invalidation like this'd manifest as terrible perf, but no - perf looks OK, broadly speaking, not more than a single percent diff.

}
}
}
const uninstantiatedType = checkExpressionWorker(node, checkMode, forceTuple);
const type = instantiateTypeWithSingleGenericCallSignature(node, uninstantiatedType, checkMode);
if (isConstEnumObjectType(type)) {
Expand Down
34 changes: 34 additions & 0 deletions tests/baselines/reference/arrayFilterBooleanExternalOverload1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
//// [tests/cases/compiler/arrayFilterBooleanExternalOverload1.ts] ////

//// [arrayFilterBooleanExternalOverload1.ts]
// #56013

// For reference, thise cases work as expected (no errors) when no external BooleanConstrudtor like overload is present
declare const maybe: boolean;
{
const id = <T>() => (t: T) => !!t;

const result1 = (maybe ? ['foo', 'bar', undefined] : [1] ).filter(id());

result1;

const result2 = ['foo', 'bar', undefined].filter(id()); // want id() = (t: string) => boolean

result2;
}


//// [arrayFilterBooleanExternalOverload1.js]
"use strict";
// #56013
{
const id = () => (t) => !!t;
const result1 = (maybe ? ['foo', 'bar', undefined] : [1]).filter(id());
result1;
const result2 = ['foo', 'bar', undefined].filter(id()); // want id() = (t: string) => boolean
result2;
}


//// [arrayFilterBooleanExternalOverload1.d.ts]
declare const maybe: boolean;
38 changes: 38 additions & 0 deletions tests/baselines/reference/arrayFilterBooleanExternalOverload1.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
//// [tests/cases/compiler/arrayFilterBooleanExternalOverload1.ts] ////

=== arrayFilterBooleanExternalOverload1.ts ===
// #56013

// For reference, thise cases work as expected (no errors) when no external BooleanConstrudtor like overload is present
declare const maybe: boolean;
>maybe : Symbol(maybe, Decl(arrayFilterBooleanExternalOverload1.ts, 3, 13))
{
const id = <T>() => (t: T) => !!t;
>id : Symbol(id, Decl(arrayFilterBooleanExternalOverload1.ts, 5, 9))
>T : Symbol(T, Decl(arrayFilterBooleanExternalOverload1.ts, 5, 16))
>t : Symbol(t, Decl(arrayFilterBooleanExternalOverload1.ts, 5, 25))
>T : Symbol(T, Decl(arrayFilterBooleanExternalOverload1.ts, 5, 16))
>t : Symbol(t, Decl(arrayFilterBooleanExternalOverload1.ts, 5, 25))

const result1 = (maybe ? ['foo', 'bar', undefined] : [1] ).filter(id());
>result1 : Symbol(result1, Decl(arrayFilterBooleanExternalOverload1.ts, 7, 9))
>(maybe ? ['foo', 'bar', undefined] : [1] ).filter : Symbol(Array.filter, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>maybe : Symbol(maybe, Decl(arrayFilterBooleanExternalOverload1.ts, 3, 13))
>undefined : Symbol(undefined)
>filter : Symbol(Array.filter, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>id : Symbol(id, Decl(arrayFilterBooleanExternalOverload1.ts, 5, 9))

result1;
>result1 : Symbol(result1, Decl(arrayFilterBooleanExternalOverload1.ts, 7, 9))

const result2 = ['foo', 'bar', undefined].filter(id()); // want id() = (t: string) => boolean
>result2 : Symbol(result2, Decl(arrayFilterBooleanExternalOverload1.ts, 11, 9))
>['foo', 'bar', undefined].filter : Symbol(Array.filter, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>undefined : Symbol(undefined)
>filter : Symbol(Array.filter, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>id : Symbol(id, Decl(arrayFilterBooleanExternalOverload1.ts, 5, 9))

result2;
>result2 : Symbol(result2, Decl(arrayFilterBooleanExternalOverload1.ts, 11, 9))
}

Loading