Enable GenerateNamesForShadowedTypeParams for types baselines #55821
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
| @@ -5298,7 +5298,7 @@ export const enum TypeFormatFlags { | |||
| None = 0, | |||
| 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.
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.
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.
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)
|
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. |
There was a problem hiding this comment.
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, but probably isn't cleaned up enough to be up to snuff with what you'd wanna see for quickinfo in the editor, since that's supposed 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 in enclosingDeclarations 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.
|
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...)