From 77cd1976dbc5016cb3558c35c41f9ca5254c919c Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 22 Jan 2021 09:03:37 -0800 Subject: [PATCH] refactor(compiler-cli): Adjust generated TCB when checkTypeOfPipes is false When `checkTypeOfPipes` is set to `false`, our TCB currently generates the a statement like the following when pipes appear in the template: `(_pipe1 as any).transform(args)` This did enable us to get _some_ information from the Language Service about pipes in this case because we still had access to the pipe instance. However, because it is immediately cast to `any`, we cannot get type information about the transform access. That means actions like "go to definition", "find references", "quick info", etc. will return incomplete information or fail altogether. Instead, this commit changes the TCB to generate `(_pipe1.transform as any)(args)`. This gives us the ability to get complete information for the LS operations listed above. --- .../typecheck/src/template_symbol_builder.ts | 15 ++---- .../ngtsc/typecheck/src/type_check_block.ts | 14 +++--- .../typecheck/test/type_check_block_spec.ts | 2 +- ...ecker__get_symbol_of_template_node_spec.ts | 36 ++++++--------- .../GOLDEN_PARTIAL.js | 1 + .../forward_referenced_pipe.ts | 1 + .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 20 ++++++-- .../ivy/test/definitions_spec.ts | 11 ++--- .../ivy/test/quick_info_spec.ts | 9 +++- .../ivy/test/references_and_rename_spec.ts | 46 ++++++++++--------- .../ivy/test/type_definitions_spec.ts | 9 ++-- 11 files changed, 85 insertions(+), 79 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts index c509777c494e67..c6416c43381138 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts @@ -430,19 +430,14 @@ export class SymbolBuilder { } private getSymbolOfPipe(expression: BindingPipe): PipeSymbol|null { - const node = findFirstMatchingNode( - this.typeCheckBlock, {withSpan: expression.sourceSpan, filter: ts.isCallExpression}); - if (node === null || !ts.isPropertyAccessExpression(node.expression)) { + const methodAccess = findFirstMatchingNode( + this.typeCheckBlock, + {withSpan: expression.nameSpan, filter: ts.isPropertyAccessExpression}); + if (methodAccess === null) { return null; } - const methodAccess = node.expression; - // Find the node for the pipe variable from the transform property access. This will be one of - // two forms: `_pipe1.transform` or `(_pipe1 as any).transform`. - const pipeVariableNode = ts.isParenthesizedExpression(methodAccess.expression) && - ts.isAsExpression(methodAccess.expression.expression) ? - methodAccess.expression.expression.expression : - methodAccess.expression; + const pipeVariableNode = methodAccess.expression; const pipeDeclaration = this.getTypeChecker().getSymbolAtLocation(pipeVariableNode); if (pipeDeclaration === undefined || pipeDeclaration.valueDeclaration === undefined) { return null; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts index 1940aa5504a01c..ffac08364c82ce 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts @@ -1594,17 +1594,19 @@ class TcbExpressionTranslator { // Use an 'any' value to at least allow the rest of the expression to be checked. pipe = NULL_AS_ANY; - } else if (this.tcb.env.config.checkTypeOfPipes) { + } else { // Use a variable declared as the pipe's type. pipe = this.tcb.env.pipeInst(pipeRef); - } else { - // Use an 'any' value when not checking the type of the pipe. - pipe = ts.createAsExpression( - this.tcb.env.pipeInst(pipeRef), ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword)); } const args = ast.args.map(arg => this.translate(arg)); - const methodAccess = ts.createPropertyAccess(pipe, 'transform'); + let methodAccess: ts.Expression = + ts.factory.createPropertyAccessExpression(pipe, 'transform'); addParseSpanInfo(methodAccess, ast.nameSpan); + if (!this.tcb.env.config.checkTypeOfPipes) { + methodAccess = ts.factory.createAsExpression( + methodAccess, ts.factory.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword)); + } + const result = ts.createCall( /* expression */ methodAccess, /* typeArguments */ undefined, diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts index 6de3126cb0e064..b2776a84538a79 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts @@ -974,7 +974,7 @@ describe('type check blocks', () => { const DISABLED_CONFIG: TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfPipes: false}; const block = tcb(TEMPLATE, PIPES, DISABLED_CONFIG); expect(block).toContain( - '((null as TestPipe) as any).transform(((ctx).a), ((ctx).b), ((ctx).c))'); + '((null as TestPipe).transform as any)(((ctx).a), ((ctx).b), ((ctx).c))'); }); }); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts index a70c8bd09c169a..6f181afe39691a 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts @@ -749,29 +749,19 @@ runInEachFileSystem(() => { BindingPipe; } - it('should get symbol for pipe', () => { - setupPipesTest(); - const pipeSymbol = templateTypeChecker.getSymbolOfNode(binding, cmp)!; - assertPipeSymbol(pipeSymbol); - expect(program.getTypeChecker().symbolToString(pipeSymbol.tsSymbol!)) - .toEqual('transform'); - expect(program.getTypeChecker().symbolToString(pipeSymbol.classSymbol.tsSymbol)) - .toEqual('TestPipe'); - expect(program.getTypeChecker().typeToString(pipeSymbol.tsType!)) - .toEqual('(value: string, repeat: number, commaSeparate: boolean) => string[]'); - }); - - it('should get symbol for pipe, checkTypeOfPipes: false', () => { - setupPipesTest(false); - const pipeSymbol = templateTypeChecker.getSymbolOfNode(binding, cmp)! as PipeSymbol; - assertPipeSymbol(pipeSymbol); - expect(pipeSymbol.tsSymbol).toBeNull(); - expect(program.getTypeChecker().typeToString(pipeSymbol.tsType!)).toEqual('any'); - expect(program.getTypeChecker().symbolToString(pipeSymbol.classSymbol.tsSymbol)) - .toEqual('TestPipe'); - expect(program.getTypeChecker().typeToString(pipeSymbol.classSymbol.tsType)) - .toEqual('TestPipe'); - }); + for (const checkTypeOfPipes of [true, false]) { + it(`should get symbol for pipe, checkTypeOfPipes: ${checkTypeOfPipes}`, () => { + setupPipesTest(checkTypeOfPipes); + const pipeSymbol = templateTypeChecker.getSymbolOfNode(binding, cmp)!; + assertPipeSymbol(pipeSymbol); + expect(program.getTypeChecker().symbolToString(pipeSymbol.tsSymbol!)) + .toEqual('transform'); + expect(program.getTypeChecker().symbolToString(pipeSymbol.classSymbol.tsSymbol)) + .toEqual('TestPipe'); + expect(program.getTypeChecker().typeToString(pipeSymbol.tsType!)) + .toEqual('(value: string, repeat: number, commaSeparate: boolean) => string[]'); + }); + } it('should get symbols for pipe expression and args', () => { setupPipesTest(false); diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/GOLDEN_PARTIAL.js b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/GOLDEN_PARTIAL.js index 25205f27b366b2..8c42057797ee24 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/GOLDEN_PARTIAL.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/GOLDEN_PARTIAL.js @@ -70,6 +70,7 @@ HostBindingComp.ɵcmp = i0.ɵɵngDeclareComponent({ version: "0.0.0-PLACEHOLDER" }] }], null, null); })(); class MyForwardPipe { + transform() { } } MyForwardPipe.ɵfac = function MyForwardPipe_Factory(t) { return new (t || MyForwardPipe)(); }; MyForwardPipe.ɵpipe = i0.ɵɵdefinePipe({ name: "my_forward_pipe", type: MyForwardPipe, pure: true }); diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/forward_referenced_pipe.ts b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/forward_referenced_pipe.ts index aa1a1a8b76b187..c127944498eab1 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/forward_referenced_pipe.ts +++ b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/forward_referenced_pipe.ts @@ -11,6 +11,7 @@ export class HostBindingComp { @Pipe({name: 'my_forward_pipe'}) class MyForwardPipe { + transform() {} } @NgModule({declarations: [HostBindingComp, MyForwardPipe]}) diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 618467ae90814c..c39ed8c84fd298 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -671,7 +671,9 @@ runInEachFileSystem(os => { // ModuleA classes @Pipe({name: 'number'}) - class PipeA {} + class PipeA { + transform() {} + } @NgModule({ declarations: [PipeA], @@ -682,7 +684,9 @@ runInEachFileSystem(os => { // ModuleB classes @Pipe({name: 'number'}) - class PipeB {} + class PipeB { + transform() {} + } @Component({ selector: 'app', @@ -714,7 +718,9 @@ runInEachFileSystem(os => { // ModuleA classes @Pipe({name: 'number'}) - class PipeA {} + class PipeA { + transform() {} + } @NgModule({ declarations: [PipeA], @@ -725,7 +731,9 @@ runInEachFileSystem(os => { // ModuleB classes @Pipe({name: 'number'}) - class PipeB {} + class PipeB { + transform() {} + } @NgModule({ declarations: [PipeB], @@ -1550,7 +1558,9 @@ runInEachFileSystem(os => { import {Component, NgModule, Pipe} from '@angular/core'; @Pipe({name: 'test'}) - export class TestPipe {} + export class TestPipe { + transform() {} + } @Component({selector: 'test-cmp', template: '{{value | test}}'}) export class TestCmp { diff --git a/packages/language-service/ivy/test/definitions_spec.ts b/packages/language-service/ivy/test/definitions_spec.ts index 5b0cd19aaaa6ce..6b00838c0e2d8f 100644 --- a/packages/language-service/ivy/test/definitions_spec.ts +++ b/packages/language-service/ivy/test/definitions_spec.ts @@ -10,7 +10,7 @@ import {absoluteFrom, AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/file import {initMockFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing'; import {extractCursorInfo, LanguageServiceTestEnvironment} from './env'; -import {assertFileNames, createModuleWithDeclarations, humanizeDocumentSpanLike} from './test_utils'; +import {assertFileNames, assertTextSpans, createModuleWithDeclarations, humanizeDocumentSpanLike} from './test_utils'; describe('definitions', () => { it('gets definition for template reference in overridden template', () => { @@ -35,7 +35,7 @@ describe('definitions', () => { assertFileNames(Array.from(definitions!), ['app.html']); }); - it('returns the pipe class as definition when checkTypeOfPipes is false', () => { + it('returns the pipe definitions when checkTypeOfPipes is false', () => { initMockFileSystem('Native'); const {cursor, text} = extractCursorInfo('{{"1/1/2020" | dat¦e}}'); const templateFile = {contents: text, name: absoluteFrom('/app.html')}; @@ -55,10 +55,9 @@ describe('definitions', () => { getDefinitionsAndAssertBoundSpan(env, absoluteFrom('/app.html'), cursor); expect(text.substr(textSpan.start, textSpan.length)).toEqual('date'); - expect(definitions.length).toEqual(1); - const [def] = definitions; - expect(def.textSpan).toContain('DatePipe'); - expect(def.contextSpan).toContain('DatePipe'); + expect(definitions.length).toEqual(3); + assertTextSpans(definitions, ['transform']); + assertFileNames(definitions, ['index.d.ts']); }); it('gets definitions for all inputs when attribute matches more than one', () => { diff --git a/packages/language-service/ivy/test/quick_info_spec.ts b/packages/language-service/ivy/test/quick_info_spec.ts index 8a23f4b5a9e90b..a870de5d8a8cd9 100644 --- a/packages/language-service/ivy/test/quick_info_spec.ts +++ b/packages/language-service/ivy/test/quick_info_spec.ts @@ -513,8 +513,13 @@ describe('quick info', () => { // checkTypeOfPipes is set to false when strict templates is false env = LanguageServiceTestEnvironment.setup(quickInfoSkeleton(), {strictTemplates: false}); const templateOverride = `

The hero's birthday is {{birthday | da¦te: "MM/dd/yy"}}

`; - expectQuickInfo( - {templateOverride, expectedSpanText: 'date', expectedDisplayString: '(pipe) DatePipe'}); + expectQuickInfo({ + templateOverride, + expectedSpanText: 'date', + expectedDisplayString: + '(pipe) DatePipe.transform(value: string | number | Date, format?: string | undefined, timezone?: ' + + 'string | undefined, locale?: string | undefined): string | null (+2 overloads)' + }); }); }); diff --git a/packages/language-service/ivy/test/references_and_rename_spec.ts b/packages/language-service/ivy/test/references_and_rename_spec.ts index d169128dc7f918..6b4723683a039f 100644 --- a/packages/language-service/ivy/test/references_and_rename_spec.ts +++ b/packages/language-service/ivy/test/references_and_rename_spec.ts @@ -734,10 +734,11 @@ describe('find references and rename locations', () => { prefixPipeFile = {name: _('/prefix-pipe.ts'), contents: cursorInfo.text}; }); - describe('when cursor is on pipe name', () => { - let cursor: number; - beforeEach(() => { - const appContentsWithCursor = ` + for (const checkTypeOfPipes of [true, false]) { + describe(`when cursor is on pipe name, checkTypeOfPipes: ${checkTypeOfPipes}`, () => { + let cursor: number; + beforeEach(() => { + const appContentsWithCursor = ` import {Component} from '@angular/core'; @Component({template: '{{birthday | prefi¦xPipe: "MM/dd/yy"}}'}) @@ -745,26 +746,29 @@ describe('find references and rename locations', () => { birthday = ''; } `; - const cursorInfo = extractCursorInfo(appContentsWithCursor); - cursor = cursorInfo.cursor; - const appFile = {name: _('/app.ts'), contents: cursorInfo.text}; - env = createModuleWithDeclarations([appFile, prefixPipeFile]); - }); + const cursorInfo = extractCursorInfo(appContentsWithCursor); + cursor = cursorInfo.cursor; + const appFile = {name: _('/app.ts'), contents: cursorInfo.text}; + // checkTypeOfPipes is set to false when strictTemplates is false + env = + createModuleWithDeclarations([appFile, prefixPipeFile], [], {strictTemplates: false}); + }); - it('should find references', () => { - const refs = getReferencesAtPosition(_('/app.ts'), cursor)!; - expect(refs.length).toBe(5); - assertFileNames(refs, ['index.d.ts', 'prefix-pipe.ts', 'app.ts']); - assertTextSpans(refs, ['transform', 'prefixPipe']); - }); + it('should find references', () => { + const refs = getReferencesAtPosition(_('/app.ts'), cursor)!; + expect(refs.length).toBe(5); + assertFileNames(refs, ['index.d.ts', 'prefix-pipe.ts', 'app.ts']); + assertTextSpans(refs, ['transform', 'prefixPipe']); + }); - it('should find rename locations', () => { - const renameLocations = getRenameLocationsAtPosition(_('/app.ts'), cursor)!; - expect(renameLocations.length).toBe(2); - assertFileNames(renameLocations, ['prefix-pipe.ts', 'app.ts']); - assertTextSpans(renameLocations, ['prefixPipe']); + it('should find rename locations', () => { + const renameLocations = getRenameLocationsAtPosition(_('/app.ts'), cursor)!; + expect(renameLocations.length).toBe(2); + assertFileNames(renameLocations, ['prefix-pipe.ts', 'app.ts']); + assertTextSpans(renameLocations, ['prefixPipe']); + }); }); - }); + } describe('when cursor is on pipe name expression', () => { it('finds rename locations', () => { diff --git a/packages/language-service/ivy/test/type_definitions_spec.ts b/packages/language-service/ivy/test/type_definitions_spec.ts index 5822586745f44d..08bca554eb9740 100644 --- a/packages/language-service/ivy/test/type_definitions_spec.ts +++ b/packages/language-service/ivy/test/type_definitions_spec.ts @@ -10,7 +10,7 @@ import {absoluteFrom} from '@angular/compiler-cli/src/ngtsc/file_system'; import {initMockFileSystem, TestFile} from '@angular/compiler-cli/src/ngtsc/file_system/testing'; import {LanguageServiceTestEnvironment} from './env'; -import {humanizeDocumentSpanLike} from './test_utils'; +import {assertFileNames, assertTextSpans, humanizeDocumentSpanLike} from './test_utils'; describe('type definitions', () => { let env: LanguageServiceTestEnvironment; @@ -41,11 +41,10 @@ describe('type definitions', () => { env = LanguageServiceTestEnvironment.setup(testFiles, {strictTemplates: false}); const definitions = getTypeDefinitionsAndAssertBoundSpan({templateOverride: '{{"1/1/2020" | dat¦e}}'}); - expect(definitions!.length).toEqual(1); + expect(definitions!.length).toEqual(3); - const [def] = definitions; - expect(def.textSpan).toContain('DatePipe'); - expect(def.contextSpan).toContain('DatePipe'); + assertTextSpans(definitions, ['transform']); + assertFileNames(definitions, ['index.d.ts']); }); function getTypeDefinitionsAndAssertBoundSpan({templateOverride}: {templateOverride: string}) {