| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Do not remove binding alias in function declarations #57020
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
Conversation
There was a problem hiding this 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
requested review from
DanielRosenwasser,
weswigham,
RyanCavanaugh and
andrewbranch
dragomirtitian
force-pushed
the
binding-renames
branch
from
17b28da to
a9790ae
Compare
dragomirtitian
force-pushed
the
binding-renames
branch
from
a9790ae to
bdabd74
Compare
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 😅)
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 |
| 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; |
There was a problem hiding this comment.
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.)
|
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. |
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