Skip to content

Commit

Permalink
fix(language-service): Do not include $event parameter in reference r…
Browse files Browse the repository at this point in the history
…esults (#40158)

Given the template
`<div (click)="doSomething($event)"></div>`

If you request references for the `$event`, the results include both `$event` and `(click)="doSomething($event)"`.

This happens because in the TCB, `$event` is passed to the `subscribe`/`addEventListener`
function as an argument. So when we ask typescript to give us the references, we
get the result from the usage in the subscribe body as well as the one passed in as an argument.

This commit adds an identifier to the `$event` parameter in the TCB so
that the result returned from `getReferencesAtPosition` can be
identified and filtered out.

fixes #40157

PR Close #40158
  • Loading branch information
atscott authored and josephperrott committed Jan 5, 2021
1 parent 4eac7e6 commit d466db8
Show file tree
Hide file tree
Showing 5 changed files with 66 additions and 33 deletions.
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/src/comments.ts
Expand Up @@ -43,6 +43,7 @@ export enum CommentTriviaType {
export enum ExpressionIdentifier {
DIRECTIVE = 'DIR',
COMPONENT_COMPLETION = 'COMPCOMP',
EVENT_PARAMETER = 'EP',
}

/** Tags the node with the given expression identifier. */
Expand Down
Expand Up @@ -1837,6 +1837,7 @@ function tcbCreateEventHandler(
/* name */ EVENT_PARAMETER,
/* questionToken */ undefined,
/* type */ eventParamType);
addExpressionIdentifier(eventParam, ExpressionIdentifier.EVENT_PARAMETER);

return ts.createFunctionExpression(
/* modifier */ undefined,
Expand Down
79 changes: 47 additions & 32 deletions packages/language-service/ivy/references.ts
Expand Up @@ -9,9 +9,11 @@ import {TmplAstBoundAttribute, TmplAstTextAttribute, TmplAstVariable} from '@ang
import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core';
import {absoluteFrom, absoluteFromSourceFile, AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/file_system';
import {DirectiveSymbol, SymbolKind, TemplateTypeChecker, TypeCheckingProgramStrategy} from '@angular/compiler-cli/src/ngtsc/typecheck/api';
import {ExpressionIdentifier, hasExpressionIdentifier} from '@angular/compiler-cli/src/ngtsc/typecheck/src/comments';
import * as ts from 'typescript';

import {getTargetAtPosition} from './template_target';
import {findTightestNode} from './ts_utils';
import {getDirectiveMatchesForAttribute, getDirectiveMatchesForElementTag, getTemplateInfoAtPosition, isWithin, TemplateInfo, toTextSpan} from './utils';

export class ReferenceBuilder {
Expand Down Expand Up @@ -135,7 +137,7 @@ export class ReferenceBuilder {
const entries: ts.ReferenceEntry[] = [];
for (const ref of refs) {
if (this.ttc.isTrackedTypeCheckFile(absoluteFrom(ref.fileName))) {
const entry = convertToTemplateReferenceEntry(ref, this.ttc);
const entry = this.convertToTemplateReferenceEntry(ref, this.ttc);
if (entry !== null) {
entries.push(entry);
}
Expand All @@ -145,37 +147,50 @@ export class ReferenceBuilder {
}
return entries;
}
}

function convertToTemplateReferenceEntry(
shimReferenceEntry: ts.ReferenceEntry,
templateTypeChecker: TemplateTypeChecker): ts.ReferenceEntry|null {
// TODO(atscott): Determine how to consistently resolve paths. i.e. with the project serverHost or
// LSParseConfigHost in the adapter. We should have a better defined way to normalize paths.
const mapping = templateTypeChecker.getTemplateMappingAtShimLocation({
shimPath: absoluteFrom(shimReferenceEntry.fileName),
positionInShimFile: shimReferenceEntry.textSpan.start,
});
if (mapping === null) {
return null;
}
const {templateSourceMapping, span} = mapping;

let templateUrl: AbsoluteFsPath;
if (templateSourceMapping.type === 'direct') {
templateUrl = absoluteFromSourceFile(templateSourceMapping.node.getSourceFile());
} else if (templateSourceMapping.type === 'external') {
templateUrl = absoluteFrom(templateSourceMapping.templateUrl);
} else {
// This includes indirect mappings, which are difficult to map directly to the code location.
// Diagnostics similarly return a synthetic template string for this case rather than a real
// location.
return null;
}
private convertToTemplateReferenceEntry(
shimReferenceEntry: ts.ReferenceEntry,
templateTypeChecker: TemplateTypeChecker): ts.ReferenceEntry|null {
const sf = this.strategy.getProgram().getSourceFile(shimReferenceEntry.fileName);
if (sf === undefined) {
return null;
}
const tcbNode = findTightestNode(sf, shimReferenceEntry.textSpan.start);
if (tcbNode === undefined ||
hasExpressionIdentifier(sf, tcbNode, ExpressionIdentifier.EVENT_PARAMETER)) {
// If the reference result is the $event parameter in the subscribe/addEventListener function
// in the TCB, we want to filter this result out of the references. We really only want to
// return references to the parameter in the template itself.
return null;
}

return {
...shimReferenceEntry,
fileName: templateUrl,
textSpan: toTextSpan(span),
};
// TODO(atscott): Determine how to consistently resolve paths. i.e. with the project serverHost
// or LSParseConfigHost in the adapter. We should have a better defined way to normalize paths.
const mapping = templateTypeChecker.getTemplateMappingAtShimLocation({
shimPath: absoluteFrom(shimReferenceEntry.fileName),
positionInShimFile: shimReferenceEntry.textSpan.start,
});
if (mapping === null) {
return null;
}
const {templateSourceMapping, span} = mapping;

let templateUrl: AbsoluteFsPath;
if (templateSourceMapping.type === 'direct') {
templateUrl = absoluteFromSourceFile(templateSourceMapping.node.getSourceFile());
} else if (templateSourceMapping.type === 'external') {
templateUrl = absoluteFrom(templateSourceMapping.templateUrl);
} else {
// This includes indirect mappings, which are difficult to map directly to the code location.
// Diagnostics similarly return a synthetic template string for this case rather than a real
// location.
return null;
}

return {
...shimReferenceEntry,
fileName: templateUrl,
textSpan: toTextSpan(span),
};
}
}
16 changes: 16 additions & 0 deletions packages/language-service/ivy/test/references_spec.ts
Expand Up @@ -107,6 +107,22 @@ describe('find references', () => {
assertTextSpans(refs, ['title']);
});

it('should work for $event in method call arguments', () => {
const {text, cursor} = extractCursorInfo(`
import {Component} from '@angular/core';
@Component({template: '<div (click)="setTitle($even¦t)"></div>'})
export class AppCmp {
setTitle(s: any) {}
}`);
const appFile = {name: _('/app.ts'), contents: text};
env = createModuleWithDeclarations([appFile]);
const refs = getReferencesAtPosition(_('/app.ts'), cursor)!;
expect(refs.length).toBe(1);

assertTextSpans(refs, ['$event']);
});

it('should work for property writes', () => {
const appFile = {
name: _('/app.ts'),
Expand Down
2 changes: 1 addition & 1 deletion packages/language-service/ivy/ts_utils.ts
Expand Up @@ -27,4 +27,4 @@ export function getParentClassDeclaration(startNode: ts.Node): ts.ClassDeclarati
startNode = startNode.parent;
}
return undefined;
}
}

0 comments on commit d466db8

Please sign in to comment.