Skip to content

Looser import/export elision blocking in visitElidableStatement #57223

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.

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

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

rbuckton
Copy link
Member

This uses a slightly less restrictive elision blocking mechanism in visitElidableStatement such that we preserve import/export elision when a "before" transform updates an import or export declaration, so long as we are able to verify that the local bindings that we would have used to detect elision have not changed.

Fixes #40603

@rbuckton rbuckton requested a review from jakebailey January 29, 2024 23:54
@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jan 29, 2024
// As the type information we would attempt to lookup to perform ellision is potentially unavailable for the synthesized nodes
// We do not reuse `visitorWorker`, as the ellidable statement syntax kinds are technically unrecognized by the switch-case in `visitTypeScript`,
// and will trigger debug failures when debug verbosity is turned up
if (parsed === node || isExportAssignment(node)) {
Copy link
Member

Choose a reason for hiding this comment

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

Are export assignments never blocked? When I test export = of a type, it's elided: https://www.typescriptlang.org/play?module=1#code/C4TwDgpgBAYg9nKBeKBGATAZgCwG4BQ+EAHmHAE7DKwK5A

Or is there something else going on and I'm confused?

Copy link
Member Author

@rbuckton rbuckton Jan 31, 2024

Choose a reason for hiding this comment

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

The purpose of blocking statement elision is to handle cases where a custom transform modifies the list of export bindings to catch cases like one where a user adds a new binding to an existing import, but we then just blindly elide the whole statement.

For export = and export default there's no change you can make to the binding list that would matter, since there isn't a binding list to begin with. The only change you could make is to the expression and we don't look at the expression of a transformed ExportAssignment when determining whether to elide, we instead look at the original parse tree node that the transformed ExportAssignment replaces.

@jakebailey
Copy link
Member

I feel partially unsure because my reading of #40603 was that changes to the import path (specifier?) should stop elision; is that not the case?

@rbuckton
Copy link
Member Author

rbuckton commented Jan 30, 2024

Per the original issue, the intent of the bug author is to rewrite paths. Pretty much everything we care about related to path resolution occurs during parse and check, not emit. The main reason we block elision is that we can't be certain what the user's intent was when they changed the node. i.e., if you add a new import specifier, should we still elide or no? Since most of these elisions are based on how the current file uses the import and not what the export actually is, the paths don't matter all that much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transforming ImportDeclaration or ExportDeclaration causes type specifiers to be output in js files
4 participants