From f40a9a9e7d09b9793a8f953b6d01cd0407f535b2 Mon Sep 17 00:00:00 2001 From: JoostK Date: Thu, 3 Oct 2019 21:54:49 +0200 Subject: [PATCH] fix(ivy): allow abstract directives to have an invalid constructor For abstract directives, i.e. directives without a selector, it may happen that their constructor is called explicitly from a subclass, hence its parameters are not required to be valid for Angular's DI purposes. Prior to this commit however, having an abstract directive with a constructor that has parameters that are not eligible for Angular's DI would produce a compilation error. A similar scenario may occur for `@Injectable`s, where an explicit `use*` definition allows for the constructor to be irrelevant. For example, the situation where `useFactory` is specified allows for the constructor to be called explicitly with any value, so its constructor parameters are not required to be valid. For `@Injectable`s this is handled by generating a DI factory function that throws. This commit implements the same solution for abstract directives, such that a compilation error is avoided while still producing an error at runtime if the type is instantiated implicitly by Angular's DI mechanism. Fixes #32981 --- .../src/ngtsc/annotations/src/directive.ts | 18 ++- .../src/ngtsc/annotations/src/injectable.ts | 36 ++---- .../src/ngtsc/annotations/src/util.ts | 15 +++ .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 105 +++++++++++++++++- packages/compiler/src/render3/view/api.ts | 2 +- 5 files changed, 142 insertions(+), 34 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index 6c6f47f928f4c..b5adec34a227d 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {ConstantPool, EMPTY_SOURCE_SPAN, Expression, Identifiers, ParseError, ParsedHostBindings, R3DirectiveMetadata, R3QueryMetadata, Statement, WrappedNodeExpr, compileDirectiveFromMetadata, makeBindingParser, parseHostBindings, verifyHostBindings} from '@angular/compiler'; +import {ConstantPool, EMPTY_SOURCE_SPAN, Expression, Identifiers, ParseError, ParsedHostBindings, R3DependencyMetadata, R3DirectiveMetadata, R3QueryMetadata, Statement, WrappedNodeExpr, compileDirectiveFromMetadata, makeBindingParser, parseHostBindings, verifyHostBindings} from '@angular/compiler'; import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; @@ -19,7 +19,7 @@ import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPr import {compileNgFactoryDefField} from './factory'; import {generateSetClassMetadataCall} from './metadata'; -import {findAngularDecorator, getValidConstructorDependencies, readBaseClass, unwrapExpression, unwrapForwardRef} from './util'; +import {findAngularDecorator, getConstructorDependencies, readBaseClass, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies} from './util'; const EMPTY_OBJECT: {[key: string]: string} = {}; @@ -233,11 +233,23 @@ export function extractDirectiveMetadata( exportAs = resolved.split(',').map(part => part.trim()); } + const rawCtorDeps = getConstructorDependencies(clazz, reflector, defaultImportRecorder, isCore); + let ctorDeps: R3DependencyMetadata[]|'invalid'|null = null; + + // Non-abstract directives (those with a selector) require valid constructor dependencies, whereas + // abstract directives are allowed to have invalid dependencies, given that a subclass may call + // the constructor explicitly. + if (selector !== null) { + ctorDeps = validateConstructorDependencies(clazz, rawCtorDeps); + } else { + ctorDeps = unwrapConstructorDependencies(rawCtorDeps); + } + // Detect if the component inherits from another class const usesInheritance = reflector.hasBaseClass(clazz); const metadata: R3DirectiveMetadata = { name: clazz.name.text, - deps: getValidConstructorDependencies(clazz, reflector, defaultImportRecorder, isCore), host, + deps: ctorDeps, host, lifecycle: { usesOnChanges, }, diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts index 08cd1ded956ee..c0501c7c509e9 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts @@ -16,7 +16,7 @@ import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPr import {compileNgFactoryDefField} from './factory'; import {generateSetClassMetadataCall} from './metadata'; -import {findAngularDecorator, getConstructorDependencies, getValidConstructorDependencies, isAngularCore, unwrapForwardRef, validateConstructorDependencies} from './util'; +import {findAngularDecorator, getConstructorDependencies, getValidConstructorDependencies, isAngularCore, unwrapConstructorDependencies, unwrapForwardRef, validateConstructorDependencies} from './util'; export interface InjectableHandlerData { meta: R3InjectableMetadata; @@ -214,46 +214,24 @@ function extractInjectableCtorDeps( // Angular's DI. // // To deal with this, @Injectable() without an argument is more lenient, and if the - // constructor - // signature does not work for DI then an ngInjectableDef that throws. + // constructor signature does not work for DI then an ngFactoryDef that throws is generated. if (strictCtorDeps) { ctorDeps = getValidConstructorDependencies(clazz, reflector, defaultImportRecorder, isCore); } else { - const possibleCtorDeps = - getConstructorDependencies(clazz, reflector, defaultImportRecorder, isCore); - if (possibleCtorDeps !== null) { - if (possibleCtorDeps.deps !== null) { - // This use of @Injectable has valid constructor dependencies. - ctorDeps = possibleCtorDeps.deps; - } else { - // This use of @Injectable is technically invalid. Generate a factory function which - // throws - // an error. - // TODO(alxhub): log warnings for the bad use of @Injectable. - ctorDeps = 'invalid'; - } - } + ctorDeps = unwrapConstructorDependencies( + getConstructorDependencies(clazz, reflector, defaultImportRecorder, isCore)); } return ctorDeps; } else if (decorator.args.length === 1) { const rawCtorDeps = getConstructorDependencies(clazz, reflector, defaultImportRecorder, isCore); - // rawCtorDeps will be null if the class has no constructor. - if (rawCtorDeps !== null) { - if (rawCtorDeps.deps !== null) { - // A constructor existed and had valid dependencies. - ctorDeps = rawCtorDeps.deps; - } else { - // A constructor existed but had invalid dependencies. - ctorDeps = 'invalid'; - } - } - if (strictCtorDeps && !meta.useValue && !meta.useExisting && !meta.useClass && !meta.useFactory) { // Since use* was not provided, validate the deps according to strictCtorDeps. - validateConstructorDependencies(clazz, rawCtorDeps); + ctorDeps = validateConstructorDependencies(clazz, rawCtorDeps); + } else { + ctorDeps = unwrapConstructorDependencies(rawCtorDeps); } } diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts index 061538260764e..debc84b9f0b48 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts @@ -133,6 +133,21 @@ export function valueReferenceToExpression( } } +export function unwrapConstructorDependencies(deps: ConstructorDeps | null): R3DependencyMetadata[]| + 'invalid'|null { + if (deps === null) { + return null; + } else if (deps.deps !== null) { + // This use of @Injectable has valid constructor dependencies. + return deps.deps; + } else { + // This use of @Injectable is technically invalid. Generate a factory function which + // throws an error. + // TODO(alxhub): log warnings for the bad use of @Injectable. + return 'invalid'; + } +} + export function getValidConstructorDependencies( clazz: ClassDeclaration, reflector: ReflectionHost, defaultImportRecorder: DefaultImportRecorder, isCore: boolean): R3DependencyMetadata[]|null { diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index b4b720ed14a71..2e3b98073b7ea 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -1263,7 +1263,7 @@ runInEachFileSystem(os => { env.write('test.ts', ` import {Injectable} from '@angular/core'; - @Injectable() + @Injectable({providedIn: 'root'}) export class Test { constructor(private notInjectable: string) {} } @@ -1289,6 +1289,70 @@ runInEachFileSystem(os => { } `); + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents).toMatch(/function Test_Factory\(t\) { throw new Error\(/ms); + }); + + it('should not give a compile-time error if an invalid @Injectable is used with useFactory', + () => { + env.tsconfig({strictInjectionParameters: true}); + env.write('test.ts', ` + import {Injectable} from '@angular/core'; + + @Injectable({ + providedIn: 'root', + useFactory: () => '42', + }) + export class Test { + constructor(private notInjectable: string) {} + } + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents).toMatch(/function Test_Factory\(t\) { throw new Error\(/ms); + }); + + it('should not give a compile-time error if an invalid @Injectable is used with useExisting', + () => { + env.tsconfig({strictInjectionParameters: true}); + env.write('test.ts', ` + import {Injectable} from '@angular/core'; + + export class MyService {} + + @Injectable({ + providedIn: 'root', + useExisting: MyService, + }) + export class Test { + constructor(private notInjectable: string) {} + } + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents).toMatch(/function Test_Factory\(t\) { throw new Error\(/ms); + }); + + it('should not give a compile-time error if an invalid @Injectable is used with useClass', + () => { + env.tsconfig({strictInjectionParameters: true}); + env.write('test.ts', ` + import {Injectable} from '@angular/core'; + + export class MyService {} + + @Injectable({ + providedIn: 'root', + useClass: MyService, + }) + export class Test { + constructor(private notInjectable: string) {} + } + `); + env.driveMain(); const jsContents = env.getContents('test.js'); expect(jsContents).toMatch(/function Test_Factory\(t\) { throw new Error\(/ms); @@ -1333,6 +1397,45 @@ runInEachFileSystem(os => { }); }); + describe('compiling invalid @Directives', () => { + describe('directives with a selector', () => { + it('should give a compile-time error if an invalid constructor is used', () => { + env.tsconfig({strictInjectionParameters: true}); + env.write('test.ts', ` + import {Directive} from '@angular/core'; + + @Directive({selector: 'app-test'}) + export class Test { + constructor(private notInjectable: string) {} + } + `); + + const errors = env.driveDiagnostics(); + expect(errors.length).toBe(1); + expect(errors[0].messageText).toContain('No suitable injection token for parameter'); + }); + }); + + describe('abstract directives', () => { + it('should generate a factory function that throws', () => { + env.tsconfig({strictInjectionParameters: false}); + env.write('test.ts', ` + import {Directive} from '@angular/core'; + + @Directive() + export class Test { + constructor(private notInjectable: string) {} + } + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents) + .toContain('Test.ngFactoryDef = function Test_Factory(t) { throw new Error('); + }); + }); + }); + describe('templateUrl and styleUrls processing', () => { const testsForResource = (resource: string) => [ // [component location, resource location, resource reference] diff --git a/packages/compiler/src/render3/view/api.ts b/packages/compiler/src/render3/view/api.ts index 44ad40ef1b53e..3995598d5a77e 100644 --- a/packages/compiler/src/render3/view/api.ts +++ b/packages/compiler/src/render3/view/api.ts @@ -41,7 +41,7 @@ export interface R3DirectiveMetadata { /** * Dependencies of the directive's constructor. */ - deps: R3DependencyMetadata[]|null; + deps: R3DependencyMetadata[]|'invalid'|null; /** * Unparsed selector of the directive, or `null` if there was no selector.