fix(language-service): Do not include $event parameter in reference results (#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
This commit is contained in:
parent
4eac7e6436
commit
d466db8285
|
@ -43,6 +43,7 @@ export enum CommentTriviaType {
|
||||||
export enum ExpressionIdentifier {
|
export enum ExpressionIdentifier {
|
||||||
DIRECTIVE = 'DIR',
|
DIRECTIVE = 'DIR',
|
||||||
COMPONENT_COMPLETION = 'COMPCOMP',
|
COMPONENT_COMPLETION = 'COMPCOMP',
|
||||||
|
EVENT_PARAMETER = 'EP',
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Tags the node with the given expression identifier. */
|
/** Tags the node with the given expression identifier. */
|
||||||
|
|
|
@ -1837,6 +1837,7 @@ function tcbCreateEventHandler(
|
||||||
/* name */ EVENT_PARAMETER,
|
/* name */ EVENT_PARAMETER,
|
||||||
/* questionToken */ undefined,
|
/* questionToken */ undefined,
|
||||||
/* type */ eventParamType);
|
/* type */ eventParamType);
|
||||||
|
addExpressionIdentifier(eventParam, ExpressionIdentifier.EVENT_PARAMETER);
|
||||||
|
|
||||||
return ts.createFunctionExpression(
|
return ts.createFunctionExpression(
|
||||||
/* modifier */ undefined,
|
/* modifier */ undefined,
|
||||||
|
|
|
@ -9,9 +9,11 @@ import {TmplAstBoundAttribute, TmplAstTextAttribute, TmplAstVariable} from '@ang
|
||||||
import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core';
|
import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core';
|
||||||
import {absoluteFrom, absoluteFromSourceFile, AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/file_system';
|
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 {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 * as ts from 'typescript';
|
||||||
|
|
||||||
import {getTargetAtPosition} from './template_target';
|
import {getTargetAtPosition} from './template_target';
|
||||||
|
import {findTightestNode} from './ts_utils';
|
||||||
import {getDirectiveMatchesForAttribute, getDirectiveMatchesForElementTag, getTemplateInfoAtPosition, isWithin, TemplateInfo, toTextSpan} from './utils';
|
import {getDirectiveMatchesForAttribute, getDirectiveMatchesForElementTag, getTemplateInfoAtPosition, isWithin, TemplateInfo, toTextSpan} from './utils';
|
||||||
|
|
||||||
export class ReferenceBuilder {
|
export class ReferenceBuilder {
|
||||||
|
@ -135,7 +137,7 @@ export class ReferenceBuilder {
|
||||||
const entries: ts.ReferenceEntry[] = [];
|
const entries: ts.ReferenceEntry[] = [];
|
||||||
for (const ref of refs) {
|
for (const ref of refs) {
|
||||||
if (this.ttc.isTrackedTypeCheckFile(absoluteFrom(ref.fileName))) {
|
if (this.ttc.isTrackedTypeCheckFile(absoluteFrom(ref.fileName))) {
|
||||||
const entry = convertToTemplateReferenceEntry(ref, this.ttc);
|
const entry = this.convertToTemplateReferenceEntry(ref, this.ttc);
|
||||||
if (entry !== null) {
|
if (entry !== null) {
|
||||||
entries.push(entry);
|
entries.push(entry);
|
||||||
}
|
}
|
||||||
|
@ -145,37 +147,50 @@ export class ReferenceBuilder {
|
||||||
}
|
}
|
||||||
return entries;
|
return entries;
|
||||||
}
|
}
|
||||||
}
|
|
||||||
|
|
||||||
function convertToTemplateReferenceEntry(
|
private convertToTemplateReferenceEntry(
|
||||||
shimReferenceEntry: ts.ReferenceEntry,
|
shimReferenceEntry: ts.ReferenceEntry,
|
||||||
templateTypeChecker: TemplateTypeChecker): ts.ReferenceEntry|null {
|
templateTypeChecker: TemplateTypeChecker): ts.ReferenceEntry|null {
|
||||||
// TODO(atscott): Determine how to consistently resolve paths. i.e. with the project serverHost or
|
const sf = this.strategy.getProgram().getSourceFile(shimReferenceEntry.fileName);
|
||||||
// LSParseConfigHost in the adapter. We should have a better defined way to normalize paths.
|
if (sf === undefined) {
|
||||||
const mapping = templateTypeChecker.getTemplateMappingAtShimLocation({
|
return null;
|
||||||
shimPath: absoluteFrom(shimReferenceEntry.fileName),
|
}
|
||||||
positionInShimFile: shimReferenceEntry.textSpan.start,
|
const tcbNode = findTightestNode(sf, shimReferenceEntry.textSpan.start);
|
||||||
});
|
if (tcbNode === undefined ||
|
||||||
if (mapping === null) {
|
hasExpressionIdentifier(sf, tcbNode, ExpressionIdentifier.EVENT_PARAMETER)) {
|
||||||
return null;
|
// 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;
|
||||||
|
}
|
||||||
|
|
||||||
|
// 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),
|
||||||
|
};
|
||||||
}
|
}
|
||||||
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),
|
|
||||||
};
|
|
||||||
}
|
}
|
||||||
|
|
|
@ -107,6 +107,22 @@ describe('find references', () => {
|
||||||
assertTextSpans(refs, ['title']);
|
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', () => {
|
it('should work for property writes', () => {
|
||||||
const appFile = {
|
const appFile = {
|
||||||
name: _('/app.ts'),
|
name: _('/app.ts'),
|
||||||
|
|
|
@ -27,4 +27,4 @@ export function getParentClassDeclaration(startNode: ts.Node): ts.ClassDeclarati
|
||||||
startNode = startNode.parent;
|
startNode = startNode.parent;
|
||||||
}
|
}
|
||||||
return undefined;
|
return undefined;
|
||||||
}
|
}
|
Loading…
Reference in New Issue