Skip to content
Permalink
Browse files

feat(ivy): error in ivy when inheriting a ctor from an undecorated ba…

…se (#34460)

Angular View Engine uses global knowledge to compile the following code:

```typescript
export class Base {
  constructor(private vcr: ViewContainerRef) {}
}

@directive({...})
export class Dir extends Base {
  // constructor inherited from base
}
```

Here, `Dir` extends `Base` and inherits its constructor. To create a `Dir`
the arguments to this inherited constructor must be obtained via dependency
injection. View Engine is able to generate a correct factory for `Dir` to do
this because via metadata it knows the arguments of `Base`'s constructor,
even if `Base` is declared in a different library.

In Ivy, DI is entirely a runtime concept. Currently `Dir` is compiled with
an ngDirectiveDef field that delegates its factory to `getInheritedFactory`.
This looks for some kind of factory function on `Base`, which comes up
empty. This case looks identical to an inheritance chain with no
constructors, which works today in Ivy.

Both of these cases will now become an error in this commit. If a decorated
class inherits from an undecorated base class, a diagnostic is produced
informing the user of the need to either explicitly declare a constructor or
to decorate the base class.

PR Close #34460
  • Loading branch information
crisbeto authored and kara committed Nov 26, 2019
1 parent 0638e65 commit f563c7c656ba9fac36e29836dd920f4ee9f4c69a
@@ -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<unknown, unknown, unknown>,
// clang-format on
// Pipe handler must be before injectable handler in list so pipe factories are printed
@@ -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.`);
});
});

@@ -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) {
@@ -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}.`);
}
@@ -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};
@@ -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) => {
@@ -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 {
@@ -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<DecoratorHandler<unknown, unknown, unknown>>,
this.reflector, evaluator, metaRegistry, this.scopeRegistry, this.metaReader,
this.defaultImportTracker, injectableRegistry, this.isCore, this.closureCompilerEnabled
) as Readonly<DecoratorHandler<unknown, unknown, unknown>>,
// 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)

0 comments on commit f563c7c

Please sign in to comment.
You can’t perform that action at this time.