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; +}