Skip to content

Commit 48aec63

Browse files
crisbetodylhunn
crisbeto
authored and
dylhunn
committed
fix(compiler-cli): identify aliased initializer functions (#54480)
Fixes that initializer functions weren't being recognized if they are aliased (e.g. `import {model as alias} from '@angular/core';`). To do this efficiently, I had to introduce the `ImportedSymbolsTracker` which scans the top-level imports of a file and allows them to be checked quickly, without having to go through the type checker. It will be useful in the future when verifying that that initializer APIs aren't used in unexpected places. I've also introduced tests specifically for the `tryParseInitializerApiMember` function so that we can test it in isolation instead of going through the various functions that call into it. PR Close #54480
1 parent d68cf17 commit 48aec63

26 files changed

+560
-129
lines changed

‎packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import ts from 'typescript';
1212
import {Cycle, CycleAnalyzer, CycleHandlingStrategy} from '../../../cycles';
1313
import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../../diagnostics';
1414
import {absoluteFrom, relative} from '../../../file_system';
15-
import {assertSuccessfulReferenceEmit, DeferredSymbolTracker, ImportedFile, LocalCompilationExtraImportsTracker, ModuleResolver, Reference, ReferenceEmitter} from '../../../imports';
15+
import {assertSuccessfulReferenceEmit, DeferredSymbolTracker, ImportedFile, ImportedSymbolsTracker, LocalCompilationExtraImportsTracker, ModuleResolver, Reference, ReferenceEmitter} from '../../../imports';
1616
import {DependencyTracker} from '../../../incremental/api';
1717
import {extractSemanticTypeParameters, SemanticDepGraphUpdater} from '../../../incremental/semantic_graph';
1818
import {IndexingContext} from '../../../indexer';
@@ -83,7 +83,8 @@ export class ComponentDecoratorHandler implements
8383
private injectableRegistry: InjectableClassRegistry,
8484
private semanticDepGraphUpdater: SemanticDepGraphUpdater|null,
8585
private annotateForClosureCompiler: boolean, private perf: PerfRecorder,
86-
private hostDirectivesResolver: HostDirectivesResolver, private includeClassMetadata: boolean,
86+
private hostDirectivesResolver: HostDirectivesResolver,
87+
private importTracker: ImportedSymbolsTracker, private includeClassMetadata: boolean,
8788
private readonly compilationMode: CompilationMode,
8889
private readonly deferredSymbolTracker: DeferredSymbolTracker,
8990
private readonly forbidOrphanRendering: boolean, private readonly enableBlockSyntax: boolean,
@@ -225,8 +226,8 @@ export class ComponentDecoratorHandler implements
225226
// @Component inherits @Directive, so begin by extracting the @Directive metadata and building
226227
// on it.
227228
const directiveResult = extractDirectiveMetadata(
228-
node, decorator, this.reflector, this.evaluator, this.refEmitter, this.referencesRegistry,
229-
this.isCore, this.annotateForClosureCompiler, this.compilationMode,
229+
node, decorator, this.reflector, this.importTracker, this.evaluator, this.refEmitter,
230+
this.referencesRegistry, this.isCore, this.annotateForClosureCompiler, this.compilationMode,
230231
this.elementSchemaRegistry.getDefaultComponentElementName(), this.useTemplatePipeline);
231232
if (directiveResult === undefined) {
232233
// `extractDirectiveMetadata` returns undefined when the @Directive has `jit: true`. In this

‎packages/compiler-cli/src/ngtsc/annotations/component/test/component_spec.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import {CycleAnalyzer, CycleHandlingStrategy, ImportGraph} from '../../../cycles
1313
import {ErrorCode, FatalDiagnosticError, ngErrorCode} from '../../../diagnostics';
1414
import {absoluteFrom} from '../../../file_system';
1515
import {runInEachFileSystem} from '../../../file_system/testing';
16-
import {DeferredSymbolTracker, ModuleResolver, Reference, ReferenceEmitter} from '../../../imports';
16+
import {DeferredSymbolTracker, ImportedSymbolsTracker, ModuleResolver, Reference, ReferenceEmitter} from '../../../imports';
1717
import {CompoundMetadataReader, DtsMetadataReader, HostDirectivesResolver, LocalMetadataRegistry, ResourceRegistry} from '../../../metadata';
1818
import {PartialEvaluator} from '../../../partial_evaluator';
1919
import {NOOP_PERF_RECORDER} from '../../../perf';
@@ -68,6 +68,7 @@ function setup(
6868
const typeCheckScopeRegistry =
6969
new TypeCheckScopeRegistry(scopeRegistry, metaReader, hostDirectivesResolver);
7070
const resourceLoader = new StubResourceLoader();
71+
const importTracker = new ImportedSymbolsTracker();
7172

7273
const handler = new ComponentDecoratorHandler(
7374
reflectionHost,
@@ -99,6 +100,7 @@ function setup(
99100
/* annotateForClosureCompiler */ false,
100101
NOOP_PERF_RECORDER,
101102
hostDirectivesResolver,
103+
importTracker,
102104
true,
103105
compilationMode,
104106
new DeferredSymbolTracker(checker, /* onlyExplicitDeferDependencyImports */ false),

‎packages/compiler-cli/src/ngtsc/annotations/directive/src/handler.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
import {compileClassMetadata, compileDeclareClassMetadata, compileDeclareDirectiveFromMetadata, compileDirectiveFromMetadata, ConstantPool, FactoryTarget, makeBindingParser, R3ClassMetadata, R3DirectiveMetadata, WrappedNodeExpr} from '@angular/compiler';
1010
import ts from 'typescript';
1111

12-
import {Reference, ReferenceEmitter} from '../../../imports';
12+
import {ImportedSymbolsTracker, Reference, ReferenceEmitter} from '../../../imports';
1313
import {extractSemanticTypeParameters, SemanticDepGraphUpdater} from '../../../incremental/semantic_graph';
1414
import {ClassPropertyMapping, DirectiveTypeCheckMeta, extractDirectiveTypeCheckMeta, HostDirectiveMeta, InputMapping, MatchSource, MetadataReader, MetadataRegistry, MetaKind} from '../../../metadata';
1515
import {PartialEvaluator} from '../../../partial_evaluator';
@@ -62,6 +62,7 @@ export class DirectiveDecoratorHandler implements
6262
private semanticDepGraphUpdater: SemanticDepGraphUpdater|null,
6363
private annotateForClosureCompiler: boolean,
6464
private perf: PerfRecorder,
65+
private importTracker: ImportedSymbolsTracker,
6566
private includeClassMetadata: boolean,
6667
private readonly compilationMode: CompilationMode,
6768
private readonly useTemplatePipeline: boolean,
@@ -104,8 +105,8 @@ export class DirectiveDecoratorHandler implements
104105
this.perf.eventCount(PerfEvent.AnalyzeDirective);
105106

106107
const directiveResult = extractDirectiveMetadata(
107-
node, decorator, this.reflector, this.evaluator, this.refEmitter, this.referencesRegistry,
108-
this.isCore, this.annotateForClosureCompiler, this.compilationMode,
108+
node, decorator, this.reflector, this.importTracker, this.evaluator, this.refEmitter,
109+
this.referencesRegistry, this.isCore, this.annotateForClosureCompiler, this.compilationMode,
109110
/* defaultSelector */ null, this.useTemplatePipeline);
110111
if (directiveResult === undefined) {
111112
return {};

‎packages/compiler-cli/src/ngtsc/annotations/directive/src/initializer_functions.ts‎

Lines changed: 92 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
import ts from 'typescript';
1010

11+
import {ImportedSymbolsTracker} from '../../../imports';
1112
import {ClassMember, ReflectionHost} from '../../../reflection';
1213
import {CORE_MODULE} from '../../common';
1314

@@ -39,6 +40,18 @@ interface InitializerFunctionMetadata {
3940
isRequired: boolean;
4041
}
4142

43+
/**
44+
* Metadata that can be inferred from an initializer
45+
* statically without going through the type checker.
46+
*/
47+
interface StaticInitializerData {
48+
/** Identifier in the initializer that refers to the Angular API. */
49+
node: ts.Identifier;
50+
51+
/** Whether the call is required. */
52+
isRequired: boolean;
53+
}
54+
4255
/**
4356
* Attempts to identify an Angular class member that is declared via
4457
* its initializer referring to a given initializer API function.
@@ -48,97 +61,110 @@ interface InitializerFunctionMetadata {
4861
*/
4962
export function tryParseInitializerApiMember<FnNames extends InitializerApiFunction[]>(
5063
fnNames: FnNames, member: Pick<ClassMember, 'value'>, reflector: ReflectionHost,
51-
isCore: boolean): InitializerFunctionMetadata&{apiName: FnNames[number]}|null {
64+
importTracker: ImportedSymbolsTracker): InitializerFunctionMetadata|null {
5265
if (member.value === null || !ts.isCallExpression(member.value)) {
5366
return null;
5467
}
68+
5569
const call = member.value;
70+
const staticResult = parseTopLevelCall(call, fnNames, importTracker) ||
71+
parseTopLevelRequiredCall(call, fnNames, importTracker) ||
72+
parseTopLevelCallFromNamespace(call, fnNames, importTracker);
5673

57-
// Extract target. Either:
58-
// - `[input]`
59-
// - `core.[input]`
60-
// - `input.[required]`
61-
// - `core.input.[required]`.
62-
let target = extractPropertyTarget(call.expression);
63-
if (target === null) {
74+
if (staticResult === null) {
6475
return null;
6576
}
6677

67-
// Find if the `target` matches one of the expected APIs we are looking for.
68-
// e.g. `input`, or `viewChild`.
69-
let apiName = fnNames.find(n => n === target!.text);
70-
71-
// Case 1: API is directly called. e.g. `input`
72-
// If no API name was matched, continue looking for `input.required`.
73-
if (apiName !== undefined) {
74-
if (!isReferenceToInitializerApiFunction(apiName, target, isCore, reflector)) {
75-
return null;
76-
}
77-
return {apiName, call, isRequired: false};
78-
}
79-
80-
// Case 2: API is the `.required`
81-
// Ensure there is a property access to `[input].required` or `[core.input].required`.
82-
if (target.text !== 'required' || !ts.isPropertyAccessExpression(call.expression)) {
78+
// Once we've statically determined that the initializer is one of the APIs we're looking for, we
79+
// need to verify it using the type checker which accounts for things like shadowed variables.
80+
// This should be done as the absolute last step since using the type check can be expensive.
81+
const resolvedImport = reflector.getImportOfIdentifier(staticResult.node);
82+
if (resolvedImport === null || !(fnNames as string[]).includes(resolvedImport.name)) {
8383
return null;
8484
}
8585

86-
// e.g. `[input.required]` (the full property access is this)
87-
const apiPropertyAccess = call.expression;
88-
// e.g. `[input].required` (we now extract the left side of the access).
89-
target = extractPropertyTarget(apiPropertyAccess.expression);
90-
if (target === null) {
91-
return null;
92-
}
86+
return {
87+
call,
88+
isRequired: staticResult.isRequired,
89+
apiName: resolvedImport.name as InitializerApiFunction,
90+
};
91+
}
9392

94-
// Find if the `target` matches one of the expected APIs are are looking for.
95-
apiName = fnNames.find(n => n === target!.text);
93+
/**
94+
* Attempts to parse a top-level call to an initializer function,
95+
* e.g. `prop = input()`. Returns null if it can't be parsed.
96+
*/
97+
function parseTopLevelCall(
98+
call: ts.CallExpression, fnNames: InitializerApiFunction[],
99+
importTracker: ImportedSymbolsTracker): StaticInitializerData|null {
100+
const node = call.expression;
96101

97-
// Ensure the call refers to the real API function from Angular core.
98-
if (apiName === undefined ||
99-
!isReferenceToInitializerApiFunction(apiName, target, isCore, reflector)) {
102+
if (!ts.isIdentifier(node)) {
100103
return null;
101104
}
102105

103-
return {
104-
apiName,
105-
call,
106-
isRequired: true,
107-
};
106+
return fnNames.some(
107+
name => importTracker.isPotentialReferenceToNamedImport(node, name, CORE_MODULE)) ?
108+
{node, isRequired: false} :
109+
null;
108110
}
109111

110112
/**
111-
* Extracts the identifier property target of a expression, supporting
112-
* one level deep property accesses.
113-
*
114-
* e.g. `input.required` will return `required`.
115-
* e.g. `input` will return `input`.
116-
*
113+
* Attempts to parse a top-level call to a required initializer,
114+
* e.g. `prop = input.required()`. Returns null if it can't be parsed.
117115
*/
118-
function extractPropertyTarget(node: ts.Expression): ts.Identifier|null {
119-
if (ts.isPropertyAccessExpression(node) && ts.isIdentifier(node.name)) {
120-
return node.name;
121-
} else if (ts.isIdentifier(node)) {
122-
return node;
116+
function parseTopLevelRequiredCall(
117+
call: ts.CallExpression, fnNames: InitializerApiFunction[],
118+
importTracker: ImportedSymbolsTracker): StaticInitializerData|null {
119+
const node = call.expression;
120+
121+
if (!ts.isPropertyAccessExpression(node) || !ts.isIdentifier(node.expression) ||
122+
node.name.text !== 'required') {
123+
return null;
123124
}
124-
return null;
125+
126+
const expression = node.expression;
127+
const matchesCoreApi = fnNames.some(
128+
name => importTracker.isPotentialReferenceToNamedImport(expression, name, CORE_MODULE));
129+
130+
return matchesCoreApi ? {node: expression, isRequired: true} : null;
125131
}
126132

133+
127134
/**
128-
* Verifies that the given identifier resolves to the given initializer API
129-
* function expression from Angular core.
135+
* Attempts to parse a top-level call to a function referenced via a namespace import,
136+
* e.g. `prop = core.input.required()`. Returns null if it can't be parsed.
130137
*/
131-
function isReferenceToInitializerApiFunction(
132-
functionName: InitializerApiFunction, target: ts.Identifier, isCore: boolean,
133-
reflector: ReflectionHost): boolean {
134-
let targetImport: {name: string, from: string}|null = reflector.getImportOfIdentifier(target);
135-
if (targetImport === null) {
136-
if (!isCore) {
137-
return false;
138-
}
139-
// We are compiling the core module, where no import can be present.
140-
targetImport = {name: target.text, from: CORE_MODULE};
138+
function parseTopLevelCallFromNamespace(
139+
call: ts.CallExpression, fnNames: InitializerApiFunction[],
140+
importTracker: ImportedSymbolsTracker): StaticInitializerData|null {
141+
const node = call.expression;
142+
143+
if (!ts.isPropertyAccessExpression(node)) {
144+
return null;
145+
}
146+
147+
let apiReference: ts.Identifier|null = null;
148+
let isRequired = false;
149+
150+
// `prop = core.input()`
151+
if (ts.isIdentifier(node.expression) && ts.isIdentifier(node.name) &&
152+
importTracker.isPotentialReferenceToNamespaceImport(node.expression, CORE_MODULE)) {
153+
apiReference = node.name;
154+
} else if (
155+
// `prop = core.input.required()`
156+
ts.isPropertyAccessExpression(node.expression) &&
157+
ts.isIdentifier(node.expression.expression) && ts.isIdentifier(node.expression.name) &&
158+
importTracker.isPotentialReferenceToNamespaceImport(
159+
node.expression.expression, CORE_MODULE) &&
160+
node.name.text === 'required') {
161+
apiReference = node.expression.name;
162+
isRequired = true;
163+
}
164+
165+
if (apiReference === null || !(fnNames as string[]).includes(apiReference.text)) {
166+
return null;
141167
}
142168

143-
return targetImport.name === functionName && targetImport.from === CORE_MODULE;
169+
return {node: apiReference, isRequired};
144170
}

‎packages/compiler-cli/src/ngtsc/annotations/directive/src/input_function.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@
88

99
import ts from 'typescript';
1010

11-
import {ErrorCode, FatalDiagnosticError} from '../../../diagnostics';
11+
import {ImportedSymbolsTracker} from '../../../imports';
1212
import {InputMapping} from '../../../metadata';
13-
import {ClassMember, ReflectionHost, reflectObjectLiteral} from '../../../reflection';
13+
import {ClassMember, ReflectionHost} from '../../../reflection';
1414

1515
import {tryParseInitializerApiMember} from './initializer_functions';
1616
import {parseAndValidateInputAndOutputOptions} from './input_output_parse_options';
@@ -21,8 +21,8 @@ import {parseAndValidateInputAndOutputOptions} from './input_output_parse_option
2121
*/
2222
export function tryParseSignalInputMapping(
2323
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
24-
isCore: boolean): InputMapping|null {
25-
const signalInput = tryParseInitializerApiMember(['input'], member, reflector, isCore);
24+
importTracker: ImportedSymbolsTracker): InputMapping|null {
25+
const signalInput = tryParseInitializerApiMember(['input'], member, reflector, importTracker);
2626
if (signalInput === null) {
2727
return null;
2828
}

‎packages/compiler-cli/src/ngtsc/annotations/directive/src/model_function.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
import ts from 'typescript';
1010

11+
import {ImportedSymbolsTracker} from '../../../imports';
1112
import {ModelMapping} from '../../../metadata';
1213
import {ClassMember, ReflectionHost} from '../../../reflection';
1314

@@ -19,8 +20,8 @@ import {parseAndValidateInputAndOutputOptions} from './input_output_parse_option
1920
*/
2021
export function tryParseSignalModelMapping(
2122
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
22-
isCore: boolean): ModelMapping|null {
23-
const model = tryParseInitializerApiMember(['model'], member, reflector, isCore);
23+
importTracker: ImportedSymbolsTracker): ModelMapping|null {
24+
const model = tryParseInitializerApiMember(['model'], member, reflector, importTracker);
2425
if (model === null) {
2526
return null;
2627
}

‎packages/compiler-cli/src/ngtsc/annotations/directive/src/output_function.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import ts from 'typescript';
1010

1111
import {ErrorCode, FatalDiagnosticError} from '../../../diagnostics';
12+
import {ImportedSymbolsTracker} from '../../../imports';
1213
import {InputOrOutput} from '../../../metadata';
1314
import {ClassMember, ReflectionHost} from '../../../reflection';
1415

@@ -21,8 +22,10 @@ import {parseAndValidateInputAndOutputOptions} from './input_output_parse_option
2122
*/
2223
export function tryParseInitializerBasedOutput(
2324
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
24-
isCore: boolean): {call: ts.CallExpression, metadata: InputOrOutput}|null {
25-
const output = tryParseInitializerApiMember(['output', 'ɵoutput'], member, reflector, isCore);
25+
importTracker: ImportedSymbolsTracker): {call: ts.CallExpression, metadata: InputOrOutput}|
26+
null {
27+
const output =
28+
tryParseInitializerApiMember(['output', 'ɵoutput'], member, reflector, importTracker);
2629
if (output === null) {
2730
return null;
2831
}

‎packages/compiler-cli/src/ngtsc/annotations/directive/src/query_functions.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {createMayBeForwardRefExpression, ForwardRefHandling, MaybeForwardRefExpr
1111
import ts from 'typescript';
1212

1313
import {ErrorCode, FatalDiagnosticError} from '../../../diagnostics';
14+
import {ImportedSymbolsTracker} from '../../../imports';
1415
import {ClassMember, ReflectionHost, reflectObjectLiteral} from '../../../reflection';
1516
import {tryUnwrapForwardRef} from '../../common';
1617

@@ -35,9 +36,10 @@ const defaultDescendantsValue = (type: QueryFunctionName) => type !== 'contentCh
3536
* @returns Resolved query metadata, or null if no query is declared.
3637
*/
3738
export function tryParseSignalQueryFromInitializer(
38-
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost, isCore: boolean):
39+
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
40+
importTracker: ImportedSymbolsTracker):
3941
{name: QueryFunctionName, metadata: R3QueryMetadata, call: ts.CallExpression}|null {
40-
const query = tryParseInitializerApiMember(queryFunctionNames, member, reflector, isCore);
42+
const query = tryParseInitializerApiMember(queryFunctionNames, member, reflector, importTracker);
4143
if (query === null) {
4244
return null;
4345
}
@@ -58,10 +60,10 @@ export function tryParseSignalQueryFromInitializer(
5860
const read = options?.has('read') ? parseReadOption(options.get('read')!) : null;
5961
const descendants = options?.has('descendants') ?
6062
parseDescendantsOption(options.get('descendants')!) :
61-
defaultDescendantsValue(query.apiName);
63+
defaultDescendantsValue(query.apiName as QueryFunctionName);
6264

6365
return {
64-
name: query.apiName,
66+
name: query.apiName as QueryFunctionName,
6567
call: query.call,
6668
metadata: {
6769
isSignal: true,

0 commit comments

Comments
 (0)