Skip to content

Commit

Permalink
fix(compiler-cli): support jumping to definitions of signal-based inp…
Browse files Browse the repository at this point in the history 
…uts (#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
  • Loading branch information
@devversion @thePunderWoman
devversion authored and thePunderWoman committed Feb 5, 2024
1 parent a42f0ba commit 9f6605d
Show file tree
Hide file tree
Showing 4 changed files with 184 additions and 24 deletions.
69 changes: 59 additions & 10 deletions packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts
View file
Expand Up @@ -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';
Expand Down Expand Up @@ -307,7 +307,6 @@ export class SymbolBuilder {
continue;
}


const target = this.getDirectiveSymbolForAccessExpression(outputFieldAccess, consumer);
if (target === null) {
continue;
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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,
};
}
63 changes: 63 additions & 0 deletions ...s/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts
View file
Expand Up @@ -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 = `<div dir [inputA]="'my input A'" [aliased]="'my inputB'"></div>`;
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<string> = null!;
inputB: InputSignal<string> = 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');
Expand Down
3 changes: 3 additions & 0 deletions packages/language-service/src/definitions.ts
View file
Expand Up @@ -48,7 +48,9 @@ export class DefinitionBuilder {
}
return getDefinitionForExpressionAtPosition(fileName, position, this.compiler);
}

const definitionMetas = this.getDefinitionMetaAtPosition(templateInfo, position);

if (definitionMetas === undefined) {
return undefined;
}
Expand All @@ -66,6 +68,7 @@ export class DefinitionBuilder {
...(this.getDefinitionsForSymbol({...definitionMeta, ...templateInfo}) ?? []));
}


if (definitions.length === 0) {
return undefined;
}
Expand Down
73 changes: 59 additions & 14 deletions packages/language-service/test/definitions_spec.ts
View file
Expand Up @@ -17,7 +17,7 @@ describe('definitions', () => {
'app.html': '',
'app.ts': `
import {Component} from '@angular/core';
@Component({templateUrl: '/app.html'})
export class AppCmp {}
`,
Expand All @@ -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 {}
`,
Expand Down Expand Up @@ -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': '<div dir inputA="abc"></div>',
'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;
Expand All @@ -108,28 +108,73 @@ 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': '<div dir inputA="abc"></div>',
'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': '<div dir (someEvent)="doSomething()"></div>',
'dir.ts': `
import {Directive, Output, EventEmitter} from '@angular/core';
@Directive({selector: '[dir]'})
export class MyDir {
@Output() someEvent = new EventEmitter<void>();
}`,
'dir2.ts': `
import {Directive, Output, EventEmitter} from '@angular/core';
@Directive({selector: '[dir]'})
export class MyDir2 {
@Output() someEvent = new EventEmitter<void>();
}`,
'app.ts': `
import {Component, NgModule} from '@angular/core';
import {CommonModule} from '@angular/common';
@Component({templateUrl: 'app.html'})
export class AppCmp {
doSomething() {}
Expand Down Expand Up @@ -159,7 +204,7 @@ describe('definitions', () => {
const files = {
'app.ts': `
import {Component} from '@angular/core';
@Component({
template: '',
styleUrls: ['./style.css'],
Expand Down Expand Up @@ -190,7 +235,7 @@ describe('definitions', () => {
`,
'app.ts': `
import {Component} from '@angular/core';
@Component({templateUrl: '/app.html'})
export class AppCmp {
myVal = {name: 'Andrew'};
Expand Down Expand Up @@ -230,12 +275,12 @@ describe('definitions', () => {
export class DollarCmp {
@Input() obs$!: string;
}
@Component({template: '<dollar-cmp [obs$]="greeting"></dollar-cmp>'})
export class AppCmp {
greeting = 'hello';
}
@NgModule({declarations: [AppCmp, DollarCmp]})
export class AppModule {}
`,
Expand Down Expand Up @@ -277,12 +322,12 @@ describe('definitions', () => {
export class DollarDir {
@Input() dollar$!: string;
}
@Component({template: '<div [dollar$]="greeting"></div>'})
export class AppCmp {
greeting = 'hello';
}
@NgModule({declarations: [AppCmp, DollarDir]})
export class AppModule {}
`,
Expand Down

0 comments on commit 9f6605d

Please sign in to comment.