Uh oh!
There was an error while loading. Please reload this page.
Preserve original type parameter names more often when shadowed - #55820
Conversation
Jake Bailey (jakebailey)
commented
Sep 21, 2023
TypeScript Bot (@typescript-bot) test top100 |
Heya Jake Bailey (@jakebailey), I've started to run the parallelized Definitely Typed test suite on this PR at 83275d2. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the diff-based user code test suite on this PR at 83275d2. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the tsc-only perf test suite on this PR at 83275d2. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the bun perf test suite on this PR at 83275d2. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the tarball bundle task on this PR at 83275d2. You can monitor the build here. |
Heya Jake Bailey (@jakebailey), I've started to run the diff-based top-repos suite on this PR at 83275d2. You can monitor the build here. Update: The results are in! |
Hey Jake Bailey (@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 |
TypeScript Bot (typescript-bot)
commented
Sep 21, 2023
Jake Bailey (@jakebailey) Here they are:CompilerComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Sep 21, 2023
Jake Bailey (@jakebailey) Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
| spreadIndices?: { first: number | undefined, last: number | undefined }; // Indices of first and last spread elements in array literal | ||
| parameterInitializerContainsUndefined?: boolean; // True if this is a parameter declaration whose type annotation contains "undefined". | ||
| fakeScopeForSignatureDeclaration?: boolean; // True if this is a fake scope injected into an enclosing declaration chain. | ||
| fakeScopeForSignatureDeclaration?: "params" | "typeParams"; // If present, this is a fake scope injected into an enclosing declaration chain. |
There was a problem hiding this comment.
Not sure if this should be an enum, I just used a string here as the least annoying way to get two truthy values.
TypeScript Bot (typescript-bot)
commented
Sep 21, 2023
Jake Bailey (@jakebailey) Here they are:CompilerComparison Report - baseline..pr
System info unknown Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Sep 21, 2023
Jake Bailey (@jakebailey) Here are the results of running the top-repos suite comparing Everything looks good! |
TypeScript Bot (typescript-bot)
commented
Sep 21, 2023
Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready. |
Looking at #55653, technically it's the other type that's being shadowed by the type parameter, and that one already is preserved (the type parameter is renamed). If it were the other way around then the current behavior might actually be correct 😉 |
Here's a test, in case this isn't already covered: classA{}consta=newA();constb=<A,>(x: A)=>aThe type parameter still needs to be renamed here. Also here's a fun bit of chicanery: Looks like an identity function, but isn't! |
This PR makes that work, check the declaration tab here: https://www.staging-typescript.org/play?ts=5.3.0-pr-55820-7#code/MYGwhgzhAECC0G9oF8BQwD2A7CAXaY0AvNFgKYDucAFAJQDc62e0ARsdADywA0AfNQAeALji1ifAkA That's actually the test case I added first. Unfortunately hover doesn't set the flag from: #55821 I don't think. I've been wondering if we should just drop this flag, so we are consistent... |
Bruce Pascoe (fatcerberus)
commented
Sep 22, 2023
This is a legal declaration? I didn't know Also if the file is a module the output is... questionable. I'd rather just have the type parameter be renamed here. exportdeclareclassA{}exportdeclareconsta: A;exportdeclareconstb: <A>(x: A)=>import("./input").A; |
Jake Bailey (jakebailey)
commented
Sep 22, 2023
Write: classA{}consta=newA();functionb<A,>(x: A){returna;}And you'll see that we already emit |
Bruce Pascoe (fatcerberus)
commented
Sep 22, 2023
Hmm, you're right, thanks. That self-import in the case of a module still bugs me a lot, though. |
| //// [declarationEmitTypeParameterNameShadowedInternally.d.ts] | ||
| export declare const foo: <T>(x: T) => <T_1>(y: T_1) => readonly [T, T_1]; |
There was a problem hiding this comment.
Actually I thought this test had failed, but actually, I don't think it did fail; this matches main, I think...
| >y : T | ||
| return inner; | ||
| >inner : <T>(y: T) => readonly [T, T] |
There was a problem hiding this comment.
Here you can see that the tooltip is very wrong because we don't enable renaming in tooltips.
There was a problem hiding this comment.
Nothing new, though:
de29cc3 to
5fb8ebbComparea9525a3 to
889812bCompare
Wesley Wigham (weswigham)
left a comment
There was a problem hiding this comment.
I know I've said it elsewhere - Not a huge fan of the double-scope thing (or basically rebuilding the signature's .locals table by hand in two parts), but this isn't much worse than what we're already doing, and fixing that is... another issue (ungh, the reading through this, my own PR, and the original PR adding this codepath made me aware of some problems in caching caused by this approach that we really aughta fix - we're caching results between unrelated queries). This is a meaningful improvement for now (and technically will hide the problem a bit - two cache nodes means you're slightly less likely to hit the incorrect cache key issue), so we should probably take it. Once it's in, I'll make a followup with how I'd prefer it to be structured, ignoring any perf issues that causes, and once the perf of that is made acceptable via other changes I've been looking into, we can try that out for size. This all just feels.... too complicated and bespoke, for what little it does.
Co-authored-by: Wesley Wigham <wwigham@gmail.com>
889812b to
abb71e9CompareJake Bailey (jakebailey)
commented
Nov 6, 2023
Dunno why CI isn't running... |
Fixes#55653
Fixes#55575
This turned out to be extremely related to #49627. Our original mechanism to solve the problem for type parameters was to be very conservative and rename when we think there might be a problem.
In this PR, I extend the mechanism from #49627 to the type parameters, introducing a second symbol table and fake scope to hold them, as we can't merge the symbols that share the same name without modifying them.
This turns out to have a positive impact on some existing baselines; see 83275d2 for the actual baseline diff.