diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index 1f745f4618..1fc2a48d9f 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -100,7 +100,7 @@ export class DecorationAnalyzer { // clang-format off new DirectiveDecoratorHandler( this.reflectionHost, this.evaluator, this.fullRegistry, this.scopeRegistry, - NOOP_DEFAULT_IMPORT_RECORDER, this.injectableRegistry, this.isCore, + this.fullMetaReader, NOOP_DEFAULT_IMPORT_RECORDER, this.injectableRegistry, this.isCore, /* annotateForClosureCompiler */ false) as DecoratorHandler, // clang-format on // Pipe handler must be before injectable handler in list so pipe factories are printed diff --git a/packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts b/packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts index bdbed94af2..91ca55a152 100644 --- a/packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts +++ b/packages/compiler-cli/ngcc/test/migrations/undecorated_parent_migration_spec.ts @@ -207,7 +207,7 @@ runInEachFileSystem(() => { it('should skip the base class if it is in a different package from the derived class', () => { const BASE_FILENAME = _('/node_modules/other-package/index.js'); - const {program, analysis, errors} = setUpAndAnalyzeProgram([ + const {errors} = setUpAndAnalyzeProgram([ { name: INDEX_FILENAME, contents: ` @@ -232,9 +232,16 @@ runInEachFileSystem(() => { ` } ]); - expect(errors).toEqual([]); - const file = analysis.get(program.getSourceFile(BASE_FILENAME) !); - expect(file).toBeUndefined(); + + expect(errors.length).toBe(1); + expect(errors[0].messageText) + .toBe( + `The directive DerivedClass inherits its constructor ` + + `from BaseClass, but the latter does not have an Angular ` + + `decorator of its own. Dependency injection will not be ` + + `able to resolve the parameters of BaseClass's ` + + `constructor. Either add a @Directive decorator to ` + + `BaseClass, or add an explicit constructor to DerivedClass.`); }); }); diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index a9993158ca..258e850489 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -26,7 +26,7 @@ import {tsSourceMapBug29300Fixed} from '../../util/src/ts_source_map_bug_29300'; import {SubsetOfKeys} from '../../util/src/typescript'; import {ResourceLoader} from './api'; -import {getProviderDiagnostics} from './diagnostics'; +import {getDirectiveDiagnostics, getProviderDiagnostics} from './diagnostics'; import {extractDirectiveMetadata, parseFieldArrayValue} from './directive'; import {compileNgFactoryDefField} from './factory'; import {generateSetClassMetadataCall} from './metadata'; @@ -550,9 +550,10 @@ export class ComponentDecoratorHandler implements diagnostics.push(...viewProviderDiagnostics); } - const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node); - if (duplicateDeclData !== null) { - diagnostics.push(makeDuplicateDeclarationError(node, duplicateDeclData, 'Component')); + const directiveDiagnostics = getDirectiveDiagnostics( + node, this.metaReader, this.evaluator, this.reflector, this.scopeRegistry, 'Component'); + if (directiveDiagnostics !== null) { + diagnostics.push(...directiveDiagnostics); } if (diagnostics.length > 0) { diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/diagnostics.ts b/packages/compiler-cli/src/ngtsc/annotations/src/diagnostics.ts index dc2f39eefd..8b2853eefb 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/diagnostics.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/diagnostics.ts @@ -10,8 +10,12 @@ import * as ts from 'typescript'; import {ErrorCode, makeDiagnostic} from '../../diagnostics'; import {Reference} from '../../imports'; -import {InjectableClassRegistry} from '../../metadata'; -import {ClassDeclaration} from '../../reflection'; +import {InjectableClassRegistry, MetadataReader} from '../../metadata'; +import {PartialEvaluator} from '../../partial_evaluator'; +import {ClassDeclaration, ReflectionHost} from '../../reflection'; +import {LocalModuleScopeRegistry} from '../../scope'; + +import {makeDuplicateDeclarationError, readBaseClass} from './util'; /** * Gets the diagnostics for a set of provider classes. @@ -41,3 +45,90 @@ Either add the @Injectable() decorator to '${provider.node.name.text}', or confi return diagnostics; } + +export function getDirectiveDiagnostics( + node: ClassDeclaration, reader: MetadataReader, evaluator: PartialEvaluator, + reflector: ReflectionHost, scopeRegistry: LocalModuleScopeRegistry, + kind: string): ts.Diagnostic[]|null { + let diagnostics: ts.Diagnostic[]|null = []; + + const addDiagnostics = (more: ts.Diagnostic | ts.Diagnostic[] | null) => { + if (more === null) { + return; + } else if (diagnostics === null) { + diagnostics = Array.isArray(more) ? more : [more]; + } else if (Array.isArray(more)) { + diagnostics.push(...more); + } else { + diagnostics.push(more); + } + }; + + const duplicateDeclarations = scopeRegistry.getDuplicateDeclarations(node); + + if (duplicateDeclarations !== null) { + addDiagnostics(makeDuplicateDeclarationError(node, duplicateDeclarations, kind)); + } + + addDiagnostics(checkInheritanceOfDirective(node, reader, reflector, evaluator)); + return diagnostics; +} + +export function checkInheritanceOfDirective( + node: ClassDeclaration, reader: MetadataReader, reflector: ReflectionHost, + evaluator: PartialEvaluator): ts.Diagnostic|null { + if (!reflector.isClass(node) || reflector.getConstructorParameters(node) !== null) { + // We should skip nodes that aren't classes. If a constructor exists, then no base class + // definition is required on the runtime side - it's legal to inherit from any class. + return null; + } + + // The extends clause is an expression which can be as dynamic as the user wants. Try to + // evaluate it, but fall back on ignoring the clause if it can't be understood. This is a View + // Engine compatibility hack: View Engine ignores 'extends' expressions that it cannot understand. + let baseClass = readBaseClass(node, reflector, evaluator); + + while (baseClass !== null) { + if (baseClass === 'dynamic') { + return null; + } + + // We can skip the base class if it has metadata. + const baseClassMeta = reader.getDirectiveMetadata(baseClass); + if (baseClassMeta !== null) { + return null; + } + + // If the base class has a blank constructor we can skip it since it can't be using DI. + const baseClassConstructorParams = reflector.getConstructorParameters(baseClass.node); + const newParentClass = readBaseClass(baseClass.node, reflector, evaluator); + + if (baseClassConstructorParams !== null && baseClassConstructorParams.length > 0) { + // This class has a non-trivial constructor, that's an error! + return getInheritedUndecoratedCtorDiagnostic(node, baseClass, reader); + } else if (baseClassConstructorParams !== null || newParentClass === null) { + // This class has a trivial constructor, or no constructor + is the + // top of the inheritance chain, so it's okay. + return null; + } + + // Go up the chain and continue + baseClass = newParentClass; + } + + return null; +} + +function getInheritedUndecoratedCtorDiagnostic( + node: ClassDeclaration, baseClass: Reference, reader: MetadataReader) { + const subclassMeta = reader.getDirectiveMetadata(new Reference(node)) !; + const dirOrComp = subclassMeta.isComponent ? 'Component' : 'Directive'; + const baseClassName = baseClass.debugName; + + return makeDiagnostic( + ErrorCode.DIRECTIVE_INHERITS_UNDECORATED_CTOR, node.name, + `The ${dirOrComp.toLowerCase()} ${node.name.text} inherits its constructor from ${baseClassName}, ` + + `but the latter does not have an Angular decorator of its own. Dependency injection will not be able to ` + + `resolve the parameters of ${baseClassName}'s constructor. Either add a @Directive decorator ` + + `to ${baseClassName}, or add an explicit constructor to ${node.name.text}.`); +} diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index e6a8206199..c9d51d2d6e 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -11,17 +11,17 @@ import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {DefaultImportRecorder, Reference} from '../../imports'; -import {InjectableClassRegistry, MetadataRegistry} from '../../metadata'; +import {InjectableClassRegistry, MetadataReader, 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 {LocalModuleScopeRegistry} from '../../scope'; import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFlags, HandlerPrecedence, ResolveResult} from '../../transform'; -import {getProviderDiagnostics} from './diagnostics'; +import {getDirectiveDiagnostics, getProviderDiagnostics} from './diagnostics'; import {compileNgFactoryDefField} from './factory'; import {generateSetClassMetadataCall} from './metadata'; -import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, makeDuplicateDeclarationError, readBaseClass, resolveProvidersRequiringFactory, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens, wrapTypeReference} from './util'; +import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, readBaseClass, resolveProvidersRequiringFactory, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens, wrapTypeReference} from './util'; const EMPTY_OBJECT: {[key: string]: string} = {}; const FIELD_DECORATORS = [ @@ -46,7 +46,7 @@ export class DirectiveDecoratorHandler implements constructor( private reflector: ReflectionHost, private evaluator: PartialEvaluator, private metaRegistry: MetadataRegistry, private scopeRegistry: LocalModuleScopeRegistry, - private defaultImportRecorder: DefaultImportRecorder, + private metaReader: MetadataReader, private defaultImportRecorder: DefaultImportRecorder, private injectableRegistry: InjectableClassRegistry, private isCore: boolean, private annotateForClosureCompiler: boolean) {} @@ -140,10 +140,10 @@ export class DirectiveDecoratorHandler implements diagnostics.push(...providerDiagnostics); } - const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node); - if (duplicateDeclData !== null) { - // This directive was declared twice (or more). - diagnostics.push(makeDuplicateDeclarationError(node, duplicateDeclData, 'Directive')); + const directiveDiagnostics = getDirectiveDiagnostics( + node, this.metaReader, this.evaluator, this.reflector, this.scopeRegistry, 'Directive'); + if (directiveDiagnostics !== null) { + diagnostics.push(...directiveDiagnostics); } return {diagnostics: diagnostics.length > 0 ? diagnostics : undefined}; 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 9d7feef0d9..b15e85ca63 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts @@ -50,8 +50,8 @@ runInEachFileSystem(() => { null); const injectableRegistry = new InjectableClassRegistry(reflectionHost); const handler = new DirectiveDecoratorHandler( - reflectionHost, evaluator, scopeRegistry, scopeRegistry, NOOP_DEFAULT_IMPORT_RECORDER, - injectableRegistry, + reflectionHost, evaluator, scopeRegistry, scopeRegistry, metaReader, + NOOP_DEFAULT_IMPORT_RECORDER, injectableRegistry, /* 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 e4fbc34866..4a2e84ef60 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts @@ -29,6 +29,12 @@ export enum ErrorCode { /** Raised when an undecorated class is passed in as a provider to a module or a directive. */ UNDECORATED_PROVIDER = 2005, + /** + * Raised when a Directive inherits its constructor from a base class without an Angular + * decorator. + */ + DIRECTIVE_INHERITS_UNDECORATED_CTOR = 2006, + SYMBOL_NOT_EXPORTED = 3001, SYMBOL_EXPORTED_UNDER_DIFFERENT_NAME = 3002, @@ -120,7 +126,7 @@ export enum ErrorCode { /** * An injectable already has a `ɵprov` property. */ - INJECTABLE_DUPLICATE_PROV = 9001 + INJECTABLE_DUPLICATE_PROV = 9001, } export function ngErrorCode(code: ErrorCode): number { diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 34e96fd181..5ce5d601fd 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -665,8 +665,9 @@ export class NgtscProgram implements api.Program { // being assignable to `unknown` when wrapped in `Readonly`). // clang-format off new DirectiveDecoratorHandler( - this.reflector, evaluator, metaRegistry, this.scopeRegistry, this.defaultImportTracker, injectableRegistry, - this.isCore, this.closureCompilerEnabled) as Readonly>, + this.reflector, evaluator, metaRegistry, this.scopeRegistry, this.metaReader, + this.defaultImportTracker, injectableRegistry, 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) diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 418feda007..b3b6927ba5 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -2895,6 +2895,9 @@ runInEachFileSystem(os => { env.write(`test.ts`, ` import {Directive} from '@angular/core'; + @Directive({ + selector: '[base]', + }) class Base {} @Directive({ @@ -5131,6 +5134,245 @@ export const Foo = Foo__PRE_R3__; }); }); + describe('inherited directives', () => { + beforeEach(() => { + env.write('local.ts', ` + import {Component, Directive, ElementRef} from '@angular/core'; + + export class BasePlain {} + + export class BasePlainWithBlankConstructor { + constructor() {} + } + + export class BasePlainWithConstructorParameters { + constructor(elementRef: ElementRef) {} + } + + @Component({ + selector: 'base-cmp', + template: 'BaseCmp', + }) + export class BaseCmp {} + + @Directive({ + selector: '[base]', + }) + export class BaseDir {} + `); + + env.write('lib.d.ts', ` + import {ɵɵComponentDefWithMeta, ɵɵDirectiveDefWithMeta, ElementRef} from '@angular/core'; + + export declare class BasePlain {} + + export declare class BasePlainWithBlankConstructor { + constructor() {} + } + + export declare class BasePlainWithConstructorParameters { + constructor(elementRef: ElementRef) {} + } + + export declare class BaseCmp { + static ɵcmp: ɵɵComponentDefWithMeta + } + + export declare class BaseDir { + static ɵdir: ɵɵDirectiveDefWithMeta; + } + `); + }); + + it('should not error when inheriting a constructor from a decorated directive class', () => { + env.tsconfig(); + env.write('test.ts', ` + import {Directive, Component} from '@angular/core'; + import {BaseDir, BaseCmp} from './local'; + + @Directive({ + selector: '[dir]', + }) + export class Dir extends BaseDir {} + + @Component({ + selector: 'test-cmp', + template: 'TestCmp', + }) + export class Cmp extends BaseCmp {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should not error when inheriting a constructor without parameters', () => { + env.tsconfig(); + env.write('test.ts', ` + import {Directive, Component} from '@angular/core'; + import {BasePlainWithBlankConstructor} from './local'; + + @Directive({ + selector: '[dir]', + }) + export class Dir extends BasePlainWithBlankConstructor {} + + @Component({ + selector: 'test-cmp', + template: 'TestCmp', + }) + export class Cmp extends BasePlainWithBlankConstructor {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should not error when inheriting from a class without a constructor', () => { + env.tsconfig(); + env.write('test.ts', ` + import {Directive, Component} from '@angular/core'; + import {BasePlain} from './local'; + + @Directive({ + selector: '[dir]', + }) + export class Dir extends BasePlain {} + + @Component({ + selector: 'test-cmp', + template: 'TestCmp', + }) + export class Cmp extends BasePlain {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should error when inheriting a constructor from an undecorated class', () => { + env.tsconfig(); + env.write('test.ts', ` + import {Directive, Component} from '@angular/core'; + import {BasePlainWithConstructorParameters} from './local'; + + @Directive({ + selector: '[dir]', + }) + export class Dir extends BasePlainWithConstructorParameters {} + + @Component({ + selector: 'test-cmp', + template: 'TestCmp', + }) + export class Cmp extends BasePlainWithConstructorParameters {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(2); + expect(diags[0].messageText).toContain('Dir'); + expect(diags[0].messageText).toContain('BasePlainWithConstructorParameters'); + expect(diags[1].messageText).toContain('Cmp'); + expect(diags[1].messageText).toContain('BasePlainWithConstructorParameters'); + }); + + it('should error when inheriting a constructor from undecorated grand super class', () => { + env.tsconfig(); + env.write('test.ts', ` + import {Directive, Component} from '@angular/core'; + import {BasePlainWithConstructorParameters} from './local'; + + class Parent extends BasePlainWithConstructorParameters {} + + @Directive({ + selector: '[dir]', + }) + export class Dir extends Parent {} + + @Component({ + selector: 'test-cmp', + template: 'TestCmp', + }) + export class Cmp extends Parent {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(2); + expect(diags[0].messageText).toContain('Dir'); + expect(diags[0].messageText).toContain('BasePlainWithConstructorParameters'); + expect(diags[1].messageText).toContain('Cmp'); + expect(diags[1].messageText).toContain('BasePlainWithConstructorParameters'); + }); + + it('should error when inheriting a constructor from undecorated grand grand super class', + () => { + env.tsconfig(); + env.write('test.ts', ` + import {Directive, Component} from '@angular/core'; + import {BasePlainWithConstructorParameters} from './local'; + + class GrandParent extends BasePlainWithConstructorParameters {} + + class Parent extends GrandParent {} + + @Directive({ + selector: '[dir]', + }) + export class Dir extends Parent {} + + @Component({ + selector: 'test-cmp', + template: 'TestCmp', + }) + export class Cmp extends Parent {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(2); + expect(diags[0].messageText).toContain('Dir'); + expect(diags[0].messageText).toContain('BasePlainWithConstructorParameters'); + expect(diags[1].messageText).toContain('Cmp'); + expect(diags[1].messageText).toContain('BasePlainWithConstructorParameters'); + }); + + it('should not error when inheriting a constructor from decorated directive or component classes in a .d.ts file', + () => { + env.tsconfig(); + env.write('test.ts', ` + import {Component, Directive} from '@angular/core'; + import {BaseDir, BaseCmp} from './lib'; + + @Directive({ + selector: '[dir]', + }) + export class Dir extends BaseDir {} + + @Component({ + selector: 'test-cmp', + template: 'TestCmp', + }) + export class Cmp extends BaseCmp {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should error when inheriting a constructor from an undecorated class in a .d.ts file', + () => { + env.tsconfig(); + env.write('test.ts', ` + import {Directive} from '@angular/core'; + + import {BasePlainWithConstructorParameters} from './lib'; + + @Directive({ + selector: '[dir]', + }) + export class Dir extends BasePlainWithConstructorParameters {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toContain('Dir'); + expect(diags[0].messageText).toContain('Base'); + }); + }); + describe('inline resources', () => { it('should process inline