Skip to content

Commit

Permalink
fix(compiler-cli): identify aliased initializer functions (#54609)
Browse files Browse the repository at this point in the history
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 #54609
  • Loading branch information
crisbeto authored and dylhunn committed Feb 27, 2024
1 parent 54b5a2f commit f5c566c
Show file tree
Hide file tree
Showing 26 changed files with 592 additions and 129 deletions.
Expand Up @@ -12,7 +12,7 @@ import ts from 'typescript';
import {Cycle, CycleAnalyzer, CycleHandlingStrategy} from '../../../cycles';
import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../../diagnostics';
import {absoluteFrom, relative} from '../../../file_system';
import {assertSuccessfulReferenceEmit, DeferredSymbolTracker, ImportedFile, LocalCompilationExtraImportsTracker, ModuleResolver, Reference, ReferenceEmitter} from '../../../imports';
import {assertSuccessfulReferenceEmit, DeferredSymbolTracker, ImportedFile, ImportedSymbolsTracker, LocalCompilationExtraImportsTracker, ModuleResolver, Reference, ReferenceEmitter} from '../../../imports';
import {DependencyTracker} from '../../../incremental/api';
import {extractSemanticTypeParameters, SemanticDepGraphUpdater} from '../../../incremental/semantic_graph';
import {IndexingContext} from '../../../indexer';
Expand Down Expand Up @@ -83,7 +83,8 @@ export class ComponentDecoratorHandler implements
private injectableRegistry: InjectableClassRegistry,
private semanticDepGraphUpdater: SemanticDepGraphUpdater|null,
private annotateForClosureCompiler: boolean, private perf: PerfRecorder,
private hostDirectivesResolver: HostDirectivesResolver, private includeClassMetadata: boolean,
private hostDirectivesResolver: HostDirectivesResolver,
private importTracker: ImportedSymbolsTracker, private includeClassMetadata: boolean,
private readonly compilationMode: CompilationMode,
private readonly deferredSymbolTracker: DeferredSymbolTracker,
private readonly forbidOrphanRendering: boolean, private readonly enableBlockSyntax: boolean,
Expand Down Expand Up @@ -225,8 +226,8 @@ export class ComponentDecoratorHandler implements
// @Component inherits @Directive, so begin by extracting the @Directive metadata and building
// on it.
const directiveResult = extractDirectiveMetadata(
node, decorator, this.reflector, this.evaluator, this.refEmitter, this.referencesRegistry,
this.isCore, this.annotateForClosureCompiler, this.compilationMode,
node, decorator, this.reflector, this.importTracker, this.evaluator, this.refEmitter,
this.referencesRegistry, this.isCore, this.annotateForClosureCompiler, this.compilationMode,
this.elementSchemaRegistry.getDefaultComponentElementName(), this.useTemplatePipeline);
if (directiveResult === undefined) {
// `extractDirectiveMetadata` returns undefined when the @Directive has `jit: true`. In this
Expand Down
Expand Up @@ -13,7 +13,7 @@ import {CycleAnalyzer, CycleHandlingStrategy, ImportGraph} from '../../../cycles
import {ErrorCode, FatalDiagnosticError, ngErrorCode} from '../../../diagnostics';
import {absoluteFrom} from '../../../file_system';
import {runInEachFileSystem} from '../../../file_system/testing';
import {DeferredSymbolTracker, ModuleResolver, Reference, ReferenceEmitter} from '../../../imports';
import {DeferredSymbolTracker, ImportedSymbolsTracker, ModuleResolver, Reference, ReferenceEmitter} from '../../../imports';
import {CompoundMetadataReader, DtsMetadataReader, HostDirectivesResolver, LocalMetadataRegistry, ResourceRegistry} from '../../../metadata';
import {PartialEvaluator} from '../../../partial_evaluator';
import {NOOP_PERF_RECORDER} from '../../../perf';
Expand Down Expand Up @@ -68,6 +68,7 @@ function setup(
const typeCheckScopeRegistry =
new TypeCheckScopeRegistry(scopeRegistry, metaReader, hostDirectivesResolver);
const resourceLoader = new StubResourceLoader();
const importTracker = new ImportedSymbolsTracker();

const handler = new ComponentDecoratorHandler(
reflectionHost,
Expand Down Expand Up @@ -99,6 +100,7 @@ function setup(
/* annotateForClosureCompiler */ false,
NOOP_PERF_RECORDER,
hostDirectivesResolver,
importTracker,
true,
compilationMode,
new DeferredSymbolTracker(checker, /* onlyExplicitDeferDependencyImports */ false),
Expand Down
Expand Up @@ -9,7 +9,7 @@
import {compileClassMetadata, compileDeclareClassMetadata, compileDeclareDirectiveFromMetadata, compileDirectiveFromMetadata, ConstantPool, FactoryTarget, makeBindingParser, R3ClassMetadata, R3DirectiveMetadata, WrappedNodeExpr} from '@angular/compiler';
import ts from 'typescript';

import {Reference, ReferenceEmitter} from '../../../imports';
import {ImportedSymbolsTracker, Reference, ReferenceEmitter} from '../../../imports';
import {extractSemanticTypeParameters, SemanticDepGraphUpdater} from '../../../incremental/semantic_graph';
import {ClassPropertyMapping, DirectiveTypeCheckMeta, extractDirectiveTypeCheckMeta, HostDirectiveMeta, InputMapping, MatchSource, MetadataReader, MetadataRegistry, MetaKind} from '../../../metadata';
import {PartialEvaluator} from '../../../partial_evaluator';
Expand Down Expand Up @@ -62,6 +62,7 @@ export class DirectiveDecoratorHandler implements
private semanticDepGraphUpdater: SemanticDepGraphUpdater|null,
private annotateForClosureCompiler: boolean,
private perf: PerfRecorder,
private importTracker: ImportedSymbolsTracker,
private includeClassMetadata: boolean,
private readonly compilationMode: CompilationMode,
private readonly useTemplatePipeline: boolean,
Expand Down Expand Up @@ -104,8 +105,8 @@ export class DirectiveDecoratorHandler implements
this.perf.eventCount(PerfEvent.AnalyzeDirective);

const directiveResult = extractDirectiveMetadata(
node, decorator, this.reflector, this.evaluator, this.refEmitter, this.referencesRegistry,
this.isCore, this.annotateForClosureCompiler, this.compilationMode,
node, decorator, this.reflector, this.importTracker, this.evaluator, this.refEmitter,
this.referencesRegistry, this.isCore, this.annotateForClosureCompiler, this.compilationMode,
/* defaultSelector */ null, this.useTemplatePipeline);
if (directiveResult === undefined) {
return {};
Expand Down
Expand Up @@ -8,6 +8,7 @@

import ts from 'typescript';

import {ImportedSymbolsTracker} from '../../../imports';
import {ClassMember, ReflectionHost} from '../../../reflection';
import {CORE_MODULE} from '../../common';

Expand Down Expand Up @@ -39,6 +40,18 @@ interface InitializerFunctionMetadata {
isRequired: boolean;
}

/**
* Metadata that can be inferred from an initializer
* statically without going through the type checker.
*/
interface StaticInitializerData {
/** Identifier in the initializer that refers to the Angular API. */
node: ts.Identifier;

/** Whether the call is required. */
isRequired: boolean;
}

/**
* Attempts to identify an Angular class member that is declared via
* its initializer referring to a given initializer API function.
Expand All @@ -48,97 +61,110 @@ interface InitializerFunctionMetadata {
*/
export function tryParseInitializerApiMember<FnNames extends InitializerApiFunction[]>(
fnNames: FnNames, member: Pick<ClassMember, 'value'>, reflector: ReflectionHost,
isCore: boolean): InitializerFunctionMetadata&{apiName: FnNames[number]}|null {
importTracker: ImportedSymbolsTracker): InitializerFunctionMetadata|null {
if (member.value === null || !ts.isCallExpression(member.value)) {
return null;
}

const call = member.value;
const staticResult = parseTopLevelCall(call, fnNames, importTracker) ||
parseTopLevelRequiredCall(call, fnNames, importTracker) ||
parseTopLevelCallFromNamespace(call, fnNames, importTracker);

// Extract target. Either:
// - `[input]`
// - `core.[input]`
// - `input.[required]`
// - `core.input.[required]`.
let target = extractPropertyTarget(call.expression);
if (target === null) {
if (staticResult === null) {
return null;
}

// Find if the `target` matches one of the expected APIs we are looking for.
// e.g. `input`, or `viewChild`.
let apiName = fnNames.find(n => n === target!.text);

// Case 1: API is directly called. e.g. `input`
// If no API name was matched, continue looking for `input.required`.
if (apiName !== undefined) {
if (!isReferenceToInitializerApiFunction(apiName, target, isCore, reflector)) {
return null;
}
return {apiName, call, isRequired: false};
}

// Case 2: API is the `.required`
// Ensure there is a property access to `[input].required` or `[core.input].required`.
if (target.text !== 'required' || !ts.isPropertyAccessExpression(call.expression)) {
// Once we've statically determined that the initializer is one of the APIs we're looking for, we
// need to verify it using the type checker which accounts for things like shadowed variables.
// This should be done as the absolute last step since using the type check can be expensive.
const resolvedImport = reflector.getImportOfIdentifier(staticResult.node);
if (resolvedImport === null || !(fnNames as string[]).includes(resolvedImport.name)) {
return null;
}

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

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

// Ensure the call refers to the real API function from Angular core.
if (apiName === undefined ||
!isReferenceToInitializerApiFunction(apiName, target, isCore, reflector)) {
if (!ts.isIdentifier(node)) {
return null;
}

return {
apiName,
call,
isRequired: true,
};
return fnNames.some(
name => importTracker.isPotentialReferenceToNamedImport(node, name, CORE_MODULE)) ?
{node, isRequired: false} :
null;
}

/**
* Extracts the identifier property target of a expression, supporting
* one level deep property accesses.
*
* e.g. `input.required` will return `required`.
* e.g. `input` will return `input`.
*
* Attempts to parse a top-level call to a required initializer,
* e.g. `prop = input.required()`. Returns null if it can't be parsed.
*/
function extractPropertyTarget(node: ts.Expression): ts.Identifier|null {
if (ts.isPropertyAccessExpression(node) && ts.isIdentifier(node.name)) {
return node.name;
} else if (ts.isIdentifier(node)) {
return node;
function parseTopLevelRequiredCall(
call: ts.CallExpression, fnNames: InitializerApiFunction[],
importTracker: ImportedSymbolsTracker): StaticInitializerData|null {
const node = call.expression;

if (!ts.isPropertyAccessExpression(node) || !ts.isIdentifier(node.expression) ||
node.name.text !== 'required') {
return null;
}
return null;

const expression = node.expression;
const matchesCoreApi = fnNames.some(
name => importTracker.isPotentialReferenceToNamedImport(expression, name, CORE_MODULE));

return matchesCoreApi ? {node: expression, isRequired: true} : null;
}


/**
* Verifies that the given identifier resolves to the given initializer API
* function expression from Angular core.
* Attempts to parse a top-level call to a function referenced via a namespace import,
* e.g. `prop = core.input.required()`. Returns null if it can't be parsed.
*/
function isReferenceToInitializerApiFunction(
functionName: InitializerApiFunction, target: ts.Identifier, isCore: boolean,
reflector: ReflectionHost): boolean {
let targetImport: {name: string, from: string}|null = reflector.getImportOfIdentifier(target);
if (targetImport === null) {
if (!isCore) {
return false;
}
// We are compiling the core module, where no import can be present.
targetImport = {name: target.text, from: CORE_MODULE};
function parseTopLevelCallFromNamespace(
call: ts.CallExpression, fnNames: InitializerApiFunction[],
importTracker: ImportedSymbolsTracker): StaticInitializerData|null {
const node = call.expression;

if (!ts.isPropertyAccessExpression(node)) {
return null;
}

let apiReference: ts.Identifier|null = null;
let isRequired = false;

// `prop = core.input()`
if (ts.isIdentifier(node.expression) && ts.isIdentifier(node.name) &&
importTracker.isPotentialReferenceToNamespaceImport(node.expression, CORE_MODULE)) {
apiReference = node.name;
} else if (
// `prop = core.input.required()`
ts.isPropertyAccessExpression(node.expression) &&
ts.isIdentifier(node.expression.expression) && ts.isIdentifier(node.expression.name) &&
importTracker.isPotentialReferenceToNamespaceImport(
node.expression.expression, CORE_MODULE) &&
node.name.text === 'required') {
apiReference = node.expression.name;
isRequired = true;
}

if (apiReference === null || !(fnNames as string[]).includes(apiReference.text)) {
return null;
}

return targetImport.name === functionName && targetImport.from === CORE_MODULE;
return {node: apiReference, isRequired};
}
Expand Up @@ -8,9 +8,9 @@

import ts from 'typescript';

import {ErrorCode, FatalDiagnosticError} from '../../../diagnostics';
import {ImportedSymbolsTracker} from '../../../imports';
import {InputMapping} from '../../../metadata';
import {ClassMember, ReflectionHost, reflectObjectLiteral} from '../../../reflection';
import {ClassMember, ReflectionHost} from '../../../reflection';

import {tryParseInitializerApiMember} from './initializer_functions';
import {parseAndValidateInputAndOutputOptions} from './input_output_parse_options';
Expand All @@ -21,8 +21,8 @@ import {parseAndValidateInputAndOutputOptions} from './input_output_parse_option
*/
export function tryParseSignalInputMapping(
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
isCore: boolean): InputMapping|null {
const signalInput = tryParseInitializerApiMember(['input'], member, reflector, isCore);
importTracker: ImportedSymbolsTracker): InputMapping|null {
const signalInput = tryParseInitializerApiMember(['input'], member, reflector, importTracker);
if (signalInput === null) {
return null;
}
Expand Down
Expand Up @@ -8,6 +8,7 @@

import ts from 'typescript';

import {ImportedSymbolsTracker} from '../../../imports';
import {ModelMapping} from '../../../metadata';
import {ClassMember, ReflectionHost} from '../../../reflection';

Expand All @@ -19,8 +20,8 @@ import {parseAndValidateInputAndOutputOptions} from './input_output_parse_option
*/
export function tryParseSignalModelMapping(
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
isCore: boolean): ModelMapping|null {
const model = tryParseInitializerApiMember(['model'], member, reflector, isCore);
importTracker: ImportedSymbolsTracker): ModelMapping|null {
const model = tryParseInitializerApiMember(['model'], member, reflector, importTracker);
if (model === null) {
return null;
}
Expand Down
Expand Up @@ -9,6 +9,7 @@
import ts from 'typescript';

import {ErrorCode, FatalDiagnosticError} from '../../../diagnostics';
import {ImportedSymbolsTracker} from '../../../imports';
import {InputOrOutput} from '../../../metadata';
import {ClassMember, ReflectionHost} from '../../../reflection';

Expand All @@ -21,8 +22,10 @@ import {parseAndValidateInputAndOutputOptions} from './input_output_parse_option
*/
export function tryParseInitializerBasedOutput(
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
isCore: boolean): {call: ts.CallExpression, metadata: InputOrOutput}|null {
const output = tryParseInitializerApiMember(['output', 'ɵoutput'], member, reflector, isCore);
importTracker: ImportedSymbolsTracker): {call: ts.CallExpression, metadata: InputOrOutput}|
null {
const output =
tryParseInitializerApiMember(['output', 'ɵoutput'], member, reflector, importTracker);
if (output === null) {
return null;
}
Expand Down
Expand Up @@ -11,6 +11,7 @@ import {createMayBeForwardRefExpression, ForwardRefHandling, MaybeForwardRefExpr
import ts from 'typescript';

import {ErrorCode, FatalDiagnosticError} from '../../../diagnostics';
import {ImportedSymbolsTracker} from '../../../imports';
import {ClassMember, ReflectionHost, reflectObjectLiteral} from '../../../reflection';
import {tryUnwrapForwardRef} from '../../common';

Expand All @@ -35,9 +36,10 @@ const defaultDescendantsValue = (type: QueryFunctionName) => type !== 'contentCh
* @returns Resolved query metadata, or null if no query is declared.
*/
export function tryParseSignalQueryFromInitializer(
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost, isCore: boolean):
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
importTracker: ImportedSymbolsTracker):
{name: QueryFunctionName, metadata: R3QueryMetadata, call: ts.CallExpression}|null {
const query = tryParseInitializerApiMember(queryFunctionNames, member, reflector, isCore);
const query = tryParseInitializerApiMember(queryFunctionNames, member, reflector, importTracker);
if (query === null) {
return null;
}
Expand All @@ -58,10 +60,10 @@ export function tryParseSignalQueryFromInitializer(
const read = options?.has('read') ? parseReadOption(options.get('read')!) : null;
const descendants = options?.has('descendants') ?
parseDescendantsOption(options.get('descendants')!) :
defaultDescendantsValue(query.apiName);
defaultDescendantsValue(query.apiName as QueryFunctionName);

return {
name: query.apiName,
name: query.apiName as QueryFunctionName,
call: query.call,
metadata: {
isSignal: true,
Expand Down

0 comments on commit f5c566c

Please sign in to comment.