Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ivy): reuse compilation scope for incremental template changes. #31932

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -75,7 +75,7 @@ export class DecorationAnalyzer {
new BaseDefDecoratorHandler(this.reflectionHost, this.evaluator, this.isCore),
new ComponentDecoratorHandler(
this.reflectionHost, this.evaluator, this.fullRegistry, this.fullMetaReader,
this.scopeRegistry, this.isCore, this.resourceManager, this.rootDirs,
this.scopeRegistry, this.scopeRegistry, this.isCore, this.resourceManager, this.rootDirs,
/* defaultPreserveWhitespaces */ false,
/* i18nUseExternalIds */ true, this.moduleResolver, this.cycleAnalyzer, this.refEmitter,
NOOP_DEFAULT_IMPORT_RECORDER),
Expand Down
12 changes: 6 additions & 6 deletions packages/compiler-cli/src/ngtsc/annotations/src/component.ts
Expand Up @@ -18,7 +18,7 @@ import {DirectiveMeta, MetadataReader, MetadataRegistry, extractDirectiveGuards}
import {flattenInheritedDirectiveMetadata} from '../../metadata/src/inheritance';
import {EnumValue, PartialEvaluator} from '../../partial_evaluator';
import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection';
import {LocalModuleScopeRegistry} from '../../scope';
import {ComponentScopeReader, LocalModuleScopeRegistry} from '../../scope';
import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence, ResolveResult} from '../../transform';
import {TypeCheckContext} from '../../typecheck';
import {NoopResourceDependencyRecorder, ResourceDependencyRecorder} from '../../util/src/resource_recorder';
Expand Down Expand Up @@ -47,8 +47,8 @@ export class ComponentDecoratorHandler implements
constructor(
private reflector: ReflectionHost, private evaluator: PartialEvaluator,
private metaRegistry: MetadataRegistry, private metaReader: MetadataReader,
private scopeRegistry: LocalModuleScopeRegistry, private isCore: boolean,
private resourceLoader: ResourceLoader, private rootDirs: string[],
private scopeReader: ComponentScopeReader, private scopeRegistry: LocalModuleScopeRegistry,
private isCore: boolean, private resourceLoader: ResourceLoader, private rootDirs: string[],
private defaultPreserveWhitespaces: boolean, private i18nUseExternalIds: boolean,
private moduleResolver: ModuleResolver, private cycleAnalyzer: CycleAnalyzer,
private refEmitter: ReferenceEmitter, private defaultImportRecorder: DefaultImportRecorder,
Expand Down Expand Up @@ -327,7 +327,7 @@ export class ComponentDecoratorHandler implements
preserveWhitespaces: true,
leadingTriviaChars: [],
});
const scope = this.scopeRegistry.getScopeForComponent(node);
const scope = this.scopeReader.getScopeForComponent(node);
const selector = analysis.meta.selector;
const matcher = new SelectorMatcher<DirectiveMeta>();
if (scope !== null) {
Expand All @@ -353,7 +353,7 @@ export class ComponentDecoratorHandler implements
if (!ts.isClassDeclaration(node)) {
return;
}
const scope = this.scopeRegistry.getScopeForComponent(node);
const scope = this.scopeReader.getScopeForComponent(node);
const matcher = new SelectorMatcher<DirectiveMeta>();
if (scope !== null) {
for (const meta of scope.compilation.directives) {
Expand All @@ -377,7 +377,7 @@ export class ComponentDecoratorHandler implements
const context = node.getSourceFile();
// Check whether this component was registered with an NgModule. If so, it should be compiled
// under that module's compilation scope.
const scope = this.scopeRegistry.getScopeForComponent(node);
const scope = this.scopeReader.getScopeForComponent(node);
let metadata = analysis.meta;
if (scope !== null) {
// Replace the empty components and directives from the analyze() step with a fully expanded
Expand Down
Expand Up @@ -60,7 +60,7 @@ runInEachFileSystem(() => {
const refEmitter = new ReferenceEmitter([]);

const handler = new ComponentDecoratorHandler(
reflectionHost, evaluator, metaRegistry, metaReader, scopeRegistry, false,
reflectionHost, evaluator, metaRegistry, metaReader, scopeRegistry, scopeRegistry, false,
new NoopResourceLoader(), [''], false, true, moduleResolver, cycleAnalyzer, refEmitter,
NOOP_DEFAULT_IMPORT_RECORDER);
const TestCmp = getDeclaration(program, _('/entry.ts'), 'TestCmp', isNamedClassDeclaration);
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel
Expand Up @@ -12,6 +12,7 @@ ts_library(
"//packages/compiler-cli/src/ngtsc/metadata",
"//packages/compiler-cli/src/ngtsc/partial_evaluator",
"//packages/compiler-cli/src/ngtsc/reflection",
"//packages/compiler-cli/src/ngtsc/scope",
"//packages/compiler-cli/src/ngtsc/util",
"@npm//typescript",
],
Expand Down
79 changes: 70 additions & 9 deletions packages/compiler-cli/src/ngtsc/incremental/src/state.ts
Expand Up @@ -12,13 +12,14 @@ import {Reference} from '../../imports';
import {DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from '../../metadata';
import {DependencyTracker} from '../../partial_evaluator';
import {ClassDeclaration} from '../../reflection';
import {ComponentScopeReader, ComponentScopeRegistry, LocalModuleScope} from '../../scope';
import {ResourceDependencyRecorder} from '../../util/src/resource_recorder';

/**
* Accumulates state between compilations.
*/
export class IncrementalState implements DependencyTracker, MetadataReader, MetadataRegistry,
ResourceDependencyRecorder {
ResourceDependencyRecorder, ComponentScopeRegistry, ComponentScopeReader {
private constructor(
private unchangedFiles: Set<ts.SourceFile>,
private metadata: Map<ts.SourceFile, FileMetadata>,
Expand Down Expand Up @@ -69,32 +70,56 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta
}

getFileDependencies(file: ts.SourceFile): ts.SourceFile[] {
const meta = this.metadata.get(file);
return meta ? Array.from(meta.fileDependencies) : [];
if (!this.metadata.has(file)) {
return [];
}
const meta = this.metadata.get(file) !;
return Array.from(meta.fileDependencies);
}

getNgModuleMetadata(ref: Reference<ClassDeclaration>): NgModuleMeta|null {
const metadata = this.metadata.get(ref.node.getSourceFile()) || null;
return metadata && metadata.ngModuleMeta.get(ref.node) || null;
if (!this.metadata.has(ref.node.getSourceFile())) {
return null;
}
const metadata = this.metadata.get(ref.node.getSourceFile()) !;
if (!metadata.ngModuleMeta.has(ref.node)) {
return null;
}
return metadata.ngModuleMeta.get(ref.node) !;
}

registerNgModuleMetadata(meta: NgModuleMeta): void {
const metadata = this.ensureMetadata(meta.ref.node.getSourceFile());
metadata.ngModuleMeta.set(meta.ref.node, meta);
}

getDirectiveMetadata(ref: Reference<ClassDeclaration>): DirectiveMeta|null {
const metadata = this.metadata.get(ref.node.getSourceFile()) || null;
return metadata && metadata.directiveMeta.get(ref.node) || null;
if (!this.metadata.has(ref.node.getSourceFile())) {
return null;
}
const metadata = this.metadata.get(ref.node.getSourceFile()) !;
if (!metadata.directiveMeta.has(ref.node)) {
return null;
}
return metadata.directiveMeta.get(ref.node) !;
}

registerDirectiveMetadata(meta: DirectiveMeta): void {
const metadata = this.ensureMetadata(meta.ref.node.getSourceFile());
metadata.directiveMeta.set(meta.ref.node, meta);
}

getPipeMetadata(ref: Reference<ClassDeclaration>): PipeMeta|null {
const metadata = this.metadata.get(ref.node.getSourceFile()) || null;
return metadata && metadata.pipeMeta.get(ref.node) || null;
if (!this.metadata.has(ref.node.getSourceFile())) {
return null;
}
const metadata = this.metadata.get(ref.node.getSourceFile()) !;
if (!metadata.pipeMeta.has(ref.node)) {
return null;
}
return metadata.pipeMeta.get(ref.node) !;
}

registerPipeMetadata(meta: PipeMeta): void {
const metadata = this.ensureMetadata(meta.ref.node.getSourceFile());
metadata.pipeMeta.set(meta.ref.node, meta);
Expand All @@ -105,6 +130,40 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta
metadata.resourcePaths.add(resourcePath);
}

registerComponentScope(clazz: ClassDeclaration, scope: LocalModuleScope): void {
const metadata = this.ensureMetadata(clazz.getSourceFile());
metadata.componentScope.set(clazz, scope);
}

getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
if (!this.metadata.has(clazz.getSourceFile())) {
return null;
}
const metadata = this.metadata.get(clazz.getSourceFile()) !;
if (!metadata.componentScope.has(clazz)) {
return null;
}
return metadata.componentScope.get(clazz) !;
}

setComponentAsRequiringRemoteScoping(clazz: ClassDeclaration): void {
petebacondarwin marked this conversation as resolved.
Show resolved Hide resolved
const metadata = this.ensureMetadata(clazz.getSourceFile());
metadata.remoteScoping.add(clazz);
}

getRequiresRemoteScope(clazz: ClassDeclaration): boolean|null {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you file a JIRA and add a TODO here...

Remote scoping is gonna be tricky. If a component requires remote scoping, it means that the component's template changing requires the module to be re-emitted. Also, we need to make sure the cycle detector works well across rebuilds. Incremental compilation is ugly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// TODO: https://angular-team.atlassian.net/browse/FW-1501
// Handle the incremental build case where a component requires remote scoping.
// This means that if the the component's template changes, it requires the module to be
// re-emitted.
// Also, we need to make sure the cycle detector works well across rebuilds.
if (!this.metadata.has(clazz.getSourceFile())) {
return null;
}
const metadata = this.metadata.get(clazz.getSourceFile()) !;
return metadata.remoteScoping.has(clazz);
}

private ensureMetadata(sf: ts.SourceFile): FileMetadata {
const metadata = this.metadata.get(sf) || new FileMetadata();
this.metadata.set(sf, metadata);
Expand All @@ -131,4 +190,6 @@ class FileMetadata {
directiveMeta = new Map<ClassDeclaration, DirectiveMeta>();
ngModuleMeta = new Map<ClassDeclaration, NgModuleMeta>();
pipeMeta = new Map<ClassDeclaration, PipeMeta>();
componentScope = new Map<ClassDeclaration, LocalModuleScope>();
remoteScoping = new Set<ClassDeclaration>();
}
14 changes: 8 additions & 6 deletions packages/compiler-cli/src/ngtsc/program.ts
Expand Up @@ -28,7 +28,7 @@ import {NOOP_PERF_RECORDER, PerfRecorder, PerfTracker} from './perf';
import {TypeScriptReflectionHost} from './reflection';
import {HostResourceLoader} from './resource_loader';
import {NgModuleRouteAnalyzer, entryPointKeyFor} from './routing';
import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from './scope';
import {CompoundComponentScopeReader, LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from './scope';
import {FactoryGenerator, FactoryInfo, GeneratedShimsHostWrapper, ShimGenerator, SummaryGenerator, TypeCheckShimGenerator, generatedFactoryTransform} from './shims';
import {ivySwitchTransform} from './switch';
import {IvyCompilation, declarationTransformFactory, ivyTransformFactory} from './transform';
Expand Down Expand Up @@ -476,7 +476,8 @@ export class NgtscProgram implements api.Program {
const localMetaReader = new CompoundMetadataReader([localMetaRegistry, this.incrementalState]);
const depScopeReader = new MetadataDtsModuleScopeResolver(dtsReader, aliasGenerator);
const scopeRegistry = new LocalModuleScopeRegistry(
localMetaReader, depScopeReader, this.refEmitter, aliasGenerator);
localMetaReader, depScopeReader, this.refEmitter, aliasGenerator, this.incrementalState);
const scopeReader = new CompoundComponentScopeReader([scopeRegistry, this.incrementalState]);
const metaRegistry =
new CompoundMetadataRegistry([localMetaRegistry, scopeRegistry, this.incrementalState]);

Expand All @@ -502,10 +503,11 @@ export class NgtscProgram implements api.Program {
const handlers = [
new BaseDefDecoratorHandler(this.reflector, evaluator, this.isCore),
new ComponentDecoratorHandler(
this.reflector, evaluator, metaRegistry, this.metaReader !, scopeRegistry, this.isCore,
this.resourceManager, this.rootDirs, this.options.preserveWhitespaces || false,
this.options.i18nUseExternalIds !== false, this.moduleResolver, this.cycleAnalyzer,
this.refEmitter, this.defaultImportTracker, this.incrementalState),
this.reflector, evaluator, metaRegistry, this.metaReader !, scopeReader, scopeRegistry,
this.isCore, this.resourceManager, this.rootDirs,
this.options.preserveWhitespaces || false, this.options.i18nUseExternalIds !== false,
this.moduleResolver, this.cycleAnalyzer, this.refEmitter, this.defaultImportTracker,
this.incrementalState),
new DirectiveDecoratorHandler(
this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore),
new InjectableDecoratorHandler(
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/scope/index.ts
Expand Up @@ -7,5 +7,6 @@
*/

export {ExportScope, ScopeData} from './src/api';
export {ComponentScopeReader, ComponentScopeRegistry, CompoundComponentScopeReader} from './src/component_scope';
export {DtsModuleScopeResolver, MetadataDtsModuleScopeResolver} from './src/dependency';
export {LocalModuleScope, LocalModuleScopeRegistry, LocalNgModuleData} from './src/local';
67 changes: 67 additions & 0 deletions packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts
@@ -0,0 +1,67 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {ClassDeclaration} from '../../reflection';
import {LocalModuleScope} from './local';

/**
* Register information about the compilation scope of components.
*/
export interface ComponentScopeRegistry {
registerComponentScope(clazz: ClassDeclaration, scope: LocalModuleScope): void;
setComponentAsRequiringRemoteScoping(clazz: ClassDeclaration): void;
}

/**
* Read information about the compilation scope of components.
*/
export interface ComponentScopeReader {
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null;
getRequiresRemoteScope(clazz: ClassDeclaration): boolean|null;
}

/**
* A noop registry that doesn't do anything.
*
* This can be used in tests and cases where we don't care about the compilation scopes
* being registered.
*/
export class NoopComponentScopeRegistry implements ComponentScopeRegistry {
registerComponentScope(clazz: ClassDeclaration, scope: LocalModuleScope): void {}
setComponentAsRequiringRemoteScoping(clazz: ClassDeclaration): void {}
}

/**
* A `ComponentScopeReader` that reads from an ordered set of child readers until it obtains the
* requested scope.
*
* This is used to combine `ComponentScopeReader`s that read from different sources (e.g. from a
* registry and from the incremental state).
*/
export class CompoundComponentScopeReader implements ComponentScopeReader {
constructor(private readers: ComponentScopeReader[]) {}

getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
for (const reader of this.readers) {
const meta = reader.getScopeForComponent(clazz);
if (meta !== null) {
return meta;
}
}
return null;
}

getRequiresRemoteScope(clazz: ClassDeclaration): boolean|null {
for (const reader of this.readers) {
const requiredScoping = reader.getRequiresRemoteScope(clazz);
if (requiredScoping !== null) {
return requiredScoping;
}
}
return null;
}
}
18 changes: 12 additions & 6 deletions packages/compiler-cli/src/ngtsc/scope/src/local.ts
Expand Up @@ -11,11 +11,12 @@ import * as ts from 'typescript';

import {ErrorCode, makeDiagnostic} from '../../diagnostics';
import {AliasGenerator, Reexport, Reference, ReferenceEmitter} from '../../imports';
import {DirectiveMeta, LocalMetadataRegistry, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from '../../metadata';
import {DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from '../../metadata';
import {ClassDeclaration} from '../../reflection';
import {identifierOfNode, nodeNameForError} from '../../util/src/typescript';

import {ExportScope, ScopeData} from './api';
import {ComponentScopeReader, ComponentScopeRegistry, NoopComponentScopeRegistry} from './component_scope';
import {DtsModuleScopeResolver} from './dependency';

export interface LocalNgModuleData {
Expand Down Expand Up @@ -58,7 +59,7 @@ export interface CompilationScope extends ScopeData {
* The `LocalModuleScopeRegistry` is also capable of producing `ts.Diagnostic` errors when Angular
* semantics are violated.
*/
export class LocalModuleScopeRegistry implements MetadataRegistry {
export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScopeReader {
/**
* Tracks whether the registry has been asked to produce scopes for a module or component. Once
* this is true, the registry cannot accept registrations of new directives/pipes/modules as it
Expand Down Expand Up @@ -102,7 +103,8 @@ export class LocalModuleScopeRegistry implements MetadataRegistry {

constructor(
private localReader: MetadataReader, private dependencyScopeReader: DtsModuleScopeResolver,
private refEmitter: ReferenceEmitter, private aliasGenerator: AliasGenerator|null) {}
private refEmitter: ReferenceEmitter, private aliasGenerator: AliasGenerator|null,
private componentScopeRegistry: ComponentScopeRegistry = new NoopComponentScopeRegistry()) {}

/**
* Add an NgModule's data to the registry.
Expand All @@ -120,10 +122,13 @@ export class LocalModuleScopeRegistry implements MetadataRegistry {
registerPipeMetadata(pipe: PipeMeta): void {}

getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
if (!this.declarationToModule.has(clazz)) {
return null;
const scope = !this.declarationToModule.has(clazz) ?
null :
this.getScopeOfModule(this.declarationToModule.get(clazz) !);
if (scope !== null) {
this.componentScopeRegistry.registerComponentScope(clazz, scope);
}
return this.getScopeOfModule(this.declarationToModule.get(clazz) !);
return scope;
}

/**
Expand Down Expand Up @@ -383,6 +388,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry {
*/
setComponentAsRequiringRemoteScoping(node: ClassDeclaration): void {
this.remoteScoping.add(node);
this.componentScopeRegistry.setComponentAsRequiringRemoteScoping(node);
}

/**
Expand Down