Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(ivy): allow abstract directives to have an invalid constructor #32987

Closed
wants to merge 1 commit into from
+307 −117
Closed

fix(ivy): allow abstract directives to have an invalid constructor #32987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/compiler-cli/src/ngtsc/annotations/src/component.ts
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {ConstantPool, CssSelector, DEFAULT_INTERPOLATION_CONFIG, DomElementSchemaRegistry, Expression, ExternalExpr, Identifiers, InterpolationConfig, LexerRange, ParseError, ParseSourceFile, ParseTemplateOptions, R3ComponentMetadata, R3TargetBinder, SchemaMetadata, SelectorMatcher, Statement, TmplAstNode, WrappedNodeExpr, compileComponentFromMetadata, makeBindingParser, parseTemplate} from '@angular/compiler';
import {ConstantPool, CssSelector, DEFAULT_INTERPOLATION_CONFIG, DomElementSchemaRegistry, Expression, ExternalExpr, Identifiers, InterpolationConfig, LexerRange, ParseError, ParseSourceFile, ParseTemplateOptions, R3ComponentMetadata, R3FactoryTarget, R3TargetBinder, SchemaMetadata, SelectorMatcher, Statement, TmplAstNode, WrappedNodeExpr, compileComponentFromMetadata, makeBindingParser, parseTemplate} from '@angular/compiler';
import * as ts from 'typescript';

import {CycleAnalyzer} from '../../cycles';
Expand Down Expand Up @@ -490,7 +490,8 @@ export class ComponentDecoratorHandler implements
CompileResult[] {
const meta = analysis.meta;
const res = compileComponentFromMetadata(meta, pool, makeBindingParser());
const factoryRes = compileNgFactoryDefField({...meta, injectFn: Identifiers.directiveInject});
const factoryRes = compileNgFactoryDefField(
{...meta, injectFn: Identifiers.directiveInject, target: R3FactoryTarget.Component});
if (analysis.metadataStmt !== null) {
factoryRes.statements.push(analysis.metadataStmt);
}
Expand Down
21 changes: 17 additions & 4 deletions packages/compiler-cli/src/ngtsc/annotations/src/directive.ts
View file
Open in desktop
Expand Up @@ -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, R3FactoryTarget, R3QueryMetadata, Statement, WrappedNodeExpr, compileDirectiveFromMetadata, makeBindingParser, parseHostBindings, verifyHostBindings} from '@angular/compiler';
import * as ts from 'typescript';

import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
Expand All @@ -19,7 +19,7 @@ import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFl

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} = {};

Expand Down Expand Up @@ -89,7 +89,8 @@ export class DirectiveDecoratorHandler implements
CompileResult[] {
const meta = analysis.meta;
const res = compileDirectiveFromMetadata(meta, pool, makeBindingParser());
const factoryRes = compileNgFactoryDefField({...meta, injectFn: Identifiers.directiveInject});
const factoryRes = compileNgFactoryDefField(
{...meta, injectFn: Identifiers.directiveInject, target: R3FactoryTarget.Directive});
if (analysis.metadataStmt !== null) {
factoryRes.statements.push(analysis.metadataStmt);
}
Expand Down Expand Up @@ -228,11 +229,23 @@ export function extractDirectiveMetadata(
exportAs = resolved.split(',').map(part => part.trim());
}

const rawCtorDeps = getConstructorDependencies(clazz, reflector, defaultImportRecorder, isCore);
let ctorDeps: R3DependencyMetadata[]|'invalid'|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,
},
Expand Down
6 changes: 3 additions & 3 deletions packages/compiler-cli/src/ngtsc/annotations/src/factory.ts
View file
Open in desktop
Expand Up @@ -6,11 +6,11 @@
* found in the LICENSE file at https://angular.io/license
*/

import {R3FactoryDefMetadata, compileFactoryFromMetadata} from '@angular/compiler';
import {R3FactoryMetadata, compileFactoryFunction} from '@angular/compiler';

import {CompileResult} from '../../transform';

export function compileNgFactoryDefField(metadata: R3FactoryDefMetadata): CompileResult {
const res = compileFactoryFromMetadata(metadata);
export function compileNgFactoryDefField(metadata: R3FactoryMetadata): CompileResult {
const res = compileFactoryFunction(metadata);
return {name: 'ɵfac', initializer: res.factory, statements: res.statements, type: res.type};
}
45 changes: 13 additions & 32 deletions packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Expression, Identifiers, LiteralExpr, R3DependencyMetadata, R3InjectableMetadata, R3ResolvedDependencyType, Statement, WrappedNodeExpr, compileInjectable as compileIvyInjectable} from '@angular/compiler';
import {Expression, Identifiers, LiteralExpr, R3DependencyMetadata, R3FactoryTarget, R3InjectableMetadata, R3ResolvedDependencyType, Statement, WrappedNodeExpr, compileInjectable as compileIvyInjectable} from '@angular/compiler';
import * as ts from 'typescript';

import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
Expand All @@ -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;
Expand Down Expand Up @@ -83,7 +83,8 @@ export class InjectableDecoratorHandler implements
type: meta.type,
typeArgumentCount: meta.typeArgumentCount,
deps: analysis.ctorDeps,
injectFn: Identifiers.inject
injectFn: Identifiers.inject,
target: R3FactoryTarget.Injectable,
});
if (analysis.metadataStmt !== null) {
factoryRes.statements.push(analysis.metadataStmt);
Expand Down Expand Up @@ -216,45 +217,25 @@ 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 a provider def (ɵprov) that throws.
// constructor signature does not work for DI then a factory definition (ɵfac) 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) {
if (strictCtorDeps && meta.useValue === undefined && meta.useExisting === undefined &&
meta.useClass === undefined && meta.useFactory === undefined) {
// Since use* was not provided, validate the deps according to strictCtorDeps.
validateConstructorDependencies(clazz, rawCtorDeps);
ctorDeps = validateConstructorDependencies(clazz, rawCtorDeps);
} else {
ctorDeps = unwrapConstructorDependencies(rawCtorDeps);
}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Identifiers, R3PipeMetadata, Statement, WrappedNodeExpr, compilePipeFromMetadata} from '@angular/compiler';
import {Identifiers, R3FactoryTarget, R3PipeMetadata, Statement, WrappedNodeExpr, compilePipeFromMetadata} from '@angular/compiler';
import * as ts from 'typescript';

import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
Expand Down Expand Up @@ -114,7 +114,7 @@ export class PipeDecoratorHandler implements DecoratorHandler<PipeHandlerData, D
const factoryRes = compileNgFactoryDefField({
...meta,
injectFn: Identifiers.directiveInject,
isPipe: true,
target: R3FactoryTarget.Pipe,
});
if (analysis.metadataStmt !== null) {
factoryRes.statements.push(analysis.metadataStmt);
Expand Down
26 changes: 26 additions & 0 deletions packages/compiler-cli/src/ngtsc/annotations/src/util.ts
View file
Open in desktop
Expand Up @@ -134,13 +134,39 @@ export function valueReferenceToExpression(
}
}

/**
* Convert `ConstructorDeps` into the `R3DependencyMetadata` array for those deps if they're valid,
* or into an `'invalid'` signal if they're not.
*
* This is a companion function to `validateConstructorDependencies` which accepts invalid deps.
*/
export function unwrapConstructorDependencies(deps: ConstructorDeps | null): R3DependencyMetadata[]|
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: maybe this should not have been extracted into a function, but rather repeated at the call sites as it previously was. The reason being that the pending TODO is not applicable to abstract directives, so this shared function may not be desirable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this LGTM :) I just changed the comments.

'invalid'|null {
if (deps === null) {
return null;
} else if (deps.deps !== null) {
// These constructor dependencies are valid.
return deps.deps;
} else {
// These deps are invalid.
return 'invalid';
}
}

export function getValidConstructorDependencies(
clazz: ClassDeclaration, reflector: ReflectionHost,
defaultImportRecorder: DefaultImportRecorder, isCore: boolean): R3DependencyMetadata[]|null {
return validateConstructorDependencies(
clazz, getConstructorDependencies(clazz, reflector, defaultImportRecorder, isCore));
}

/**
* Validate that `ConstructorDeps` does not have any invalid dependencies and convert them into the
* `R3DependencyMetadata` array if so, or raise a diagnostic if some deps are invalid.
*
* This is a companion function to `unwrapConstructorDependencies` which does not accept invalid
* deps.
*/
export function validateConstructorDependencies(
clazz: ClassDeclaration, deps: ConstructorDeps | null): R3DependencyMetadata[]|null {
if (deps === null) {
Expand Down
3 changes: 1 addition & 2 deletions packages/compiler-cli/src/transformers/api.ts
View file
Open in desktop
Expand Up @@ -66,8 +66,7 @@ export interface CompilerOptions extends ts.CompilerOptions {
// be determined. When this value option is not provided or is `false`, constructor
// parameters of classes marked with `@Injectable` whose type cannot be resolved will
// produce a warning. With this option `true`, they produce an error. When this option is
// not provided is treated as if it were `false`. In Angular 6.0, if this option is not
// provided, it will be treated as `true`.
// not provided is treated as if it were `false`.
strictInjectionParameters?: boolean;

// Whether to generate a flat module index of the given name and the corresponding
Expand Down