From 47784dab35cf0974be5ea6201087d2d4ba0aacf7 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 | 3 +- ...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 | 50 ++++++++++--------- .../ivy/test/type_definitions_spec.ts | 9 ++-- 11 files changed, 87 insertions(+), 82 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 cd7bc32c27b075..904970050205de 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 cd62b16c39f331..47ad813ae3fcd3 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 @@ -1712,17 +1712,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 b15735bb9060ea..08fc36e09b9ec6 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 @@ -992,7 +992,8 @@ describe('type check blocks', () => { const DISABLED_CONFIG: TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfPipes: false}; const block = tcb(TEMPLATE, PIPES, DISABLED_CONFIG); expect(block).toContain('var _pipe1: i0.TestPipe = null!;'); - expect(block).toContain('((_pipe1 as any).transform(((ctx).a), ((ctx).b), ((ctx).c)));'); + expect(block).toContain( + '((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 f08ad4b72e6add..9fec2bbda4f453 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 @@ -722,29 +722,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 ec7381e4d5b188..7c28109ba7bf32 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 @@ -71,6 +71,7 @@ i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDE }] }] }); class MyForwardPipe { + transform() { } } MyForwardPipe.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyForwardPipe, deps: [], target: i0.ɵɵFactoryTarget.Pipe }); MyForwardPipe.ɵpipe = i0.ɵɵngDeclarePipe({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyForwardPipe, name: "my_forward_pipe" }); 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 f98377ea0f8e6f..ea26ae2de416d5 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -757,7 +757,9 @@ function allTests(os: string) { // ModuleA classes @Pipe({name: 'number'}) - class PipeA {} + class PipeA { + transform() {} + } @NgModule({ declarations: [PipeA], @@ -768,7 +770,9 @@ function allTests(os: string) { // ModuleB classes @Pipe({name: 'number'}) - class PipeB {} + class PipeB { + transform() {} + } @Component({ selector: 'app', @@ -800,7 +804,9 @@ function allTests(os: string) { // ModuleA classes @Pipe({name: 'number'}) - class PipeA {} + class PipeA { + transform() {} + } @NgModule({ declarations: [PipeA], @@ -811,7 +817,9 @@ function allTests(os: string) { // ModuleB classes @Pipe({name: 'number'}) - class PipeB {} + class PipeB { + transform() {} + } @NgModule({ declarations: [PipeB], @@ -1647,7 +1655,9 @@ function allTests(os: string) { 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 818c42ac4284b2..b1ed3ef3fdc2c4 100644 --- a/packages/language-service/ivy/test/definitions_spec.ts +++ b/packages/language-service/ivy/test/definitions_spec.ts @@ -8,7 +8,7 @@ import {initMockFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing'; -import {assertFileNames, createModuleAndProjectWithDeclarations, humanizeDocumentSpanLike, LanguageServiceTestEnv, OpenBuffer} from '../testing'; +import {assertFileNames, assertTextSpans, createModuleAndProjectWithDeclarations, humanizeDocumentSpanLike, LanguageServiceTestEnv, OpenBuffer} from '../testing'; 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 files = { 'app.ts': ` @@ -57,10 +57,9 @@ describe('definitions', () => { const {textSpan, definitions} = getDefinitionsAndAssertBoundSpan(env, template); expect(template.contents.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 1160c80000e41f..c8a1f0b9a76ecc 100644 --- a/packages/language-service/ivy/test/quick_info_spec.ts +++ b/packages/language-service/ivy/test/quick_info_spec.ts @@ -559,8 +559,13 @@ describe('quick info', () => { // checkTypeOfPipes is set to false when strict templates is false project = env.addProject('test', 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)' + }); }); it('should still get quick info if there is an invalid css resource', () => { 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 0d57ee37a1c9c6..642c9e701c5c5e 100644 --- a/packages/language-service/ivy/test/references_and_rename_spec.ts +++ b/packages/language-service/ivy/test/references_and_rename_spec.ts @@ -754,11 +754,12 @@ describe('find references and rename locations', () => { } }`; - describe('when cursor is on pipe name', () => { - let file: OpenBuffer; - beforeEach(() => { - const files = { - 'app.ts': ` + for (const checkTypeOfPipes of [true, false]) { + describe(`when cursor is on pipe name, checkTypeOfPipes: ${checkTypeOfPipes}`, () => { + let file: OpenBuffer; + beforeEach(() => { + const files = { + 'app.ts': ` import {Component} from '@angular/core'; @Component({template: '{{birthday | prefixPipe: "MM/dd/yy"}}'}) @@ -766,29 +767,30 @@ describe('find references and rename locations', () => { birthday = ''; } `, - 'prefix-pipe.ts': prefixPipe - }; + 'prefix-pipe.ts': prefixPipe + }; - env = LanguageServiceTestEnv.setup(); - const project = createModuleAndProjectWithDeclarations(env, 'test', files); - file = project.openFile('app.ts'); - file.moveCursorToText('prefi¦xPipe:'); - }); + env = LanguageServiceTestEnv.setup(); + const project = createModuleAndProjectWithDeclarations(env, 'test', files); + file = project.openFile('app.ts'); + file.moveCursorToText('prefi¦xPipe:'); + }); - it('should find references', () => { - const refs = getReferencesAtPosition(file)!; - 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(file)!; + 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(file)!; - expect(renameLocations.length).toBe(2); - assertFileNames(renameLocations, ['prefix-pipe.ts', 'app.ts']); - assertTextSpans(renameLocations, ['prefixPipe']); + it('should find rename locations', () => { + const renameLocations = getRenameLocationsAtPosition(file)!; + 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 ebe6ff11fac213..a20c0d30d20dd7 100644 --- a/packages/language-service/ivy/test/type_definitions_spec.ts +++ b/packages/language-service/ivy/test/type_definitions_spec.ts @@ -8,7 +8,7 @@ import {initMockFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing'; -import {humanizeDocumentSpanLike, LanguageServiceTestEnv, Project} from '../testing'; +import {assertFileNames, assertTextSpans, humanizeDocumentSpanLike, LanguageServiceTestEnv, Project} from '../testing'; describe('type definitions', () => { let env: LanguageServiceTestEnv; @@ -33,11 +33,10 @@ describe('type definitions', () => { const project = env.addProject('test', files, {strictTemplates: false}); const definitions = getTypeDefinitionsAndAssertBoundSpan(project, {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(