-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Add elaboration when call fails all overloads but succeeds against the implementation signature #41001
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.
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
Add elaboration when call fails all overloads but succeeds against the implementation signature #41001
Changes from all commits
a49099f
6b29f36
de20443
ca26ca1
f324fde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27227,10 +27227,10 @@ namespace ts { | |
// is just important for choosing the best signature. So in the case where there is only one | ||
// signature, the subtype pass is useless. So skipping it is an optimization. | ||
if (candidates.length > 1) { | ||
result = chooseOverload(candidates, subtypeRelation, signatureHelpTrailingComma); | ||
result = chooseOverload(candidates, subtypeRelation, isSingleNonGenericCandidate, signatureHelpTrailingComma); | ||
} | ||
if (!result) { | ||
result = chooseOverload(candidates, assignableRelation, signatureHelpTrailingComma); | ||
result = chooseOverload(candidates, assignableRelation, isSingleNonGenericCandidate, signatureHelpTrailingComma); | ||
} | ||
if (result) { | ||
return result; | ||
|
@@ -27255,6 +27255,7 @@ namespace ts { | |
if (last.declaration && candidatesForArgumentError.length > 3) { | ||
addRelatedInfo(d, createDiagnosticForNode(last.declaration, Diagnostics.The_last_overload_is_declared_here)); | ||
} | ||
addImplementationSuccessElaboration(last, d); | ||
diagnostics.add(d); | ||
} | ||
} | ||
|
@@ -27290,14 +27291,19 @@ namespace ts { | |
const chain = chainDiagnosticMessages( | ||
map(diags, d => typeof d.messageText === "string" ? (d as DiagnosticMessageChain) : d.messageText), | ||
Diagnostics.No_overload_matches_this_call); | ||
const related = flatMap(diags, d => (d as Diagnostic).relatedInformation) as DiagnosticRelatedInformation[]; | ||
// The below is a spread to guarantee we get a new (mutable) array - our `flatMap` helper tries to do "smart" optimizations where it reuses input | ||
// arrays and the emptyArray singleton where possible, which is decidedly not what we want while we're still constructing this diagnostic | ||
const related = [...flatMap(diags, d => (d as Diagnostic).relatedInformation) as DiagnosticRelatedInformation[]]; | ||
let diag: Diagnostic; | ||
if (every(diags, d => d.start === diags[0].start && d.length === diags[0].length && d.file === diags[0].file)) { | ||
const { file, start, length } = diags[0]; | ||
diagnostics.add({ file, start, length, code: chain.code, category: chain.category, messageText: chain, relatedInformation: related }); | ||
diag = { file, start, length, code: chain.code, category: chain.category, messageText: chain, relatedInformation: related }; | ||
} | ||
else { | ||
diagnostics.add(createDiagnosticForNodeFromMessageChain(node, chain, related)); | ||
diag = createDiagnosticForNodeFromMessageChain(node, chain, related); | ||
} | ||
addImplementationSuccessElaboration(candidatesForArgumentError[0], diag); | ||
diagnostics.add(diag); | ||
} | ||
} | ||
else if (candidateForArgumentArityError) { | ||
|
@@ -27322,7 +27328,28 @@ namespace ts { | |
|
||
return getCandidateForOverloadFailure(node, candidates, args, !!candidatesOutArray); | ||
|
||
function chooseOverload(candidates: Signature[], relation: ESMap<string, RelationComparisonResult>, signatureHelpTrailingComma = false) { | ||
function addImplementationSuccessElaboration(failed: Signature, diagnostic: Diagnostic) { | ||
weswigham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const oldCandidatesForArgumentError = candidatesForArgumentError; | ||
const oldCandidateForArgumentArityError = candidateForArgumentArityError; | ||
const oldCandidateForTypeArgumentError = candidateForTypeArgumentError; | ||
|
||
const declCount = length(failed.declaration?.symbol.declarations); | ||
const isOverload = declCount > 1; | ||
const implDecl = isOverload ? find(failed.declaration?.symbol.declarations || emptyArray, d => nodeIsPresent((d as FunctionLikeDeclaration).body)) : undefined; | ||
weswigham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (implDecl) { | ||
weswigham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const candidate = getSignatureFromDeclaration(implDecl as FunctionLikeDeclaration); | ||
const isSingleNonGenericCandidate = !candidate.typeParameters; | ||
if (chooseOverload([candidate], assignableRelation, isSingleNonGenericCandidate)) { | ||
addRelatedInfo(diagnostic, createDiagnosticForNode(implDecl, Diagnostics.The_call_would_have_succeeded_against_this_implementation_but_implementation_signatures_of_overloads_are_not_externally_visible)); | ||
} | ||
weswigham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this mutate the related list? (and is that why you clone into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
weswigham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
candidatesForArgumentError = oldCandidatesForArgumentError; | ||
candidateForArgumentArityError = oldCandidateForArgumentArityError; | ||
candidateForTypeArgumentError = oldCandidateForTypeArgumentError; | ||
} | ||
|
||
function chooseOverload(candidates: Signature[], relation: ESMap<string, RelationComparisonResult>, isSingleNonGenericCandidate: boolean, signatureHelpTrailingComma = false) { | ||
candidatesForArgumentError = undefined; | ||
candidateForArgumentArityError = undefined; | ||
candidateForTypeArgumentError = undefined; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,4 +15,5 @@ tests/cases/compiler/functionOverloads2.ts(4,13): error TS2769: No overload matc | |
!!! error TS2769: Overload 1 of 2, '(bar: string): string', gave the following error. | ||
!!! error TS2769: Argument of type 'boolean' is not assignable to parameter of type 'string'. | ||
!!! error TS2769: Overload 2 of 2, '(bar: number): number', gave the following error. | ||
!!! error TS2769: Argument of type 'boolean' is not assignable to parameter of type 'number'. | ||
!!! error TS2769: Argument of type 'boolean' is not assignable to parameter of type 'number'. | ||
!!! related TS2793 tests/cases/compiler/functionOverloads2.ts:3:10: The call would have succeeded against this implementation, but implementation signatures of overloads are not externally visible. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😍 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
tests/cases/compiler/overloadErrorMatchesImplementationElaboaration.ts(8,12): error TS2345: Argument of type 'number[]' is not assignable to parameter of type 'string'. | ||
|
||
|
||
==== tests/cases/compiler/overloadErrorMatchesImplementationElaboaration.ts (1 errors) ==== | ||
class EventAggregator | ||
{ | ||
publish(event: string, data?: any): void; | ||
publish<T>(event: T): void {} | ||
} | ||
|
||
var ea: EventAggregator; | ||
ea.publish([1,2,3]); | ||
~~~~~~~ | ||
!!! error TS2345: Argument of type 'number[]' is not assignable to parameter of type 'string'. | ||
!!! related TS2793 tests/cases/compiler/overloadErrorMatchesImplementationElaboaration.ts:4:5: The call would have succeeded against this implementation, but implementation signatures of overloads are not externally visible. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
//// [overloadErrorMatchesImplementationElaboaration.ts] | ||
class EventAggregator | ||
{ | ||
publish(event: string, data?: any): void; | ||
publish<T>(event: T): void {} | ||
} | ||
|
||
var ea: EventAggregator; | ||
ea.publish([1,2,3]); | ||
|
||
//// [overloadErrorMatchesImplementationElaboaration.js] | ||
var EventAggregator = /** @class */ (function () { | ||
function EventAggregator() { | ||
} | ||
EventAggregator.prototype.publish = function (event) { }; | ||
return EventAggregator; | ||
}()); | ||
var ea; | ||
ea.publish([1, 2, 3]); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
=== tests/cases/compiler/overloadErrorMatchesImplementationElaboaration.ts === | ||
class EventAggregator | ||
>EventAggregator : Symbol(EventAggregator, Decl(overloadErrorMatchesImplementationElaboaration.ts, 0, 0)) | ||
{ | ||
publish(event: string, data?: any): void; | ||
>publish : Symbol(EventAggregator.publish, Decl(overloadErrorMatchesImplementationElaboaration.ts, 1, 1), Decl(overloadErrorMatchesImplementationElaboaration.ts, 2, 45)) | ||
>event : Symbol(event, Decl(overloadErrorMatchesImplementationElaboaration.ts, 2, 12)) | ||
>data : Symbol(data, Decl(overloadErrorMatchesImplementationElaboaration.ts, 2, 26)) | ||
|
||
publish<T>(event: T): void {} | ||
>publish : Symbol(EventAggregator.publish, Decl(overloadErrorMatchesImplementationElaboaration.ts, 1, 1), Decl(overloadErrorMatchesImplementationElaboaration.ts, 2, 45)) | ||
>T : Symbol(T, Decl(overloadErrorMatchesImplementationElaboaration.ts, 3, 12)) | ||
>event : Symbol(event, Decl(overloadErrorMatchesImplementationElaboaration.ts, 3, 15)) | ||
>T : Symbol(T, Decl(overloadErrorMatchesImplementationElaboaration.ts, 3, 12)) | ||
} | ||
|
||
var ea: EventAggregator; | ||
>ea : Symbol(ea, Decl(overloadErrorMatchesImplementationElaboaration.ts, 6, 3)) | ||
>EventAggregator : Symbol(EventAggregator, Decl(overloadErrorMatchesImplementationElaboaration.ts, 0, 0)) | ||
|
||
ea.publish([1,2,3]); | ||
>ea.publish : Symbol(EventAggregator.publish, Decl(overloadErrorMatchesImplementationElaboaration.ts, 1, 1), Decl(overloadErrorMatchesImplementationElaboaration.ts, 2, 45)) | ||
>ea : Symbol(ea, Decl(overloadErrorMatchesImplementationElaboaration.ts, 6, 3)) | ||
>publish : Symbol(EventAggregator.publish, Decl(overloadErrorMatchesImplementationElaboaration.ts, 1, 1), Decl(overloadErrorMatchesImplementationElaboaration.ts, 2, 45)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
=== tests/cases/compiler/overloadErrorMatchesImplementationElaboaration.ts === | ||
class EventAggregator | ||
>EventAggregator : EventAggregator | ||
{ | ||
publish(event: string, data?: any): void; | ||
>publish : (event: string, data?: any) => void | ||
>event : string | ||
>data : any | ||
|
||
publish<T>(event: T): void {} | ||
>publish : (event: string, data?: any) => void | ||
>event : T | ||
} | ||
|
||
var ea: EventAggregator; | ||
>ea : EventAggregator | ||
|
||
ea.publish([1,2,3]); | ||
>ea.publish([1,2,3]) : void | ||
>ea.publish : (event: string, data?: any) => void | ||
>ea : EventAggregator | ||
>publish : (event: string, data?: any) => void | ||
>[1,2,3] : number[] | ||
>1 : 1 | ||
>2 : 2 | ||
>3 : 3 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we ever figure out what the performance impact of
[]
vsemptyArray
is? Last I heard is that[]
is better?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...eh? In this case, it's just because we mutate
relatedInformation
in some places, so using a global singleton is bad.