skills/angular__angular
1
0
Fork 0
mirror of https://github.com/angular/angular.git synced 2026-09-14 13:54:52 +08:00
Code Issues Packages Projects Releases Wiki Activity

refactor(compiler-cli): Fix regressions caused by ts.typechecker removal

Browse Source 
removing ts.typechecker in a prior refactor caused some regressions, particularly when multiple directives
appear on a single elemnt. this is now addressed by using an id for directives and storing that in the tcb comment
This commit is contained in:
Andrew Scott
2026-04-07 15:34:47 -07:00
parent e95d84684f
commit 30c950f133
9 changed files with 155 additions and 137 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/compiler-cli/src/ngtsc/typecheck/src/comments.ts
+33 -1
View File
@@ -182,7 +182,39 @@ export function hasExpressionIdentifier(
return false;
}
const commentText = sourceFile.text.substring(pos + 2, end - 2);
return commentText === `${CommentTriviaType.EXPRESSION_TYPE_IDENTIFIER}:${identifier}`;
const prefix = `${CommentTriviaType.EXPRESSION_TYPE_IDENTIFIER}:${identifier}`;
return commentText === prefix || commentText.startsWith(prefix + ':');
}) || false
);
}
export function readDirectiveIdFromComment(
sourceFile: ts.SourceFile,
node: ts.Node,
): number | null {
let id: number | null = null;
ts.forEachTrailingCommentRange(sourceFile.text, node.getEnd(), (pos, end, kind) => {
if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) {
return;
}
const commentText = sourceFile.text.substring(pos + 2, end - 2);
const prefix = `${CommentTriviaType.EXPRESSION_TYPE_IDENTIFIER}:${ExpressionIdentifier.DIRECTIVE}:`;
const hostPrefix = `${CommentTriviaType.EXPRESSION_TYPE_IDENTIFIER}:${ExpressionIdentifier.HOST_DIRECTIVE}:`;
let matchedPrefix: string | null = null;
if (commentText.startsWith(prefix)) {
matchedPrefix = prefix;
} else if (commentText.startsWith(hostPrefix)) {
matchedPrefix = hostPrefix;
}
if (matchedPrefix !== null) {
const idStr = commentText.substring(matchedPrefix.length);
const parsed = parseInt(idStr, 10);
if (!isNaN(parsed)) {
id = parsed;
}
}
});
return id;
}
packages/compiler-cli/src/ngtsc/typecheck/src/ops/codegen.ts
+2 -2
View File
@@ -84,8 +84,8 @@ export class TcbExpr {
* Tags the expression with an identifier.
* @param identifier Identifier to apply to the expression.
*/
addExpressionIdentifier(identifier: ExpressionIdentifier): this {
this.identifierComment = `${CommentTriviaType.EXPRESSION_TYPE_IDENTIFIER}:${identifier}`;
addExpressionIdentifier(identifier: ExpressionIdentifier, id?: number): this {
this.identifierComment = `${CommentTriviaType.EXPRESSION_TYPE_IDENTIFIER}:${identifier}${id !== undefined ? `:${id}` : ''}`;
return this;
}
packages/compiler-cli/src/ngtsc/typecheck/src/ops/directive_constructor.ts
+2 -1
View File
@@ -37,6 +37,7 @@ export class TcbDirectiveCtorOp extends TcbOp {
private node: DirectiveOwner,
private dir: TcbDirectiveMetadata,
private customFormControlType: CustomFormControlType | null,
private directiveIndex?: number,
) {
super();
}
@@ -78,7 +79,7 @@ export class TcbDirectiveCtorOp extends TcbOp {
this.dir.matchSource === MatchSource.HostDirective
? ExpressionIdentifier.HOST_DIRECTIVE
: ExpressionIdentifier.DIRECTIVE;
id.addExpressionIdentifier(identifier).addParseSpanInfo(span);
id.addExpressionIdentifier(identifier, this.directiveIndex).addParseSpanInfo(span);
for (const attr of boundAttrs) {
// Skip text attributes if configured to do so.
packages/compiler-cli/src/ngtsc/typecheck/src/ops/directive_type.ts
+2 -1
View File
@@ -24,6 +24,7 @@ export abstract class TcbDirectiveTypeOpBase extends TcbOp {
protected scope: Scope,
protected node: DirectiveOwner,
protected dir: TcbDirectiveMetadata,
protected directiveIndex?: number,
) {
super();
}
@@ -64,7 +65,7 @@ export abstract class TcbDirectiveTypeOpBase extends TcbOp {
? ExpressionIdentifier.HOST_DIRECTIVE
: ExpressionIdentifier.DIRECTIVE;
const id = new TcbExpr(this.tcb.allocateId())
.addExpressionIdentifier(identifier)
.addExpressionIdentifier(identifier, this.directiveIndex)
.addParseSpanInfo(span);
this.scope.addStatement(declareVariable(id, type));
return id;
packages/compiler-cli/src/ngtsc/typecheck/src/ops/scope.ts
+12 -8
View File
@@ -603,8 +603,9 @@ export class Scope {
}
const dirMap = new Map<TcbDirectiveMetadata, number>();
for (const dir of directives) {
this.appendDirectiveInputs(dir, node, dirMap, directives);
for (let i = 0; i < directives.length; i++) {
const dir = directives[i];
this.appendDirectiveInputs(dir, node, dirMap, directives, i);
}
this.directiveOpMap.set(node, dirMap);
@@ -680,8 +681,9 @@ export class Scope {
if (directives !== null && directives.length > 0) {
const dirMap = new Map<TcbDirectiveMetadata, number>();
for (const dir of directives) {
this.appendDirectiveInputs(dir, node, dirMap, directives);
for (let i = 0; i < directives.length; i++) {
const dir = directives[i];
this.appendDirectiveInputs(dir, node, dirMap, directives, i);
for (const propertyName of dir.inputs.propertyNames) {
claimedInputs.add(propertyName);
@@ -750,11 +752,12 @@ export class Scope {
node: TmplAstElement | TmplAstTemplate | TmplAstComponent | TmplAstDirective,
dirMap: Map<TcbDirectiveMetadata, number>,
allDirectiveMatches: TcbDirectiveMetadata[],
directiveIndex?: number,
): void {
const nodeIsFormControl = isFormControl(allDirectiveMatches);
const customFormControlType = nodeIsFormControl ? getCustomFieldDirectiveType(dir) : null;
const directiveOp = this.getDirectiveOp(dir, node, customFormControlType);
const directiveOp = this.getDirectiveOp(dir, node, customFormControlType, directiveIndex);
const dirIndex = this.opQueue.push(directiveOp) - 1;
dirMap.set(dir, dirIndex);
@@ -779,21 +782,22 @@ export class Scope {
dir: TcbDirectiveMetadata,
node: DirectiveOwner,
customFieldType: CustomFormControlType | null,
directiveIndex?: number,
): TcbOp {
if (!dir.isGeneric) {
// The most common case is that when a directive is not generic, we use the normal
// `TcbNonDirectiveTypeOp`.
return new TcbNonGenericDirectiveTypeOp(this.tcb, this, node, dir);
return new TcbNonGenericDirectiveTypeOp(this.tcb, this, node, dir, directiveIndex);
} else if (!dir.requiresInlineTypeCtor || this.tcb.env.config.useInlineTypeConstructors) {
// For generic directives, we use a type constructor to infer types. If a directive requires
// an inline type constructor, then inlining must be available to use the
// `TcbDirectiveCtorOp`. If not we, we fallback to using `any` – see below.
return new TcbDirectiveCtorOp(this.tcb, this, node, dir, customFieldType);
return new TcbDirectiveCtorOp(this.tcb, this, node, dir, customFieldType, directiveIndex);
}
// If inlining is not available, then we give up on inferring the generic params, and use
// `any` type for the directive's generic parameters.
return new TcbGenericDirectiveTypeWithAnyParamsOp(this.tcb, this, node, dir);
return new TcbGenericDirectiveTypeWithAnyParamsOp(this.tcb, this, node, dir, directiveIndex);
}
private appendSelectorlessDirectives(
packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts
+37 -115
View File
@@ -66,6 +66,7 @@ import {
findAllMatchingNodes,
findFirstMatchingNode,
hasExpressionIdentifier,
readDirectiveIdFromComment,
} from './comments';
import {TypeCheckData} from './context';
import {isAccessExpression, isDirectiveDeclaration} from './ts_util';
@@ -207,36 +208,52 @@ export class SymbolBuilder {
templateNode: TmplAstElement | TmplAstTemplate | TmplAstComponent | TmplAstDirective,
): DirectiveSymbol[] {
const elementSourceSpan = templateNode.startSourceSpan ?? templateNode.sourceSpan;
const nodes = findAllMatchingNodes(this.typeCheckBlock, {
withSpan: elementSourceSpan,
filter: isDirectiveDeclaration,
});
const symbols: DirectiveSymbol[] = [];
const seenDirectives = new Set<ts.ClassDeclaration>();
const boundDirectives = this.typeCheckData.boundTarget.getDirectivesOfNode(templateNode) ?? [];
let boundDirectives = this.typeCheckData.boundTarget.getDirectivesOfNode(templateNode) ?? [];
let symbols = this.getDirectiveSymbolsForDirectives(boundDirectives, elementSourceSpan);
// 'getDirectivesOfNode' will not return the directives intended for an element
// on a microsyntax template, for example '<div *ngFor="let user of users;" dir>',
// the 'dir' will be skipped, but it's needed in language service.
if (!(templateNode instanceof TmplAstDirective)) {
const firstChild = templateNode.children?.[0];
if (firstChild instanceof TmplAstElement) {
const firstChild = templateNode.children.find(
(c): c is TmplAstElement => c instanceof TmplAstElement,
);
if (firstChild !== undefined) {
const isMicrosyntaxTemplate =
templateNode instanceof TmplAstTemplate &&
sourceSpanEqual(firstChild.sourceSpan, templateNode.sourceSpan);
if (isMicrosyntaxTemplate) {
const firstChildDirectives =
this.typeCheckData.boundTarget.getDirectivesOfNode(firstChild);
if (firstChildDirectives !== null && boundDirectives.length > 0) {
boundDirectives = boundDirectives.concat(firstChildDirectives);
} else if (firstChildDirectives !== null) {
boundDirectives = firstChildDirectives;
if (firstChildDirectives !== null) {
const childSymbols = this.getDirectiveSymbolsForDirectives(
firstChildDirectives,
elementSourceSpan,
);
// Merge symbols, avoiding duplicates
for (const symbol of childSymbols) {
if (!symbols.some((s) => s.ref.node === symbol.ref.node)) {
symbols.push(symbol);
}
}
}
}
}
}
return symbols;
}
private getDirectiveSymbolsForDirectives(
boundDirectives: TypeCheckableDirectiveMeta[],
span: ParseSourceSpan,
): DirectiveSymbol[] {
const nodes = findAllMatchingNodes(this.typeCheckBlock, {
withSpan: span,
filter: isDirectiveDeclaration,
});
const hostDirectiveMap = new Map<ts.Node, HostDirectiveMeta>();
for (const d of boundDirectives) {
if (d.hostDirectives) {
@@ -248,33 +265,14 @@ export class SymbolBuilder {
}
}
for (let i = 0; i < nodes.length; i++) {
const node = nodes[i];
let nodeName: string | null = null;
let typeNode = ts.isTypeNode(node)
? node
: ts.isIdentifier(node) && node.parent && ts.isVariableDeclaration(node.parent)
? node.parent.type
: null;
if (typeNode && ts.isTypeReferenceNode(typeNode)) {
const typeName = typeNode.typeName;
nodeName = ts.isIdentifier(typeName) ? typeName.text : typeName.right.text;
} else if (typeNode && ts.isIntersectionTypeNode(typeNode)) {
const first = typeNode.types[0];
if (ts.isTypeReferenceNode(first)) {
const typeName = first.typeName;
nodeName = ts.isIdentifier(typeName) ? typeName.text : typeName.right.text;
}
}
// Match by name with index fallback
let meta = boundDirectives[i];
if (nodeName) {
meta =
boundDirectives.find((m) => m.ref.node.name && m.ref.node.name.text === nodeName) ?? meta;
}
const symbols: DirectiveSymbol[] = [];
const seenDirectives = new Set<ts.ClassDeclaration>();
const sf = this.typeCheckBlock.getSourceFile();
for (const node of nodes) {
const id = readDirectiveIdFromComment(sf, node);
if (id === null) continue;
const meta = boundDirectives[id];
if (!meta) continue;
const declaration = meta.ref.node as unknown as ts.ClassDeclaration;
@@ -316,85 +314,9 @@ export class SymbolBuilder {
}
}
// Sort to ensure host directives appear first (matching test expectations)
symbols.sort((a, b) => {
if (a.matchSource === MatchSource.HostDirective && b.matchSource === MatchSource.Selector) {
return -1;
}
if (a.matchSource === MatchSource.Selector && b.matchSource === MatchSource.HostDirective) {
return 1;
}
return 0;
});
return symbols;
}
private getDirectiveMeta(
host: TmplAstTemplate | TmplAstElement | TmplAstComponent | TmplAstDirective,
directiveDeclaration: ts.ClassDeclaration,
): TypeCheckableDirectiveMeta | null {
let directives = this.typeCheckData.boundTarget.getDirectivesOfNode(host);
// `getDirectivesOfNode` will not return the directives intended for an element
// on a microsyntax template, for example `<div *ngFor="let user of users;" dir>`,
// the `dir` will be skipped, but it's needed in language service.
if (!(host instanceof TmplAstDirective)) {
const firstChild = host.children[0];
if (firstChild instanceof TmplAstElement) {
const isMicrosyntaxTemplate =
host instanceof TmplAstTemplate &&
sourceSpanEqual(firstChild.sourceSpan, host.sourceSpan);
if (isMicrosyntaxTemplate) {
const firstChildDirectives =
this.typeCheckData.boundTarget.getDirectivesOfNode(firstChild);
if (firstChildDirectives !== null && directives !== null) {
directives = directives.concat(firstChildDirectives);
} else {
directives = directives ?? firstChildDirectives;
}
}
}
}
if (directives === null) {
return null;
}
const directive = directives.find((m) =>
isSameDirectiveDeclaration(m.ref.node, directiveDeclaration),
);
if (directive) {
return directive;
}
const originalFile = (directiveDeclaration.getSourceFile() as MaybeSourceFileWithOriginalFile)[
NgOriginalFile
];
if (originalFile !== undefined) {
// This is a preliminary check ahead of a more expensive search
const hasPotentialCandidate = directives.find(
(m) => m.ref.node.name.text === directiveDeclaration.name?.text,
);
if (hasPotentialCandidate) {
// In case the TCB has been inlined,
// We will look for a matching class
// If we find one, we look for it in the directives array
const classWithSameName = findMatchingDirective(originalFile, directiveDeclaration);
if (classWithSameName !== null) {
return (
directives.find((m) => isSameDirectiveDeclaration(m.ref.node, classWithSameName)) ??
null
);
}
}
}
// Really nothing was found
return null;
}
private getDirectiveModule(declaration: ts.ClassDeclaration): ClassDeclaration | null {
const scope = this.componentScopeReader.getScopeForComponent(declaration as ClassDeclaration);
if (scope === null || scope.kind !== ComponentScopeKind.NgModule) {
packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts
+48 -7
View File
@@ -2395,6 +2395,43 @@ runInEachFileSystem(() => {
expect(symbol.directives.map((d) => d.ref.node.name.text)).toEqual(['Dep']);
});
it('should get symbol for a selector attribute when there are multiple directives', () => {
const fileName = absoluteFrom('/main.ts');
const matListItem = {
name: 'MatListItem',
selector: '[mat-list-item]',
file: absoluteFrom('/list.ts'),
type: 'directive' as const,
};
const routerLink = {
name: 'RouterLink',
selector: '[routerLink]',
file: absoluteFrom('/router.ts'),
type: 'directive' as const,
inputs: {routerLink: 'routerLink'},
};
const {program, templateTypeChecker} = setup([
{
fileName,
templates: {'Cmp': '<a mat-list-item routerLink="path"></a>'},
declarations: [matListItem, routerLink],
},
{fileName: matListItem.file, source: 'export class MatListItem {}'},
{fileName: routerLink.file, source: 'export class RouterLink { routerLink!: string; }'},
]);
const sf = getSourceFileOrError(program, fileName);
const cmp = getClass(sf, 'Cmp');
const elements = getAstElements(templateTypeChecker, cmp);
const element = elements[0];
const matListItemAttr = element.attributes.find((a) => a.name === 'mat-list-item')!;
const symbol = templateTypeChecker.getSymbolOfNode(matListItemAttr, cmp);
expect(symbol).toBeTruthy();
assertDomBindingSymbol(symbol!);
assertElementSymbol(symbol!.host);
expect(symbol!.host.directives.map((d) => d.ref.node.name.text)).toContain('MatListItem');
});
it('should get symbol of a selectorless directive', () => {
const fileName = absoluteFrom('/main.ts');
const dep = getDep('Dep', '/dep.ts');
@@ -3050,16 +3087,20 @@ runInEachFileSystem(() => {
const element = nodes[0] as TmplAstElement;
const symbol = templateTypeChecker.getSymbolOfNode(element, cmp)!;
assertElementSymbol(symbol);
expect(
symbol.directives.map((d) => ({
name: d.ref.node.name.text,
matchSource: d.matchSource,
})),
).toEqual([
const actual = symbol.directives.map((d) => ({
name: d.ref.node.name.text,
matchSource: d.matchSource,
}));
actual.sort((a, b) => a.name.localeCompare(b.name));
const expected = [
{name: 'DepInnerHost', matchSource: MatchSource.HostDirective},
{name: 'DepHost', matchSource: MatchSource.HostDirective},
{name: 'Dep', matchSource: MatchSource.Selector},
]);
];
expected.sort((a, b) => a.name.localeCompare(b.name));
expect(actual).toEqual(expected);
});
});
});
packages/language-service/src/quick_info.ts
+2 -1
View File
@@ -111,8 +111,9 @@ export class QuickInfoBuilder {
case SymbolKind.SelectorlessDirective:
return this.getQuickInfoForSelectorlessSymbol(symbol);
case SymbolKind.Expression:
case SymbolKind.Directive:
return this.getQuickInfoAtTcbLocation(symbol.tcbLocation);
case SymbolKind.Directive:
return this.getQuickInfoForDirectiveSymbol(symbol);
}
}
packages/language-service/src/references_and_rename_utils.ts
+17 -1
View File
@@ -117,10 +117,26 @@ export function getTargetDetailsAtTemplatePosition(
const templateTarget = node;
switch (symbol.kind) {
case SymbolKind.Directive:
case SymbolKind.Template:
// If the symbol is a Directive and the target is an attribute, it means the attribute
// matched a directive selector. We need to handle this to support finding references
// for directives matching an attribute (e.g. `<div dir></div>`).
if (
templateTarget instanceof TmplAstTextAttribute ||
templateTarget instanceof TmplAstBoundAttribute
) {
const pos = getPositionForDirective(symbol, templateTypeChecker);
details.push({
typescriptLocations: pos ? [pos] : [],
templateTarget,
symbol,
});
break;
}
// References to elements, templates, and directives will be through template references
// (#ref). They shouldn't be used directly for a Language Service reference request.
break;
case SymbolKind.Template:
break;
case SymbolKind.Element: {
const matches = getDirectiveMatchesForElementTag(symbol.templateNode, symbol.directives);
details.push({