Skip to content

Commit

Permalink
feat(ivy): throw compilation error when providing undecorated classes (
Browse files Browse the repository at this point in the history
…angular#34460)

Adds a compilation error if the consumer tries to pass in an undecorated class into the `providers` of an `NgModule`, or the `providers`/`viewProviders` arrays of a `Directive`/`Component`.

PR Close angular#34460
  • Loading branch information
crisbeto authored and kara committed Dec 18, 2019
1 parent 6057c7a commit dcc8ff4
Show file tree
Hide file tree
Showing 20 changed files with 613 additions and 60 deletions.
15 changes: 9 additions & 6 deletions packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@
*/
import {ConstantPool} from '@angular/compiler';
import * as ts from 'typescript';

import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, PipeDecoratorHandler, ReferencesRegistry, ResourceLoader} from '../../../src/ngtsc/annotations';
import {CycleAnalyzer, ImportGraph} from '../../../src/ngtsc/cycles';
import {isFatalDiagnosticError} from '../../../src/ngtsc/diagnostics';
import {FileSystem, LogicalFileSystem, absoluteFrom, dirname, resolve} from '../../../src/ngtsc/file_system';
import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, NOOP_DEFAULT_IMPORT_RECORDER, PrivateExportAliasingHost, Reexport, ReferenceEmitter} from '../../../src/ngtsc/imports';
import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, LocalMetadataRegistry} from '../../../src/ngtsc/metadata';
import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, InjectableClassRegistry, LocalMetadataRegistry} from '../../../src/ngtsc/metadata';
import {PartialEvaluator} from '../../../src/ngtsc/partial_evaluator';
import {ClassDeclaration} from '../../../src/ngtsc/reflection';
import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from '../../../src/ngtsc/scope';
Expand All @@ -30,6 +31,7 @@ import {AnalyzedClass, AnalyzedFile, CompiledClass, CompiledFile, DecorationAnal
import {NOOP_DEPENDENCY_TRACKER, analyzeDecorators, isWithinPackage} from './util';



/**
* Simple class that resolves and loads files directly from the filesystem.
*/
Expand Down Expand Up @@ -85,35 +87,36 @@ export class DecorationAnalyzer {
new PartialEvaluator(this.reflectionHost, this.typeChecker, /* dependencyTracker */ null);
importGraph = new ImportGraph(this.moduleResolver);
cycleAnalyzer = new CycleAnalyzer(this.importGraph);
injectableRegistry = new InjectableClassRegistry(this.reflectionHost);
handlers: DecoratorHandler<unknown, unknown, unknown>[] = [
new ComponentDecoratorHandler(
this.reflectionHost, this.evaluator, this.fullRegistry, this.fullMetaReader,
this.scopeRegistry, this.scopeRegistry, this.isCore, this.resourceManager, this.rootDirs,
/* defaultPreserveWhitespaces */ false,
/* i18nUseExternalIds */ true, this.bundle.enableI18nLegacyMessageIdFormat,
this.moduleResolver, this.cycleAnalyzer, this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER,
NOOP_DEPENDENCY_TRACKER, /* annotateForClosureCompiler */ false),
NOOP_DEPENDENCY_TRACKER, this.injectableRegistry, /* annotateForClosureCompiler */ false),
// See the note in ngtsc about why this cast is needed.
// clang-format off
new DirectiveDecoratorHandler(
this.reflectionHost, this.evaluator, this.fullRegistry, this.scopeRegistry,
NOOP_DEFAULT_IMPORT_RECORDER, this.isCore,
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
// before injectable factories (so injectable factories can delegate to them)
new PipeDecoratorHandler(
this.reflectionHost, this.evaluator, this.metaRegistry, this.scopeRegistry,
NOOP_DEFAULT_IMPORT_RECORDER, this.isCore),
NOOP_DEFAULT_IMPORT_RECORDER, this.injectableRegistry, this.isCore),
new InjectableDecoratorHandler(
this.reflectionHost, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore,
/* strictCtorDeps */ false, /* errorOnDuplicateProv */ false),
/* strictCtorDeps */ false, this.injectableRegistry, /* errorOnDuplicateProv */ false),
new NgModuleDecoratorHandler(
this.reflectionHost, this.evaluator, this.fullMetaReader, this.fullRegistry,
this.scopeRegistry, this.referencesRegistry, this.isCore, /* routeAnalyzer */ null,
this.refEmitter,
/* factoryTracker */ null, NOOP_DEFAULT_IMPORT_RECORDER,
/* annotateForClosureCompiler */ false),
/* annotateForClosureCompiler */ false, this.injectableRegistry),
];
migrations: Migration[] = [
new UndecoratedParentMigration(),
Expand Down
90 changes: 72 additions & 18 deletions packages/compiler-cli/src/ngtsc/annotations/src/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {absoluteFrom, relative} from '../../file_system';
import {DefaultImportRecorder, ModuleResolver, Reference, ReferenceEmitter} from '../../imports';
import {DependencyTracker} from '../../incremental/api';
import {IndexingContext} from '../../indexer';
import {DirectiveMeta, MetadataReader, MetadataRegistry, extractDirectiveGuards} from '../../metadata';
import {DirectiveMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry, extractDirectiveGuards} from '../../metadata';
import {flattenInheritedDirectiveMetadata} from '../../metadata/src/inheritance';
import {EnumValue, PartialEvaluator} from '../../partial_evaluator';
import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection';
Expand All @@ -26,10 +26,11 @@ 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 {extractDirectiveMetadata, parseFieldArrayValue} from './directive';
import {compileNgFactoryDefField} from './factory';
import {generateSetClassMetadataCall} from './metadata';
import {findAngularDecorator, isAngularCoreReference, isExpressionForwardReference, makeDuplicateDeclarationError, readBaseClass, unwrapExpression, wrapFunctionExpressionsInParens} from './util';
import {findAngularDecorator, isAngularCoreReference, isExpressionForwardReference, makeDuplicateDeclarationError, readBaseClass, resolveProvidersRequiringFactory, unwrapExpression, wrapFunctionExpressionsInParens} from './util';

const EMPTY_MAP = new Map<string, Expression>();
const EMPTY_ARRAY: any[] = [];
Expand All @@ -53,6 +54,18 @@ export interface ComponentAnalysisData {
guards: ReturnType<typeof extractDirectiveGuards>;
template: ParsedTemplateWithSource;
metadataStmt: Statement|null;

/**
* Providers extracted from the `providers` field of the component annotation which will require
* an Angular factory definition at runtime.
*/
providersRequiringFactory: Set<Reference<ClassDeclaration>>|null;

/**
* Providers extracted from the `viewProviders` field of the component annotation which will
* require an Angular factory definition at runtime.
*/
viewProvidersRequiringFactory: Set<Reference<ClassDeclaration>>|null;
}

export type ComponentResolutionData = Pick<R3ComponentMetadata, ComponentMetadataResolvedFields>;
Expand All @@ -71,7 +84,9 @@ export class ComponentDecoratorHandler implements
private enableI18nLegacyMessageIdFormat: boolean, private moduleResolver: ModuleResolver,
private cycleAnalyzer: CycleAnalyzer, private refEmitter: ReferenceEmitter,
private defaultImportRecorder: DefaultImportRecorder,
private depTracker: DependencyTracker|null, private annotateForClosureCompiler: boolean) {}
private depTracker: DependencyTracker|null,
private injectableRegistry: InjectableClassRegistry,
private annotateForClosureCompiler: boolean) {}

private literalCache = new Map<Decorator, ts.ObjectLiteralExpression>();
private elementSchemaRegistry = new DomElementSchemaRegistry();
Expand Down Expand Up @@ -186,12 +201,27 @@ export class ComponentDecoratorHandler implements
}
}, undefined) !;

const viewProviders: Expression|null = component.has('viewProviders') ?
new WrappedNodeExpr(
this.annotateForClosureCompiler ?
wrapFunctionExpressionsInParens(component.get('viewProviders') !) :
component.get('viewProviders') !) :
null;

// Note that we could technically combine the `viewProvidersRequiringFactory` and
// `providersRequiringFactory` into a single set, but we keep the separate so that
// we can distinguish where an error is coming from when logging the diagnostics in `resolve`.
let viewProvidersRequiringFactory: Set<Reference<ClassDeclaration>>|null = null;
let providersRequiringFactory: Set<Reference<ClassDeclaration>>|null = null;
let wrappedViewProviders: Expression|null = null;

if (component.has('viewProviders')) {
const viewProviders = component.get('viewProviders') !;
viewProvidersRequiringFactory =
resolveProvidersRequiringFactory(viewProviders, this.reflector, this.evaluator);
wrappedViewProviders = new WrappedNodeExpr(
this.annotateForClosureCompiler ? wrapFunctionExpressionsInParens(viewProviders) :
viewProviders);
}

if (component.has('providers')) {
providersRequiringFactory = resolveProvidersRequiringFactory(
component.get('providers') !, this.reflector, this.evaluator);
}

// Parse the template.
// If a preanalyze phase was executed, the template may already exist in parsed form, so check
Expand Down Expand Up @@ -290,14 +320,16 @@ export class ComponentDecoratorHandler implements
// These will be replaced during the compilation step, after all `NgModule`s have been
// analyzed and the full compilation scope for the component can be realized.
animations,
viewProviders,
viewProviders: wrappedViewProviders,
i18nUseExternalIds: this.i18nUseExternalIds, relativeContextFilePath,
},
guards: extractDirectiveGuards(node, this.reflector),
metadataStmt: generateSetClassMetadataCall(
node, this.reflector, this.defaultImportRecorder, this.isCore,
this.annotateForClosureCompiler),
template,
providersRequiringFactory,
viewProvidersRequiringFactory,
},
};
if (changeDetection !== null) {
Expand All @@ -321,6 +353,8 @@ export class ComponentDecoratorHandler implements
isComponent: true,
baseClass: analysis.baseClass, ...analysis.guards,
});

this.injectableRegistry.registerInjectable(node);
}

index(
Expand Down Expand Up @@ -395,14 +429,6 @@ export class ComponentDecoratorHandler implements

resolve(node: ClassDeclaration, analysis: Readonly<ComponentAnalysisData>):
ResolveResult<ComponentResolutionData> {
const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node);
if (duplicateDeclData !== null) {
// This component was declared twice (or more).
return {
diagnostics: [makeDuplicateDeclarationError(node, duplicateDeclData, 'Component')],
};
}

const context = node.getSourceFile();
// Check whether this component was registered with an NgModule. If so, it should be compiled
// under that module's compilation scope.
Expand Down Expand Up @@ -505,6 +531,34 @@ export class ComponentDecoratorHandler implements
this.scopeRegistry.setComponentAsRequiringRemoteScoping(node);
}
}

const diagnostics: ts.Diagnostic[] = [];

if (analysis.providersRequiringFactory !== null &&
analysis.meta.providers instanceof WrappedNodeExpr) {
const providerDiagnostics = getProviderDiagnostics(
analysis.providersRequiringFactory, analysis.meta.providers !.node,
this.injectableRegistry);
diagnostics.push(...providerDiagnostics);
}

if (analysis.viewProvidersRequiringFactory !== null &&
analysis.meta.viewProviders instanceof WrappedNodeExpr) {
const viewProviderDiagnostics = getProviderDiagnostics(
analysis.viewProvidersRequiringFactory, analysis.meta.viewProviders !.node,
this.injectableRegistry);
diagnostics.push(...viewProviderDiagnostics);
}

const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node);
if (duplicateDeclData !== null) {
diagnostics.push(makeDuplicateDeclarationError(node, duplicateDeclData, 'Component'));
}

if (diagnostics.length > 0) {
return {diagnostics};
}

return {data};
}

Expand Down
43 changes: 43 additions & 0 deletions packages/compiler-cli/src/ngtsc/annotations/src/diagnostics.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import * as ts from 'typescript';

import {ErrorCode, makeDiagnostic} from '../../diagnostics';
import {Reference} from '../../imports';
import {InjectableClassRegistry} from '../../metadata';
import {ClassDeclaration} from '../../reflection';

/**
* Gets the diagnostics for a set of provider classes.
* @param providerClasses Classes that should be checked.
* @param providersDeclaration Node that declares the providers array.
* @param registry Registry that keeps track of the registered injectable classes.
*/
export function getProviderDiagnostics(
providerClasses: Set<Reference<ClassDeclaration>>, providersDeclaration: ts.Expression,
registry: InjectableClassRegistry): ts.Diagnostic[] {
const diagnostics: ts.Diagnostic[] = [];

for (const provider of providerClasses) {
if (registry.isInjectable(provider.node)) {
continue;
}

const contextNode = provider.getOriginForDiagnostics(providersDeclaration);
diagnostics.push(makeDiagnostic(
ErrorCode.UNDECORATED_PROVIDER, contextNode,
`The class '${provider.node.name.text}' cannot be created via dependency injection, as it does not have an Angular decorator. This will result in an error at runtime.
Either add the @Injectable() decorator to '${provider.node.name.text}', or configure a different provider (such as a provider with 'useFactory').
`,
[{node: provider.node, messageText: `'${provider.node.name.text}' is declared here.`}]));
}

return diagnostics;
}
45 changes: 31 additions & 14 deletions packages/compiler-cli/src/ngtsc/annotations/src/directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,17 @@ import * as ts from 'typescript';

import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
import {DefaultImportRecorder, Reference} from '../../imports';
import {MetadataRegistry} from '../../metadata';
import {InjectableClassRegistry, 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 {compileNgFactoryDefField} from './factory';
import {generateSetClassMetadataCall} from './metadata';
import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, makeDuplicateDeclarationError, readBaseClass, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens, wrapTypeReference} from './util';
import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, makeDuplicateDeclarationError, readBaseClass, resolveProvidersRequiringFactory, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens, wrapTypeReference} from './util';

const EMPTY_OBJECT: {[key: string]: string} = {};
const FIELD_DECORATORS = [
Expand All @@ -37,13 +38,16 @@ export interface DirectiveHandlerData {
guards: ReturnType<typeof extractDirectiveGuards>;
meta: R3DirectiveMetadata;
metadataStmt: Statement|null;
providersRequiringFactory: Set<Reference<ClassDeclaration>>|null;
}

export class DirectiveDecoratorHandler implements
DecoratorHandler<Decorator|null, DirectiveHandlerData, unknown> {
constructor(
private reflector: ReflectionHost, private evaluator: PartialEvaluator,
private metaRegistry: MetadataRegistry, private scopeRegistry: LocalModuleScopeRegistry,
private defaultImportRecorder: DefaultImportRecorder, private isCore: boolean,
private defaultImportRecorder: DefaultImportRecorder,
private injectableRegistry: InjectableClassRegistry, private isCore: boolean,
private annotateForClosureCompiler: boolean) {}

readonly precedence = HandlerPrecedence.PRIMARY;
Expand Down Expand Up @@ -88,14 +92,20 @@ export class DirectiveDecoratorHandler implements
return {};
}

let providersRequiringFactory: Set<Reference<ClassDeclaration>>|null = null;
if (directiveResult !== undefined && directiveResult.decorator.has('providers')) {
providersRequiringFactory = resolveProvidersRequiringFactory(
directiveResult.decorator.get('providers') !, this.reflector, this.evaluator);
}

return {
analysis: {
meta: analysis,
metadataStmt: generateSetClassMetadataCall(
node, this.reflector, this.defaultImportRecorder, this.isCore,
this.annotateForClosureCompiler),
baseClass: readBaseClass(node, this.reflector, this.evaluator),
guards: extractDirectiveGuards(node, this.reflector),
guards: extractDirectiveGuards(node, this.reflector), providersRequiringFactory
}
};
}
Expand All @@ -115,18 +125,28 @@ export class DirectiveDecoratorHandler implements
isComponent: false,
baseClass: analysis.baseClass, ...analysis.guards,
});

this.injectableRegistry.registerInjectable(node);
}

resolve(node: ClassDeclaration): ResolveResult<unknown> {
resolve(node: ClassDeclaration, analysis: DirectiveHandlerData): ResolveResult<unknown> {
const diagnostics: ts.Diagnostic[] = [];

if (analysis.providersRequiringFactory !== null &&
analysis.meta.providers instanceof WrappedNodeExpr) {
const providerDiagnostics = getProviderDiagnostics(
analysis.providersRequiringFactory, analysis.meta.providers !.node,
this.injectableRegistry);
diagnostics.push(...providerDiagnostics);
}

const duplicateDeclData = this.scopeRegistry.getDuplicateDeclarations(node);
if (duplicateDeclData !== null) {
// This directive was declared twice (or more).
return {
diagnostics: [makeDuplicateDeclarationError(node, duplicateDeclData, 'Directive')],
};
diagnostics.push(makeDuplicateDeclarationError(node, duplicateDeclData, 'Directive'));
}

return {};
return {diagnostics: diagnostics.length > 0 ? diagnostics : undefined};
}

compile(
Expand Down Expand Up @@ -160,10 +180,8 @@ export function extractDirectiveMetadata(
clazz: ClassDeclaration, decorator: Readonly<Decorator|null>, reflector: ReflectionHost,
evaluator: PartialEvaluator, defaultImportRecorder: DefaultImportRecorder, isCore: boolean,
flags: HandlerFlags, annotateForClosureCompiler: boolean,
defaultSelector: string | null = null): {
decorator: Map<string, ts.Expression>,
metadata: R3DirectiveMetadata,
}|undefined {
defaultSelector: string | null =
null): {decorator: Map<string, ts.Expression>, metadata: R3DirectiveMetadata}|undefined {
let directive: Map<string, ts.Expression>;
if (decorator === null || decorator.args === null || decorator.args.length === 0) {
directive = new Map<string, ts.Expression>();
Expand Down Expand Up @@ -390,7 +408,6 @@ export function extractQueriesFromDecorator(
view: R3QueryMetadata[],
} {
const content: R3QueryMetadata[] = [], view: R3QueryMetadata[] = [];
const expr = unwrapExpression(queryData);
if (!ts.isObjectLiteralExpression(queryData)) {
throw new Error(`queries metadata must be an object literal`);
}
Expand Down
Loading

0 comments on commit dcc8ff4

Please sign in to comment.