From e7da4040d64bc9055c183cc3ecbbb24eee82596f Mon Sep 17 00:00:00 2001 From: crisbeto Date: Thu, 20 Aug 2020 23:21:21 +0200 Subject: [PATCH] fix(compiler-cli): adding references to const enums in runtime code (#38542) We had a couple of places where we were assuming that if a particular symbol has a value, then it will exist at runtime. This is true in most cases, but it breaks down for `const` enums. Fixes #38513. PR Close #38542 --- .../src/ngtsc/reflection/src/type_to_value.ts | 5 +- .../downlevel_decorators_transform.ts | 15 +++-- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 47 ++++++++++++++ .../downlevel_decorators_transform_spec.ts | 64 +++++++++++++------ 4 files changed, 105 insertions(+), 26 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/reflection/src/type_to_value.ts b/packages/compiler-cli/src/ngtsc/reflection/src/type_to_value.ts index acef675409..cf17312b60 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/src/type_to_value.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/src/type_to_value.ts @@ -35,8 +35,9 @@ export function typeToValue( const {local, decl} = symbols; // It's only valid to convert a type reference to a value reference if the type actually - // has a value declaration associated with it. - if (decl.valueDeclaration === undefined) { + // has a value declaration associated with it. Note that const enums are an exception, + // because while they do have a value declaration, they don't exist at runtime. + if (decl.valueDeclaration === undefined || decl.flags & ts.SymbolFlags.ConstEnum) { return noValueDeclaration(typeNode, decl.declarations[0]); } diff --git a/packages/compiler-cli/src/transformers/downlevel_decorators_transform.ts b/packages/compiler-cli/src/transformers/downlevel_decorators_transform.ts index aa30ce4da1..87bbd085fb 100644 --- a/packages/compiler-cli/src/transformers/downlevel_decorators_transform.ts +++ b/packages/compiler-cli/src/transformers/downlevel_decorators_transform.ts @@ -298,15 +298,20 @@ function typeReferenceToExpression( } /** - * Returns true if the given symbol refers to a value (as distinct from a type). + * Checks whether a given symbol refers to a value that exists at runtime (as distinct from a type). * * Expands aliases, which is important for the case where * import * as x from 'some-module'; * and x is now a value (the module object). */ -function symbolIsValue(tc: ts.TypeChecker, sym: ts.Symbol): boolean { - if (sym.flags & ts.SymbolFlags.Alias) sym = tc.getAliasedSymbol(sym); - return (sym.flags & ts.SymbolFlags.Value) !== 0; +function symbolIsRuntimeValue(typeChecker: ts.TypeChecker, symbol: ts.Symbol): boolean { + if (symbol.flags & ts.SymbolFlags.Alias) { + symbol = typeChecker.getAliasedSymbol(symbol); + } + + // Note that const enums are a special case, because + // while they have a value, they don't exist at runtime. + return (symbol.flags & ts.SymbolFlags.Value & ts.SymbolFlags.ConstEnumExcludes) !== 0; } /** ParameterDecorationInfo describes the information for a single constructor parameter. */ @@ -351,7 +356,7 @@ export function getDownlevelDecoratorsTransform( const symbol = typeChecker.getSymbolAtLocation(name); // Check if the entity name references a symbol that is an actual value. If it is not, it // cannot be referenced by an expression, so return undefined. - if (!symbol || !symbolIsValue(typeChecker, symbol) || !symbol.declarations || + if (!symbol || !symbolIsRuntimeValue(typeChecker, symbol) || !symbol.declarations || symbol.declarations.length === 0) { return undefined; } diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 58575c1a4e..596de1ef14 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -4369,6 +4369,53 @@ runInEachFileSystem(os => { expect(jsContents).toMatch(setClassMetadataRegExp('type: undefined')); }); + it('should use `undefined` in setClassMetadata for const enums', () => { + env.write(`keycodes.ts`, ` + export const enum KeyCodes {A, B}; + `); + env.write(`test.ts`, ` + import {Component, Inject} from '@angular/core'; + import {KeyCodes} from './keycodes'; + + @Component({ + selector: 'some-comp', + template: '...', + }) + export class SomeComp { + constructor(@Inject('arg-token') arg: KeyCodes) {} + } + `); + + env.driveMain(); + const jsContents = trim(env.getContents('test.js')); + expect(jsContents).not.toContain(`import { KeyCodes } from './keycodes';`); + // Note: `type: undefined` below, since KeyCodes can't be represented as a value + expect(jsContents).toMatch(setClassMetadataRegExp('type: undefined')); + }); + + it('should preserve the types of non-const enums in setClassMetadata', () => { + env.write(`keycodes.ts`, ` + export enum KeyCodes {A, B}; + `); + env.write(`test.ts`, ` + import {Component, Inject} from '@angular/core'; + import {KeyCodes} from './keycodes'; + + @Component({ + selector: 'some-comp', + template: '...', + }) + export class SomeComp { + constructor(@Inject('arg-token') arg: KeyCodes) {} + } + `); + + env.driveMain(); + const jsContents = trim(env.getContents('test.js')); + expect(jsContents).toContain(`import { KeyCodes } from './keycodes';`); + expect(jsContents).toMatch(setClassMetadataRegExp('type: i1.KeyCodes')); + }); + it('should use `undefined` in setClassMetadata if types originate from type-only imports', () => { env.write(`types.ts`, ` diff --git a/packages/compiler-cli/test/transformers/downlevel_decorators_transform_spec.ts b/packages/compiler-cli/test/transformers/downlevel_decorators_transform_spec.ts index d1200b92a0..51e316cb6b 100644 --- a/packages/compiler-cli/test/transformers/downlevel_decorators_transform_spec.ts +++ b/packages/compiler-cli/test/transformers/downlevel_decorators_transform_spec.ts @@ -192,7 +192,7 @@ describe('downlevel decorator transform', () => { it('should downlevel Angular-decorated class member', () => { const {output} = transform(` import {Input} from '@angular/core'; - + export class MyDir { @Input() disabled: boolean = false; } @@ -231,7 +231,7 @@ describe('downlevel decorator transform', () => { const {output} = transform(` import {Input} from '@angular/core'; import {MyOtherClass} from './other-file'; - + export class MyDir { @Input() trigger: HTMLElement; @Input() fromOtherFile: MyOtherClass; @@ -255,7 +255,7 @@ describe('downlevel decorator transform', () => { ` import {Directive} from '@angular/core'; import {MyOtherClass} from './other-file'; - + @Directive() export class MyDir { constructor(other: MyOtherClass) {} @@ -281,7 +281,7 @@ describe('downlevel decorator transform', () => { ` import {Directive} from '@angular/core'; import {MyOtherClass} from './other-file'; - + @Directive() export class MyDir { constructor(other: MyOtherClass) {} @@ -307,7 +307,7 @@ describe('downlevel decorator transform', () => { const {output} = transform(` import {Directive} from '@angular/core'; import * as externalFile from './other-file'; - + @Directive() export class MyDir { constructor(other: externalFile.MyOtherClass) {} @@ -329,11 +329,11 @@ describe('downlevel decorator transform', () => { it('should properly serialize constructor parameter with local qualified name type', () => { const {output} = transform(` import {Directive} from '@angular/core'; - + namespace other { export class OtherClass {} }; - + @Directive() export class MyDir { constructor(other: other.OtherClass) {} @@ -355,7 +355,7 @@ describe('downlevel decorator transform', () => { it('should properly downlevel constructor parameter decorators', () => { const {output} = transform(` import {Inject, Directive, DOCUMENT} from '@angular/core'; - + @Directive() export class MyDir { constructor(@Inject(DOCUMENT) document: Document) {} @@ -376,7 +376,7 @@ describe('downlevel decorator transform', () => { it('should properly downlevel constructor parameters with union type', () => { const {output} = transform(` import {Optional, Directive, NgZone} from '@angular/core'; - + @Directive() export class MyDir { constructor(@Optional() ngZone: NgZone|null) {} @@ -546,18 +546,20 @@ describe('downlevel decorator transform', () => { export default interface { hello: false; } + export const enum KeyCodes {A, B} `); const {output} = transform(` import {Directive, Inject} from '@angular/core'; import * as angular from './external'; - import {IOverlay} from './external'; + import {IOverlay, KeyCodes} from './external'; import TypeFromDefaultImport from './external'; @Directive() export class MyDir { constructor(@Inject('$state') param: angular.IState, @Inject('$overlay') other: IOverlay, - @Inject('$default') default: TypeFromDefaultImport) {} + @Inject('$default') default: TypeFromDefaultImport, + @Inject('$keyCodes') keyCodes: KeyCodes) {} } `); @@ -570,7 +572,8 @@ describe('downlevel decorator transform', () => { MyDir.ctorParameters = () => [ { type: undefined, decorators: [{ type: core_1.Inject, args: ['$state',] }] }, { type: undefined, decorators: [{ type: core_1.Inject, args: ['$overlay',] }] }, - { type: undefined, decorators: [{ type: core_1.Inject, args: ['$default',] }] } + { type: undefined, decorators: [{ type: core_1.Inject, args: ['$default',] }] }, + { type: undefined, decorators: [{ type: core_1.Inject, args: ['$keyCodes',] }] } ]; `); }); @@ -593,7 +596,7 @@ describe('downlevel decorator transform', () => { const {output} = transform( ` import {Directive} from '@angular/core'; - + export class MyInjectedClass {} @Directive() @@ -609,13 +612,36 @@ describe('downlevel decorator transform', () => { expect(output).not.toContain('tslib'); }); + it('should capture a non-const enum used as a constructor type', () => { + const {output} = transform(` + import {Component} from '@angular/core'; + + export enum Values {A, B}; + + @Component({template: 'hello'}) + export class MyComp { + constructor(v: Values) {} + } + `); + + expect(diagnostics.length).toBe(0); + expect(output).toContain(dedent` + MyComp.decorators = [ + { type: core_1.Component, args: [{ template: 'hello' },] } + ]; + MyComp.ctorParameters = () => [ + { type: Values } + ];`); + expect(output).not.toContain('tslib'); + }); + describe('class decorators skipped', () => { beforeEach(() => skipClassDecorators = true); it('should not downlevel Angular class decorators', () => { const {output} = transform(` import {Injectable} from '@angular/core'; - + @Injectable() export class MyService {} `); @@ -632,10 +658,10 @@ describe('downlevel decorator transform', () => { it('should downlevel constructor parameters', () => { const {output} = transform(` import {Injectable} from '@angular/core'; - + @Injectable() export class InjectClass {} - + @Injectable() export class MyService { constructor(dep: InjectClass) {} @@ -658,10 +684,10 @@ describe('downlevel decorator transform', () => { it('should downlevel constructor parameter decorators', () => { const {output} = transform(` import {Injectable, Inject} from '@angular/core'; - + @Injectable() export class InjectClass {} - + @Injectable() export class MyService { constructor(@Inject('test') dep: InjectClass) {} @@ -684,7 +710,7 @@ describe('downlevel decorator transform', () => { it('should downlevel class member Angular decorators', () => { const {output} = transform(` import {Injectable, Input} from '@angular/core'; - + export class MyService { @Input() disabled: boolean; }