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

Do not remove binding alias in function declarations #57020

Merged
merged 1 commit into from Jan 12, 2024

Conversation

dragomirtitian
Copy link
Contributor

@dragomirtitian dragomirtitian commented Jan 11, 2024

Revert change that removes binding aliases from declaration files. Doing this correctly implies doing a lot more work than previously anticipated.

Fixes #56991
Fixes #56992

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jan 11, 2024
Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

All of the examples that are broken feel like they should be fixable; I know some of this I've tackled in typeToTypeNode, but the same strategies probably don't work in declaration emit. I'm fine reverting this so it doesn't go out incorrectly and we can try and tackle it another day.

@jakebailey
Copy link
Member

This also partially reverts #41044 as it removes all removal; I was actually expecting this to have caused d.ts errors because #41044 also introduced an error for unused renamings... but that didn't happen?

@dragomirtitian
Copy link
Contributor Author

All of the examples that are broken feel like they should be fixable; I know some of this I've tackled in typeToTypeNode, but the same strategies probably don't work in declaration emit. I'm fine reverting this so it doesn't go out incorrectly and we can try and tackle it another day.

The problems are not unsolvable for sure. Even in declaration emit I think we ca solve it. But the problem is that it will introduce a lot of perf cost. Seems like it's not worth the complexity. (I belatedly arrived at the same conclusion as @andrewbranch 😅)

This also partially reverts #41044 as it removes all removal; I was actually expecting this to have caused d.ts errors because #41044 also introduced an error for unused renamings... but that didn't happen?

The error for unused renames is still there. I did not remove it. It only seems to pop up on some kinds of declarations for example function types let o: ({n : a}: any) => void;

type F3 = ({ a, b, c }: O) => any;
type F4 = ({ a }: O) => any;
type F5 = ({ a, b, c }: O) => any;
type F2 = ({ a: string }: O) => any;
Copy link
Member

Choose a reason for hiding this comment

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

The error for unused renames is still there. I did not remove it. It only seems to pop up on some kinds of declarations for example function types let o: ({n : a}: any) => void;

Just to point out an example from the PR, this is code that I believed would have gained an error: https://www.typescriptlang.org/play?#code/C4TwDgpgBA8lC8UDeBYAUFTUCGB+AXFAM7ABOAlgHYDmA3OllAEaGUCuAtkxKfRlgGNWnbr3QBfPulCQoAMQBMCKAAokOQiQo0o4wjACUCAHw5KIWkA

Yet, there's no error baseline. That being said, it looks like for declaration files we straight up skip checking the final list:

if (!node.isDeclarationFile) {
    checkPotentialUncheckedRenamedBindingElementsInTypes();
}

So, I think everything is fine? (Other than that I don't think it makes any sense that we still collect potentialUnusedRenamedBindingElementsInTypes in declaration files only to not read them later, which is not new.)

@jakebailey
Copy link
Member

jakebailey commented Jan 12, 2024

After checking the above and noticing #41044 (comment) saying explicitly that this does not show in dts files, I'm now confident that this PR is working correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
None yet
4 participants