From 99d85828829198b10e1d4f81197dfb6a4d7defbd Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Fri, 1 Feb 2019 12:23:21 -0800 Subject: [PATCH] feat(ivy): support @Injectable on already decorated classes (#28523) Previously, ngtsc would throw an error if two decorators were matched on the same class simultaneously. However, @Injectable is a special case, and it appears frequently on component, directive, and pipe classes. For pipes in particular, it's a common pattern to treat the pipe class also as an injectable service. ngtsc actually lacked the capability to compile multiple matching decorators on a class, so this commit adds support for that. Decorator handlers (and thus the decorators they match) are classified into three categories: PRIMARY, SHARED, and WEAK. PRIMARY handlers compile decorators that cannot coexist with other primary decorators. The handlers for Component, Directive, Pipe, and NgModule are marked as PRIMARY. A class may only have one decorator from this group. SHARED handlers compile decorators that can coexist with others. Injectable is the only decorator in this category, meaning it's valid to put an @Injectable decorator on a previously decorated class. WEAK handlers behave like SHARED, but are dropped if any non-WEAK handler matches a class. The handler which compiles ngBaseDef is WEAK, since ngBaseDef is only needed if a class doesn't otherwise have a decorator. Tests are added to validate that @Injectable can coexist with the other decorators and that an error is generated when mixing the primaries. PR Close #28523 --- .../ngcc/src/analysis/decoration_analyzer.ts | 69 +++-- .../test/analysis/decoration_analyzer_spec.ts | 25 +- .../src/ngtsc/annotations/src/base_def.ts | 17 +- .../src/ngtsc/annotations/src/component.ts | 17 +- .../src/ngtsc/annotations/src/directive.ts | 16 +- .../src/ngtsc/annotations/src/injectable.ts | 16 +- .../src/ngtsc/annotations/src/ng_module.ts | 16 +- .../src/ngtsc/annotations/src/pipe.ts | 16 +- .../ngtsc/annotations/test/component_spec.ts | 4 +- .../src/ngtsc/diagnostics/src/code.ts | 5 + .../src/ngtsc/transform/src/api.ts | 38 ++- .../src/ngtsc/transform/src/compilation.ts | 245 +++++++++++++----- .../src/ngtsc/transform/src/transform.ts | 8 +- .../src/ngtsc/util/src/typescript.ts | 8 + .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 89 +++++++ 15 files changed, 466 insertions(+), 123 deletions(-) diff --git a/packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts index 03149452bc..2224c3723a 100644 --- a/packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts @@ -14,7 +14,7 @@ import {BaseDefDecoratorHandler, ComponentDecoratorHandler, DirectiveDecoratorHa import {CycleAnalyzer, ImportGraph} from '../../../ngtsc/cycles'; import {ModuleResolver, TsReferenceResolver} from '../../../ngtsc/imports'; import {PartialEvaluator} from '../../../ngtsc/partial_evaluator'; -import {CompileResult, DecoratorHandler} from '../../../ngtsc/transform'; +import {CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../../ngtsc/transform'; import {DecoratedClass} from '../host/decorated_class'; import {NgccReflectionHost} from '../host/ngcc_host'; import {isDefined} from '../utils'; @@ -26,8 +26,7 @@ export interface AnalyzedFile { export interface AnalyzedClass extends DecoratedClass { diagnostics?: ts.Diagnostic[]; - handler: DecoratorHandler; - analysis: any; + matches: {handler: DecoratorHandler; analysis: any;}[]; } export interface CompiledClass extends AnalyzedClass { compilation: CompileResult[]; } @@ -43,7 +42,7 @@ export const DecorationAnalyses = Map; export interface MatchingHandler { handler: DecoratorHandler; - match: M; + detected: M; } /** @@ -118,21 +117,52 @@ export class DecorationAnalyzer { protected analyzeClass(clazz: DecoratedClass): AnalyzedClass|null { const matchingHandlers = this.handlers .map(handler => { - const match = + const detected = handler.detect(clazz.declaration, clazz.decorators); - return {handler, match}; + return {handler, detected}; }) .filter(isMatchingHandler); - if (matchingHandlers.length > 1) { - throw new Error('TODO.Diagnostic: Class has multiple Angular decorators.'); - } if (matchingHandlers.length === 0) { return null; } - const {handler, match} = matchingHandlers[0]; - const {analysis, diagnostics} = handler.analyze(clazz.declaration, match); - return {...clazz, handler, analysis, diagnostics}; + const detections: {handler: DecoratorHandler, detected: DetectResult}[] = []; + let hasWeakHandler: boolean = false; + let hasNonWeakHandler: boolean = false; + let hasPrimaryHandler: boolean = false; + + for (const {handler, detected} of matchingHandlers) { + if (hasNonWeakHandler && handler.precedence === HandlerPrecedence.WEAK) { + continue; + } else if (hasWeakHandler && handler.precedence !== HandlerPrecedence.WEAK) { + // Clear all the WEAK handlers from the list of matches. + detections.length = 0; + } + if (hasPrimaryHandler && handler.precedence === HandlerPrecedence.PRIMARY) { + throw new Error(`TODO.Diagnostic: Class has multiple incompatible Angular decorators.`); + } + + detections.push({handler, detected}); + if (handler.precedence === HandlerPrecedence.WEAK) { + hasWeakHandler = true; + } else if (handler.precedence === HandlerPrecedence.SHARED) { + hasNonWeakHandler = true; + } else if (handler.precedence === HandlerPrecedence.PRIMARY) { + hasNonWeakHandler = true; + hasPrimaryHandler = true; + } + } + + const matches: {handler: DecoratorHandler, analysis: any}[] = []; + const allDiagnostics: ts.Diagnostic[] = []; + for (const {handler, detected} of detections) { + const {analysis, diagnostics} = handler.analyze(clazz.declaration, detected.metadata); + if (diagnostics !== undefined) { + allDiagnostics.push(...diagnostics); + } + matches.push({handler, analysis}); + } + return {...clazz, matches, diagnostics: allDiagnostics.length > 0 ? allDiagnostics : undefined}; } protected compileFile(analyzedFile: AnalyzedFile): CompiledFile { @@ -145,15 +175,20 @@ export class DecorationAnalyzer { } protected compileClass(clazz: AnalyzedClass, constantPool: ConstantPool): CompileResult[] { - let compilation = clazz.handler.compile(clazz.declaration, clazz.analysis, constantPool); - if (!Array.isArray(compilation)) { - compilation = [compilation]; + const compilations: CompileResult[] = []; + for (const {handler, analysis} of clazz.matches) { + const result = handler.compile(clazz.declaration, analysis, constantPool); + if (Array.isArray(result)) { + compilations.push(...result); + } else { + compilations.push(result); + } } - return compilation; + return compilations; } } function isMatchingHandler(handler: Partial>): handler is MatchingHandler { - return !!handler.match; + return !!handler.detected; } diff --git a/packages/compiler-cli/src/ngcc/test/analysis/decoration_analyzer_spec.ts b/packages/compiler-cli/src/ngcc/test/analysis/decoration_analyzer_spec.ts index e4a145825f..889b8dbbec 100644 --- a/packages/compiler-cli/src/ngcc/test/analysis/decoration_analyzer_spec.ts +++ b/packages/compiler-cli/src/ngcc/test/analysis/decoration_analyzer_spec.ts @@ -8,7 +8,7 @@ import * as ts from 'typescript'; import {Decorator} from '../../../ngtsc/reflection'; -import {DecoratorHandler} from '../../../ngtsc/transform'; +import {DecoratorHandler, DetectResult} from '../../../ngtsc/transform'; import {DecorationAnalyses, DecorationAnalyzer} from '../../src/analysis/decoration_analyzer'; import {NgccReferencesRegistry} from '../../src/analysis/ngcc_references_registry'; import {Esm2015ReflectionHost} from '../../src/host/esm2015_host'; @@ -62,15 +62,24 @@ function createTestHandler() { 'compile', ]); // Only detect the Component decorator - handler.detect.and.callFake((node: ts.Declaration, decorators: Decorator[]) => { - if (!decorators) { - return undefined; - } - return decorators.find(d => d.name === 'Component'); - }); + handler.detect.and.callFake( + (node: ts.Declaration, decorators: Decorator[]): DetectResult| undefined => { + if (!decorators) { + return undefined; + } + const metadata = decorators.find(d => d.name === 'Component'); + if (metadata === undefined) { + return undefined; + } else { + return { + metadata, + trigger: metadata.node, + }; + } + }); // The "test" analysis is just the name of the decorator being analyzed handler.analyze.and.callFake( - ((decl: ts.Declaration, dec: Decorator) => ({analysis: dec.name, diagnostics: null}))); + ((decl: ts.Declaration, dec: Decorator) => ({analysis: dec.name, diagnostics: undefined}))); // The "test" compilation result is just the name of the decorator being compiled handler.compile.and.callFake(((decl: ts.Declaration, analysis: any) => ({analysis}))); return handler; diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/base_def.ts b/packages/compiler-cli/src/ngtsc/annotations/src/base_def.ts index 47c64bdb61..68941ae221 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/base_def.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/base_def.ts @@ -11,7 +11,7 @@ import * as ts from 'typescript'; import {PartialEvaluator} from '../../partial_evaluator'; import {ClassMember, Decorator, ReflectionHost} from '../../reflection'; -import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform'; +import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../transform'; import {isAngularCore} from './util'; function containsNgTopLevelDecorator(decorators: Decorator[] | null): boolean { @@ -28,8 +28,10 @@ export class BaseDefDecoratorHandler implements DecoratorHandler { constructor(private reflector: ReflectionHost, private evaluator: PartialEvaluator) {} - detect(node: ts.ClassDeclaration, decorators: Decorator[]|null): R3BaseRefDecoratorDetection - |undefined { + readonly precedence = HandlerPrecedence.WEAK; + + detect(node: ts.ClassDeclaration, decorators: Decorator[]|null): + DetectResult|undefined { if (containsNgTopLevelDecorator(decorators)) { // If the class is already decorated by @Component or @Directive let that // DecoratorHandler handle this. BaseDef is unnecessary. @@ -56,7 +58,14 @@ export class BaseDefDecoratorHandler implements } }); - return result; + if (result !== undefined) { + return { + metadata: result, + trigger: null, + }; + } else { + return undefined; + } } analyze(node: ts.ClassDeclaration, metadata: R3BaseRefDecoratorDetection): diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index d78247cda9..019c87a685 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -12,10 +12,10 @@ import * as ts from 'typescript'; import {CycleAnalyzer} from '../../cycles'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; -import {ModuleResolver, Reference, ResolvedReference} from '../../imports'; +import {ModuleResolver, ResolvedReference} from '../../imports'; import {EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {Decorator, ReflectionHost, filterToMembersWithDecorator, reflectObjectLiteral} from '../../reflection'; -import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform'; +import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../transform'; import {TypeCheckContext} from '../../typecheck'; import {tsSourceMapBug29300Fixed} from '../../util/src/ts_source_map_bug_29300'; @@ -49,13 +49,22 @@ export class ComponentDecoratorHandler implements private literalCache = new Map(); private elementSchemaRegistry = new DomElementSchemaRegistry(); + readonly precedence = HandlerPrecedence.PRIMARY; - detect(node: ts.Declaration, decorators: Decorator[]|null): Decorator|undefined { + detect(node: ts.Declaration, decorators: Decorator[]|null): DetectResult|undefined { if (!decorators) { return undefined; } - return decorators.find( + const decorator = decorators.find( decorator => decorator.name === 'Component' && (this.isCore || isAngularCore(decorator))); + if (decorator !== undefined) { + return { + trigger: decorator.node, + metadata: decorator, + }; + } else { + return undefined; + } } preanalyze(node: ts.ClassDeclaration, decorator: Decorator): Promise|undefined { diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index 61f2a72e2d..6a7ddfc0c2 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -13,7 +13,7 @@ import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {Reference, ResolvedReference} from '../../imports'; import {EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {ClassMember, ClassMemberKind, Decorator, ReflectionHost, filterToMembersWithDecorator, reflectObjectLiteral} from '../../reflection'; -import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform'; +import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../transform'; import {generateSetClassMetadataCall} from './metadata'; import {SelectorScopeRegistry} from './selector_scope'; @@ -31,12 +31,22 @@ export class DirectiveDecoratorHandler implements private reflector: ReflectionHost, private evaluator: PartialEvaluator, private scopeRegistry: SelectorScopeRegistry, private isCore: boolean) {} - detect(node: ts.Declaration, decorators: Decorator[]|null): Decorator|undefined { + readonly precedence = HandlerPrecedence.PRIMARY; + + detect(node: ts.Declaration, decorators: Decorator[]|null): DetectResult|undefined { if (!decorators) { return undefined; } - return decorators.find( + const decorator = decorators.find( decorator => decorator.name === 'Directive' && (this.isCore || isAngularCore(decorator))); + if (decorator !== undefined) { + return { + trigger: decorator.node, + metadata: decorator, + }; + } else { + return undefined; + } } analyze(node: ts.ClassDeclaration, decorator: Decorator): AnalysisOutput { diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts index c3434fadce..6bd05da041 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts @@ -11,7 +11,7 @@ import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; -import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform'; +import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../transform'; import {generateSetClassMetadataCall} from './metadata'; import {getConstructorDependencies, getValidConstructorDependencies, isAngularCore, validateConstructorDependencies} from './util'; @@ -30,12 +30,22 @@ export class InjectableDecoratorHandler implements private reflector: ReflectionHost, private isCore: boolean, private strictCtorDeps: boolean) { } - detect(node: ts.Declaration, decorators: Decorator[]|null): Decorator|undefined { + readonly precedence = HandlerPrecedence.SHARED; + + detect(node: ts.Declaration, decorators: Decorator[]|null): DetectResult|undefined { if (!decorators) { return undefined; } - return decorators.find( + const decorator = decorators.find( decorator => decorator.name === 'Injectable' && (this.isCore || isAngularCore(decorator))); + if (decorator !== undefined) { + return { + trigger: decorator.node, + metadata: decorator, + }; + } else { + return undefined; + } } analyze(node: ts.ClassDeclaration, decorator: Decorator): AnalysisOutput { diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index 99c2863a76..9b5cc2d361 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -14,7 +14,7 @@ import {Reference, ResolvedReference} from '../../imports'; import {PartialEvaluator, ResolvedValue} from '../../partial_evaluator'; import {Decorator, ReflectionHost, reflectObjectLiteral, typeNodeToValueExpr} from '../../reflection'; import {NgModuleRouteAnalyzer} from '../../routing'; -import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform'; +import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../transform'; import {generateSetClassMetadataCall} from './metadata'; import {ReferencesRegistry} from './references_registry'; @@ -39,12 +39,22 @@ export class NgModuleDecoratorHandler implements DecoratorHandler|undefined { if (!decorators) { return undefined; } - return decorators.find( + const decorator = decorators.find( decorator => decorator.name === 'NgModule' && (this.isCore || isAngularCore(decorator))); + if (decorator !== undefined) { + return { + trigger: decorator.node, + metadata: decorator, + }; + } else { + return undefined; + } } analyze(node: ts.ClassDeclaration, decorator: Decorator): AnalysisOutput { diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts b/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts index 9ebee27f00..643a0da6e4 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/pipe.ts @@ -12,7 +12,7 @@ import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {PartialEvaluator} from '../../partial_evaluator'; import {Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; -import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform'; +import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../transform'; import {generateSetClassMetadataCall} from './metadata'; import {SelectorScopeRegistry} from './selector_scope'; @@ -28,12 +28,22 @@ export class PipeDecoratorHandler implements DecoratorHandler|undefined { if (!decorators) { return undefined; } - return decorators.find( + const decorator = decorators.find( decorator => decorator.name === 'Pipe' && (this.isCore || isAngularCore(decorator))); + if (decorator !== undefined) { + return { + trigger: decorator.node, + metadata: decorator, + }; + } else { + return undefined; + } } analyze(clazz: ts.ClassDeclaration, decorator: Decorator): AnalysisOutput { diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts index 42256a0670..08abb0b890 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts @@ -59,7 +59,7 @@ describe('ComponentDecoratorHandler', () => { return fail('Failed to recognize @Component'); } try { - handler.analyze(TestCmp, detected); + handler.analyze(TestCmp, detected.metadata); return fail('Analysis should have failed'); } catch (err) { if (!(err instanceof FatalDiagnosticError)) { @@ -68,7 +68,7 @@ describe('ComponentDecoratorHandler', () => { const diag = err.toDiagnostic(); expect(diag.code).toEqual(ivyCode(ErrorCode.DECORATOR_ARG_NOT_LITERAL)); expect(diag.file.fileName.endsWith('entry.ts')).toBe(true); - expect(diag.start).toBe(detected.args ![0].getStart()); + expect(diag.start).toBe(detected.metadata.args ![0].getStart()); } }); }); diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts index 5b3338859d..0ae49e9cd6 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts @@ -13,6 +13,11 @@ export enum ErrorCode { DECORATOR_ON_ANONYMOUS_CLASS = 1004, DECORATOR_UNEXPECTED = 1005, + /** + * This error code indicates that there are incompatible decorators on a type. + */ + DECORATOR_COLLISION = 1006, + VALUE_HAS_WRONG_TYPE = 1010, VALUE_NOT_LITERAL = 1011, diff --git a/packages/compiler-cli/src/ngtsc/transform/src/api.ts b/packages/compiler-cli/src/ngtsc/transform/src/api.ts index 31f146c202..ab86acdb18 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/api.ts @@ -12,6 +12,29 @@ import * as ts from 'typescript'; import {Decorator} from '../../reflection'; import {TypeCheckContext} from '../../typecheck'; +export enum HandlerPrecedence { + /** + * Handler with PRIMARY precedence cannot overlap - there can only be one on a given class. + * + * If more than one PRIMARY handler matches a class, an error is produced. + */ + PRIMARY, + + /** + * Handlers with SHARED precedence can match any class, possibly in addition to a single PRIMARY + * handler. + * + * It is not an error for a class to have any number of SHARED handlers. + */ + SHARED, + + /** + * Handlers with WEAK precedence that match a class are ignored if any handlers with stronger + * precedence match a class. + */ + WEAK, +} + /** * Provides the interface between a decorator compiler from @angular/compiler and the Typescript @@ -22,11 +45,19 @@ import {TypeCheckContext} from '../../typecheck'; * and Typescript source, invoking the decorator compiler, and returning the result. */ export interface DecoratorHandler { + /** + * The precedence of a handler controls how it interacts with other handlers that match the same + * class. + * + * See the descriptions on `HandlerPrecedence` for an explanation of the behaviors involved. + */ + readonly precedence: HandlerPrecedence; + /** * Scan a set of reflected decorators and determine if this handler is responsible for compilation * of one of them. */ - detect(node: ts.Declaration, decorators: Decorator[]|null): M|undefined; + detect(node: ts.Declaration, decorators: Decorator[]|null): DetectResult|undefined; /** @@ -63,6 +94,11 @@ export interface DecoratorHandler { |CompileResult[]; } +export interface DetectResult { + trigger: ts.Node|null; + metadata: M; +} + /** * The output of an analysis operation, consisting of possibly an arbitrary analysis object (used as * the input to code generation) and potentially diagnostics if there were errors uncovered during diff --git a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts index aff334317c..7ce20e9b12 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts @@ -9,24 +9,32 @@ import {ConstantPool} from '@angular/compiler'; import * as ts from 'typescript'; -import {FatalDiagnosticError} from '../../diagnostics'; +import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {ImportRewriter} from '../../imports'; -import {Decorator, ReflectionHost, reflectNameOfDeclaration} from '../../reflection'; +import {ReflectionHost, reflectNameOfDeclaration} from '../../reflection'; import {TypeCheckContext} from '../../typecheck'; +import {getSourceFile} from '../../util/src/typescript'; -import {AnalysisOutput, CompileResult, DecoratorHandler} from './api'; +import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from './api'; import {DtsFileTransformer} from './declaration'; - +const EMPTY_ARRAY: any = []; /** * Record of an adapter which decided to emit a static field, and the analysis it performed to * prepare for that operation. */ -interface EmitFieldOperation { - adapter: DecoratorHandler; - analysis: AnalysisOutput; - metadata: M; +interface MatchedHandler { + handler: DecoratorHandler; + analyzed: AnalysisOutput|null; + detected: DetectResult; +} + +interface IvyClass { + matchedHandlers: MatchedHandler[]; + + hasWeakHandlers: boolean; + hasPrimaryHandler: boolean; } /** @@ -40,8 +48,7 @@ export class IvyCompilation { * Tracks classes which have been analyzed and found to have an Ivy decorator, and the * information recorded about them for later compilation. */ - private analysis = new Map>(); - private typeCheckMap = new Map>(); + private ivyClasses = new Map(); /** * Tracks factory information which needs to be generated. @@ -73,6 +80,83 @@ export class IvyCompilation { analyzeAsync(sf: ts.SourceFile): Promise|undefined { return this.analyze(sf, true); } + private detectHandlersForClass(node: ts.Declaration): IvyClass|null { + // The first step is to reflect the decorators. + const classDecorators = this.reflector.getDecoratorsOfDeclaration(node); + let ivyClass: IvyClass|null = null; + + // Look through the DecoratorHandlers to see if any are relevant. + for (const handler of this.handlers) { + // An adapter is relevant if it matches one of the decorators on the class. + const detected = handler.detect(node, classDecorators); + if (detected === undefined) { + // This handler didn't match. + continue; + } + + const isPrimaryHandler = handler.precedence === HandlerPrecedence.PRIMARY; + const isWeakHandler = handler.precedence === HandlerPrecedence.WEAK; + const match = { + handler, + analyzed: null, detected, + }; + + if (ivyClass === null) { + // This is the first handler to match this class. This path is a fast path through which + // most classes will flow. + ivyClass = { + matchedHandlers: [match], + hasPrimaryHandler: isPrimaryHandler, + hasWeakHandlers: isWeakHandler, + }; + this.ivyClasses.set(node, ivyClass); + } else { + // This is at least the second handler to match this class. This is a slower path that some + // classes will go through, which validates that the set of decorators applied to the class + // is valid. + + // Validate according to rules as follows: + // + // * WEAK handlers are removed if a non-WEAK handler matches. + // * Only one PRIMARY handler can match at a time. Any other PRIMARY handler matching a + // class with an existing PRIMARY handler is an error. + + if (!isWeakHandler && ivyClass.hasWeakHandlers) { + // The current handler is not a WEAK handler, but the class has other WEAK handlers. + // Remove them. + ivyClass.matchedHandlers = ivyClass.matchedHandlers.filter( + field => field.handler.precedence !== HandlerPrecedence.WEAK); + ivyClass.hasWeakHandlers = false; + } else if (isWeakHandler && !ivyClass.hasWeakHandlers) { + // The current handler is a WEAK handler, but the class has non-WEAK handlers already. + // Drop the current one. + continue; + } + + if (isPrimaryHandler && ivyClass.hasPrimaryHandler) { + // The class already has a PRIMARY handler, and another one just matched. + this._diagnostics.push({ + category: ts.DiagnosticCategory.Error, + code: Number('-99' + ErrorCode.DECORATOR_COLLISION), + file: getSourceFile(node), + start: node.getStart(undefined, false), + length: node.getWidth(), + messageText: 'Two incompatible decorators on class', + }); + this.ivyClasses.delete(node); + return null; + } + + // Otherwise, it's safe to accept the multiple decorators here. Update some of the metadata + // regarding this class. + ivyClass.matchedHandlers.push(match); + ivyClass.hasPrimaryHandler = ivyClass.hasPrimaryHandler || isPrimaryHandler; + } + } + + return ivyClass; + } + /** * Analyze a source file and produce diagnostics for it (if any). */ @@ -82,48 +166,31 @@ export class IvyCompilation { const promises: Promise[] = []; const analyzeClass = (node: ts.Declaration): void => { - // The first step is to reflect the decorators. - const classDecorators = this.reflector.getDecoratorsOfDeclaration(node); + const ivyClass = this.detectHandlersForClass(node); - // Look through the DecoratorHandlers to see if any are relevant. - this.handlers.forEach(adapter => { + // If the class has no Ivy behavior (or had errors), skip it. + if (ivyClass === null) { + return; + } - // An adapter is relevant if it matches one of the decorators on the class. - const metadata = adapter.detect(node, classDecorators); - if (metadata === undefined) { - return; - } - - const completeAnalysis = () => { - // Check for multiple decorators on the same node. Technically speaking this - // could be supported, but right now it's an error. - if (this.analysis.has(node)) { - throw new Error('TODO.Diagnostic: Class has multiple Angular decorators.'); - } - - // Run analysis on the metadata. This will produce either diagnostics, an - // analysis result, or both. + // Loop through each matched handler that needs to be analyzed and analyze it, either + // synchronously or asynchronously. + for (const match of ivyClass.matchedHandlers) { + // The analyze() function will run the analysis phase of the handler. + const analyze = () => { try { - const analysis = adapter.analyze(node, metadata); - if (analysis.analysis !== undefined) { - this.analysis.set(node, { - adapter, - analysis: analysis.analysis, - metadata: metadata, - }); - if (!!analysis.typeCheck) { - this.typeCheckMap.set(node, adapter); - } + match.analyzed = match.handler.analyze(node, match.detected.metadata); + + if (match.analyzed.diagnostics !== undefined) { + this._diagnostics.push(...match.analyzed.diagnostics); } - if (analysis.diagnostics !== undefined) { - this._diagnostics.push(...analysis.diagnostics); - } - - if (analysis.factorySymbolName !== undefined && this.sourceToFactorySymbols !== null && + if (match.analyzed.factorySymbolName !== undefined && + this.sourceToFactorySymbols !== null && this.sourceToFactorySymbols.has(sf.fileName)) { - this.sourceToFactorySymbols.get(sf.fileName) !.add(analysis.factorySymbolName); + this.sourceToFactorySymbols.get(sf.fileName) !.add(match.analyzed.factorySymbolName); } + } catch (err) { if (err instanceof FatalDiagnosticError) { this._diagnostics.push(err.toDiagnostic()); @@ -133,17 +200,25 @@ export class IvyCompilation { } }; - if (preanalyze && adapter.preanalyze !== undefined) { - const preanalysis = adapter.preanalyze(node, metadata); + // If preanalysis was requested and a preanalysis step exists, then run preanalysis. + // Otherwise, skip directly to analysis. + if (preanalyze && match.handler.preanalyze !== undefined) { + // Preanalysis might return a Promise, indicating an async operation was necessary. Or it + // might return undefined, indicating no async step was needed and analysis can proceed + // immediately. + const preanalysis = match.handler.preanalyze(node, match.detected.metadata); if (preanalysis !== undefined) { - promises.push(preanalysis.then(() => completeAnalysis())); + // Await the results of preanalysis before running analysis. + promises.push(preanalysis.then(analyze)); } else { - completeAnalysis(); + // No async preanalysis needed, skip directly to analysis. + analyze(); } } else { - completeAnalysis(); + // Not in preanalysis mode or not needed for this handler, skip directly to analysis. + analyze(); } - }); + } }; const visit = (node: ts.Node): void => { @@ -164,17 +239,23 @@ export class IvyCompilation { } resolve(): void { - this.analysis.forEach((op, decl) => { - if (op.adapter.resolve !== undefined) { - op.adapter.resolve(decl, op.analysis); + this.ivyClasses.forEach((ivyClass, node) => { + for (const match of ivyClass.matchedHandlers) { + if (match.handler.resolve !== undefined && match.analyzed !== null && + match.analyzed.analysis !== undefined) { + match.handler.resolve(node, match.analyzed.analysis); + } } }); } typeCheck(context: TypeCheckContext): void { - this.typeCheckMap.forEach((handler, node) => { - if (handler.typeCheck !== undefined) { - handler.typeCheck(context, node, this.analysis.get(node) !.analysis); + this.ivyClasses.forEach((ivyClass, node) => { + for (const match of ivyClass.matchedHandlers) { + if (match.handler.typeCheck !== undefined && match.analyzed !== null && + match.analyzed.analysis !== undefined) { + match.handler.typeCheck(context, node, match.analyzed.analysis); + } } }); } @@ -186,37 +267,59 @@ export class IvyCompilation { compileIvyFieldFor(node: ts.Declaration, constantPool: ConstantPool): CompileResult[]|undefined { // Look to see whether the original node was analyzed. If not, there's nothing to do. const original = ts.getOriginalNode(node) as ts.Declaration; - if (!this.analysis.has(original)) { + if (!this.ivyClasses.has(original)) { return undefined; } - const op = this.analysis.get(original) !; - // Run the actual compilation, which generates an Expression for the Ivy field. - let res: CompileResult|CompileResult[] = op.adapter.compile(node, op.analysis, constantPool); - if (!Array.isArray(res)) { - res = [res]; + const ivyClass = this.ivyClasses.get(original) !; + + let res: CompileResult[] = []; + + for (const match of ivyClass.matchedHandlers) { + if (match.analyzed === null || match.analyzed.analysis === undefined) { + continue; + } + + const compileMatchRes = match.handler.compile(node, match.analyzed.analysis, constantPool); + if (!Array.isArray(compileMatchRes)) { + res.push(compileMatchRes); + } else { + res.push(...compileMatchRes); + } } - // Look up the .d.ts transformer for the input file and record that a field was generated, - // which will allow the .d.ts to be transformed later. + // Look up the .d.ts transformer for the input file and record that at least one field was + // generated, which will allow the .d.ts to be transformed later. const fileName = original.getSourceFile().fileName; const dtsTransformer = this.getDtsTransformer(fileName); dtsTransformer.recordStaticField(reflectNameOfDeclaration(node) !, res); - // Return the instruction to the transformer so the field will be added. - return res; + // Return the instruction to the transformer so the fields will be added. + return res.length > 0 ? res : undefined; } /** * Lookup the `ts.Decorator` which triggered transformation of a particular class declaration. */ - ivyDecoratorFor(node: ts.Declaration): Decorator|undefined { + ivyDecoratorsFor(node: ts.Declaration): ts.Decorator[] { const original = ts.getOriginalNode(node) as ts.Declaration; - if (!this.analysis.has(original)) { - return undefined; + + if (!this.ivyClasses.has(original)) { + return EMPTY_ARRAY; + } + const ivyClass = this.ivyClasses.get(original) !; + const decorators: ts.Decorator[] = []; + + for (const match of ivyClass.matchedHandlers) { + if (match.analyzed === null || match.analyzed.analysis === undefined) { + continue; + } + if (match.detected.trigger !== null && ts.isDecorator(match.detected.trigger)) { + decorators.push(match.detected.trigger); + } } - return this.analysis.get(original) !.metadata; + return decorators; } /** diff --git a/packages/compiler-cli/src/ngtsc/transform/src/transform.ts b/packages/compiler-cli/src/ngtsc/transform/src/transform.ts index c648b29198..5223ee376a 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/transform.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/transform.ts @@ -68,8 +68,7 @@ class IvyVisitor extends Visitor { node = ts.updateClassDeclaration( node, // Remove the decorator which triggered this compilation, leaving the others alone. - maybeFilterDecorator( - node.decorators, this.compilation.ivyDecoratorFor(node) !.node as ts.Decorator), + maybeFilterDecorator(node.decorators, this.compilation.ivyDecoratorsFor(node)), node.modifiers, node.name, node.typeParameters, node.heritageClauses || [], // Map over the class members and remove any Angular decorators from them. members.map(member => this._stripAngularDecorators(member))); @@ -208,11 +207,12 @@ function transformIvySourceFile( function maybeFilterDecorator( decorators: ts.NodeArray| undefined, - toRemove: ts.Decorator): ts.NodeArray|undefined { + toRemove: ts.Decorator[]): ts.NodeArray|undefined { if (decorators === undefined) { return undefined; } - const filtered = decorators.filter(dec => ts.getOriginalNode(dec) !== toRemove); + const filtered = decorators.filter( + dec => toRemove.find(decToRemove => ts.getOriginalNode(dec) === decToRemove) === undefined); if (filtered.length === 0) { return undefined; } diff --git a/packages/compiler-cli/src/ngtsc/util/src/typescript.ts b/packages/compiler-cli/src/ngtsc/util/src/typescript.ts index e0816ef3b1..3ef6e50cab 100644 --- a/packages/compiler-cli/src/ngtsc/util/src/typescript.ts +++ b/packages/compiler-cli/src/ngtsc/util/src/typescript.ts @@ -26,3 +26,11 @@ export function isFromDtsFile(node: ts.Node): boolean { } return sf !== undefined && D_TS.test(sf.fileName); } + +export function getSourceFile(node: ts.Node): ts.SourceFile { + // In certain transformation contexts, `ts.Node.getSourceFile()` can actually return `undefined`, + // despite the type signature not allowing it. In that event, get the `ts.SourceFile` via the + // original node instead (which works). + const directSf = node.getSourceFile() as ts.SourceFile | undefined; + return directSf !== undefined ? directSf : ts.getOriginalNode(node).getSourceFile(); +} diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index c68ddf492d..41f8515b79 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -624,6 +624,95 @@ describe('ngtsc behavioral tests', () => { 'i0.ɵNgModuleDefWithMeta'); }); + describe('multiple decorators on classes', () => { + it('should compile @Injectable on Components, Directives, Pipes, and Modules', () => { + env.tsconfig(); + env.write('test.ts', ` + import {Component, Directive, Injectable, NgModule, Pipe} from '@angular/core'; + + @Component({selector: 'test', template: 'test'}) + @Injectable() + export class TestCmp {} + + @Directive({selector: 'test'}) + @Injectable() + export class TestDir {} + + @Pipe({name: 'test'}) + @Injectable() + export class TestPipe {} + + @NgModule({declarations: [TestCmp, TestDir, TestPipe]}) + @Injectable() + export class TestNgModule {} + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + const dtsContents = env.getContents('test.d.ts'); + + // Validate that each class has the primary definition. + expect(jsContents).toContain('TestCmp.ngComponentDef ='); + expect(jsContents).toContain('TestDir.ngDirectiveDef ='); + expect(jsContents).toContain('TestPipe.ngPipeDef ='); + expect(jsContents).toContain('TestNgModule.ngModuleDef ='); + + // Validate that each class also has an injectable definition. + expect(jsContents).toContain('TestCmp.ngInjectableDef ='); + expect(jsContents).toContain('TestDir.ngInjectableDef ='); + expect(jsContents).toContain('TestPipe.ngInjectableDef ='); + expect(jsContents).toContain('TestNgModule.ngInjectableDef ='); + + // Validate that each class's .d.ts declaration has the primary definition. + expect(dtsContents).toContain('ComponentDefWithMeta { + env.tsconfig(); + env.write('test.ts', ` + import {Component, Directive} from '@angular/core'; + + @Component({selector: 'test', template: 'test'}) + @Directive({selector: 'test'}) + class ShouldNotCompile {} + `); + + const errors = env.driveDiagnostics(); + expect(errors.length).toBe(1); + expect(errors[0].messageText).toContain('Two incompatible decorators on class'); + }); + + + + it('should leave decorators present on jit: true directives', () => { + env.tsconfig(); + env.write('test.ts', ` + import {Directive, Inject} from '@angular/core'; + + @Directive({ + selector: 'test', + jit: true, + }) + export class Test { + constructor(@Inject('foo') foo: string) {} + } + `); + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('Directive({'); + expect(jsContents).toContain('__param(0, Inject'); + }); + }); + describe('compiling invalid @Injectables', () => { describe('with strictInjectionParameters = true', () => { it('should give a compile-time error if an invalid @Injectable is used with no arguments',