-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Extract function types from function and arrow expressions. #60234
Extract function types from function and arrow expressions. #60234
Conversation
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
@typescript-bot test it |
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
declare var foo3: () => () => /*elided*/ any; | ||
declare var x: () => () => /*elided*/ 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.
Do you know why this is changing? It seems a little awkward for this self referential type to be printed one level deep like this rather than the single elided any (which would cause fewer downstream errors)
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 base cause is that at the root level syntactic printing is now always given a try. And in this case the parameter list can be copied, it's just the return type that needs to fallback on type printing. I think I can revert this behavior.
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.
I looked into it and I don't think I can easily revert to the old behavior. The type checked printer only knows a type is too recursive to represent when it tries to print it. So when I see the function expression, I try to reuse types from the expression. When doing so, I discover the missing return type and fallback to type checker printing which then discovers the type is recursive.
What specifically is the worry in this case ? () => any
and () => () => any
are both bad, they both allow calling the function, so I don't really think the types are worse.
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.
Moreso if someone actually calls it and uses the result, that result will not be any
and may cause follow-on errors. But I think this particular case is exceeding rare and weird anyway. Like, the function is clearly returning a function, so maybe that safety is better.
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.
In general, this seems fine to me, though I am unsure if we want to sneak this in before the 5.7 RC. @weswigham @DanielRosenwasser do you have an opinion here?
5.7 is cut, main is open for 5.8; I'm just going to merge this now. I don't think we need this for 5.7 or anything. |
This PR brings TS emit closer to what an external tool could emit without type information.