From 459af57a313b4579a03f51865109ad0f5ccdeb16 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 (#40523) 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. PR Close #40523 --- .../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 + .../incremental_semantic_changes_spec.ts | 8 ++- .../test/ngtsc/incremental_spec.ts | 16 +++-- .../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 | 60 ++++++++++--------- .../ivy/test/ts_utils_spec.ts | 12 +++- .../ivy/test/type_definitions_spec.ts | 9 ++- 14 files changed, 119 insertions(+), 95 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 cd7bc32c27..9049700502 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 cd62b16c39..47ad813ae3 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 b15735bb90..ce6ba8fd1b 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,7 @@ 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('((_pipe1.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 f08ad4b72e..9fec2bbda4 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 ec7381e4d5..7c28109ba7 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 aa1a1a8b76..c127944498 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/incremental_semantic_changes_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_semantic_changes_spec.ts index c6b466c5b3..42db9c4c66 100644 --- a/packages/compiler-cli/test/ngtsc/incremental_semantic_changes_spec.ts +++ b/packages/compiler-cli/test/ngtsc/incremental_semantic_changes_spec.ts @@ -1361,7 +1361,9 @@ runInEachFileSystem(() => { @Pipe({ name: 'dep', }) - export class DepB {} + export class DepB { + transform() {} + } `); env.write('module.ts', ` import {NgModule} from '@angular/core'; @@ -1385,7 +1387,9 @@ runInEachFileSystem(() => { @Pipe({ name: 'dep', }) - export class DepA {} + export class DepA { + transform() {} + } @Directive({ selector: 'dep', diff --git a/packages/compiler-cli/test/ngtsc/incremental_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_spec.ts index 9f6c4f56e1..75780f19a8 100644 --- a/packages/compiler-cli/test/ngtsc/incremental_spec.ts +++ b/packages/compiler-cli/test/ngtsc/incremental_spec.ts @@ -200,7 +200,9 @@ runInEachFileSystem(() => { import {Pipe} from '@angular/core'; @Pipe({name: 'myPipe'}) - export class MyPipe {} + export class MyPipe { + transform() {} + } `); env.write('module.ts', ` import {NgModule, NO_ERRORS_SCHEMA} from '@angular/core'; @@ -238,7 +240,9 @@ runInEachFileSystem(() => { import {Pipe} from '@angular/core'; @Pipe({name: 'foo_changed'}) - export class FooPipe {} + export class FooPipe { + transform() {} + } `); env.driveMain(); const written = env.getFilesWrittenSinceLastFlush(); @@ -910,7 +914,9 @@ runInEachFileSystem(() => { import {Pipe} from '@angular/core'; @Pipe({name: 'foo'}) - export class FooPipe {} + export class FooPipe { + transform() {} + } `); env.write('foo_module.ts', ` import {NgModule} from '@angular/core'; @@ -940,7 +946,9 @@ runInEachFileSystem(() => { import {Pipe} from '@angular/core'; @Pipe({name: 'foo'}) - export class BarPipe {} + export class BarPipe { + transform() {} + } `); env.write('bar_module.ts', ` import {NgModule} from '@angular/core'; diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index f98377ea0f..ea26ae2de4 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 818c42ac42..b1ed3ef3fd 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 1160c80000..c8a1f0b9a7 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 45bdf5f192..c12b9142cd 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,35 +767,36 @@ 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']); + }); - it('should get rename info', () => { - const result = file.getRenameInfo() as ts.RenameInfoSuccess; - expect(result.canRename).toEqual(true); - expect(result.displayName).toEqual('prefixPipe'); + it('should get rename info', () => { + const result = file.getRenameInfo() as ts.RenameInfoSuccess; + expect(result.canRename).toEqual(true); + expect(result.displayName).toEqual('prefixPipe'); + }); }); - }); + } describe('when cursor is on pipe name expression', () => { it('finds rename locations and rename info', () => { diff --git a/packages/language-service/ivy/test/ts_utils_spec.ts b/packages/language-service/ivy/test/ts_utils_spec.ts index 657516f8e4..6922a94497 100644 --- a/packages/language-service/ivy/test/ts_utils_spec.ts +++ b/packages/language-service/ivy/test/ts_utils_spec.ts @@ -1,3 +1,11 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + import {initMockFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing'; import * as ts from 'typescript'; import {LanguageServiceTestEnv, OpenBuffer, Project} from '../testing'; @@ -7,7 +15,7 @@ describe('ts utils', () => { describe('collectMemberMethods', () => { beforeEach(() => { initMockFileSystem('Native'); - }) + }); it('gets only methods in class, not getters, setters, or properties', () => { const files = { @@ -69,7 +77,7 @@ describe('ts utils', () => { function getMemberMethodNames(project: Project, file: OpenBuffer): string[] { const sf = project.getSourceFile('app.ts')!; const node = findTightestNode(sf, file.cursor)!; - expect(ts.isClassDeclaration(node.parent)).toBe(true) + expect(ts.isClassDeclaration(node.parent)).toBe(true); return collectMemberMethods(node.parent as ts.ClassDeclaration, project.getTypeChecker()) .map(m => m.name.getText()) .sort(); diff --git a/packages/language-service/ivy/test/type_definitions_spec.ts b/packages/language-service/ivy/test/type_definitions_spec.ts index ebe6ff11fa..a20c0d30d2 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(