From 30c950f13337a0b7e77e2f86eb77af8d9577e7cc Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Tue, 7 Apr 2026 15:34:47 -0700 Subject: [PATCH] refactor(compiler-cli): Fix regressions caused by ts.typechecker removal 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 --- .../src/ngtsc/typecheck/src/comments.ts | 34 +++- .../src/ngtsc/typecheck/src/ops/codegen.ts | 4 +- .../src/ops/directive_constructor.ts | 3 +- .../ngtsc/typecheck/src/ops/directive_type.ts | 3 +- .../src/ngtsc/typecheck/src/ops/scope.ts | 20 ++- .../typecheck/src/template_symbol_builder.ts | 152 +++++------------- ...ecker__get_symbol_of_template_node_spec.ts | 55 ++++++- packages/language-service/src/quick_info.ts | 3 +- .../src/references_and_rename_utils.ts | 18 ++- 9 files changed, 155 insertions(+), 137 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/comments.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/comments.ts index ae863cc45d1..6baf5296d4f 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/comments.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/comments.ts @@ -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; +} diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/ops/codegen.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/ops/codegen.ts index 3d8bbe31e76..e11dc5f28a8 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/ops/codegen.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/ops/codegen.ts @@ -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; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/ops/directive_constructor.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/ops/directive_constructor.ts index 83105e65706..261bafdea4a 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/ops/directive_constructor.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/ops/directive_constructor.ts @@ -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. diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/ops/directive_type.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/ops/directive_type.ts index 9416b2f32e7..fe50689d2bc 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/ops/directive_type.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/ops/directive_type.ts @@ -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; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/ops/scope.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/ops/scope.ts index b863875d3d4..3f9de532e74 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/ops/scope.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/ops/scope.ts @@ -603,8 +603,9 @@ export class Scope { } const dirMap = new Map(); - 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(); - 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, 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( diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts index bb6c1ecf4cb..23af2c14177 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts @@ -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(); + 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 '
', // 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(); 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(); + 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 `
`, - // 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) { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts index 29d356c6144..b02db87c3d4 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts @@ -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': ''}, + 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); }); }); }); diff --git a/packages/language-service/src/quick_info.ts b/packages/language-service/src/quick_info.ts index 2c9aab318ea..f506e9487ab 100644 --- a/packages/language-service/src/quick_info.ts +++ b/packages/language-service/src/quick_info.ts @@ -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); } } diff --git a/packages/language-service/src/references_and_rename_utils.ts b/packages/language-service/src/references_and_rename_utils.ts index cb0e3fcdcd8..9c88a399127 100644 --- a/packages/language-service/src/references_and_rename_utils.ts +++ b/packages/language-service/src/references_and_rename_utils.ts @@ -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. `
`). + 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({