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 angular#32981

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,
     },

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

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 {

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]

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.