cargdev/angular
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(ivy): correctly type class declarations in ngtsc/ngcc (#29209)

Browse Source 
Previously, several `ngtsc` and `ngcc` APIs dealing with class
declaration nodes used inconsistent types. For example, some methods of
the `DecoratorHandler` interface expected a `ts.Declaration` argument,
but actual `DecoratorHandler` implementations specified a stricter
`ts.ClassDeclaration` type.

As a result, the stricter methods would operate under the incorrect
assumption that their arguments were of type `ts.ClassDeclaration`,
while the actual arguments might be of different types (e.g. `ngcc`
would call them with `ts.FunctionDeclaration` or
`ts.VariableDeclaration` arguments, when compiling ES5 code).

Additionally, since we need those class declarations to be referenced in
other parts of the program, `ngtsc`/`ngcc` had to either repeatedly
check for `ts.isIdentifier(node.name)` or assume there was a `name`
identifier and use `node.name!`. While this assumption happens to be
true in the current implementation, working around type-checking is
error-prone (e.g. the assumption might stop being true in the future).

This commit fixes this by introducing a new type to be used for such
class declarations (`ts.Declaration & {name: ts.Identifier}`) and using
it consistently throughput the code.

PR Close #29209
This commit is contained in:
George Kalpakas
2019-03-20 12:10:57 +02:00
committed by Miško Hevery
parent 2d859a8c3a
commit bb6a3632f6
36 changed files with 229 additions and 194 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
packages/compiler-cli/src/ngtsc/scope/src/api.ts
View File

@ -6,9 +6,8 @@
* found in the LICENSE file at https://angular.io/license
*/
import * as ts from 'typescript';
import {Reference} from '../../imports';
import {ClassDeclaration} from '../../reflection';
import {TypeCheckableDirectiveMeta} from '../../typecheck';
/**
@ -51,6 +50,6 @@ export interface ScopeDirective extends TypeCheckableDirectiveMeta {
* Metadata for a given pipe within an NgModule's scope.
*/
export interface ScopePipe {
ref: Reference<ts.Declaration>;
ref: Reference<ClassDeclaration>;
name: string;
}

22
packages/compiler-cli/src/ngtsc/scope/src/dependency.ts
View File

@ -9,13 +9,13 @@
import * as ts from 'typescript';
import {AliasGenerator, Reference} from '../../imports';
import {ReflectionHost} from '../../reflection';
import {ClassDeclaration, ReflectionHost} from '../../reflection';
import {ExportScope, ScopeDirective, ScopePipe} from './api';
import {extractDirectiveGuards, extractReferencesFromType, readStringArrayType, readStringMapType, readStringType} from './util';
export interface DtsModuleScopeResolver {
resolve(ref: Reference<ts.ClassDeclaration>): ExportScope|null;
resolve(ref: Reference<ClassDeclaration>): ExportScope|null;
}
/**
@ -29,7 +29,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver {
/**
* Cache which holds fully resolved scopes for NgModule classes from .d.ts files.
*/
private cache = new Map<ts.ClassDeclaration, ExportScope|null>();
private cache = new Map<ClassDeclaration, ExportScope|null>();
constructor(
private checker: ts.TypeChecker, private reflector: ReflectionHost,
@ -42,7 +42,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver {
* This operation relies on a `Reference` instead of a direct TypeScrpt node as the `Reference`s
* produced depend on how the original NgModule was imported.
*/
resolve(ref: Reference<ts.ClassDeclaration>): ExportScope|null {
resolve(ref: Reference<ClassDeclaration>): ExportScope|null {
const clazz = ref.node;
const sourceFile = clazz.getSourceFile();
if (!sourceFile.isDeclarationFile) {
@ -64,7 +64,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver {
return null;
}
const declarations = new Set<ts.Declaration>();
const declarations = new Set<ClassDeclaration>();
for (const declRef of meta.declarations) {
declarations.add(declRef.node);
}
@ -171,7 +171,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver {
/**
* Read directive (or component) metadata from a referenced class in a .d.ts file.
*/
private readScopeDirectiveFromClassWithDef(ref: Reference<ts.ClassDeclaration>): ScopeDirective
private readScopeDirectiveFromClassWithDef(ref: Reference<ClassDeclaration>): ScopeDirective
|null {
const clazz = ref.node;
const def = this.reflector.getMembersOfClass(clazz).find(
@ -193,7 +193,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver {
return {
ref,
name: clazz.name !.text,
name: clazz.name.text,
isComponent: def.name === 'ngComponentDef', selector,
exportAs: readStringArrayType(def.type.typeArguments[2]),
inputs: readStringMapType(def.type.typeArguments[3]),
@ -206,7 +206,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver {
/**
* Read pipe metadata from a referenced class in a .d.ts file.
*/
private readScopePipeFromClassWithDef(ref: Reference<ts.ClassDeclaration>): ScopePipe|null {
private readScopePipeFromClassWithDef(ref: Reference<ClassDeclaration>): ScopePipe|null {
const def = this.reflector.getMembersOfClass(ref.node).find(
field => field.isStatic && field.name === 'ngPipeDef');
if (def === undefined) {
@ -248,7 +248,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver {
* Raw metadata read from the .d.ts info of an ngModuleDef field on a compiled NgModule class.
*/
interface RawDependencyMetadata {
declarations: Reference<ts.ClassDeclaration>[];
imports: Reference<ts.ClassDeclaration>[];
exports: Reference<ts.ClassDeclaration>[];
declarations: Reference<ClassDeclaration>[];
imports: Reference<ClassDeclaration>[];
exports: Reference<ClassDeclaration>[];
}

48
packages/compiler-cli/src/ngtsc/scope/src/local.ts
View File

@ -11,15 +11,16 @@ import * as ts from 'typescript';
import {ErrorCode, makeDiagnostic} from '../../diagnostics';
import {AliasGenerator, Reexport, Reference, ReferenceEmitter} from '../../imports';
import {ClassDeclaration} from '../../reflection';
import {identifierOfNode, nodeNameForError} from '../../util/src/typescript';
import {ExportScope, ScopeData, ScopeDirective, ScopePipe} from './api';
import {DtsModuleScopeResolver} from './dependency';
export interface LocalNgModuleData {
declarations: Reference<ts.Declaration>[];
imports: Reference<ts.Declaration>[];
exports: Reference<ts.Declaration>[];
declarations: Reference<ClassDeclaration>[];
imports: Reference<ClassDeclaration>[];
exports: Reference<ClassDeclaration>[];
}
export interface LocalModuleScope extends ExportScope {
@ -57,17 +58,17 @@ export class LocalModuleScopeRegistry {
/**
* Metadata for each local NgModule registered.
*/
private ngModuleData = new Map<ts.Declaration, LocalNgModuleData>();
private ngModuleData = new Map<ClassDeclaration, LocalNgModuleData>();
/**
* Metadata for each local directive registered.
*/
private directiveData = new Map<ts.Declaration, ScopeDirective>();
private directiveData = new Map<ClassDeclaration, ScopeDirective>();
/**
* Metadata for each local pipe registered.
*/
private pipeData = new Map<ts.Declaration, ScopePipe>();
private pipeData = new Map<ClassDeclaration, ScopePipe>();
/**
* A map of components from the current compilation unit to the NgModule which declared them.
@ -76,7 +77,7 @@ export class LocalModuleScopeRegistry {
* contain directives. This doesn't cause any problems but isn't useful as there is no concept of
* a directive's compilation scope.
*/
private declarationToModule = new Map<ts.Declaration, ts.Declaration>();
private declarationToModule = new Map<ClassDeclaration, ClassDeclaration>();
/**
* A cache of calculated `LocalModuleScope`s for each NgModule declared in the current program.
@ -84,7 +85,7 @@ export class LocalModuleScopeRegistry {
* A value of `undefined` indicates the scope was invalid and produced errors (therefore,
* diagnostics should exist in the `scopeErrors` map).
*/
private cache = new Map<ts.Declaration, LocalModuleScope|undefined>();
private cache = new Map<ClassDeclaration, LocalModuleScope|undefined>();
/**
* Tracks whether a given component requires "remote scoping".
@ -94,12 +95,12 @@ export class LocalModuleScopeRegistry {
* around cyclic import issues). This is not used in calculation of `LocalModuleScope`s, but is
* tracked here for convenience.
*/
private remoteScoping = new Set<ts.Declaration>();
private remoteScoping = new Set<ClassDeclaration>();
/**
* Tracks errors accumulated in the processing of scopes for each module declaration.
*/
private scopeErrors = new Map<ts.Declaration, ts.Diagnostic[]>();
private scopeErrors = new Map<ClassDeclaration, ts.Diagnostic[]>();
constructor(
private dependencyScopeReader: DtsModuleScopeResolver, private refEmitter: ReferenceEmitter,
@ -108,7 +109,7 @@ export class LocalModuleScopeRegistry {
/**
* Add an NgModule's data to the registry.
*/
registerNgModule(clazz: ts.Declaration, data: LocalNgModuleData): void {
registerNgModule(clazz: ClassDeclaration, data: LocalNgModuleData): void {
this.assertCollecting();
this.ngModuleData.set(clazz, data);
for (const decl of data.declarations) {
@ -126,7 +127,7 @@ export class LocalModuleScopeRegistry {
this.pipeData.set(pipe.ref.node, pipe);
}
getScopeForComponent(clazz: ts.ClassDeclaration): LocalModuleScope|null {
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
if (!this.declarationToModule.has(clazz)) {
return null;
}
@ -141,7 +142,7 @@ export class LocalModuleScopeRegistry {
* `LocalModuleScope` for the given NgModule if one can be produced, and `null` if no scope is
* available or the scope contains errors.
*/
getScopeOfModule(clazz: ts.Declaration): LocalModuleScope|null {
getScopeOfModule(clazz: ClassDeclaration): LocalModuleScope|null {
const scope = this.getScopeOfModuleInternal(clazz);
// Translate undefined -> null.
return scope !== undefined ? scope : null;
@ -151,7 +152,7 @@ export class LocalModuleScopeRegistry {
* Retrieves any `ts.Diagnostic`s produced during the calculation of the `LocalModuleScope` for
* the given NgModule, or `null` if no errors were present.
*/
getDiagnosticsOfModule(clazz: ts.Declaration): ts.Diagnostic[]|null {
getDiagnosticsOfModule(clazz: ClassDeclaration): ts.Diagnostic[]|null {
// Required to ensure the errors are populated for the given class. If it has been processed
// before, this will be a no-op due to the scope cache.
this.getScopeOfModule(clazz);
@ -167,7 +168,7 @@ export class LocalModuleScopeRegistry {
* Implementation of `getScopeOfModule` which differentiates between no scope being available
* (returns `null`) and a scope being produced with errors (returns `undefined`).
*/
private getScopeOfModuleInternal(clazz: ts.Declaration): LocalModuleScope|null|undefined {
private getScopeOfModuleInternal(clazz: ClassDeclaration): LocalModuleScope|null|undefined {
// Seal the registry to protect the integrity of the `LocalModuleScope` cache.
this.sealed = true;
@ -218,8 +219,7 @@ export class LocalModuleScopeRegistry {
for (const decl of ngModule.declarations) {
if (this.directiveData.has(decl.node)) {
const directive = this.directiveData.get(decl.node) !;
compilationDirectives.set(
decl.node, {...directive, ref: decl as Reference<ts.ClassDeclaration>});
compilationDirectives.set(decl.node, {...directive, ref: decl});
} else if (this.pipeData.has(decl.node)) {
const pipe = this.pipeData.get(decl.node) !;
compilationPipes.set(decl.node, {...pipe, ref: decl});
@ -303,7 +303,7 @@ export class LocalModuleScopeRegistry {
let reexports: Reexport[]|null = null;
if (this.aliasGenerator !== null) {
reexports = [];
const addReexport = (ref: Reference<ts.Declaration>) => {
const addReexport = (ref: Reference<ClassDeclaration>) => {
if (!declared.has(ref.node) && ref.node.getSourceFile() !== sourceFile) {
const exportName = this.aliasGenerator !.aliasSymbolName(ref.node, sourceFile);
if (ref.alias && ref.alias instanceof ExternalExpr) {
@ -362,12 +362,14 @@ export class LocalModuleScopeRegistry {
/**
* Check whether a component requires remote scoping.
*/
getRequiresRemoteScope(node: ts.Declaration): boolean { return this.remoteScoping.has(node); }
getRequiresRemoteScope(node: ClassDeclaration): boolean { return this.remoteScoping.has(node); }
/**
* Set a component as requiring remote scoping.
*/
setComponentAsRequiringRemoteScoping(node: ts.Declaration): void { this.remoteScoping.add(node); }
setComponentAsRequiringRemoteScoping(node: ClassDeclaration): void {
this.remoteScoping.add(node);
}
/**
* Look up the `ExportScope` of a given `Reference` to an NgModule.
@ -380,8 +382,8 @@ export class LocalModuleScopeRegistry {
* array parameter.
*/
private getExportedScope(
ref: Reference<ts.Declaration>, diagnostics: ts.Diagnostic[], ownerForErrors: ts.Declaration,
type: 'import'|'export'): ExportScope|null|undefined {
ref: Reference<ClassDeclaration>, diagnostics: ts.Diagnostic[],
ownerForErrors: ts.Declaration, type: 'import'|'export'): ExportScope|null|undefined {
if (ref.node.getSourceFile().isDeclarationFile) {
// The NgModule is declared in a .d.ts file. Resolve it with the `DependencyScopeReader`.
if (!ts.isClassDeclaration(ref.node)) {
@ -394,7 +396,7 @@ export class LocalModuleScopeRegistry {
`Appears in the NgModule.${type}s of ${nodeNameForError(ownerForErrors)}, but could not be resolved to an NgModule`));
return undefined;
}
return this.dependencyScopeReader.resolve(ref as Reference<ts.ClassDeclaration>);
return this.dependencyScopeReader.resolve(ref);
} else {
// The NgModule is declared locally in the current program. Resolve it from the registry.
return this.getScopeOfModuleInternal(ref.node);

10
packages/compiler-cli/src/ngtsc/scope/src/util.ts
View File

@ -9,12 +9,12 @@
import * as ts from 'typescript';
import {Reference} from '../../imports';
import {ClassMemberKind, ReflectionHost, reflectTypeEntityToDeclaration} from '../../reflection';
import {nodeDebugInfo} from '../../util/src/typescript';
import {ClassDeclaration, ClassMemberKind, ReflectionHost, reflectTypeEntityToDeclaration} from '../../reflection';
import {isNamedClassDeclaration, nodeDebugInfo} from '../../util/src/typescript';
export function extractReferencesFromType(
checker: ts.TypeChecker, def: ts.TypeNode, ngModuleImportedFrom: string | null,
resolutionContext: string): Reference<ts.ClassDeclaration>[] {
resolutionContext: string): Reference<ClassDeclaration>[] {
if (!ts.isTupleTypeNode(def)) {
return [];
}
@ -24,8 +24,8 @@ export function extractReferencesFromType(
}
const type = element.exprName;
const {node, from} = reflectTypeEntityToDeclaration(type, checker);
if (!ts.isClassDeclaration(node)) {
throw new Error(`Expected ClassDeclaration: ${nodeDebugInfo(node)}`);
if (!isNamedClassDeclaration(node)) {
throw new Error(`Expected named ClassDeclaration: ${nodeDebugInfo(node)}`);
}
const specifier = (from !== null && !from.startsWith('.') ? from : ngModuleImportedFrom);
if (specifier !== null) {

12
packages/compiler-cli/src/ngtsc/scope/test/dependency_spec.ts
View File

@ -10,7 +10,7 @@ import {ExternalExpr, ExternalReference} from '@angular/compiler';
import * as ts from 'typescript';
import {AliasGenerator, FileToModuleHost, Reference} from '../../imports';
import {TypeScriptReflectionHost} from '../../reflection';
import {ClassDeclaration, TypeScriptReflectionHost} from '../../reflection';
import {makeProgram} from '../../testing/in_memory_typescript';
import {ExportScope} from '../src/api';
import {MetadataDtsModuleScopeResolver} from '../src/dependency';
@ -42,7 +42,7 @@ export declare type PipeMeta<A, B> = never;
*/
function makeTestEnv(
modules: {[module: string]: string}, aliasGenerator: AliasGenerator | null = null): {
refs: {[name: string]: Reference<ts.ClassDeclaration>},
refs: {[name: string]: Reference<ClassDeclaration>},
resolver: MetadataDtsModuleScopeResolver,
} {
// Map the modules object to an array of files for `makeProgram`.
@ -123,7 +123,7 @@ describe('MetadataDtsModuleScopeResolver', () => {
export declare class Dir {
static ngDirectiveDef: DirectiveMeta<Dir, '[dir]', never, never, never, never>;
}
export declare class ModuleA {
static ngModuleDef: ModuleMeta<ModuleA, [typeof Dir], never, [typeof Dir]>;
}
@ -270,13 +270,13 @@ describe('MetadataDtsModuleScopeResolver', () => {
});
});
function scopeToRefs(scope: ExportScope): Reference<ts.ClassDeclaration>[] {
function scopeToRefs(scope: ExportScope): Reference<ClassDeclaration>[] {
const directives = scope.exported.directives.map(dir => dir.ref);
const pipes = scope.exported.pipes.map(pipe => pipe.ref as Reference<ts.ClassDeclaration>);
const pipes = scope.exported.pipes.map(pipe => pipe.ref);
return [...directives, ...pipes].sort((a, b) => a.debugName !.localeCompare(b.debugName !));
}
function getAlias(ref: Reference<ts.ClassDeclaration>): ExternalReference|null {
function getAlias(ref: Reference<ClassDeclaration>): ExternalReference|null {
if (ref.alias === null) {
return null;
} else {

15
packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts
View File

@ -9,16 +9,17 @@
import * as ts from 'typescript';
import {Reference, ReferenceEmitter} from '../../imports';
import {ClassDeclaration} from '../../reflection';
import {ScopeData, ScopeDirective, ScopePipe} from '../src/api';
import {DtsModuleScopeResolver} from '../src/dependency';
import {LocalModuleScopeRegistry} from '../src/local';
function registerFakeRefs(registry: LocalModuleScopeRegistry):
{[name: string]: Reference<ts.ClassDeclaration>} {
const get = (target: {}, name: string): Reference<ts.ClassDeclaration> => {
{[name: string]: Reference<ClassDeclaration>} {
const get = (target: {}, name: string): Reference<ClassDeclaration> => {
const sf = ts.createSourceFile(
name + '.ts', `export class ${name} {}`, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS);
const clazz = sf.statements[0] as ts.ClassDeclaration;
const clazz = sf.statements[0] as unknown as ClassDeclaration;
const ref = new Reference(clazz);
if (name.startsWith('Dir') || name.startsWith('Cmp')) {
registry.registerDirective(fakeDirective(ref));
@ -136,7 +137,7 @@ describe('LocalModuleScopeRegistry', () => {
});
});
function fakeDirective(ref: Reference<ts.ClassDeclaration>): ScopeDirective {
function fakeDirective(ref: Reference<ClassDeclaration>): ScopeDirective {
const name = ref.debugName !;
return {
ref,
@ -152,16 +153,16 @@ function fakeDirective(ref: Reference<ts.ClassDeclaration>): ScopeDirective {
};
}
function fakePipe(ref: Reference<ts.ClassDeclaration>): ScopePipe {
function fakePipe(ref: Reference<ClassDeclaration>): ScopePipe {
const name = ref.debugName !;
return {ref, name};
}
class MockDtsModuleScopeResolver implements DtsModuleScopeResolver {
resolve(ref: Reference<ts.ClassDeclaration>): null { return null; }
resolve(ref: Reference<ClassDeclaration>): null { return null; }
}
function scopeToRefs(scopeData: ScopeData): Reference<ts.Declaration>[] {
function scopeToRefs(scopeData: ScopeData): Reference<ClassDeclaration>[] {
const directives = scopeData.directives.map(dir => dir.ref);
const pipes = scopeData.pipes.map(pipe => pipe.ref);
return [...directives, ...pipes].sort((a, b) => a.debugName !.localeCompare(b.debugName !));