From aa034b68253ef7d758d944d7ef6aca02770b81fa Mon Sep 17 00:00:00 2001 From: JoostK Date: Sat, 28 Oct 2023 21:59:19 +0200 Subject: [PATCH 1/2] fix(compiler-cli): properly emit literal types in input coercion function arguments This commit fixes an issue where using literal types in the arguments of an input coercion function could result in emitting invalid output, due to an assumption that TypeScript makes when emitting literal types. Specifically, it takes the literal's text from its containing source file, but this breaks when the literal type node has been transplanted into a different source file. This issue has surfaced in the type-check code generator and is already being addressed there, so this commit moves the relevant `TypeEmitter` class from the `typecheck` module to the `translator` module, such that it can be reused for emitting types in the type translator. Fixes #51672 --- .../src/ngtsc/translator/index.ts | 1 + .../src/type_emitter.ts | 0 .../ngtsc/translator/src/type_translator.ts | 70 ++++--------------- .../typecheck/src/type_parameter_emitter.ts | 4 +- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 27 +++++++ 5 files changed, 44 insertions(+), 58 deletions(-) rename packages/compiler-cli/src/ngtsc/{typecheck => translator}/src/type_emitter.ts (100%) diff --git a/packages/compiler-cli/src/ngtsc/translator/index.ts b/packages/compiler-cli/src/ngtsc/translator/index.ts index 7fa09856f8f5..3995b25f86be 100644 --- a/packages/compiler-cli/src/ngtsc/translator/index.ts +++ b/packages/compiler-cli/src/ngtsc/translator/index.ts @@ -11,6 +11,7 @@ export {ImportGenerator, NamedImport} from './src/api/import_generator'; export {Context} from './src/context'; export {Import, ImportManager} from './src/import_manager'; export {ExpressionTranslatorVisitor, RecordWrappedNodeFn, TranslatorOptions} from './src/translator'; +export {canEmitType, TypeEmitter, TypeReferenceTranslator} from './src/type_emitter'; export {translateType} from './src/type_translator'; export {attachComments, createTemplateMiddle, createTemplateTail, TypeScriptAstFactory} from './src/typescript_ast_factory'; export {translateExpression, translateStatement} from './src/typescript_translator'; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_emitter.ts b/packages/compiler-cli/src/ngtsc/translator/src/type_emitter.ts similarity index 100% rename from packages/compiler-cli/src/ngtsc/typecheck/src/type_emitter.ts rename to packages/compiler-cli/src/ngtsc/translator/src/type_emitter.ts diff --git a/packages/compiler-cli/src/ngtsc/translator/src/type_translator.ts b/packages/compiler-cli/src/ngtsc/translator/src/type_translator.ts index 4a3a0d3f2d98..885fc57b23f9 100644 --- a/packages/compiler-cli/src/ngtsc/translator/src/type_translator.ts +++ b/packages/compiler-cli/src/ngtsc/translator/src/type_translator.ts @@ -14,6 +14,7 @@ import {ReflectionHost} from '../../reflection'; import {Context} from './context'; import {ImportManager} from './import_manager'; +import {TypeEmitter} from './type_emitter'; export function translateType( @@ -79,12 +80,13 @@ class TypeTranslatorVisitor implements o.ExpressionVisitor, o.TypeVisitor { return ts.factory.createTypeLiteralNode([indexSignature]); } - visitTransplantedType(ast: o.TransplantedType, context: any) { + visitTransplantedType(ast: o.TransplantedType, context: Context) { if (!ts.isTypeNode(ast.type)) { throw new Error(`A TransplantedType must wrap a TypeNode`); } - return this.translateTransplantedTypeNode(ast.type, context); + const emitter = new TypeEmitter(typeRef => this.translateTypeReference(typeRef, context)); + return emitter.emitType(ast.type); } visitReadVarExpr(ast: o.ReadVarExpr, context: Context): ts.TypeQueryNode { @@ -253,17 +255,13 @@ class TypeTranslatorVisitor implements o.ExpressionVisitor, o.TypeVisitor { return typeNode; } - /** - * Translates a type reference node so that all of its references - * are imported into the context file. - */ - private translateTransplantedTypeReferenceNode( - node: ts.TypeReferenceNode&{typeName: ts.Identifier}, context: any): ts.TypeReferenceNode { - const declaration = this.reflector.getDeclarationOfIdentifier(node.typeName); - + private translateTypeReference(type: ts.TypeReferenceNode, context: Context): ts.TypeReferenceNode + |null { + const target = ts.isIdentifier(type.typeName) ? type.typeName : type.typeName.right; + const declaration = this.reflector.getDeclarationOfIdentifier(target); if (declaration === null) { throw new Error( - `Unable to statically determine the declaration file of type node ${node.typeName.text}`); + `Unable to statically determine the declaration file of type node ${target.text}`); } const emittedType = this.refEmitter.emit( @@ -271,52 +269,14 @@ class TypeTranslatorVisitor implements o.ExpressionVisitor, o.TypeVisitor { ImportFlags.NoAliasing | ImportFlags.AllowTypeImports | ImportFlags.AllowRelativeDtsImports); - assertSuccessfulReferenceEmit(emittedType, node, 'type'); - - const result = emittedType.expression.visitExpression(this, context); + assertSuccessfulReferenceEmit(emittedType, target, 'type'); - if (!ts.isTypeReferenceNode(result)) { - throw new Error(`Expected TypeReferenceNode when referencing the type for ${ - node.typeName.text}, but received ${ts.SyntaxKind[result.kind]}`); - } + const typeNode = this.translateExpression(emittedType.expression, context); - // If the original node doesn't have any generic parameters we return the results. - if (node.typeArguments === undefined || node.typeArguments.length === 0) { - return result; + if (!ts.isTypeReferenceNode(typeNode)) { + throw new Error( + `Expected TypeReferenceNode for emitted reference, got ${ts.SyntaxKind[typeNode.kind]}.`); } - - // If there are any generics, we have to reflect them as well. - const translatedArgs = - node.typeArguments.map(arg => this.translateTransplantedTypeNode(arg, context)); - - return ts.factory.updateTypeReferenceNode( - result, result.typeName, ts.factory.createNodeArray(translatedArgs)); - } - - /** - * Translates a type node so that all of the type references it - * contains are imported and can be referenced in the context file. - */ - private translateTransplantedTypeNode(rootNode: ts.TypeNode, context: any): ts.TypeNode { - const factory: ts.TransformerFactory = transformContext => root => { - const walk = (node: ts.Node): ts.Node => { - if (ts.isTypeReferenceNode(node) && ts.isIdentifier(node.typeName)) { - const translated = - this.translateTransplantedTypeReferenceNode(node as ts.TypeReferenceNode & { - typeName: ts.Identifier; - }, context); - - if (translated !== node) { - return translated; - } - } - - return ts.visitEachChild(node, walk, transformContext); - }; - - return ts.visitNode(root, walk); - }; - - return ts.transform(rootNode, [factory]).transformed[0] as ts.TypeNode; + return typeNode; } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_parameter_emitter.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_parameter_emitter.ts index 066afb37d901..817eb8fb3bc1 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_parameter_emitter.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_parameter_emitter.ts @@ -9,9 +9,7 @@ import ts from 'typescript'; import {OwningModule, Reference} from '../../imports'; import {DeclarationNode, ReflectionHost} from '../../reflection'; - -import {canEmitType, TypeEmitter} from './type_emitter'; - +import {canEmitType, TypeEmitter} from '../../translator'; /** * See `TypeEmitter` for more information on the emitting process. diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index bca4e1ddf5e6..77b290aa5f36 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -8748,6 +8748,33 @@ function allTests(os: string) { expect(dtsContents).toContain('static ngAcceptInputType_value: i1.ExternalToNumberType;'); }); + it('should compile an input referencing an imported function with literal types', () => { + env.write('/transforms.ts', ` + export function toBoolean(value: boolean | '' | 'true' | 'false'): boolean { + return !!value; + } + `); + env.write('/test.ts', ` + import {Directive, Input} from '@angular/core'; + import {toBoolean} from './transforms'; + + @Directive({standalone: true}) + export class Dir { + @Input({transform: toBoolean}) value!: number; + } + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + const dtsContents = env.getContents('test.d.ts'); + + expect(jsContents).toContain('inputs: { value: ["value", "value", toBoolean] }'); + expect(jsContents).toContain('features: [i0.ɵɵInputTransformsFeature]'); + expect(dtsContents) + .toContain(`static ngAcceptInputType_value: boolean | "" | "true" | "false";`); + }); + it('should compile a directive input with a transform function with a `this` typing', () => { env.write('/test.ts', ` import {Directive, Input} from '@angular/core'; From 0d1a0c760f489870a0fe4f4adaa166ff3812f761 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 29 Oct 2023 00:53:50 +0200 Subject: [PATCH 2/2] fix(compiler-cli): use originally used module specifier for transform functions Prior to this change, the transform function would be referenced with a potentially relative import into an external declaration file. Such imports are not portable and should not be created in this context. This commit addresses the issue by threading though the originally used module specifier by means of the `Reference` type. Fixes #52324 --- goldens/public-api/common/index.md | 2 +- .../ngtsc/annotations/directive/src/shared.ts | 8 +++-- .../src/ngtsc/metadata/src/api.ts | 2 +- .../ngtsc/translator/src/type_translator.ts | 32 +++++++++++++------ .../src/ngtsc/typecheck/src/environment.ts | 4 +-- .../ngtsc/typecheck/src/type_check_block.ts | 7 ++-- .../ngtsc/typecheck/src/type_constructor.ts | 2 +- .../typecheck/test/type_check_block_spec.ts | 5 +-- .../typecheck/test/type_constructor_spec.ts | 4 +-- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 4 +-- 10 files changed, 45 insertions(+), 25 deletions(-) diff --git a/goldens/public-api/common/index.md b/goldens/public-api/common/index.md index 6b18b132b921..e7a32f13bf87 100644 --- a/goldens/public-api/common/index.md +++ b/goldens/public-api/common/index.md @@ -615,7 +615,7 @@ export class NgOptimizedImage implements OnInit, OnChanges, OnDestroy { // (undocumented) static ngAcceptInputType_height: unknown; // (undocumented) - static ngAcceptInputType_ngSrc: string | i1_2.SafeValue; + static ngAcceptInputType_ngSrc: string | i0.ɵSafeValue; // (undocumented) static ngAcceptInputType_priority: unknown; // (undocumented) diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts b/packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts index 5b0988ad2ff2..cb7dc6ae103d 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts @@ -779,7 +779,10 @@ function parseInputTransformFunction( // Treat functions with no arguments as `unknown` since returning // the same value from the transform function is valid. if (!firstParam) { - return {node, type: ts.factory.createKeywordTypeNode(ts.SyntaxKind.UnknownKeyword)}; + return { + node, + type: new Reference(ts.factory.createKeywordTypeNode(ts.SyntaxKind.UnknownKeyword)) + }; } // This should be caught by `noImplicitAny` already, but null check it just in case. @@ -795,7 +798,8 @@ function parseInputTransformFunction( assertEmittableInputType(firstParam.type, clazz.getSourceFile(), reflector, refEmitter); - return {node, type: firstParam.type}; + const viaModule = value instanceof Reference ? value.bestGuessOwningModule : null; + return {node, type: new Reference(firstParam.type, viaModule)}; } /** diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts index 21e845a2ccd3..968f464a99a8 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts @@ -140,7 +140,7 @@ export type InputMapping = InputOrOutput&{ /** Metadata for an input's transform function. */ export interface InputTransform { node: ts.Node; - type: ts.TypeNode; + type: Reference; } /** diff --git a/packages/compiler-cli/src/ngtsc/translator/src/type_translator.ts b/packages/compiler-cli/src/ngtsc/translator/src/type_translator.ts index 885fc57b23f9..c0bb3e8e84bf 100644 --- a/packages/compiler-cli/src/ngtsc/translator/src/type_translator.ts +++ b/packages/compiler-cli/src/ngtsc/translator/src/type_translator.ts @@ -9,7 +9,7 @@ import * as o from '@angular/compiler'; import ts from 'typescript'; -import {assertSuccessfulReferenceEmit, ImportFlags, Reference, ReferenceEmitter} from '../../imports'; +import {assertSuccessfulReferenceEmit, ImportFlags, OwningModule, Reference, ReferenceEmitter} from '../../imports'; import {ReflectionHost} from '../../reflection'; import {Context} from './context'; @@ -80,13 +80,17 @@ class TypeTranslatorVisitor implements o.ExpressionVisitor, o.TypeVisitor { return ts.factory.createTypeLiteralNode([indexSignature]); } - visitTransplantedType(ast: o.TransplantedType, context: Context) { - if (!ts.isTypeNode(ast.type)) { + visitTransplantedType(ast: o.TransplantedType, context: Context) { + const node = ast.type instanceof Reference ? ast.type.node : ast.type; + if (!ts.isTypeNode(node)) { throw new Error(`A TransplantedType must wrap a TypeNode`); } - const emitter = new TypeEmitter(typeRef => this.translateTypeReference(typeRef, context)); - return emitter.emitType(ast.type); + const viaModule = ast.type instanceof Reference ? ast.type.bestGuessOwningModule : null; + + const emitter = + new TypeEmitter(typeRef => this.translateTypeReference(typeRef, context, viaModule)); + return emitter.emitType(node); } visitReadVarExpr(ast: o.ReadVarExpr, context: Context): ts.TypeQueryNode { @@ -255,8 +259,9 @@ class TypeTranslatorVisitor implements o.ExpressionVisitor, o.TypeVisitor { return typeNode; } - private translateTypeReference(type: ts.TypeReferenceNode, context: Context): ts.TypeReferenceNode - |null { + private translateTypeReference( + type: ts.TypeReferenceNode, context: Context, + viaModule: OwningModule|null): ts.TypeReferenceNode|null { const target = ts.isIdentifier(type.typeName) ? type.typeName : type.typeName.right; const declaration = this.reflector.getDeclarationOfIdentifier(target); if (declaration === null) { @@ -264,10 +269,17 @@ class TypeTranslatorVisitor implements o.ExpressionVisitor, o.TypeVisitor { `Unable to statically determine the declaration file of type node ${target.text}`); } + let owningModule = viaModule; + if (declaration.viaModule !== null) { + owningModule = { + specifier: declaration.viaModule, + resolutionContext: type.getSourceFile().fileName, + }; + } + + const reference = new Reference(declaration.node, owningModule); const emittedType = this.refEmitter.emit( - new Reference(declaration.node), this.contextFile, - ImportFlags.NoAliasing | ImportFlags.AllowTypeImports | - ImportFlags.AllowRelativeDtsImports); + reference, this.contextFile, ImportFlags.NoAliasing | ImportFlags.AllowTypeImports); assertSuccessfulReferenceEmit(emittedType, target, 'type'); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts index bc97ea3ae2d8..edd234dfaa23 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts @@ -175,9 +175,9 @@ export class Environment implements ReferenceEmitEnvironment { /** * Generates a `ts.TypeNode` representing a type that is being referenced from a different place * in the program. Any type references inside the transplanted type will be rewritten so that - * they can be imported in the context fiel. + * they can be imported in the context file. */ - referenceTransplantedType(type: TransplantedType): ts.TypeNode { + referenceTransplantedType(type: TransplantedType>): ts.TypeNode { return translateType( type, this.contextFile, this.reflector, this.refEmitter, this.importManager); } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts index e0534583c2ca..ae6c8acf4407 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts @@ -717,7 +717,7 @@ class TcbDirectiveInputsOp extends TcbOp { if (this.dir.coercedInputFields.has(fieldName)) { let type: ts.TypeNode; - if (transformType) { + if (transformType !== null) { type = this.tcb.env.referenceTransplantedType(new TransplantedType(transformType)); } else { // The input has a coercion declaration which should be used instead of assigning the @@ -2067,7 +2067,10 @@ class Scope { interface TcbBoundAttribute { attribute: TmplAstBoundAttribute|TmplAstTextAttribute; - inputs: {fieldName: ClassPropertyName, required: boolean, transformType: ts.TypeNode|null}[]; + inputs: + {fieldName: ClassPropertyName, + required: boolean, + transformType: Reference|null}[]; } /** diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts index d3792fdeed76..4e92ededbe64 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts @@ -150,7 +150,7 @@ function constructTypeCtorParameter( /* type */ transform == null ? tsCreateTypeQueryForCoercedInput(rawType.typeName, classPropertyName) : - transform.type)); + transform.type.node)); } } if (plainKeys.length > 0) { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts index 6c5b4fc9e5a0..d4f92b5e5aa7 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts @@ -9,6 +9,7 @@ import ts from 'typescript'; import {initMockFileSystem} from '../../file_system/testing'; +import {Reference} from '../../imports'; import {TypeCheckingConfig} from '../api'; import {ALL_ENABLED_CONFIG, tcb, TestDeclaration, TestDirective} from '../testing'; @@ -611,10 +612,10 @@ describe('type check blocks', () => { transform: { node: ts.factory.createFunctionDeclaration( undefined, undefined, undefined, undefined, [], undefined, undefined), - type: ts.factory.createUnionTypeNode([ + type: new Reference(ts.factory.createUnionTypeNode([ ts.factory.createKeywordTypeNode(ts.SyntaxKind.BooleanKeyword), ts.factory.createKeywordTypeNode(ts.SyntaxKind.StringKeyword), - ]) + ])) }, }, }, diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts index 8fb26caa7d4a..9c5cf005d649 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts @@ -171,10 +171,10 @@ TestClass.ngTypeCtor({value: 'test'}); bindingPropertyName: 'baz', required: false, transform: { - type: ts.factory.createUnionTypeNode([ + type: new Reference(ts.factory.createUnionTypeNode([ ts.factory.createKeywordTypeNode(ts.SyntaxKind.BooleanKeyword), ts.factory.createKeywordTypeNode(ts.SyntaxKind.StringKeyword), - ]), + ])), node: ts.factory.createFunctionDeclaration( undefined, undefined, undefined, undefined, [], undefined, undefined) } diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 77b290aa5f36..3da7a2d7d11f 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -8713,7 +8713,7 @@ function allTests(os: string) { expect(jsContents).toContain(`import { externalToNumber } from 'external';`); expect(jsContents).toContain('inputs: { value: ["value", "value", externalToNumber] }'); expect(jsContents).toContain('features: [i0.ɵɵInputTransformsFeature]'); - expect(dtsContents).toContain('import * as i1 from "./node_modules/external/index";'); + expect(dtsContents).toContain('import * as i1 from "external";'); expect(dtsContents).toContain('static ngAcceptInputType_value: i1.ExternalToNumberType;'); }); @@ -8744,7 +8744,7 @@ function allTests(os: string) { expect(jsContents) .toContain('inputs: { value: ["value", "value", (value) => value ? 1 : 0] }'); expect(jsContents).toContain('features: [i0.ɵɵInputTransformsFeature]'); - expect(dtsContents).toContain('import * as i1 from "./node_modules/external/index";'); + expect(dtsContents).toContain('import * as i1 from "external";'); expect(dtsContents).toContain('static ngAcceptInputType_value: i1.ExternalToNumberType;'); });