From 9f6605d11b7ee75f289b5a2ed69e201d65b038d8 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 23 Jan 2024 11:28:23 +0000 Subject: [PATCH] fix(compiler-cli): support jumping to definitions of signal-based inputs (#54233) This fixes the definitions for signal-based inputs in the language service and type checking symbol builder. Signal inputs emit a slightly different output. The output works well for comppletion and was designed to affect language service minimally. Turns out there is a small adjustment needed for the definition symbols. PR Close #54233 --- .../typecheck/src/template_symbol_builder.ts | 69 +++++++++++++++--- ...ecker__get_symbol_of_template_node_spec.ts | 63 ++++++++++++++++ packages/language-service/src/definitions.ts | 3 + .../language-service/test/definitions_spec.ts | 73 +++++++++++++++---- 4 files changed, 184 insertions(+), 24 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 df99e1e2ea2aa..47299f2d56e17 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 @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {AST, ASTWithSource, BindingPipe, Call, ParseSourceSpan, PropertyRead, PropertyWrite, SafePropertyRead, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable} from '@angular/compiler'; +import {AST, ASTWithSource, BindingPipe, ParseSourceSpan, PropertyRead, PropertyWrite, R3Identifiers, SafePropertyRead, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable} from '@angular/compiler'; import ts from 'typescript'; import {AbsoluteFsPath} from '../../file_system'; @@ -307,7 +307,6 @@ export class SymbolBuilder { continue; } - const target = this.getDirectiveSymbolForAccessExpression(outputFieldAccess, consumer); if (target === null) { continue; @@ -355,12 +354,33 @@ export class SymbolBuilder { continue; } - const symbolInfo = this.getSymbolOfTsNode(node.left); + const signalInputAssignment = unwrapSignalInputWriteTAccessor(node.left); + let symbolInfo: TsNodeSymbolInfo|null = null; + + // Signal inputs need special treatment because they are generated with an extra keyed + // access. E.g. `_t1.prop[WriteT_ACCESSOR_SYMBOL]`. Observations: + // - The keyed access for the write type needs to be resolved for the "input type". + // - The definition symbol of the input should be the input class member, and not the + // internal write accessor. Symbol should resolve `_t1.prop`. + if (signalInputAssignment !== null) { + const fieldSymbol = this.getSymbolOfTsNode(signalInputAssignment.fieldExpr); + const typeSymbol = this.getSymbolOfTsNode(signalInputAssignment.typeExpr); + + symbolInfo = fieldSymbol === null || typeSymbol === null ? null : { + tcbLocation: fieldSymbol.tcbLocation, + tsSymbol: fieldSymbol.tsSymbol, + tsType: typeSymbol.tsType, + }; + } else { + symbolInfo = this.getSymbolOfTsNode(node.left); + } + if (symbolInfo === null || symbolInfo.tsSymbol === null) { continue; } - const target = this.getDirectiveSymbolForAccessExpression(node.left, consumer); + const target = this.getDirectiveSymbolForAccessExpression( + signalInputAssignment?.fieldExpr ?? node.left, consumer); if (target === null) { continue; } @@ -379,11 +399,10 @@ export class SymbolBuilder { } private getDirectiveSymbolForAccessExpression( - node: ts.ElementAccessExpression|ts.PropertyAccessExpression, + fieldAccessExpr: ts.ElementAccessExpression|ts.PropertyAccessExpression, {isComponent, selector, isStructural}: TypeCheckableDirectiveMeta): DirectiveSymbol|null { - // In either case, `_t1["index"]` or `_t1.index`, `node.expression` is _t1. - // The retrieved symbol for _t1 will be the variable declaration. - const tsSymbol = this.getTypeChecker().getSymbolAtLocation(node.expression); + // In all cases, `_t1["index"]` or `_t1.index`, `node.expression` is _t1. + const tsSymbol = this.getTypeChecker().getSymbolAtLocation(fieldAccessExpr.expression); if (tsSymbol?.declarations === undefined || tsSymbol.declarations.length === 0 || selector === null) { return null; @@ -621,8 +640,6 @@ export class SymbolBuilder { let tsSymbol: ts.Symbol|undefined; if (ts.isPropertyAccessExpression(node)) { tsSymbol = this.getTypeChecker().getSymbolAtLocation(node.name); - } else if (ts.isElementAccessExpression(node)) { - tsSymbol = this.getTypeChecker().getSymbolAtLocation(node.argumentExpression); } else { tsSymbol = this.getTypeChecker().getSymbolAtLocation(node); } @@ -666,3 +683,35 @@ function anyNodeFilter(n: ts.Node): n is ts.Node { function sourceSpanEqual(a: ParseSourceSpan, b: ParseSourceSpan) { return a.start.offset === b.start.offset && a.end.offset === b.end.offset; } + +function unwrapSignalInputWriteTAccessor(expr: ts.LeftHandSideExpression): (null|{ + fieldExpr: ts.ElementAccessExpression | ts.PropertyAccessExpression, + typeExpr: ts.ElementAccessExpression +}) { + // e.g. `_t2.inputA[i2.ɵINPUT_SIGNAL_BRAND_WRITE_TYPE]` + // 1. Assert that we are dealing with an element access expression. + // 2. Assert that we are dealing with a signal brand symbol access in the argument expression. + if (!ts.isElementAccessExpression(expr) || + !ts.isPropertyAccessExpression(expr.argumentExpression)) { + return null; + } + + // Assert that the property access in the element access is a simple identifier and + // refers to `ɵINPUT_SIGNAL_BRAND_WRITE_TYPE`. + if (!ts.isIdentifier(expr.argumentExpression.name) || + expr.argumentExpression.name.text !== R3Identifiers.InputSignalBrandWriteType.name) { + return null; + } + + // Assert that the `_t2.inputA` is actually either a keyed element access, or + // property access expression. This is checked for type safety and to catch unexpected cases. + if (!ts.isPropertyAccessExpression(expr.expression) && + !ts.isElementAccessExpression(expr.expression)) { + throw new Error('Unexpected expression for signal input write type.'); + } + + return { + fieldExpr: expr.expression, + typeExpr: expr, + }; +} 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 59a8b9941cdce..58b0c6b2779fb 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 @@ -1135,6 +1135,69 @@ runInEachFileSystem(() => { .toEqual('inputB'); }); + it('can retrieve a symbol for a signal-input binding', () => { + const fileName = absoluteFrom('/main.ts'); + const dirFile = absoluteFrom('/dir.ts'); + const templateString = `
`; + const {program, templateTypeChecker} = setup([ + { + fileName, + templates: {'Cmp': templateString}, + declarations: [{ + name: 'TestDir', + selector: '[dir]', + file: dirFile, + type: 'directive', + inputs: { + inputA: { + bindingPropertyName: 'inputA', + isSignal: true, + classPropertyName: 'inputA', + required: false, + transform: null, + }, + inputB: { + bindingPropertyName: 'aliased', + isSignal: true, + classPropertyName: 'inputB', + required: true, + transform: null, + } + }, + }] + }, + { + fileName: dirFile, + source: ` + import {InputSignal} from '@angular/core'; + + export class TestDir { + inputA: InputSignal = null!; + inputB: InputSignal = null!; + }`, + templates: {}, + } + ]); + const sf = getSourceFileOrError(program, fileName); + const cmp = getClass(sf, 'Cmp'); + + const nodes = templateTypeChecker.getTemplate(cmp)!; + + const inputAbinding = (nodes[0] as TmplAstElement).inputs[0]; + const aSymbol = templateTypeChecker.getSymbolOfNode(inputAbinding, cmp)!; + assertInputBindingSymbol(aSymbol); + expect((aSymbol.bindings[0].tsSymbol!.declarations![0] as ts.PropertyDeclaration) + .name.getText()) + .toEqual('inputA'); + + const inputBbinding = (nodes[0] as TmplAstElement).inputs[1]; + const bSymbol = templateTypeChecker.getSymbolOfNode(inputBbinding, cmp)!; + assertInputBindingSymbol(bSymbol); + expect((bSymbol.bindings[0].tsSymbol!.declarations![0] as ts.PropertyDeclaration) + .name.getText()) + .toEqual('inputB'); + }); + it('does not retrieve a symbol for an input when undeclared', () => { const fileName = absoluteFrom('/main.ts'); const dirFile = absoluteFrom('/dir.ts'); diff --git a/packages/language-service/src/definitions.ts b/packages/language-service/src/definitions.ts index d4cc56ae21348..b38d66c184d68 100644 --- a/packages/language-service/src/definitions.ts +++ b/packages/language-service/src/definitions.ts @@ -48,7 +48,9 @@ export class DefinitionBuilder { } return getDefinitionForExpressionAtPosition(fileName, position, this.compiler); } + const definitionMetas = this.getDefinitionMetaAtPosition(templateInfo, position); + if (definitionMetas === undefined) { return undefined; } @@ -66,6 +68,7 @@ export class DefinitionBuilder { ...(this.getDefinitionsForSymbol({...definitionMeta, ...templateInfo}) ?? [])); } + if (definitions.length === 0) { return undefined; } diff --git a/packages/language-service/test/definitions_spec.ts b/packages/language-service/test/definitions_spec.ts index 89cf58b4cdd3c..863732a984568 100644 --- a/packages/language-service/test/definitions_spec.ts +++ b/packages/language-service/test/definitions_spec.ts @@ -17,7 +17,7 @@ describe('definitions', () => { 'app.html': '', 'app.ts': ` import {Component} from '@angular/core'; - + @Component({templateUrl: '/app.html'}) export class AppCmp {} `, @@ -41,7 +41,7 @@ describe('definitions', () => { 'app.ts': ` import {Component, NgModule} from '@angular/core'; import {CommonModule} from '@angular/common'; - + @Component({templateUrl: 'app.html'}) export class AppCmp {} `, @@ -69,21 +69,21 @@ describe('definitions', () => { 'app.ts': ` import {Component, NgModule} from '@angular/core'; import {CommonModule} from '@angular/common'; - + @Component({templateUrl: 'app.html'}) export class AppCmp {} `, 'app.html': '
', 'dir.ts': ` import {Directive, Input} from '@angular/core'; - + @Directive({selector: '[dir]'}) export class MyDir { @Input() inputA!: any; }`, 'dir2.ts': ` import {Directive, Input} from '@angular/core'; - + @Directive({selector: '[dir]'}) export class MyDir2 { @Input() inputA!: any; @@ -108,20 +108,65 @@ describe('definitions', () => { assertFileNames([def, def2], ['dir2.ts', 'dir.ts']); }); + it('gets definitions for all signal-inputs when attribute matches more than one', () => { + initMockFileSystem('Native'); + const files = { + 'app.ts': ` + import {Component, NgModule} from '@angular/core'; + import {CommonModule} from '@angular/common'; + + @Component({templateUrl: 'app.html'}) + export class AppCmp {} + `, + 'app.html': '
', + 'dir.ts': ` + import {Directive, input} from '@angular/core'; + + @Directive({selector: '[dir]'}) + export class MyDir { + inputA = input(); + }`, + 'dir2.ts': ` + import {Directive, input} from '@angular/core'; + + @Directive({selector: '[dir]'}) + export class MyDir2 { + inputA = input(); + }` + + }; + const env = LanguageServiceTestEnv.setup(); + const project = createModuleAndProjectWithDeclarations(env, 'test', files); + const template = project.openFile('app.html'); + template.moveCursorToText('inpu¦tA="abc"'); + + const {textSpan, definitions} = getDefinitionsAndAssertBoundSpan(env, template); + expect(template.contents.slice(textSpan.start, 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']); + }); + it('gets definitions for all outputs when attribute matches more than one', () => { initMockFileSystem('Native'); const files = { 'app.html': '
', 'dir.ts': ` import {Directive, Output, EventEmitter} from '@angular/core'; - + @Directive({selector: '[dir]'}) export class MyDir { @Output() someEvent = new EventEmitter(); }`, 'dir2.ts': ` import {Directive, Output, EventEmitter} from '@angular/core'; - + @Directive({selector: '[dir]'}) export class MyDir2 { @Output() someEvent = new EventEmitter(); @@ -129,7 +174,7 @@ describe('definitions', () => { 'app.ts': ` import {Component, NgModule} from '@angular/core'; import {CommonModule} from '@angular/common'; - + @Component({templateUrl: 'app.html'}) export class AppCmp { doSomething() {} @@ -159,7 +204,7 @@ describe('definitions', () => { const files = { 'app.ts': ` import {Component} from '@angular/core'; - + @Component({ template: '', styleUrls: ['./style.css'], @@ -190,7 +235,7 @@ describe('definitions', () => { `, 'app.ts': ` import {Component} from '@angular/core'; - + @Component({templateUrl: '/app.html'}) export class AppCmp { myVal = {name: 'Andrew'}; @@ -230,12 +275,12 @@ describe('definitions', () => { export class DollarCmp { @Input() obs$!: string; } - + @Component({template: ''}) export class AppCmp { greeting = 'hello'; } - + @NgModule({declarations: [AppCmp, DollarCmp]}) export class AppModule {} `, @@ -277,12 +322,12 @@ describe('definitions', () => { export class DollarDir { @Input() dollar$!: string; } - + @Component({template: '
'}) export class AppCmp { greeting = 'hello'; } - + @NgModule({declarations: [AppCmp, DollarDir]}) export class AppModule {} `,