Skip to content
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

Fix CJS export of typedef and class w/latebound names #55053

Merged
merged 3 commits into from
Sep 21, 2023

Conversation

sandersn
Copy link
Member

Fixes #53967

The problem is that the weird, ad-hoc CJS merge needs to happen at two different times. The first is its existing location in getCommonJsExportEquals. The second is getResolvedMembersOrExportsOfSymbol, which combines normal, early-bound symbols with late-bound symbols.

The CJS merge happens before the addition of late-bound symbols, so a merge will result in the loss of [Symbol.iterable] from the bug. But manually merging the resolvedExports/Members during the CJS merge also doesn't work, because it prevents the module's resolvedExports/Members from being filled in later.

Instead, this PR delays part of the CJS merge to
getResolvedMembersOrExportsOfSymbol. Unfortunately, this requires retrieving the original, unmerged symbols--since their resolvedExports/Members are already cached. I do this by looking at the symbols for each of the merged symbol's declarations, which feels wrong.

Please suggest alternate ways to do this. I don't think I understand the late-binding machinery well enough, so it's likely there's a cleaner way.

Fixes microsoft#53967

The problem is that the weird, ad-hoc CJS merge needs to happen at two different
times.  The first is its existing location in getCommonJsExportEquals.
The second is getResolvedMembersOrExportsOfSymbol, which combines
normal, early-bound symbols with late-bound symbols.

The CJS merge happens before the addition of late-bound symbols, so a
merge will result in the loss of [Symbol.iterable] from the bug.
But manually merging the resolvedExports/Members during the CJS merge
also doesn't work, because it prevents the module's
resolvedExports/Members from being filled in later.

Instead, this PR delays part of the CJS merge to
getResolvedMembersOrExportsOfSymbol. Unfortunately, this requires
retrieving the original, unmerged symbols--since their
resolvedExports/Members are already cached. I do this by looking at the
symbols for each of the merged symbol's declarations, which feels wrong.

Please suggest alternate ways to do this. I don't think I understand the
late-binding machinery well enough, so it's likely there's a cleaner
way.
@sandersn sandersn added this to Not started in PR Backlog Jul 28, 2023
@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Jul 28, 2023
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

I don't think I understand the late-binding machinery well enough, so it's likely there's a cleaner way.

The cleaner way is probably breaking the cycle between late-binding and cjs export merging, I think by making it so module members bind directly into the export= symbol if it's present (which probably requires a 2-pass process), rather than the containing module - basically moving what getCommonJsExportEquals does up into the binder phase. There's probably a bunch of knock-on effects to this, though, since I know the js declaration emitter, at least, already does a bunch of hacks (or at least stuff similar to this where it goes back and looks at the original unmerged symbols) to work around the odd symbol structure getCommonJsExportEquals creates.

In any case, this is probably fine - it's just piling a similar workaround into an already ugly space in the compiler - but maybe @rbuckton has more thoughts from the late bound member angle?

PR Backlog automation moved this from Waiting on reviewers to Needs merge Sep 13, 2023
@sandersn sandersn merged commit cf084b6 into microsoft:main Sep 21, 2023
19 checks passed
PR Backlog automation moved this from Needs merge to Done Sep 21, 2023
@sandersn sandersn deleted the cjs-typedef-latebound-exports branch September 21, 2023 19:25
snovader pushed a commit to mestro-se/TypeScript that referenced this pull request Sep 23, 2023
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
Archived in project
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Unrelated JSDoc annotation causes class to not be assignable to itself or iterable
4 participants