From da2be4b71061a376bb5eeac0b92643d2b6dae4b7 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Tue, 15 Dec 2020 15:55:54 -0800 Subject: [PATCH] refactor(compiler-cli): Return symbols for all matching inputs (#40144) This commit ensures that the template type checker returns symbols for all inputs if an attribute binds to more than one. PR Close #40144 --- .../typecheck/src/template_symbol_builder.ts | 42 ++++---- ...ecker__get_symbol_of_template_node_spec.ts | 14 +-- .../ivy/test/definitions_spec.ts | 102 +++++++++++------- .../ivy/test/references_spec.ts | 17 +-- .../language-service/ivy/test/test_utils.ts | 11 ++ 5 files changed, 108 insertions(+), 78 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 0342b11066..9dc4a15c1e 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 @@ -13,7 +13,7 @@ import {AbsoluteFsPath} from '../../file_system'; import {ClassDeclaration} from '../../reflection'; import {ComponentScopeReader} from '../../scope'; import {isAssignment} from '../../util/src/typescript'; -import {DirectiveSymbol, DomBindingSymbol, ElementSymbol, ExpressionSymbol, InputBindingSymbol, OutputBindingSymbol, PipeSymbol, ReferenceSymbol, ShimLocation, Symbol, SymbolKind, TemplateSymbol, TsNodeSymbolInfo, TypeCheckableDirectiveMeta, VariableSymbol} from '../api'; +import {BindingSymbol, DirectiveSymbol, DomBindingSymbol, ElementSymbol, ExpressionSymbol, InputBindingSymbol, OutputBindingSymbol, PipeSymbol, ReferenceSymbol, ShimLocation, Symbol, SymbolKind, TemplateSymbol, TsNodeSymbolInfo, TypeCheckableDirectiveMeta, VariableSymbol} from '../api'; import {ExpressionIdentifier, findAllMatchingNodes, findFirstMatchingNode, hasExpressionIdentifier} from './comments'; import {TemplateData} from './context'; @@ -253,31 +253,35 @@ export class SymbolBuilder { return host !== null ? {kind: SymbolKind.DomBinding, host} : null; } - const node = findFirstMatchingNode( + const nodes = findAllMatchingNodes( this.typeCheckBlock, {withSpan: binding.sourceSpan, filter: isAssignment}); - if (node === null || !isAccessExpression(node.left)) { - return null; - } + const bindings: BindingSymbol[] = []; + for (const node of nodes) { + if (!isAccessExpression(node.left)) { + continue; + } - const symbolInfo = this.getSymbolOfTsNode(node.left); - if (symbolInfo === null || symbolInfo.tsSymbol === null) { - return null; - } + const symbolInfo = this.getSymbolOfTsNode(node.left); + if (symbolInfo === null || symbolInfo.tsSymbol === null) { + continue; + } - const target = this.getDirectiveSymbolForAccessExpression(node.left, consumer); - if (target === null) { - return null; - } - - return { - kind: SymbolKind.Input, - bindings: [{ + const target = this.getDirectiveSymbolForAccessExpression(node.left, consumer); + if (target === null) { + continue; + } + bindings.push({ ...symbolInfo, tsSymbol: symbolInfo.tsSymbol, kind: SymbolKind.Binding, target, - }], - }; + }); + } + if (bindings.length === 0) { + return null; + } + + return {kind: SymbolKind.Input, bindings}; } private getDirectiveSymbolForAccessExpression( 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 6d64f68774..850b3494b9 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 @@ -1136,7 +1136,7 @@ runInEachFileSystem(() => { .toEqual('TestDir'); }); - it('returns the first directive match when two directives have the same input', () => { + it('returns the all inputs when two directives have the same input', () => { const fileName = absoluteFrom('/main.ts'); const dirFile = absoluteFrom('/dir.ts'); const templateString = `
`; @@ -1178,12 +1178,12 @@ runInEachFileSystem(() => { const inputAbinding = (nodes[0] as TmplAstElement).inputs[0]; const symbol = templateTypeChecker.getSymbolOfNode(inputAbinding, cmp)!; assertInputBindingSymbol(symbol); - expect( - (symbol.bindings[0].tsSymbol!.declarations[0] as ts.PropertyDeclaration).name.getText()) - .toEqual('inputA'); - expect((symbol.bindings[0].tsSymbol!.declarations[0] as ts.PropertyDeclaration) - .parent.name?.text) - .toEqual('TestDir'); + expect(new Set(symbol.bindings.map( + b => (b.tsSymbol!.declarations[0] as ts.PropertyDeclaration).name.getText()))) + .toEqual(new Set(['inputA', 'otherDirInputA'])); + expect(new Set(symbol.bindings.map( + b => (b.tsSymbol!.declarations[0] as ts.PropertyDeclaration).parent.name?.text))) + .toEqual(new Set(['TestDir', 'OtherDir'])); }); }); diff --git a/packages/language-service/ivy/test/definitions_spec.ts b/packages/language-service/ivy/test/definitions_spec.ts index 80eadd2bca..9700e16124 100644 --- a/packages/language-service/ivy/test/definitions_spec.ts +++ b/packages/language-service/ivy/test/definitions_spec.ts @@ -6,63 +6,93 @@ * found in the LICENSE file at https://angular.io/license */ -import {absoluteFrom} from '@angular/compiler-cli/src/ngtsc/file_system'; -import {initMockFileSystem, TestFile} from '@angular/compiler-cli/src/ngtsc/file_system/testing'; +import {absoluteFrom, AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/file_system'; +import {initMockFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing'; -import {LanguageServiceTestEnvironment} from './env'; -import {humanizeDefinitionInfo} from './test_utils'; +import {extractCursorInfo, LanguageServiceTestEnvironment} from './env'; +import {assertFileNames, createModuleWithDeclarations, humanizeDefinitionInfo} from './test_utils'; describe('definitions', () => { - let env: LanguageServiceTestEnvironment; - it('returns the pipe class as definition when checkTypeOfPipes is false', () => { initMockFileSystem('Native'); - const testFiles: TestFile[] = [ - { - name: absoluteFrom('/app.ts'), - contents: ` + const {cursor, text} = extractCursorInfo('{{"1/1/2020" | dat¦e}}'); + const templateFile = {contents: text, name: absoluteFrom('/app.html')}; + const appFile = { + name: absoluteFrom('/app.ts'), + contents: ` import {Component, NgModule} from '@angular/core'; import {CommonModule} from '@angular/common'; @Component({templateUrl: 'app.html'}) export class AppCmp {} - - @NgModule({declarations: [AppCmp], imports: [CommonModule]}) - export class AppModule {} `, - isRoot: true - }, - { - name: absoluteFrom('/app.html'), - contents: `Will be overridden`, - } - ]; + }; + const env = createModuleWithDeclarations([appFile], [templateFile]); // checkTypeOfPipes is set to false when strict templates is false - env = LanguageServiceTestEnvironment.setup(testFiles, {strictTemplates: false}); - const definitions = getDefinitionsAndAssertBoundSpan( - {templateOverride: '{{"1/1/2020" | dat¦e}}', expectedSpanText: 'date'}); - expect(definitions!.length).toEqual(1); + const {textSpan, 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'); }); + it('gets definitions for all inputs when attribute matches more than one', () => { + initMockFileSystem('Native'); + const {cursor, text} = extractCursorInfo('
'); + const templateFile = {contents: text, name: absoluteFrom('/app.html')}; + const dirFile = { + name: absoluteFrom('/dir.ts'), + contents: ` + import {Directive, Input} from '@angular/core'; + + @Directive({selector: '[dir]'}) + export class MyDir { + @Input() inputA!: any; + }`, + }; + const dirFile2 = { + name: absoluteFrom('/dir2.ts'), + contents: ` + import {Directive, Input} from '@angular/core'; + + @Directive({selector: '[dir]'}) + export class MyDir2 { + @Input() inputA!: any; + }`, + }; + const appFile = { + name: absoluteFrom('/app.ts'), + contents: ` + import {Component, NgModule} from '@angular/core'; + import {CommonModule} from '@angular/common'; + + @Component({templateUrl: 'app.html'}) + export class AppCmp {} + ` + }; + const env = createModuleWithDeclarations([appFile, dirFile, dirFile2], [templateFile]); + const {textSpan, definitions} = + getDefinitionsAndAssertBoundSpan(env, absoluteFrom('/app.html'), cursor); + expect(text.substr(textSpan.start, textSpan.length)).toEqual('inputA'); + + expect(definitions.length).toEqual(2); + const [def, def2] = definitions; + expect(def.textSpan).toContain('inputA'); + expect(def2.textSpan).toContain('inputA'); + // TODO(atscott): investigate why the text span includes more than just 'inputA' + // assertTextSpans([def, def2], ['inputA']); + assertFileNames([def, def2], ['dir2.ts', 'dir.ts']); + }); + function getDefinitionsAndAssertBoundSpan( - {templateOverride, expectedSpanText}: {templateOverride: string, expectedSpanText: string}): - Array<{textSpan: string, contextSpan: string | undefined, fileName: string}> { - const {cursor, text} = - env.overrideTemplateWithCursor(absoluteFrom('/app.ts'), 'AppCmp', templateOverride); + env: LanguageServiceTestEnvironment, fileName: AbsoluteFsPath, cursor: number) { env.expectNoSourceDiagnostics(); - env.expectNoTemplateDiagnostics(absoluteFrom('/app.ts'), 'AppCmp'); - const definitionAndBoundSpan = - env.ngLS.getDefinitionAndBoundSpan(absoluteFrom('/app.html'), cursor); + const definitionAndBoundSpan = env.ngLS.getDefinitionAndBoundSpan(fileName, cursor); const {textSpan, definitions} = definitionAndBoundSpan!; - expect(text.substring(textSpan.start, textSpan.start + textSpan.length)) - .toEqual(expectedSpanText); expect(definitions).toBeTruthy(); - const overrides = new Map(); - overrides.set(absoluteFrom('/app.ts'), text); - return definitions!.map(d => humanizeDefinitionInfo(d, env.host, overrides)); + return {textSpan, definitions: definitions!.map(d => humanizeDefinitionInfo(d, env.host))}; } }); diff --git a/packages/language-service/ivy/test/references_spec.ts b/packages/language-service/ivy/test/references_spec.ts index fa21130919..2908260168 100644 --- a/packages/language-service/ivy/test/references_spec.ts +++ b/packages/language-service/ivy/test/references_spec.ts @@ -11,7 +11,7 @@ import {initMockFileSystem, TestFile} from '@angular/compiler-cli/src/ngtsc/file import * as ts from 'typescript/lib/tsserverlibrary'; import {extractCursorInfo, LanguageServiceTestEnvironment} from './env'; -import {createModuleWithDeclarations, getText} from './test_utils'; +import {assertFileNames, assertTextSpans, createModuleWithDeclarations, getText} from './test_utils'; describe('find references', () => { let env: LanguageServiceTestEnvironment; @@ -832,21 +832,6 @@ describe('find references', () => { } }); -function assertFileNames(refs: Array<{fileName: string}>, expectedFileNames: string[]) { - const actualPaths = refs.map(r => r.fileName); - const actualFileNames = actualPaths.map(p => last(p.split('/'))); - expect(new Set(actualFileNames)).toEqual(new Set(expectedFileNames)); -} - -function assertTextSpans(refs: Array<{textSpan: string}>, expectedTextSpans: string[]) { - const actualSpans = refs.map(ref => ref.textSpan); - expect(new Set(actualSpans)).toEqual(new Set(expectedTextSpans)); -} - -function last(array: T[]): T { - return array[array.length - 1]; -} - type Stringy = { [P in keyof T]: string; }; diff --git a/packages/language-service/ivy/test/test_utils.ts b/packages/language-service/ivy/test/test_utils.ts index 6c414ed064..371fbd2bbb 100644 --- a/packages/language-service/ivy/test/test_utils.ts +++ b/packages/language-service/ivy/test/test_utils.ts @@ -75,3 +75,14 @@ export function humanizeDefinitionInfo( undefined, }; } + +export function assertFileNames(refs: Array<{fileName: string}>, expectedFileNames: string[]) { + const actualPaths = refs.map(r => r.fileName); + const actualFileNames = actualPaths.map(p => last(p.split('/'))); + expect(new Set(actualFileNames)).toEqual(new Set(expectedFileNames)); +} + +export function assertTextSpans(items: Array<{textSpan: string}>, expectedTextSpans: string[]) { + const actualSpans = items.map(item => item.textSpan); + expect(new Set(actualSpans)).toEqual(new Set(expectedTextSpans)); +} \ No newline at end of file