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

Cherry pick PR #57887 into release-5.4 #57898

Merged
merged 2 commits into from Mar 28, 2024

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Mar 21, 2024

See #57887

I have no idea why the baselines are so big in the cherry pick compared to main, but they're all improvements?

@@ -2,6 +2,6 @@

=== ParameterList5.ts ===
function A(): (public B) => C {
>A : () => (B: any) => C
>A : () => (public B) => C
Copy link
Member

Choose a reason for hiding this comment

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

Changes like this one aren't in main because #57772 added checks/transforms to prevent them. Here, for example, now we lookup B successfully and think "hey, I'll just reuse the node" (which also happens with the isDeclarationName fix added in #57772, for this specific case), but the node's got a modifier that needs deleting and a : any that needs adding that it doesn't add, while in #57772 I added a line to the visitor to add the missing : any and drop the public from existing nodes in the node builder. There's a couple changes like this in here. They're not that bad a change because it's not like this node's form nowhere - it's the input, so it's at least no worse than the input - but seeing an error in the input (like this) and just dumping the erroneous node into the output is something we usually try to avoid. We could also backport the changes to visitExistingNodeTreeSymbols from that PR if we want to minimize the diff.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does that mean that this PR is wrong without that other PR? Or is this PR safe to include in a patch by itself?

@DanielRosenwasser DanielRosenwasser merged commit 3caec2c into microsoft:release-5.4 Mar 28, 2024
18 checks passed
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 5.4.32 milestone Mar 28, 2024
@DanielRosenwasser DanielRosenwasser changed the title Cherry pick PR 57887 into release-5.4 Cherry pick PR #57887 into release-5.4 Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants