From 6280cf95b4a4cf7d00c85564191b418802cd85ac Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Fri, 27 Mar 2020 15:32:51 +0100 Subject: [PATCH] fix(language-service): Improve signature selection by finding exact match (#37494) The function signature selection algorithm is totally naive. It'd unconditionally pick the first signature if there are multiple overloads. This commit improves the algorithm by returning an exact match if one exists. PR Close #37494 --- .../src/typescript_symbols.ts | 24 ++++++++++++++++++- .../language-service/test/completions_spec.ts | 14 +++++++++++ .../language-service/test/project/app/main.ts | 1 + .../test/project/app/parsing-cases.ts | 16 ++++++++++++- 4 files changed, 53 insertions(+), 2 deletions(-) diff --git a/packages/language-service/src/typescript_symbols.ts b/packages/language-service/src/typescript_symbols.ts index 48e568c1f5..408adff8c5 100644 --- a/packages/language-service/src/typescript_symbols.ts +++ b/packages/language-service/src/typescript_symbols.ts @@ -222,13 +222,35 @@ function signaturesOf(type: ts.Type, context: TypeContext): Signature[] { return type.getCallSignatures().map(s => new SignatureWrapper(s, context)); } -function selectSignature(type: ts.Type, context: TypeContext, _types: Symbol[]): Signature| +function selectSignature(type: ts.Type, context: TypeContext, types: Symbol[]): Signature| undefined { // TODO: Do a better job of selecting the right signature. TypeScript does not currently support a // Type Relationship API (see https://github.com/angular/vscode-ng-language-service/issues/143). // Consider creating a TypeCheckBlock host in the language service that may also act as a // scratchpad for type comparisons. const signatures = type.getCallSignatures(); + const passedInTypes: Array = types.map(type => { + if (type instanceof TypeWrapper) { + return type.tsType; + } + }); + // Try to select a matching signature in which all parameter types match. + // Note that this is just a best-effort approach, because we're checking for + // strict type equality rather than compatibility. + // For example, if the signature contains a ReadonlyArray and the + // passed parameter type is an Array, this will fail. + function allParameterTypesMatch(signature: ts.Signature) { + const tc = context.checker; + return signature.getParameters().every((parameter: ts.Symbol, i: number) => { + const type = tc.getTypeOfSymbolAtLocation(parameter, parameter.valueDeclaration); + return type === passedInTypes[i]; + }); + } + const exactMatch = signatures.find(allParameterTypesMatch); + if (exactMatch) { + return new SignatureWrapper(exactMatch, context); + } + // If not, fallback to a naive selection return signatures.length ? new SignatureWrapper(signatures[0], context) : undefined; } diff --git a/packages/language-service/test/completions_spec.ts b/packages/language-service/test/completions_spec.ts index 63af0e4e07..d8fe23cd1f 100644 --- a/packages/language-service/test/completions_spec.ts +++ b/packages/language-service/test/completions_spec.ts @@ -832,6 +832,20 @@ describe('completions', () => { expectContain(completions, CompletionKind.METHOD, ['charAt', 'substring']); }); }); + + it('should select the right signature for a pipe given exact type', () => { + mockHost.override(TEST_TEMPLATE, '{{ ("world" | prefixPipe:"hello").~{cursor} }}'); + const m1 = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor'); + const c1 = ngLS.getCompletionsAtPosition(TEST_TEMPLATE, m1.start); + // should resolve to transform(value: string, prefix: string): string + expectContain(c1, CompletionKind.METHOD, ['charCodeAt', 'trim']); + + mockHost.override(TEST_TEMPLATE, '{{ (456 | prefixPipe:123).~{cursor} }}'); + const m2 = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor'); + const c2 = ngLS.getCompletionsAtPosition(TEST_TEMPLATE, m2.start); + // should resolve to transform(value: number, prefix: number): number + expectContain(c2, CompletionKind.METHOD, ['toFixed', 'toExponential']); + }); }); function expectContain( diff --git a/packages/language-service/test/project/app/main.ts b/packages/language-service/test/project/app/main.ts index 7b49ad6c1d..3206ad2be6 100644 --- a/packages/language-service/test/project/app/main.ts +++ b/packages/language-service/test/project/app/main.ts @@ -22,6 +22,7 @@ import * as ParsingCases from './parsing-cases'; ParsingCases.StringModel, ParsingCases.TemplateReference, ParsingCases.TestComponent, + ParsingCases.TestPipe, ParsingCases.WithContextDirective, ] }) diff --git a/packages/language-service/test/project/app/parsing-cases.ts b/packages/language-service/test/project/app/parsing-cases.ts index 0d069a4b2a..703c1b71ef 100644 --- a/packages/language-service/test/project/app/parsing-cases.ts +++ b/packages/language-service/test/project/app/parsing-cases.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Component, Directive, EventEmitter, Input, OnChanges, Output, SimpleChanges, TemplateRef, ViewContainerRef} from '@angular/core'; +import {Component, Directive, EventEmitter, Input, OnChanges, Output, Pipe, PipeTransform, SimpleChanges, TemplateRef, ViewContainerRef} from '@angular/core'; import {Hero} from './app.component'; @@ -69,6 +69,20 @@ export class WithContextDirective { } } +@Pipe({ + name: 'prefixPipe', +}) +export class TestPipe implements PipeTransform { + transform(value: string, prefix: string): string; + transform(value: number, prefix: number): number; + transform(value: string|number, prefix: string|number): string|number { + if (typeof value === 'string') { + return `${prefix} ${value}`; + } + return parseInt(`${prefix}${value}`, 10 /* radix */); + } +} + /** * This Component provides the `test-comp` selector. */