Preserve original type parameter names more often when shadowed #55820
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test top100 |
|
Heya @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 @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 @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 @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 @jakebailey, I've started to run the tarball bundle task on this PR at 83275d2. You can monitor the build here. |
|
Heya @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 @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:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@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! |
| @@ -5987,7 +5987,7 @@ export interface NodeLinks { | |||
| decoratorSignature?: Signature; // Signature for decorator as if invoked by the runtime. | |||
| 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.
|
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @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: class A { }
const a = new A();
const b = <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... |
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. export declare class A {
}
export declare const a: A;
export declare const b: <A>(x: A) => import("./input").A; |
|
Write: class A { }
const a = new A();
function b<A,>(x: A) { return a; }And you'll see that we already emit |
|
Hmm, you're right, thanks. That self-import in the case of a module still bugs me a lot, though. |
jakebailey
changed the title
|
|
||
|
|
||
| //// [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.
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.
typescript-bot
added
For Milestone Bug
|
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.