From 763f8d470acf0bd75d692dad2b095455ab603e5d Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Fri, 13 Dec 2019 14:29:05 -0800 Subject: [PATCH] fix(ivy): validate the NgModule declarations field (#34404) This commit adds three previously missing validations to NgModule.declarations: 1. It checks that declared classes are actually within the current compilation. 2. It checks that declared classes are directives, components, or pipes. 3. It checks that classes are declared in at most one NgModule. PR Close #34404 --- .../ngcc/src/analysis/decoration_analyzer.ts | 9 +- .../src/ngtsc/annotations/src/component.ts | 10 +- .../src/ngtsc/annotations/src/directive.ts | 22 ++- .../src/ngtsc/annotations/src/ng_module.ts | 71 +++++++-- .../src/ngtsc/annotations/src/pipe.ts | 21 ++- .../src/ngtsc/annotations/src/util.ts | 63 +++++++- .../ngtsc/annotations/test/directive_spec.ts | 2 +- .../src/ngtsc/diagnostics/src/code.ts | 5 + .../src/ngtsc/imports/src/references.ts | 20 +++ .../src/ngtsc/metadata/src/api.ts | 10 ++ .../src/ngtsc/metadata/src/dts.ts | 1 + packages/compiler-cli/src/ngtsc/program.ts | 9 +- .../compiler-cli/src/ngtsc/scope/index.ts | 2 +- .../compiler-cli/src/ngtsc/scope/src/local.ts | 79 +++++++++- .../src/ngtsc/scope/test/local_spec.ts | 14 ++ .../compiler-cli/test/ngtsc/scope_spec.ts | 139 +++++++++++++++++- 16 files changed, 440 insertions(+), 37 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index fca42c24d6..9c799d0ae2 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -96,14 +96,15 @@ export class DecorationAnalyzer { // See the note in ngtsc about why this cast is needed. // clang-format off new DirectiveDecoratorHandler( - this.reflectionHost, this.evaluator, this.fullRegistry, NOOP_DEFAULT_IMPORT_RECORDER, - this.isCore, /* annotateForClosureCompiler */ false) as DecoratorHandler, + this.reflectionHost, this.evaluator, this.fullRegistry, this.scopeRegistry, + NOOP_DEFAULT_IMPORT_RECORDER, this.isCore, + /* annotateForClosureCompiler */ false) as DecoratorHandler, // clang-format on // Pipe handler must be before injectable handler in list so pipe factories are printed // before injectable factories (so injectable factories can delegate to them) new PipeDecoratorHandler( - this.reflectionHost, this.evaluator, this.metaRegistry, NOOP_DEFAULT_IMPORT_RECORDER, - this.isCore), + this.reflectionHost, this.evaluator, this.metaRegistry, this.scopeRegistry, + NOOP_DEFAULT_IMPORT_RECORDER, this.isCore), new InjectableDecoratorHandler( this.reflectionHost, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore, /* strictCtorDeps */ false, /* errorOnDuplicateProv */ false), diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 28a6dc8684..1293a676e2 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -29,7 +29,7 @@ import {ResourceLoader} from './api'; import {extractDirectiveMetadata, parseFieldArrayValue} from './directive'; import {compileNgFactoryDefField} from './factory'; import {generateSetClassMetadataCall} from './metadata'; -import {findAngularDecorator, isAngularCoreReference, isExpressionForwardReference, readBaseClass, unwrapExpression, wrapFunctionExpressionsInParens} from './util'; +import {findAngularDecorator, isAngularCoreReference, isExpressionForwardReference, makeDuplicateDeclarationError, readBaseClass, unwrapExpression, wrapFunctionExpressionsInParens} from './util'; const EMPTY_MAP = new Map(); const EMPTY_ARRAY: any[] = []; @@ -385,6 +385,14 @@ export class ComponentDecoratorHandler implements resolve(node: ClassDeclaration, analysis: Readonly): ResolveResult { + const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node); + if (duplicateDeclData !== null) { + // This component was declared twice (or more). + return { + diagnostics: [makeDuplicateDeclarationError(node, duplicateDeclData, 'Component')], + }; + } + 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. diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index 5600b840f7..64bf416728 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -15,11 +15,12 @@ import {MetadataRegistry} from '../../metadata'; import {extractDirectiveGuards} from '../../metadata/src/util'; import {DynamicValue, EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, ClassMember, ClassMemberKind, Decorator, ReflectionHost, filterToMembersWithDecorator, reflectObjectLiteral} from '../../reflection'; -import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFlags, HandlerPrecedence} from '../../transform'; +import {LocalModuleScopeRegistry} from '../../scope'; +import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFlags, HandlerPrecedence, ResolveResult} from '../../transform'; import {compileNgFactoryDefField} from './factory'; import {generateSetClassMetadataCall} from './metadata'; -import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, readBaseClass, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens} from './util'; +import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, makeDuplicateDeclarationError, readBaseClass, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens} from './util'; const EMPTY_OBJECT: {[key: string]: string} = {}; const FIELD_DECORATORS = [ @@ -41,8 +42,9 @@ export class DirectiveDecoratorHandler implements DecoratorHandler { constructor( private reflector: ReflectionHost, private evaluator: PartialEvaluator, - private metaRegistry: MetadataRegistry, private defaultImportRecorder: DefaultImportRecorder, - private isCore: boolean, private annotateForClosureCompiler: boolean) {} + private metaRegistry: MetadataRegistry, private scopeRegistry: LocalModuleScopeRegistry, + private defaultImportRecorder: DefaultImportRecorder, private isCore: boolean, + private annotateForClosureCompiler: boolean) {} readonly precedence = HandlerPrecedence.PRIMARY; readonly name = DirectiveDecoratorHandler.name; @@ -115,6 +117,18 @@ export class DirectiveDecoratorHandler implements }); } + resolve(node: ClassDeclaration): ResolveResult { + const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node); + if (duplicateDeclData !== null) { + // This directive was declared twice (or more). + return { + diagnostics: [makeDuplicateDeclarationError(node, duplicateDeclData, 'Directive')], + }; + } + + return {}; + } + compile( node: ClassDeclaration, analysis: Readonly, resolution: Readonly, pool: ConstantPool): CompileResult[] { diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index e52d9605c9..9ba2c1186e 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -9,7 +9,7 @@ import {CUSTOM_ELEMENTS_SCHEMA, Expression, ExternalExpr, InvokeFunctionExpr, LiteralArrayExpr, LiteralExpr, NO_ERRORS_SCHEMA, R3Identifiers, R3InjectorMetadata, R3NgModuleMetadata, R3Reference, STRING_TYPE, SchemaMetadata, Statement, WrappedNodeExpr, compileInjector, compileNgModule} from '@angular/compiler'; import * as ts from 'typescript'; -import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; +import {ErrorCode, FatalDiagnosticError, makeDiagnostic} from '../../diagnostics'; import {DefaultImportRecorder, Reference, ReferenceEmitter} from '../../imports'; import {MetadataReader, MetadataRegistry} from '../../metadata'; import {PartialEvaluator, ResolvedValue} from '../../partial_evaluator'; @@ -22,13 +22,14 @@ import {getSourceFile} from '../../util/src/typescript'; import {generateSetClassMetadataCall} from './metadata'; import {ReferencesRegistry} from './references_registry'; -import {combineResolvers, findAngularDecorator, forwardRefResolver, getValidConstructorDependencies, isExpressionForwardReference, toR3Reference, unwrapExpression, wrapFunctionExpressionsInParens} from './util'; +import {combineResolvers, findAngularDecorator, forwardRefResolver, getReferenceOriginForDiagnostics, getValidConstructorDependencies, isExpressionForwardReference, toR3Reference, unwrapExpression, wrapFunctionExpressionsInParens} from './util'; export interface NgModuleAnalysis { mod: R3NgModuleMetadata; inj: R3InjectorMetadata; metadataStmt: Statement|null; declarations: Reference[]; + rawDeclarations: ts.Expression|null; schemas: SchemaMetadata[]; imports: Reference[]; exports: Reference[]; @@ -104,13 +105,37 @@ export class NgModuleDecoratorHandler implements forwardRefResolver, ]); + const diagnostics: ts.Diagnostic[] = []; + // Extract the module declarations, imports, and exports. let declarationRefs: Reference[] = []; + let rawDeclarations: ts.Expression|null = null; if (ngModule.has('declarations')) { - const expr = ngModule.get('declarations') !; - const declarationMeta = this.evaluator.evaluate(expr, forwardRefResolver); - declarationRefs = this.resolveTypeList(expr, declarationMeta, name, 'declarations'); + rawDeclarations = ngModule.get('declarations') !; + const declarationMeta = this.evaluator.evaluate(rawDeclarations, forwardRefResolver); + declarationRefs = + this.resolveTypeList(rawDeclarations, declarationMeta, name, 'declarations'); + + // Look through the declarations to make sure they're all a part of the current compilation. + for (const ref of declarationRefs) { + if (ref.node.getSourceFile().isDeclarationFile) { + const errorNode: ts.Expression = getReferenceOriginForDiagnostics(ref, rawDeclarations); + + diagnostics.push(makeDiagnostic( + ErrorCode.NGMODULE_INVALID_DECLARATION, errorNode, + `Cannot declare '${ref.node.name.text}' in an NgModule as it's not a part of the current compilation.`, + [{ + node: ref.node.name, + messageText: `'${ref.node.name.text}' is declared here.`, + }])); + } + } } + + if (diagnostics.length > 0) { + return {diagnostics}; + } + let importRefs: Reference[] = []; let rawImports: ts.Expression|null = null; if (ngModule.has('imports')) { @@ -245,7 +270,7 @@ export class NgModuleDecoratorHandler implements schemas: schemas, mod: ngModuleDef, inj: ngInjectorDef, - declarations: declarationRefs, + declarations: declarationRefs, rawDeclarations, imports: importRefs, exports: exportRefs, metadataStmt: generateSetClassMetadataCall( @@ -266,6 +291,7 @@ export class NgModuleDecoratorHandler implements declarations: analysis.declarations, imports: analysis.imports, exports: analysis.exports, + rawDeclarations: analysis.rawDeclarations, }); if (this.factoryTracker !== null) { @@ -276,11 +302,35 @@ export class NgModuleDecoratorHandler implements resolve(node: ClassDeclaration, analysis: Readonly): ResolveResult { const scope = this.scopeRegistry.getScopeOfModule(node); - const diagnostics = this.scopeRegistry.getDiagnosticsOfModule(node) || undefined; + const diagnostics: ts.Diagnostic[] = []; + + const scopeDiagnostics = this.scopeRegistry.getDiagnosticsOfModule(node); + if (scopeDiagnostics !== null) { + diagnostics.push(...scopeDiagnostics); + } + const data: NgModuleResolution = { injectorImports: [], }; + for (const decl of analysis.declarations) { + if (this.metaReader.getDirectiveMetadata(decl) === null && + this.metaReader.getPipeMetadata(decl) === null) { + // This declaration is neither a directive(or component) nor a pipe, so produce a diagnostic + // for it. + + // Locate the error on the 'declarations' field of the NgModule decorator to start. + const errorNode: ts.Expression = + getReferenceOriginForDiagnostics(decl, analysis.rawDeclarations !); + diagnostics.push(makeDiagnostic( + ErrorCode.NGMODULE_INVALID_DECLARATION, errorNode, + `The class '${decl.node.name.text}' is listed in the declarations of the NgModule '${node.name.text}', but is not a directive, a component, or a pipe. + +Either remove it from the NgModule's declarations, or add an appropriate Angular decorator.`, + [{node: decl.node.name, messageText: `'${decl.node.name.text}' is declared here.`}])); + } + } + if (scope !== null) { // Using the scope information, extend the injector's imports using the modules that are // specified as module exports. @@ -302,12 +352,15 @@ export class NgModuleDecoratorHandler implements } } + if (diagnostics.length > 0) { + return {diagnostics}; + } + if (scope === null || scope.reexports === null) { - return {data, diagnostics}; + return {data}; } else { return { data, - diagnostics, reexports: scope.reexports, }; } diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts b/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts index 48db224b9e..fa3b831d0b 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts @@ -14,11 +14,12 @@ import {DefaultImportRecorder, Reference} from '../../imports'; import {MetadataRegistry} from '../../metadata'; import {PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; -import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../transform'; +import {LocalModuleScopeRegistry} from '../../scope'; +import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence, ResolveResult} from '../../transform'; import {compileNgFactoryDefField} from './factory'; import {generateSetClassMetadataCall} from './metadata'; -import {findAngularDecorator, getValidConstructorDependencies, unwrapExpression} from './util'; +import {findAngularDecorator, getValidConstructorDependencies, makeDuplicateDeclarationError, unwrapExpression} from './util'; export interface PipeHandlerData { meta: R3PipeMetadata; @@ -28,8 +29,8 @@ export interface PipeHandlerData { export class PipeDecoratorHandler implements DecoratorHandler { constructor( private reflector: ReflectionHost, private evaluator: PartialEvaluator, - private metaRegistry: MetadataRegistry, private defaultImportRecorder: DefaultImportRecorder, - private isCore: boolean) {} + private metaRegistry: MetadataRegistry, private scopeRegistry: LocalModuleScopeRegistry, + private defaultImportRecorder: DefaultImportRecorder, private isCore: boolean) {} readonly precedence = HandlerPrecedence.PRIMARY; readonly name = PipeDecoratorHandler.name; @@ -115,6 +116,18 @@ export class PipeDecoratorHandler implements DecoratorHandler { + const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node); + if (duplicateDeclData !== null) { + // This pipe was declared twice (or more). + return { + diagnostics: [makeDuplicateDeclarationError(node, duplicateDeclData, 'Pipe')], + }; + } + + return {}; + } + compile(node: ClassDeclaration, analysis: Readonly): CompileResult[] { const meta = analysis.meta; const res = compilePipeFromMetadata(meta); diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts index 37be1de1c1..d10ff5f2c1 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts @@ -9,10 +9,11 @@ import {Expression, ExternalExpr, R3DependencyMetadata, R3Reference, R3ResolvedDependencyType, WrappedNodeExpr} from '@angular/compiler'; import * as ts from 'typescript'; -import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; +import {ErrorCode, FatalDiagnosticError, makeDiagnostic} from '../../diagnostics'; import {DefaultImportRecorder, ImportMode, Reference, ReferenceEmitter} from '../../imports'; import {ForeignFunctionResolver, PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, CtorParameter, Decorator, Import, ReflectionHost, TypeValueReference, isNamedClassDeclaration} from '../../reflection'; +import {DeclarationData} from '../../scope'; export enum ConstructorDepErrorKind { NO_SUITABLE_TOKEN, @@ -375,4 +376,62 @@ const parensWrapperTransformerFactory: ts.TransformerFactory = */ export function wrapFunctionExpressionsInParens(expression: ts.Expression): ts.Expression { return ts.transform(expression, [parensWrapperTransformerFactory]).transformed[0]; -} \ No newline at end of file +} + +/** + * Given a 'container' expression and a `Reference` extracted from that container, produce a + * `ts.Expression` to use in a diagnostic which best indicates the position within the container + * expression that generated the `Reference`. + * + * For example, given a `Reference` to the class 'Bar' and the containing expression: + * `[Foo, Bar, Baz]`, this function would attempt to return the `ts.Identifier` for `Bar` within the + * array. This could be used to produce a nice diagnostic context: + * + * ```text + * [Foo, Bar, Baz] + * ~~~ + * ``` + * + * If no specific node can be found, then the `fallback` expression is used, which defaults to the + * entire containing expression. + */ +export function getReferenceOriginForDiagnostics( + ref: Reference, container: ts.Expression, fallback: ts.Expression = container): ts.Expression { + const id = ref.getIdentityInExpression(container); + if (id !== null) { + return id; + } + + return fallback; +} + +/** + * Create a `ts.Diagnostic` which indicates the given class is part of the declarations of two or + * more NgModules. + * + * The resulting `ts.Diagnostic` will have a context entry for each NgModule showing the point where + * the directive/pipe exists in its `declarations` (if possible). + */ +export function makeDuplicateDeclarationError( + node: ClassDeclaration, data: DeclarationData[], kind: string): ts.Diagnostic { + const context: {node: ts.Node; messageText: string;}[] = []; + for (const decl of data) { + if (decl.rawDeclarations === null) { + continue; + } + // Try to find the reference to the declaration within the declarations array, to hang the + // error there. If it can't be found, fall back on using the NgModule's name. + const contextNode = + getReferenceOriginForDiagnostics(decl.ref, decl.rawDeclarations, decl.ngModule.name); + context.push({ + node: contextNode, + messageText: + `'${node.name.text}' is listed in the declarations of the NgModule '${decl.ngModule.name.text}'.`, + }); + } + + // Finally, produce the diagnostic. + return makeDiagnostic( + ErrorCode.NGMODULE_DECLARATION_NOT_UNIQUE, node.name, + `The ${kind} '${node.name.text}' is declared by more than one NgModule.`, context); +} diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts index 4c1e7438b0..25588c6bec 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts @@ -49,7 +49,7 @@ runInEachFileSystem(() => { metaReader, new MetadataDtsModuleScopeResolver(dtsReader, null), new ReferenceEmitter([]), null); const handler = new DirectiveDecoratorHandler( - reflectionHost, evaluator, scopeRegistry, NOOP_DEFAULT_IMPORT_RECORDER, + reflectionHost, evaluator, scopeRegistry, scopeRegistry, NOOP_DEFAULT_IMPORT_RECORDER, /* isCore */ false, /* annotateForClosureCompiler */ false); const analyzeDirective = (dirName: string) => { diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts index 10e3c3ee44..4fa4890638 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts @@ -70,6 +70,11 @@ export enum ErrorCode { */ NGMODULE_REEXPORT_NAME_COLLISION = 6006, + /** + * Raised when a directive/pipe is part of the declarations of two or more NgModules. + */ + NGMODULE_DECLARATION_NOT_UNIQUE = 6007, + /** * Raised when ngcc tries to inject a synthetic decorator over one that already exists. */ diff --git a/packages/compiler-cli/src/ngtsc/imports/src/references.ts b/packages/compiler-cli/src/ngtsc/imports/src/references.ts index 78101a2ba5..f50dd581b3 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/references.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/references.ts @@ -115,6 +115,26 @@ export class Reference { return this.identifiers.find(id => id.getSourceFile() === context) || null; } + /** + * Get a `ts.Identifier` for this `Reference` that exists within the given expression. + * + * This is very useful for producing `ts.Diagnostic`s that reference `Reference`s that were + * extracted from some larger expression, as it can be used to pinpoint the `ts.Identifier` within + * the expression from which the `Reference` originated. + */ + getIdentityInExpression(expr: ts.Expression): ts.Identifier|null { + const sf = expr.getSourceFile(); + return this.identifiers.find(id => { + if (id.getSourceFile() !== sf) { + return false; + } + + // This identifier is a match if its position lies within the given expression. + return id.pos >= expr.pos && id.end <= expr.end; + }) || + null; + } + cloneWithAlias(alias: Expression): Reference { const ref = new Reference(this.node, this.bestGuessOwningModule); ref.identifiers = [...this.identifiers]; diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts index bde0ee4c71..302456a2bf 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts @@ -7,10 +7,12 @@ */ import {DirectiveMeta as T2DirectiveMeta, SchemaMetadata} from '@angular/compiler'; +import * as ts from 'typescript'; import {Reference} from '../../imports'; import {ClassDeclaration} from '../../reflection'; + /** * Metadata collected for an `NgModule`. */ @@ -20,6 +22,14 @@ export interface NgModuleMeta { imports: Reference[]; exports: Reference[]; schemas: SchemaMetadata[]; + + /** + * The raw `ts.Expression` which gave rise to `declarations`, if one exists. + * + * If this is `null`, then either no declarations exist, or no expression was available (likely + * because the module came from a .d.ts file). + */ + rawDeclarations: ts.Expression|null; } /** diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts index 2559fd6a35..9e85145fd9 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts @@ -55,6 +55,7 @@ export class DtsMetadataReader implements MetadataReader { imports: extractReferencesFromType( this.checker, importMetadata, ref.ownedByModuleGuess, resolutionContext), schemas: [], + rawDeclarations: null, }; } diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 42ba27f535..0bc91d2f2a 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -661,13 +661,16 @@ export class NgtscProgram implements api.Program { this.incrementalDriver.depGraph, this.closureCompilerEnabled), // TODO(alxhub): understand why the cast here is necessary (something to do with `null` not // being assignable to `unknown` when wrapped in `Readonly`). + // clang-format off new DirectiveDecoratorHandler( - this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore, - this.closureCompilerEnabled) as Readonly>, + this.reflector, evaluator, metaRegistry, this.scopeRegistry, this.defaultImportTracker, + this.isCore, this.closureCompilerEnabled) as Readonly>, + // clang-format on // Pipe handler must be before injectable handler in list so pipe factories are printed // before injectable factories (so injectable factories can delegate to them) new PipeDecoratorHandler( - this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore), + this.reflector, evaluator, metaRegistry, this.scopeRegistry, this.defaultImportTracker, + this.isCore), new InjectableDecoratorHandler( this.reflector, this.defaultImportTracker, this.isCore, this.options.strictInjectionParameters || false), diff --git a/packages/compiler-cli/src/ngtsc/scope/index.ts b/packages/compiler-cli/src/ngtsc/scope/index.ts index 35e548926b..3d433fba0d 100644 --- a/packages/compiler-cli/src/ngtsc/scope/index.ts +++ b/packages/compiler-cli/src/ngtsc/scope/index.ts @@ -9,4 +9,4 @@ export {ExportScope, ScopeData} from './src/api'; export {ComponentScopeReader, CompoundComponentScopeReader} from './src/component_scope'; export {DtsModuleScopeResolver, MetadataDtsModuleScopeResolver} from './src/dependency'; -export {LocalModuleScope, LocalModuleScopeRegistry, LocalNgModuleData} from './src/local'; +export {DeclarationData, LocalModuleScope, LocalModuleScopeRegistry, LocalNgModuleData} from './src/local'; diff --git a/packages/compiler-cli/src/ngtsc/scope/src/local.ts b/packages/compiler-cli/src/ngtsc/scope/src/local.ts index 671528178b..7d1d1f4a9e 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/local.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/local.ts @@ -75,7 +75,14 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop * 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(); + private declarationToModule = new Map(); + + /** + * This maps from the directive/pipe class to a map of data for each NgModule that declares the + * directive/pipe. This data is needed to produce an error for the given class. + */ + private duplicateDeclarations = + new Map>(); private moduleToRef = new Map>(); @@ -111,9 +118,12 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop */ registerNgModuleMetadata(data: NgModuleMeta): void { this.assertCollecting(); + const ngModule = data.ref.node; this.moduleToRef.set(data.ref.node, data.ref); + // Iterate over the module's declarations, and add them to declarationToModule. If duplicates + // are found, they're instead tracked in duplicateDeclarations. for (const decl of data.declarations) { - this.declarationToModule.set(decl.node, data.ref.node); + this.registerDeclarationOfModule(ngModule, decl, data.rawDeclarations); } } @@ -124,10 +134,25 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null { const scope = !this.declarationToModule.has(clazz) ? null : - this.getScopeOfModule(this.declarationToModule.get(clazz) !); + this.getScopeOfModule(this.declarationToModule.get(clazz) !.ngModule); return scope; } + /** + * If `node` is declared in more than one NgModule (duplicate declaration), then get the + * `DeclarationData` for each offending declaration. + * + * Ordinarily a class is only declared in one NgModule, in which case this function returns + * `null`. + */ + getDuplicateDeclarations(node: ClassDeclaration): DeclarationData[]|null { + if (!this.duplicateDeclarations.has(node)) { + return null; + } + + return Array.from(this.duplicateDeclarations.get(node) !.values()); + } + /** * Collects registered data for a module and its directives/pipes and convert it into a full * `LocalModuleScope`. @@ -165,15 +190,51 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop */ getCompilationScopes(): CompilationScope[] { const scopes: CompilationScope[] = []; - this.declarationToModule.forEach((ngModule, declaration) => { - const scope = this.getScopeOfModule(ngModule); + this.declarationToModule.forEach((declData, declaration) => { + const scope = this.getScopeOfModule(declData.ngModule); if (scope !== null) { - scopes.push({declaration, ngModule, ...scope.compilation}); + scopes.push({declaration, ngModule: declData.ngModule, ...scope.compilation}); } }); return scopes; } + private registerDeclarationOfModule( + ngModule: ClassDeclaration, decl: Reference, + rawDeclarations: ts.Expression|null): void { + const declData: DeclarationData = { + ngModule, + ref: decl, rawDeclarations, + }; + + // First, check for duplicate declarations of the same directive/pipe. + if (this.duplicateDeclarations.has(decl.node)) { + // This directive/pipe has already been identified as being duplicated. Add this module to the + // map of modules for which a duplicate declaration exists. + this.duplicateDeclarations.get(decl.node) !.set(ngModule, declData); + } else if ( + this.declarationToModule.has(decl.node) && + this.declarationToModule.get(decl.node) !.ngModule !== ngModule) { + // This directive/pipe is already registered as declared in another module. Mark it as a + // duplicate instead. + const duplicateDeclMap = new Map(); + const firstDeclData = this.declarationToModule.get(decl.node) !; + + // Being detected as a duplicate means there are two NgModules (for now) which declare this + // directive/pipe. Add both of them to the duplicate tracking map. + duplicateDeclMap.set(firstDeclData.ngModule, firstDeclData); + duplicateDeclMap.set(ngModule, declData); + this.duplicateDeclarations.set(decl.node, duplicateDeclMap); + + // Remove the directive/pipe from `declarationToModule` as it's a duplicate declaration, and + // therefore not valid. + this.declarationToModule.delete(decl.node); + } else { + // This is the first declaration of this directive/pipe, so map it. + this.declarationToModule.set(decl.node, declData); + } + } + /** * Implementation of `getScopeOfModule` which accepts a reference to a class and differentiates * between: @@ -514,3 +575,9 @@ function reexportCollision( {node: refB.node.name, messageText: childMessageText}, ]); } + +export interface DeclarationData { + ngModule: ClassDeclaration; + ref: Reference; + rawDeclarations: ts.Expression|null; +} diff --git a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts index f0b6b6eb01..e6235501e5 100644 --- a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts +++ b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts @@ -53,6 +53,7 @@ describe('LocalModuleScopeRegistry', () => { declarations: [Dir1, Dir2, Pipe1], exports: [Dir1, Pipe1], schemas: [], + rawDeclarations: null, }); const scope = scopeRegistry.getScopeOfModule(Module.node) !; @@ -69,6 +70,7 @@ describe('LocalModuleScopeRegistry', () => { declarations: [DirA], exports: [], schemas: [], + rawDeclarations: null, }); metaRegistry.registerNgModuleMetadata({ ref: new Reference(ModuleB.node), @@ -76,6 +78,7 @@ describe('LocalModuleScopeRegistry', () => { declarations: [DirB], imports: [], schemas: [], + rawDeclarations: null, }); metaRegistry.registerNgModuleMetadata({ ref: new Reference(ModuleC.node), @@ -83,6 +86,7 @@ describe('LocalModuleScopeRegistry', () => { exports: [DirCE], imports: [], schemas: [], + rawDeclarations: null, }); const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) !; @@ -99,6 +103,7 @@ describe('LocalModuleScopeRegistry', () => { imports: [], declarations: [], schemas: [], + rawDeclarations: null, }); metaRegistry.registerNgModuleMetadata({ ref: new Reference(ModuleB.node), @@ -106,6 +111,7 @@ describe('LocalModuleScopeRegistry', () => { exports: [Dir], imports: [], schemas: [], + rawDeclarations: null, }); const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) !; @@ -122,6 +128,7 @@ describe('LocalModuleScopeRegistry', () => { imports: [ModuleB, ModuleC], exports: [DirA, DirA, DirB, ModuleB], schemas: [], + rawDeclarations: null, }); metaRegistry.registerNgModuleMetadata({ ref: new Reference(ModuleB.node), @@ -129,6 +136,7 @@ describe('LocalModuleScopeRegistry', () => { imports: [], exports: [DirB], schemas: [], + rawDeclarations: null, }); metaRegistry.registerNgModuleMetadata({ ref: new Reference(ModuleC.node), @@ -136,6 +144,7 @@ describe('LocalModuleScopeRegistry', () => { imports: [], exports: [ModuleB], schemas: [], + rawDeclarations: null, }); const scope = scopeRegistry.getScopeOfModule(ModuleA.node) !; @@ -159,6 +168,7 @@ describe('LocalModuleScopeRegistry', () => { imports: [], declarations: [DirInModule], schemas: [], + rawDeclarations: null, }); const scope = scopeRegistry.getScopeOfModule(Module.node) !; @@ -174,6 +184,7 @@ describe('LocalModuleScopeRegistry', () => { imports: [ModuleB], declarations: [], schemas: [], + rawDeclarations: null, }); metaRegistry.registerNgModuleMetadata({ ref: new Reference(ModuleB.node), @@ -181,6 +192,7 @@ describe('LocalModuleScopeRegistry', () => { exports: [Dir], imports: [], schemas: [], + rawDeclarations: null, }); const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) !; @@ -196,6 +208,7 @@ describe('LocalModuleScopeRegistry', () => { imports: [], declarations: [], schemas: [], + rawDeclarations: null, }); metaRegistry.registerNgModuleMetadata({ ref: new Reference(ModuleB.node), @@ -203,6 +216,7 @@ describe('LocalModuleScopeRegistry', () => { exports: [Dir], imports: [], schemas: [], + rawDeclarations: null, }); expect(scopeRegistry.getScopeOfModule(ModuleA.node)).toBe(null); diff --git a/packages/compiler-cli/test/ngtsc/scope_spec.ts b/packages/compiler-cli/test/ngtsc/scope_spec.ts index 965eca21a1..22300e35cd 100644 --- a/packages/compiler-cli/test/ngtsc/scope_spec.ts +++ b/packages/compiler-cli/test/ngtsc/scope_spec.ts @@ -28,6 +28,129 @@ runInEachFileSystem(() => { }); describe('diagnostics', () => { + describe('declarations', () => { + it('should detect when a random class is declared', () => { + env.write('test.ts', ` + import {NgModule} from '@angular/core'; + + export class RandomClass {} + + @NgModule({ + declarations: [RandomClass], + }) + export class Module {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + const node = diagnosticToNode(diags[0], ts.isIdentifier); + expect(node.text).toEqual('RandomClass'); + expect(diags[0].messageText).toContain('is not a directive, a component, or a pipe.'); + }); + + it('should detect when a declaration lives outside the current compilation', () => { + env.write('dir.d.ts', ` + import {ɵɵDirectiveDefWithMeta} from '@angular/core'; + + export declare class ExternalDir { + static ɵdir: ɵɵDirectiveDefWithMeta; + } + `); + env.write('test.ts', ` + import {NgModule} from '@angular/core'; + import {ExternalDir} from './dir'; + + @NgModule({ + declarations: [ExternalDir], + }) + export class Module {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + const node = diagnosticToNode(diags[0], ts.isIdentifier); + expect(node.text).toEqual('ExternalDir'); + expect(diags[0].messageText).toContain(`not a part of the current compilation`); + }); + + it('should detect when a declaration is shared between two modules', () => { + env.write('test.ts', ` + import {Directive, NgModule} from '@angular/core'; + + @Directive({selector: '[test]'}) + export class TestDir {} + + @NgModule({ + declarations: [TestDir] + }) + export class ModuleA {} + + @NgModule({ + declarations: [TestDir], + }) + export class ModuleB {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + const node = findContainingClass(diagnosticToNode(diags[0], ts.isIdentifier)); + expect(node.name !.text).toEqual('TestDir'); + + const relatedNodes = new Set(diags[0].relatedInformation !.map( + related => + findContainingClass(diagnosticToNode(related, ts.isIdentifier)).name !.text)); + expect(relatedNodes).toContain('ModuleA'); + expect(relatedNodes).toContain('ModuleB'); + expect(relatedNodes.size).toBe(2); + }); + + it('should detect when a declaration is repeated within the same module', () => { + env.write('test.ts', ` + import {Directive, NgModule} from '@angular/core'; + + @Directive({selector: '[test]'}) + export class TestDir {} + + + @NgModule({ + declarations: [TestDir, TestDir], + }) + export class Module {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should detect when a declaration is shared between two modules, and is repeated within them', + () => { + env.write('test.ts', ` + import {Directive, NgModule} from '@angular/core'; + + @Directive({selector: '[test]'}) + export class TestDir {} + + @NgModule({ + declarations: [TestDir, TestDir] + }) + export class ModuleA {} + + @NgModule({ + declarations: [TestDir, TestDir], + }) + export class ModuleB {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + const node = findContainingClass(diagnosticToNode(diags[0], ts.isIdentifier)); + expect(node.name !.text).toEqual('TestDir'); + + const relatedNodes = new Set(diags[0].relatedInformation !.map( + related => + findContainingClass(diagnosticToNode(related, ts.isIdentifier)).name !.text)); + expect(relatedNodes).toContain('ModuleA'); + expect(relatedNodes).toContain('ModuleB'); + expect(relatedNodes.size).toBe(2); + }); + }); describe('imports', () => { it('should emit imports in a pure function call', () => { env.write('test.ts', ` @@ -182,8 +305,9 @@ runInEachFileSystem(() => { }); function diagnosticToNode( - diagnostic: ts.Diagnostic | Diagnostic, guard: (node: ts.Node) => node is T): T { - const diag = diagnostic as ts.Diagnostic; + diagnostic: ts.Diagnostic | Diagnostic | ts.DiagnosticRelatedInformation, + guard: (node: ts.Node) => node is T): T { + const diag = diagnostic as ts.Diagnostic | ts.DiagnosticRelatedInformation; if (diag.file === undefined) { throw new Error(`Expected ts.Diagnostic to have a file source`); } @@ -192,3 +316,14 @@ runInEachFileSystem(() => { return node as T; } }); + +function findContainingClass(node: ts.Node): ts.ClassDeclaration { + while (!ts.isClassDeclaration(node)) { + if (node.parent && node.parent !== node) { + node = node.parent; + } else { + throw new Error('Expected node to have a ClassDeclaration parent'); + } + } + return node; +}