Skip to content

Commit

Permalink
Revert "feat(compiler-cli): exclude abstract classes from `strictInje…
Browse files Browse the repository at this point in the history 
…ctionParameters` requirement (#44615)" (#45862)

This reverts commit 9cf14ff.

PR Close #45862
  • Loading branch information
@dylhunn
dylhunn committed May 4, 2022
1 parent fa755b2 commit a2d5358
Show file tree
Hide file tree
Showing 23 changed files with 114 additions and 632 deletions.
1 change: 0 additions & 1 deletion goldens/public-api/compiler-cli/error_code.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ export enum ErrorCode {
IMPORT_CYCLE_DETECTED = 3003,
IMPORT_GENERATION_FAILURE = 3004,
INJECTABLE_DUPLICATE_PROV = 9001,
INJECTABLE_INHERITS_INVALID_CONSTRUCTOR = 2013,
INLINE_TCB_REQUIRED = 8900,
INLINE_TYPE_CTOR_REQUIRED = 8901,
INVALID_BANANA_IN_BOX = 8101,
Expand Down
14 changes: 6 additions & 8 deletions packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,12 @@ import ts from 'typescript';

import {ParsedConfiguration} from '../../..';
import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, PipeDecoratorHandler, ReferencesRegistry, ResourceLoader} from '../../../src/ngtsc/annotations';
import {InjectableClassRegistry} from '../../../src/ngtsc/annotations/common';
import {CycleAnalyzer, CycleHandlingStrategy, ImportGraph} from '../../../src/ngtsc/cycles';
import {isFatalDiagnosticError} from '../../../src/ngtsc/diagnostics';
import {absoluteFromSourceFile, LogicalFileSystem, ReadonlyFileSystem} from '../../../src/ngtsc/file_system';
import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, PrivateExportAliasingHost, Reexport, ReferenceEmitter} from '../../../src/ngtsc/imports';
import {SemanticSymbol} from '../../../src/ngtsc/incremental/semantic_graph';
import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, LocalMetadataRegistry, ResourceRegistry} from '../../../src/ngtsc/metadata';
import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry, ResourceRegistry} from '../../../src/ngtsc/metadata';
import {PartialEvaluator} from '../../../src/ngtsc/partial_evaluator';
import {NOOP_PERF_RECORDER} from '../../../src/ngtsc/perf';
import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver, TypeCheckScopeRegistry} from '../../../src/ngtsc/scope';
Expand Down Expand Up @@ -98,15 +97,14 @@ export class DecorationAnalyzer {
new PartialEvaluator(this.reflectionHost, this.typeChecker, /* dependencyTracker */ null);
importGraph = new ImportGraph(this.typeChecker, NOOP_PERF_RECORDER);
cycleAnalyzer = new CycleAnalyzer(this.importGraph);
injectableRegistry = new InjectableClassRegistry(this.reflectionHost, this.isCore);
injectableRegistry = new InjectableClassRegistry(this.reflectionHost);
typeCheckScopeRegistry = new TypeCheckScopeRegistry(this.scopeRegistry, this.fullMetaReader);
handlers: DecoratorHandler<unknown, unknown, SemanticSymbol|null, unknown>[] = [
new ComponentDecoratorHandler(
this.reflectionHost, this.evaluator, this.fullRegistry, this.fullMetaReader,
this.scopeRegistry, this.dtsModuleScopeResolver, this.scopeRegistry,
this.typeCheckScopeRegistry, new ResourceRegistry(), this.isCore,
/* strictCtorDeps */ false, this.resourceManager, this.rootDirs,
!!this.compilerOptions.preserveWhitespaces,
this.typeCheckScopeRegistry, new ResourceRegistry(), this.isCore, this.resourceManager,
this.rootDirs, !!this.compilerOptions.preserveWhitespaces,
/* i18nUseExternalIds */ true, this.bundle.enableI18nLegacyMessageIdFormat,
/* usePoisonedData */ false,
/* i18nNormalizeLineEndingsInICUs */ false, this.moduleResolver, this.cycleAnalyzer,
Expand All @@ -119,7 +117,7 @@ export class DecorationAnalyzer {
// clang-format off
new DirectiveDecoratorHandler(
this.reflectionHost, this.evaluator, this.fullRegistry, this.scopeRegistry,
this.fullMetaReader, this.injectableRegistry, this.isCore, /* strictCtorDeps */ false,
this.fullMetaReader, this.injectableRegistry, this.isCore,
/* semanticDepGraphUpdater */ null,
!!this.compilerOptions.annotateForClosureCompiler,
// In ngcc we want to compile undecorated classes with Angular features. As of
Expand All @@ -136,7 +134,7 @@ export class DecorationAnalyzer {
this.reflectionHost, this.evaluator, this.metaRegistry, this.scopeRegistry,
this.injectableRegistry, this.isCore, NOOP_PERF_RECORDER),
new InjectableDecoratorHandler(
this.reflectionHost, this.evaluator, this.isCore,
this.reflectionHost, this.isCore,
/* strictCtorDeps */ false, this.injectableRegistry, NOOP_PERF_RECORDER,
/* errorOnDuplicateProv */ false),
new NgModuleDecoratorHandler(
Expand Down
1 change: 0 additions & 1 deletion packages/compiler-cli/src/ngtsc/annotations/common/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ export * from './src/di';
export * from './src/diagnostics';
export * from './src/evaluation';
export * from './src/factory';
export * from './src/injectable_registry';
export * from './src/metadata';
export * from './src/references_registry';
export * from './src/schema';
Expand Down
125 changes: 34 additions & 91 deletions packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ import ts from 'typescript';

import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../../diagnostics';
import {Reference} from '../../../imports';
import {InjectableClassRegistry, MetadataReader} from '../../../metadata';
import {describeResolvedType, DynamicValue, PartialEvaluator, ResolvedValue, traceDynamicValue} from '../../../partial_evaluator';
import {ClassDeclaration, ReflectionHost} from '../../../reflection';
import {DeclarationData, LocalModuleScopeRegistry} from '../../../scope';
import {identifierOfNode} from '../../../util/src/typescript';

import {InjectableClassRegistry} from './injectable_registry';
import {isAbstractClassDeclaration, readBaseClass} from './util';
import {readBaseClass} from './util';


/**
Expand Down Expand Up @@ -102,10 +102,7 @@ export function getProviderDiagnostics(
const diagnostics: ts.Diagnostic[] = [];

for (const provider of providerClasses) {
const injectableMeta = registry.getInjectableMeta(provider.node);
if (injectableMeta !== null) {
// The provided type is recognized as injectable, so we don't report a diagnostic for this
// provider.
if (registry.isInjectable(provider.node)) {
continue;
}

Expand All @@ -127,9 +124,9 @@ Either add the @Injectable() decorator to '${
}

export function getDirectiveDiagnostics(
node: ClassDeclaration, injectableRegistry: InjectableClassRegistry,
evaluator: PartialEvaluator, reflector: ReflectionHost, scopeRegistry: LocalModuleScopeRegistry,
strictInjectionParameters: boolean, kind: 'Directive'|'Component'): ts.Diagnostic[]|null {
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) => {
Expand All @@ -150,8 +147,7 @@ export function getDirectiveDiagnostics(
addDiagnostics(makeDuplicateDeclarationError(node, duplicateDeclarations, kind));
}

addDiagnostics(checkInheritanceOfInjectable(
node, injectableRegistry, reflector, evaluator, strictInjectionParameters, kind));
addDiagnostics(checkInheritanceOfDirective(node, reader, reflector, evaluator));
return diagnostics;
}

Expand All @@ -163,42 +159,9 @@ export function getUndecoratedClassWithAngularFeaturesDiagnostic(node: ClassDecl
`Angular decorator.`);
}

export function checkInheritanceOfInjectable(
node: ClassDeclaration, injectableRegistry: InjectableClassRegistry, reflector: ReflectionHost,
evaluator: PartialEvaluator, strictInjectionParameters: boolean,
kind: 'Directive'|'Component'|'Pipe'|'Injectable'): ts.Diagnostic|null {
const classWithCtor = findInheritedCtor(node, injectableRegistry, reflector, evaluator);
if (classWithCtor === null || classWithCtor.isCtorValid) {
// The class does not inherit a constructor, or the inherited constructor is compatible
// with DI; no need to report a diagnostic.
return null;
}

if (!classWithCtor.isDecorated) {
// The inherited constructor exists in a class that does not have an Angular decorator.
// This is an error, as there won't be a factory definition available for DI to invoke
// the constructor.
return getInheritedUndecoratedCtorDiagnostic(node, classWithCtor.ref, kind);
}

if (!strictInjectionParameters || isAbstractClassDeclaration(node)) {
// An invalid constructor is only reported as error under `strictInjectionParameters` and
// only for concrete classes; follow the same exclusions for derived types.
return null;
}

return getInheritedInvalidCtorDiagnostic(node, classWithCtor.ref, kind);
}

interface ClassWithCtor {
ref: Reference<ClassDeclaration>;
isCtorValid: boolean;
isDecorated: boolean;
}

export function findInheritedCtor(
node: ClassDeclaration, injectableRegistry: InjectableClassRegistry, reflector: ReflectionHost,
evaluator: PartialEvaluator): ClassWithCtor|null {
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.
Expand All @@ -215,64 +178,44 @@ export function findInheritedCtor(
return null;
}

const injectableMeta = injectableRegistry.getInjectableMeta(baseClass.node);
if (injectableMeta !== null) {
if (injectableMeta.ctorDeps !== null) {
// The class has an Angular decorator with a constructor.
return {
ref: baseClass,
isCtorValid: injectableMeta.ctorDeps !== 'invalid',
isDecorated: true,
};
}
} else {
const baseClassConstructorParams = reflector.getConstructorParameters(baseClass.node);
if (baseClassConstructorParams !== null) {
// The class is not decorated, but it does have constructor. An undecorated class is only
// allowed to have a constructor without parameters, otherwise it is invalid.
return {
ref: baseClass,
isCtorValid: baseClassConstructorParams.length === 0,
isDecorated: false,
};
}
// 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 = readBaseClass(baseClass.node, reflector, evaluator);
baseClass = newParentClass;
}

return null;
}

function getInheritedInvalidCtorDiagnostic(
node: ClassDeclaration, baseClass: Reference,
kind: 'Directive'|'Component'|'Pipe'|'Injectable') {
const baseClassName = baseClass.debugName;

return makeDiagnostic(
ErrorCode.INJECTABLE_INHERITS_INVALID_CONSTRUCTOR, node.name,
`The ${kind.toLowerCase()} ${node.name.text} inherits its constructor from ${
baseClassName}, ` +
`but the latter has a constructor parameter that is not compatible with dependency injection. ` +
`Either add an explicit constructor to ${node.name.text} or change ${
baseClassName}'s constructor to ` +
`use parameters that are valid for DI.`);
}

function getInheritedUndecoratedCtorDiagnostic(
node: ClassDeclaration, baseClass: Reference,
kind: 'Directive'|'Component'|'Pipe'|'Injectable') {
node: ClassDeclaration, baseClass: Reference, reader: MetadataReader) {
const subclassMeta = reader.getDirectiveMetadata(new Reference(node))!;
const dirOrComp = subclassMeta.isComponent ? 'Component' : 'Directive';
const baseClassName = baseClass.debugName;
const baseNeedsDecorator =
kind === 'Component' || kind === 'Directive' ? 'Directive' : 'Injectable';

return makeDiagnostic(
ErrorCode.DIRECTIVE_INHERITS_UNDECORATED_CTOR, node.name,
`The ${kind.toLowerCase()} ${node.name.text} inherits its constructor from ${
`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 @${
baseNeedsDecorator} decorator ` +
`resolve the parameters of ${
baseClassName}'s constructor. Either add a @Directive decorator ` +
`to ${baseClassName}, or add an explicit constructor to ${node.name.text}.`);
}
50 changes: 0 additions & 50 deletions packages/compiler-cli/src/ngtsc/annotations/common/src/injectable_registry.ts
View file

This file was deleted.

5 changes: 0 additions & 5 deletions packages/compiler-cli/src/ngtsc/annotations/common/src/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -380,8 +380,3 @@ export function resolveImportedFile(
// Figure out what file is being imported.
return moduleResolver.resolveModule(expr.value.moduleName!, origin.fileName);
}

export function isAbstractClassDeclaration(clazz: ClassDeclaration): boolean {
return clazz.modifiers !== undefined &&
clazz.modifiers.some(mod => mod.kind === ts.SyntaxKind.AbstractKeyword);
}
24 changes: 10 additions & 14 deletions packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {assertSuccessfulReferenceEmit, ImportedFile, ModuleResolver, Reference,
import {DependencyTracker} from '../../../incremental/api';
import {extractSemanticTypeParameters, SemanticDepGraphUpdater} from '../../../incremental/semantic_graph';
import {IndexingContext} from '../../../indexer';
import {DirectiveMeta, extractDirectiveTypeCheckMeta, MetadataReader, MetadataRegistry, MetaKind, PipeMeta, ResourceRegistry} from '../../../metadata';
import {DirectiveMeta, extractDirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry, MetaKind, PipeMeta, ResourceRegistry} from '../../../metadata';
import {PartialEvaluator} from '../../../partial_evaluator';
import {PerfEvent, PerfRecorder} from '../../../perf';
import {ClassDeclaration, DeclarationNode, Decorator, ReflectionHost, reflectObjectLiteral} from '../../../reflection';
Expand All @@ -26,7 +26,7 @@ import {TypeCheckContext} from '../../../typecheck/api';
import {ExtendedTemplateChecker} from '../../../typecheck/extended/api';
import {getSourceFile} from '../../../util/src/typescript';
import {Xi18nContext} from '../../../xi18n';
import {compileDeclareFactory, compileNgFactoryDefField, compileResults, extractClassMetadata, extractSchemas, findAngularDecorator, getDirectiveDiagnostics, getProviderDiagnostics, InjectableClassRegistry, isExpressionForwardReference, readBaseClass, resolveEnumValue, resolveImportedFile, resolveLiteral, resolveProvidersRequiringFactory, ResourceLoader, toFactoryMetadata, wrapFunctionExpressionsInParens} from '../../common';
import {compileDeclareFactory, compileNgFactoryDefField, compileResults, extractClassMetadata, extractSchemas, findAngularDecorator, getDirectiveDiagnostics, getProviderDiagnostics, isExpressionForwardReference, readBaseClass, resolveEnumValue, resolveImportedFile, resolveLiteral, resolveProvidersRequiringFactory, ResourceLoader, toFactoryMetadata, wrapFunctionExpressionsInParens} from '../../common';
import {extractDirectiveMetadata, parseFieldArrayValue} from '../../directive';
import {NgModuleSymbol} from '../../ng_module';

Expand All @@ -51,13 +51,12 @@ export class ComponentDecoratorHandler implements
private scopeRegistry: LocalModuleScopeRegistry,
private typeCheckScopeRegistry: TypeCheckScopeRegistry,
private resourceRegistry: ResourceRegistry, private isCore: boolean,
private strictCtorDeps: boolean, private resourceLoader: ResourceLoader,
private rootDirs: ReadonlyArray<string>, private defaultPreserveWhitespaces: boolean,
private i18nUseExternalIds: boolean, private enableI18nLegacyMessageIdFormat: boolean,
private usePoisonedData: boolean, private i18nNormalizeLineEndingsInICUs: boolean,
private moduleResolver: ModuleResolver, private cycleAnalyzer: CycleAnalyzer,
private cycleHandlingStrategy: CycleHandlingStrategy, private refEmitter: ReferenceEmitter,
private depTracker: DependencyTracker|null,
private resourceLoader: ResourceLoader, private rootDirs: ReadonlyArray<string>,
private defaultPreserveWhitespaces: boolean, private i18nUseExternalIds: boolean,
private enableI18nLegacyMessageIdFormat: boolean, private usePoisonedData: boolean,
private i18nNormalizeLineEndingsInICUs: boolean, private moduleResolver: ModuleResolver,
private cycleAnalyzer: CycleAnalyzer, private cycleHandlingStrategy: CycleHandlingStrategy,
private refEmitter: ReferenceEmitter, private depTracker: DependencyTracker|null,
private injectableRegistry: InjectableClassRegistry,
private semanticDepGraphUpdater: SemanticDepGraphUpdater|null,
private annotateForClosureCompiler: boolean, private perf: PerfRecorder) {}
Expand Down Expand Up @@ -483,9 +482,7 @@ export class ComponentDecoratorHandler implements
});

this.resourceRegistry.registerResources(analysis.resources, node);
this.injectableRegistry.registerInjectable(node, {
ctorDeps: analysis.meta.deps,
});
this.injectableRegistry.registerInjectable(node);
}

index(
Expand Down Expand Up @@ -822,8 +819,7 @@ export class ComponentDecoratorHandler implements
}

const directiveDiagnostics = getDirectiveDiagnostics(
node, this.injectableRegistry, this.evaluator, this.reflector, this.scopeRegistry,
this.strictCtorDeps, 'Component');
node, this.metaReader, this.evaluator, this.reflector, this.scopeRegistry, 'Component');
if (directiveDiagnostics !== null) {
diagnostics.push(...directiveDiagnostics);
}
Expand Down
Loading

0 comments on commit a2d5358

Please sign in to comment.