fix(compiler-cli): various fixes to input transform type emitting by JoostK · Pull Request #52437 · angular/angular

JoostK · 2023-10-28T22:55:55Z

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

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

…tion 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 angular#51672

JoostK · 2023-10-28T22:59:10Z

packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts

This is still broken, but likely hasn't caused issues because type constructors in the TCB are not translated to template code so they are never reported to users.

The type node should go through the emitter process here as well, as we can't simply dump the original type node in a completely different source file as is currently done. Fixing it requires some more extensive changes though so I kept it broken for now.

… 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 angular#52324

crisbeto · 2023-10-29T12:10:13Z

Would this fix #51424 or do we still need #51474?

JoostK · 2023-10-29T13:26:25Z

Would this fix #51424 or do we still need #51474?

Unfortunately the TCB generator also didn't support ambient types, so I'd expect that #51474 is still relevant.

pkozlowski-opensource

LGTM

Reviewed-for: public-api

crisbeto · 2023-10-30T18:21:44Z

packages/compiler-cli/src/ngtsc/translator/src/type_translator.ts


-  visitTransplantedType(ast: o.TransplantedType<ts.Node>, context: any) {
-    if (!ts.isTypeNode(ast.type)) {
+  visitTransplantedType(ast: o.TransplantedType<unknown>, context: Context) {


nit: would it make sense to use something like ts.Node | Reference here?

I decided not to, because the T of TransplantedType is not constrained so using ts.Node | Reference here might not be accurate.

crisbeto · 2023-10-30T18:25:36Z

packages/compiler-cli/src/ngtsc/translator/src/type_translator.ts

-    if (!ts.isTypeReferenceNode(result)) {
-      throw new Error(`Expected TypeReferenceNode when referencing the type for ${
-          node.typeName.text}, but received ${ts.SyntaxKind[result.kind]}`);
+    let owningModule = viaModule;


I think we can reduce this to:

const owningModule = declaration.viaModule === null ? viaModule : owningModule = { specifier: declaration.viaModule, resolutionContext: type.getSourceFile().fileName, };

That would do the same indeed, but I find the current logic more readable.

AndrewKushnir

Reviewed-for: public-api

alxhub · 2023-10-31T20:42:56Z

This PR was merged into the repository by commit 581eff4.

…tion arguments (#52437) 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 PR Close #52437

… functions (#52437) 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 PR Close #52437

…tion arguments (#52437) 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 PR Close #52437

… functions (#52437) 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 PR Close #52437

angular-automatic-lock-bot · 2023-12-01T00:10:47Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

…tion arguments (angular#52437) 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 angular#51672 PR Close angular#52437

… functions (angular#52437) 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 angular#52324 PR Close angular#52437

JoostK added target: patch This PR is targeted for the next patch release area: compiler Issues related to `ngc`, Angular's template compiler labels Oct 28, 2023

ngbot bot modified the milestone: Backlog Oct 28, 2023

JoostK commented Oct 28, 2023

View reviewed changes

JoostK force-pushed the ngtsc/coerced-input-types branch from 0e52fbb to 2f973e6 Compare October 29, 2023 10:20

JoostK force-pushed the ngtsc/coerced-input-types branch from 2f973e6 to 0d1a0c7 Compare October 29, 2023 11:02

JoostK marked this pull request as ready for review October 29, 2023 11:16

JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 29, 2023

pullapprove bot requested review from alxhub, dylhunn and pkozlowski-opensource October 29, 2023 11:16

pkozlowski-opensource approved these changes Oct 30, 2023

View reviewed changes

pullapprove bot requested a review from AndrewKushnir October 30, 2023 12:30

JoostK requested a review from crisbeto October 30, 2023 18:01

crisbeto approved these changes Oct 30, 2023

View reviewed changes

AndrewKushnir approved these changes Oct 30, 2023

View reviewed changes

pullapprove bot requested a review from AndrewKushnir October 30, 2023 22:21

AndrewKushnir approved these changes Oct 30, 2023

View reviewed changes

AndrewKushnir removed request for alxhub and dylhunn October 30, 2023 22:21

AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 30, 2023

alxhub closed this in 0c7accf Oct 31, 2023

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compiler-cli): various fixes to input transform type emitting#52437

fix(compiler-cli): various fixes to input transform type emitting#52437
JoostK wants to merge 2 commits intoangular:mainfrom
JoostK:ngtsc/coerced-input-types

JoostK commented Oct 28, 2023

Uh oh!

JoostK Oct 28, 2023

Uh oh!

crisbeto commented Oct 29, 2023

Uh oh!

JoostK commented Oct 29, 2023

Uh oh!

pkozlowski-opensource left a comment

Uh oh!

crisbeto Oct 30, 2023

Uh oh!

JoostK Oct 30, 2023

Uh oh!

crisbeto Oct 30, 2023

Uh oh!

JoostK Oct 30, 2023

Uh oh!

AndrewKushnir left a comment

Uh oh!

alxhub commented Oct 31, 2023

Uh oh!

angular-automatic-lock-bot bot commented Dec 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

JoostK commented Oct 28, 2023

Uh oh!

JoostK Oct 28, 2023

Choose a reason for hiding this comment

Uh oh!

crisbeto commented Oct 29, 2023

Uh oh!

JoostK commented Oct 29, 2023

Uh oh!

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

Uh oh!

crisbeto Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

JoostK Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

crisbeto Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

JoostK Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

alxhub commented Oct 31, 2023

Uh oh!

angular-automatic-lock-bot bot commented Dec 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants