fix(ivy): track cyclic imports that are added (#29040)
ngtsc has cyclic import detection, to determine when adding an import to a directive or pipe would create a cycle. However, this detection must also account for already inserted imports, as it's possible for both directions of a circular import to be inserted by Ivy (as opposed to at least one of those edges existing in the user's program). This commit fixes the circular import detection for components to take into consideration already added edges. This is difficult for one critical reason: only edges to files which will *actually* be imported should be considered. However, that depends on which directives & pipes are used in a given template, which is currently only known by running the TemplateDefinitionBuilder during the 'compile' phase. This is too late; the decision whether to use remote scoping (which consults the import graph) is made during the 'resolve' phase, before any compilation has taken place. Thus, the only way to correctly consider synthetic edges is for the compiler to know exactly which directives & pipes are used in a template during 'resolve'. There are two ways to achieve this: 1) refactor `TemplateDefinitionBuilder` to do its work in two phases, with directive matching occurring as a separate step which can be performed earlier. 2) use the `R3TargetBinder` in the 'resolve' phase to independently bind the template and get information about used directives. Option 1 is ideal, but option 2 is currently used for practical reasons. The cost of binding the template can be shared with template-typechecking. PR Close #29040
This commit is contained in:
Alex Rickabaugh
Andrew Kushnir
parent
b50283ed67
commit
3e5c1bcb9f
|
|
@ -6,7 +6,7 @@
|
|||
* found in the LICENSE file at https://angular.io/license
|
||||
*/
|
||||
|
||||
import {ConstantPool, CssSelector, DEFAULT_INTERPOLATION_CONFIG, DomElementSchemaRegistry, Expression, ExternalExpr, InterpolationConfig, LexerRange, ParseError, R3ComponentMetadata, SelectorMatcher, Statement, TmplAstNode, WrappedNodeExpr, compileComponentFromMetadata, makeBindingParser, parseTemplate} from '@angular/compiler';
|
||||
import {BoundTarget, ConstantPool, CssSelector, DEFAULT_INTERPOLATION_CONFIG, DomElementSchemaRegistry, Expression, ExternalExpr, InterpolationConfig, LexerRange, ParseError, R3ComponentMetadata, R3TargetBinder, SelectorMatcher, Statement, TmplAstNode, WrappedNodeExpr, compileComponentFromMetadata, makeBindingParser, parseTemplate} from '@angular/compiler';
|
||||
import * as path from 'path';
|
||||
import * as ts from 'typescript';
|
||||
|
||||
|
|
@ -48,6 +48,7 @@ export class ComponentDecoratorHandler implements
|
|||
private refEmitter: ReferenceEmitter) {}
|
||||
|
||||
private literalCache = new Map<Decorator, ts.ObjectLiteralExpression>();
|
||||
private boundTemplateCache = new Map<ts.Declaration, BoundTarget<ScopeDirective>>();
|
||||
private elementSchemaRegistry = new DomElementSchemaRegistry();
|
||||
|
||||
/**
|
||||
|
|
@ -323,7 +324,8 @@ export class ComponentDecoratorHandler implements
|
|||
for (const meta of scope.compilation.directives) {
|
||||
matcher.addSelectables(CssSelector.parse(meta.selector), meta);
|
||||
}
|
||||
ctx.addTemplate(node as ts.ClassDeclaration, meta.parsedTemplate, matcher);
|
||||
const bound = new R3TargetBinder(matcher).bind({template: meta.parsedTemplate});
|
||||
ctx.addTemplate(node, bound);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -337,8 +339,17 @@ export class ComponentDecoratorHandler implements
|
|||
// Replace the empty components and directives from the analyze() step with a fully expanded
|
||||
// scope. This is possible now because during resolve() the whole compilation unit has been
|
||||
// fully analyzed.
|
||||
const directives = scope.compilation.directives.map(
|
||||
dir => ({selector: dir.selector, expression: this.refEmitter.emit(dir.ref, context)}));
|
||||
|
||||
const matcher = new SelectorMatcher<ScopeDirective&{expression: Expression}>();
|
||||
const directives: {selector: string, expression: Expression}[] = [];
|
||||
|
||||
for (const dir of scope.compilation.directives) {
|
||||
const {ref, selector} = dir;
|
||||
const expression = this.refEmitter.emit(ref, context);
|
||||
directives.push({selector, expression});
|
||||
|
||||
matcher.addSelectables(CssSelector.parse(selector), {...dir, expression});
|
||||
}
|
||||
const pipes = new Map<string, Expression>();
|
||||
for (const pipe of scope.compilation.pipes) {
|
||||
pipes.set(pipe.name, this.refEmitter.emit(pipe.ref, context));
|
||||
|
|
@ -346,18 +357,35 @@ export class ComponentDecoratorHandler implements
|
|||
|
||||
// Scan through the references of the `scope.directives` array and check whether
|
||||
// any import which needs to be generated for the directive would create a cycle.
|
||||
const origin = node.getSourceFile();
|
||||
const cycleDetected = directives.some(dir => this._isCyclicImport(dir.expression, origin)) ||
|
||||
Array.from(pipes.values()).some(pipe => this._isCyclicImport(pipe, origin));
|
||||
const cycleDetected = directives.some(dir => this._isCyclicImport(dir.expression, context)) ||
|
||||
Array.from(pipes.values()).some(pipe => this._isCyclicImport(pipe, context));
|
||||
if (!cycleDetected) {
|
||||
const wrapDirectivesAndPipesInClosure =
|
||||
directives.some(
|
||||
dir => isExpressionForwardReference(dir.expression, node.name !, origin)) ||
|
||||
dir => isExpressionForwardReference(dir.expression, node.name !, context)) ||
|
||||
Array.from(pipes.values())
|
||||
.some(pipe => isExpressionForwardReference(pipe, node.name !, origin));
|
||||
.some(pipe => isExpressionForwardReference(pipe, node.name !, context));
|
||||
metadata.directives = directives;
|
||||
metadata.pipes = pipes;
|
||||
metadata.wrapDirectivesAndPipesInClosure = wrapDirectivesAndPipesInClosure;
|
||||
for (const dir of directives) {
|
||||
this._recordSyntheticImport(dir.expression, context);
|
||||
}
|
||||
for (const [name, pipe] of pipes) {
|
||||
this._recordSyntheticImport(pipe, context);
|
||||
}
|
||||
|
||||
const binder = new R3TargetBinder(matcher);
|
||||
const bound = binder.bind({template: metadata.template.nodes});
|
||||
for (const {expression} of bound.getUsedDirectives()) {
|
||||
this._recordSyntheticImport(expression, context);
|
||||
}
|
||||
|
||||
for (const name of bound.getUsedPipes()) {
|
||||
if (pipes.has(name)) {
|
||||
this._recordSyntheticImport(pipes.get(name) !, context);
|
||||
}
|
||||
}
|
||||
} else {
|
||||
this.scopeRegistry.setComponentAsRequiringRemoteScoping(node);
|
||||
}
|
||||
|
|
@ -549,13 +577,17 @@ export class ComponentDecoratorHandler implements
|
|||
};
|
||||
}
|
||||
|
||||
private _isCyclicImport(expr: Expression, origin: ts.SourceFile): boolean {
|
||||
private _expressionToImportedFile(expr: Expression, origin: ts.SourceFile): ts.SourceFile|null {
|
||||
if (!(expr instanceof ExternalExpr)) {
|
||||
return false;
|
||||
return null;
|
||||
}
|
||||
|
||||
// Figure out what file is being imported.
|
||||
const imported = this.moduleResolver.resolveModuleName(expr.value.moduleName !, origin);
|
||||
return this.moduleResolver.resolveModuleName(expr.value.moduleName !, origin);
|
||||
}
|
||||
|
||||
private _isCyclicImport(expr: Expression, origin: ts.SourceFile): boolean {
|
||||
const imported = this._expressionToImportedFile(expr, origin);
|
||||
if (imported === null) {
|
||||
return false;
|
||||
}
|
||||
|
|
@ -563,6 +595,15 @@ export class ComponentDecoratorHandler implements
|
|||
// Check whether the import is legal.
|
||||
return this.cycleAnalyzer.wouldCreateCycle(origin, imported);
|
||||
}
|
||||
|
||||
private _recordSyntheticImport(expr: Expression, origin: ts.SourceFile): void {
|
||||
const imported = this._expressionToImportedFile(expr, origin);
|
||||
if (imported === null) {
|
||||
return;
|
||||
}
|
||||
|
||||
this.cycleAnalyzer.recordSyntheticImport(origin, imported);
|
||||
}
|
||||
}
|
||||
|
||||
function getTemplateRange(templateExpr: ts.Expression) {
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@
|
|||
* found in the LICENSE file at https://angular.io/license
|
||||
*/
|
||||
|
||||
import {R3TargetBinder, SelectorMatcher, TmplAstNode} from '@angular/compiler';
|
||||
import {BoundTarget} from '@angular/compiler';
|
||||
import * as ts from 'typescript';
|
||||
|
||||
import {NoopImportRewriter, ReferenceEmitter} from '../../imports';
|
||||
|
|
@ -47,22 +47,13 @@ export class TypeCheckContext {
|
|||
* @param template AST nodes of the template being recorded.
|
||||
* @param matcher `SelectorMatcher` which tracks directives that are in scope for this template.
|
||||
*/
|
||||
addTemplate(
|
||||
node: ts.ClassDeclaration, template: TmplAstNode[],
|
||||
matcher: SelectorMatcher<TypeCheckableDirectiveMeta>): void {
|
||||
addTemplate(node: ts.ClassDeclaration, boundTarget: BoundTarget<TypeCheckableDirectiveMeta>):
|
||||
void {
|
||||
// Only write TCBs for named classes.
|
||||
if (node.name === undefined) {
|
||||
throw new Error(`Assertion: class must be named`);
|
||||
}
|
||||
|
||||
// Bind the template, which will:
|
||||
// - Extract the metadata needed to generate type check blocks.
|
||||
// - Perform directive matching, which informs the context which directives are used in the
|
||||
// template. This allows generation of type constructors for only those directives which
|
||||
// are actually used by the templates.
|
||||
const binder = new R3TargetBinder(matcher);
|
||||
const boundTarget = binder.bind({template});
|
||||
|
||||
// Get all of the directives used in the template and record type constructors for all of them.
|
||||
boundTarget.getUsedDirectives().forEach(dir => {
|
||||
const dirNode = dir.ref.node;
|
||||
|
|
|
|||
|
|
@ -2045,6 +2045,41 @@ describe('ngtsc behavioral tests', () => {
|
|||
/i\d\.ɵsetComponentScope\(NormalComponent,\s+\[NormalComponent,\s+CyclicComponent\],\s+\[\]\)/);
|
||||
expect(jsContents).not.toContain('/*__PURE__*/ i0.ɵsetComponentScope');
|
||||
});
|
||||
|
||||
it('should detect a cycle added entirely during compilation', () => {
|
||||
env.tsconfig();
|
||||
env.write('test.ts', `
|
||||
import {NgModule} from '@angular/core';
|
||||
import {ACmp} from './a';
|
||||
import {BCmp} from './b';
|
||||
|
||||
@NgModule({declarations: [ACmp, BCmp]})
|
||||
export class Module {}
|
||||
`);
|
||||
env.write('a.ts', `
|
||||
import {Component} from '@angular/core';
|
||||
|
||||
@Component({
|
||||
selector: 'a-cmp',
|
||||
template: '<b-cmp></b-cmp>',
|
||||
})
|
||||
export class ACmp {}
|
||||
`);
|
||||
env.write('b.ts', `
|
||||
import {Component} from '@angular/core';
|
||||
|
||||
@Component({
|
||||
selector: 'b-cmp',
|
||||
template: '<a-cmp></a-cmp>',
|
||||
})
|
||||
export class BCmp {}
|
||||
`);
|
||||
env.driveMain();
|
||||
const aJsContents = env.getContents('a.js');
|
||||
const bJsContents = env.getContents('b.js');
|
||||
expect(aJsContents).toMatch(/import \* as i\d? from ".\/b"/);
|
||||
expect(bJsContents).not.toMatch(/import \* as i\d? from ".\/a"/);
|
||||
});
|
||||
});
|
||||
|
||||
describe('multiple local refs', () => {
|
||||
|
|
|
|||
|
|
@ -136,4 +136,9 @@ export interface BoundTarget<DirectiveT extends DirectiveMeta> {
|
|||
* Get a list of all the directives used by the target.
|
||||
*/
|
||||
getUsedDirectives(): DirectiveT[];
|
||||
|
||||
/**
|
||||
* Get a list of all the pipes used by the target.
|
||||
*/
|
||||
getUsedPipes(): string[];
|
||||
}
|
||||
|
|
|
|||
|
|
@ -6,13 +6,14 @@
|
|||
* found in the LICENSE file at https://angular.io/license
|
||||
*/
|
||||
|
||||
import {AST, ImplicitReceiver, MethodCall, PropertyRead, PropertyWrite, RecursiveAstVisitor, SafeMethodCall, SafePropertyRead} from '../../expression_parser/ast';
|
||||
import {AST, BindingPipe, ImplicitReceiver, MethodCall, PropertyRead, PropertyWrite, RecursiveAstVisitor, SafeMethodCall, SafePropertyRead} from '../../expression_parser/ast';
|
||||
import {CssSelector, SelectorMatcher} from '../../selector';
|
||||
import {BoundAttribute, BoundEvent, BoundText, Content, Element, Icu, Node, Reference, Template, Text, TextAttribute, Variable, Visitor} from '../r3_ast';
|
||||
|
||||
import {BoundTarget, DirectiveMeta, Target, TargetBinder} from './t2_api';
|
||||
import {getAttrsForDirectiveMatching} from './util';
|
||||
|
||||
|
||||
/**
|
||||
* Processes `Target`s with a given set of directives and performs a binding operation, which
|
||||
* returns an object similar to TypeScript's `ts.TypeChecker` that contains knowledge about the
|
||||
|
|
@ -44,9 +45,10 @@ export class R3TargetBinder<DirectiveT extends DirectiveMeta> implements TargetB
|
|||
DirectiveBinder.apply(target.template, this.directiveMatcher);
|
||||
// Finally, run the TemplateBinder to bind references, variables, and other entities within the
|
||||
// template. This extracts all the metadata that doesn't depend on directive matching.
|
||||
const {expressions, symbols, nestingLevel} = TemplateBinder.apply(target.template, scope);
|
||||
const {expressions, symbols, nestingLevel, usedPipes} =
|
||||
TemplateBinder.apply(target.template, scope);
|
||||
return new R3BoundTarget(
|
||||
target, directives, bindings, references, expressions, symbols, nestingLevel);
|
||||
target, directives, bindings, references, expressions, symbols, nestingLevel, usedPipes);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -331,9 +333,11 @@ class DirectiveBinder<DirectiveT extends DirectiveMeta> implements Visitor {
|
|||
class TemplateBinder extends RecursiveAstVisitor implements Visitor {
|
||||
private visitNode: (node: Node) => void;
|
||||
|
||||
private pipesUsed: string[] = [];
|
||||
|
||||
private constructor(
|
||||
private bindings: Map<AST, Reference|Variable>,
|
||||
private symbols: Map<Reference|Variable, Template>,
|
||||
private symbols: Map<Reference|Variable, Template>, private usedPipes: Set<string>,
|
||||
private nestingLevel: Map<Template, number>, private scope: Scope,
|
||||
private template: Template|null, private level: number) {
|
||||
super();
|
||||
|
|
@ -358,16 +362,18 @@ class TemplateBinder extends RecursiveAstVisitor implements Visitor {
|
|||
expressions: Map<AST, Reference|Variable>,
|
||||
symbols: Map<Variable|Reference, Template>,
|
||||
nestingLevel: Map<Template, number>,
|
||||
usedPipes: Set<string>,
|
||||
} {
|
||||
const expressions = new Map<AST, Reference|Variable>();
|
||||
const symbols = new Map<Variable|Reference, Template>();
|
||||
const nestingLevel = new Map<Template, number>();
|
||||
const usedPipes = new Set<string>();
|
||||
// The top-level template has nesting level 0.
|
||||
const binder = new TemplateBinder(
|
||||
expressions, symbols, nestingLevel, scope, template instanceof Template ? template : null,
|
||||
0);
|
||||
expressions, symbols, usedPipes, nestingLevel, scope,
|
||||
template instanceof Template ? template : null, 0);
|
||||
binder.ingest(template);
|
||||
return {expressions, symbols, nestingLevel};
|
||||
return {expressions, symbols, nestingLevel, usedPipes};
|
||||
}
|
||||
|
||||
private ingest(template: Template|Node[]): void {
|
||||
|
|
@ -405,7 +411,8 @@ class TemplateBinder extends RecursiveAstVisitor implements Visitor {
|
|||
// Next, recurse into the template using its scope, and bumping the nesting level up by one.
|
||||
const childScope = this.scope.getChildScope(template);
|
||||
const binder = new TemplateBinder(
|
||||
this.bindings, this.symbols, this.nestingLevel, childScope, template, this.level + 1);
|
||||
this.bindings, this.symbols, this.usedPipes, this.nestingLevel, childScope, template,
|
||||
this.level + 1);
|
||||
binder.ingest(template);
|
||||
}
|
||||
|
||||
|
|
@ -437,8 +444,10 @@ class TemplateBinder extends RecursiveAstVisitor implements Visitor {
|
|||
visitBoundEvent(event: BoundEvent) { event.handler.visit(this); }
|
||||
|
||||
visitBoundText(text: BoundText) { text.value.visit(this); }
|
||||
|
||||
|
||||
visitPipe(ast: BindingPipe, context: any): any {
|
||||
this.usedPipes.add(ast.name);
|
||||
return super.visitPipe(ast, context);
|
||||
}
|
||||
|
||||
// These five types of AST expressions can refer to expression roots, which could be variables
|
||||
// or references in the current scope.
|
||||
|
|
@ -500,7 +509,7 @@ export class R3BoundTarget<DirectiveT extends DirectiveMeta> implements BoundTar
|
|||
{directive: DirectiveT, node: Element|Template}|Element|Template>,
|
||||
private exprTargets: Map<AST, Reference|Variable>,
|
||||
private symbols: Map<Reference|Variable, Template>,
|
||||
private nestingLevel: Map<Template, number>) {}
|
||||
private nestingLevel: Map<Template, number>, private usedPipes: Set<string>) {}
|
||||
|
||||
getDirectivesOfNode(node: Element|Template): DirectiveT[]|null {
|
||||
return this.directives.get(node) || null;
|
||||
|
|
@ -531,4 +540,6 @@ export class R3BoundTarget<DirectiveT extends DirectiveMeta> implements BoundTar
|
|||
this.directives.forEach(dirs => dirs.forEach(dir => set.add(dir)));
|
||||
return Array.from(set.values());
|
||||
}
|
||||
|
||||
getUsedPipes(): string[] { return Array.from(this.usedPipes); }
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue