Skip to content

Commit

Permalink
fix(language-service): Support 'go to definition' for two-way bindings (
Browse files Browse the repository at this point in the history 
#40185)

Rather than expecting that a position in a template only targets a
single node, this commit adjusts the approach to account for two way
bindings. In particular, we attempt to get definitions for each targeted
node and then return the combination of all results, or `undefined` if
none of the target nodes had definitions.

PR Close #40185
  • Loading branch information
@atscott
atscott committed Jan 7, 2021
1 parent d70c26c commit a9d8c22
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 62 deletions.
122 changes: 76 additions & 46 deletions packages/language-service/ivy/definitions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,29 @@ export class DefinitionBuilder {
}
return getDefinitionForExpressionAtPosition(fileName, position, this.compiler);
}
const definitionMeta = this.getDefinitionMetaAtPosition(templateInfo, position);
// The `$event` of event handlers would point to the $event parameter in the shim file, as in
// `_outputHelper(_t3["x"]).subscribe(function ($event): any { $event }) ;`
// If we wanted to return something for this, it would be more appropriate for something like
// `getTypeDefinition`.
if (definitionMeta === undefined || isDollarEvent(definitionMeta.node)) {
const definitionMetas = this.getDefinitionMetaAtPosition(templateInfo, position);
if (definitionMetas === undefined) {
return undefined;
}
const definitions: ts.DefinitionInfo[] = [];
for (const definitionMeta of definitionMetas) {
// The `$event` of event handlers would point to the $event parameter in the shim file, as in
// `_outputHelper(_t3["x"]).subscribe(function ($event): any { $event }) ;`
// If we wanted to return something for this, it would be more appropriate for something like
// `getTypeDefinition`.
if (isDollarEvent(definitionMeta.node)) {
continue;
}

definitions.push(
...(this.getDefinitionsForSymbol({...definitionMeta, ...templateInfo}) ?? []));
}

const definitions = this.getDefinitionsForSymbol({...definitionMeta, ...templateInfo});
return {definitions, textSpan: getTextSpanOfNode(definitionMeta.node)};
if (definitions.length === 0) {
return undefined;
}

return {definitions, textSpan: getTextSpanOfNode(definitionMetas[0].node)};
}

private getDefinitionsForSymbol({symbol, node, parent, component}: DefinitionMeta&
Expand Down Expand Up @@ -123,42 +135,55 @@ export class DefinitionBuilder {
if (templateInfo === undefined) {
return;
}
const definitionMeta = this.getDefinitionMetaAtPosition(templateInfo, position);
if (definitionMeta === undefined) {
const definitionMetas = this.getDefinitionMetaAtPosition(templateInfo, position);
if (definitionMetas === undefined) {
return undefined;
}

const {symbol, node} = definitionMeta;
switch (symbol.kind) {
case SymbolKind.Directive:
case SymbolKind.DomBinding:
case SymbolKind.Element:
case SymbolKind.Template:
return this.getTypeDefinitionsForTemplateInstance(symbol, node);
case SymbolKind.Output:
case SymbolKind.Input: {
const bindingDefs = this.getTypeDefinitionsForSymbols(...symbol.bindings);
// Also attempt to get directive matches for the input name. If there is a directive that
// has the input name as part of the selector, we want to return that as well.
const directiveDefs = this.getDirectiveTypeDefsForBindingNode(
node, definitionMeta.parent, templateInfo.component);
return [...bindingDefs, ...directiveDefs];
}
case SymbolKind.Pipe: {
if (symbol.tsSymbol !== null) {
return this.getTypeDefinitionsForSymbols(symbol);
} else {
// If there is no `ts.Symbol` for the pipe transform, we want to return the
// type definition (the pipe class).
return this.getTypeDefinitionsForSymbols(symbol.classSymbol);
const definitions: ts.DefinitionInfo[] = [];
for (const {symbol, node, parent} of definitionMetas) {
switch (symbol.kind) {
case SymbolKind.Directive:
case SymbolKind.DomBinding:
case SymbolKind.Element:
case SymbolKind.Template:
definitions.push(...this.getTypeDefinitionsForTemplateInstance(symbol, node));
break;
case SymbolKind.Output:
case SymbolKind.Input: {
const bindingDefs = this.getTypeDefinitionsForSymbols(...symbol.bindings);
definitions.push(...bindingDefs);
// Also attempt to get directive matches for the input name. If there is a directive that
// has the input name as part of the selector, we want to return that as well.
const directiveDefs =
this.getDirectiveTypeDefsForBindingNode(node, parent, templateInfo.component);
definitions.push(...directiveDefs);
break;
}
case SymbolKind.Pipe: {
if (symbol.tsSymbol !== null) {
definitions.push(...this.getTypeDefinitionsForSymbols(symbol));
} else {
// If there is no `ts.Symbol` for the pipe transform, we want to return the
// type definition (the pipe class).
definitions.push(...this.getTypeDefinitionsForSymbols(symbol.classSymbol));
}
break;
}
case SymbolKind.Reference:
definitions.push(
...this.getTypeDefinitionsForSymbols({shimLocation: symbol.targetLocation}));
break;
case SymbolKind.Expression:
definitions.push(...this.getTypeDefinitionsForSymbols(symbol));
break;
case SymbolKind.Variable: {
definitions.push(
...this.getTypeDefinitionsForSymbols({shimLocation: symbol.initializerLocation}));
break;
}
}
case SymbolKind.Reference:
return this.getTypeDefinitionsForSymbols({shimLocation: symbol.targetLocation});
case SymbolKind.Expression:
return this.getTypeDefinitionsForSymbols(symbol);
case SymbolKind.Variable:
return this.getTypeDefinitionsForSymbols({shimLocation: symbol.initializerLocation});
return definitions;
}
}

Expand Down Expand Up @@ -221,21 +246,26 @@ export class DefinitionBuilder {
}

private getDefinitionMetaAtPosition({template, component}: TemplateInfo, position: number):
DefinitionMeta|undefined {
DefinitionMeta[]|undefined {
const target = getTargetAtPosition(template, position);
if (target === null) {
return undefined;
}
const {context, parent} = target;

const node =
context.kind === TargetNodeKind.TwoWayBindingContext ? context.nodes[0] : context.node;
const nodes =
context.kind === TargetNodeKind.TwoWayBindingContext ? context.nodes : [context.node];

const symbol = this.compiler.getTemplateTypeChecker().getSymbolOfNode(node, component);
if (symbol === null) {
return undefined;

const definitionMetas: DefinitionMeta[] = [];
for (const node of nodes) {
const symbol = this.compiler.getTemplateTypeChecker().getSymbolOfNode(node, component);
if (symbol === null) {
continue;
}
definitionMetas.push({node, parent, symbol});
}
return {node, parent, symbol};
return definitionMetas.length > 0 ? definitionMetas : undefined;
}
}

Expand Down
28 changes: 12 additions & 16 deletions packages/language-service/ivy/test/legacy/definitions_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ describe('definitions', () => {
const {position} =
service.overwriteInlineTemplate(APP_COMPONENT, `<ng-templ¦ate></ng-template>`);
const definitionAndBoundSpan = ngLS.getDefinitionAndBoundSpan(APP_COMPONENT, position);
expect(definitionAndBoundSpan!.definitions).toEqual([]);
expect(definitionAndBoundSpan).toBeUndefined();
});
});

Expand Down Expand Up @@ -181,17 +181,14 @@ describe('definitions', () => {
templateOverride: `<test-comp string-model [(mo¦del)]="title"></test-comp>`,
expectedSpanText: 'model',
});
// TODO(atscott): This should really return 2 definitions, 1 for the input and 1 for the
// output.
// The TemplateTypeChecker also only returns the first match in the TCB for a given
// sourceSpan so even if we also requested the TmplAstBoundEvent, we'd still get back the
// symbol for the
// @Input because the input appears first in the TCB and they have the same sourceSpan.
expect(definitions!.length).toEqual(1);
expect(definitions!.length).toEqual(2);

const [def] = definitions;
expect(def.textSpan).toEqual('model');
expect(def.contextSpan).toEqual(`@Input() model: string = 'model';`);
const [inputDef, outputDef] = definitions;
expect(inputDef.textSpan).toEqual('model');
expect(inputDef.contextSpan).toEqual(`@Input() model: string = 'model';`);
expect(outputDef.textSpan).toEqual('modelChange');
expect(outputDef.contextSpan)
.toEqual(`@Output() modelChange: EventEmitter<string> = new EventEmitter();`);
});
});

Expand Down Expand Up @@ -245,12 +242,11 @@ describe('definitions', () => {

describe('references', () => {
it('should work for element reference declarations', () => {
const definitions = getDefinitionsAndAssertBoundSpan({
templateOverride: `<div #cha¦rt></div>{{chart}}`,
expectedSpanText: 'chart',
});
const {position} =
service.overwriteInlineTemplate(APP_COMPONENT, `<div #cha¦rt></div>{{chart}}`);
const definitionAndBoundSpan = ngLS.getDefinitionAndBoundSpan(APP_COMPONENT, position);
// We're already at the definition, so nothing is returned
expect(definitions).toEqual([]);
expect(definitionAndBoundSpan).toBeUndefined();
});

it('should work for element reference uses', () => {
Expand Down

0 comments on commit a9d8c22

Please sign in to comment.