-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Consider enclosing declaration when serializing inferred return types #59170
Conversation
@typescript-bot test it |
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@@ -79,7 +79,7 @@ class C extends B { | |||
>A : typeof A | |||
> : ^^^^^^^^ | |||
>then : <S extends A>(cb: (x: A) => S) => Chain<S> |
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 examples like these, I think it's incredibly weird that the old behavior had us not reusing effectively anything but the return type annotation.
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
>fun : () => import("bbb").INode<T> | ||
> : ^^^^^^ ^^^^^ ^^^^^ ^ | ||
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
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 old reuse here seems dubious to me; create
is imported, so how are we reusing nodes from another file? Similarly to a lot of what changes in this PR.
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
@typescript-bot test it |
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
@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! |
1 similar comment
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
// To ensure we don't serialize the wrong type we check that that return type of the signature corresponds to the declaration return type signature | ||
if (annotation && getTypeFromTypeNode(context, annotation) === type) { | ||
const result = tryReuseExistingTypeNodeHelper(context, annotation); | ||
const annotation = getNonlocalEffectiveReturnTypeAnnotationNode(signature.declaration); |
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.
Are we sure removing this check getTypeFromTypeNode(context, annotation) === type
is a good idea? I think there are cases where the type of the annotation might differ from the type of the signature.
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.
That's what tryReuseExistingTypeNode
does internally and is how it differs from tryReuseExistingTypeNodeHelper
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.
Right; removing this didn't change anything except reducing some type instantiations. Sorry, I rebased that particular thing away because I felt pretty confident that I could remove the equality here.
Interestingly, this is not the fix for the type display issue from DefinitelyTyped/DefinitelyTyped#69838. |
I can confirm that installing the npm package referenced here #59170 (comment) fixes the type display issue reported in Effect-TS/effect#3171 |
Thanks for confirming; I'm going to make one last attempt to make a repro that exhibits the "cannot be named" error before merging/backporting. |
Okay, I give up trying to produce a test case for the cannot be named error. |
@typescript-bot cherry-pick this to release-5.5 |
Hey, @jakebailey! I was unable to cherry-pick this PR. Check the logs at: https://github.com/microsoft/TypeScript/actions/runs/9863981289 |
Okay, guess I'll do it manually |
In playing around with the repros from #58937 and Effect-TS/effect#3171, I tried to go through the changes in #58546 to narrow things down to what might have caused the problem with inferred/unannotated return types.
One thread to pull on is that removing the
context.mapper
stuff seemed to fix the problem, but cause other undesirable changes.Another area of note was
serializeReturnTypeForSignatureWorker
; he doc fortryReuseExistingTypeNodeHelper
says:serializeReturnTypeForSignatureWorker
suspiciously callstryReuseExistingTypeNodeHelper
directly. I just swappedtryReuseExistingTypeNode
in with parameters that seemed to make sense, and the repro cases appeared to snap back to what they used to be. For example, the declaration emit in Effect-TS/effect#3171 original post looked like this in 5.4:But in 5.5 is:
Note that we copies
Kind
out of the call signature oftraverse
. With this PR, we go to:Which is the best of both, in that
Kind
has gone away, and we still kept the better type parameter renaming behavior.A number of other baselines "regressed" in that they seem to have less node reuse, but some appear to be more reasonable in some ways. Notably, no dts emit in our tests changed, which is good, but indicates that we need more testing here.
This is extremely unscientific. If this PR looks wrong, it should at least provide someone else with some info that could track the issue down better.
I'm not going to say that this is the fix to #58937 without a bit more consensus.Fixes #58937
cc @weswigham @dragomirtitian