- Notifications
You must be signed in to change notification settings - Fork13.1k
Enable GenerateNamesForShadowedTypeParams for types baselines#55821
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Enable GenerateNamesForShadowedTypeParams for types baselines#55821
Uh oh!
There was an error while loading.Please reload this page.
Conversation
typescript-bot commentedSep 21, 2023
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document themon our wiki's API Breaking Changes page. Also, please make sure@DanielRosenwasser and@RyanCavanaugh are aware of the changes, just as a heads up. |
| NoTruncation=1<<0,// Don't truncate typeToString result | ||
| WriteArrayAsGenericType=1<<1,// Write Array<T> instead T[] | ||
| //hole because there's a hole in node builder flags | ||
| GenerateNamesForShadowedTypeParams=1<<2,//When a type parameter T is shadowing another T, generate a name for it so it can still be referenced |
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.
This API change is to be able to use this flag via the type printing system, since TypeFormatFlags and NodeBuilderFlags overlap intentionally.
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 am not sure if changing this option in baselines is good or not since Quickinfo doesnt use this flag and thats what our type baselines go for?
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.
Honestly I was thinking about whether or not this should be the default for quick info; it too can cause confusion as in:#55820 (comment)
sandersn left a comment
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 agree, I think this should be used with quickinfo too.
jakebailey commentedNov 3, 2023
For sake of comparison, I rebased#55820 on top of this PR just to see how much that PR did; the answer is a lot:jakebailey@47e6ca5 You can see how that other PR removes a load of extra renames, which is good. |
weswigham left a comment
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.
We definitely want to get better at reusing the original names where possible - those sibling overloads with extra renames in particular are pretty ugly. The extra renames don't really matter for type baselines (AFAIK, part of the reason this flags exists was just so nobody'd have to review the baseline changes in this PR - which, being honest, I've only glanced through, not examined in full detail), and has minimal impact on declaration emit, butprobably isn't cleaned up enough to be up to snuff with what you'd wanna see for quickinfo in the editor, since that'ssupposed to be as-written wherever possible, and otherwise usually as simple as we can keep it. Might be with the other PR, though, not really sure.
In any case, type baselines are just for us, and if we think they're more useful by logging these renames, then let's go for it. Minimally it gives us more coverage for the type parameter renaming logic in the node builder, which previously were only exercised by declaration emit tests. Only thing maybe concerning (in the abstract) is that type baselines pass inenclosingDeclarations of just about every node in the tree to the node builder, while declaration emit usually only passes in top-level declarations and signature declarations. Hopefully we don't quietly encode that top-level/signature-only assumption into one of the name-generation codepaths.
jakebailey commentedNov 6, 2023
Thanks; I'm going to merge this and rebase#55820 on top of it as I think it'll make the change a lot clearer. This technically may make backporting to 5.3 harder, but that branch is locked off and it should be easy to update the baselines. |
This option is enabled when using declaration emit, but not in types baselines. I think that if we had this enabled, we may have seen issues fixed by PRs like#55820 sooner, and it's also "more reflective" of what the actual types are.
(Really I wonder if we should have this as the default everywhere...)