Skip to content

Generate names for type parameter declarations in inferred types #23902

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

Merged

Conversation

weswigham
Copy link
Member

Fixes #16313

@weswigham weswigham requested review from rbuckton, mhegazy and a user May 4, 2018 21:35
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should get another review though since I'm not very familiar with the emitter

@@ -3031,7 +3031,7 @@ namespace ts {
// Options
NoTruncation = 1 << 0, // Don't truncate result
WriteArrayAsGenericType = 1 << 1, // Write Array<T> instead T[]
// empty space
GenerateNamesForShadowedTypeParams = 1 << 2, // When a type parameter T is shadowing another T, generate a name for it so it can still be referenced
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need a flag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't wanna bother renaming the conflicts in quickinfo and signature help, at least in my opinion.

@@ -3444,6 +3445,7 @@ declare namespace ts {
function createFileLevelUniqueName(text: string): Identifier;
/** Create a unique name generated for a node. */
function getGeneratedNameForNode(node: Node): Identifier;
function getOptimisticScopedGeneratedNameForName(node: Identifier): Identifier;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be public?

if (optimistic) {
if (checkFn(baseName)) {
generatedNames.set(baseName, true);
if (scoped) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything that goes in generatedNames is already reserved throughout the entire generated output, so scoping these doesn't seem to make any sense. Why was this needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scoped causes it to go into reservedNames instead of generatedNames; this allows, eg, T_1 to be reused in adjacent scopes.

@@ -197,6 +197,15 @@ namespace ts {
return name;
}

export function getOptimisticScopedGeneratedNameForName(node: Identifier): Identifier {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a special factory function for this? Couldn't we just add the extra flags to autoGenerateflags after calling one of the existing factory functions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably? But I'd also probably still write that into a function so it stays as an inline call as the use site.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather we just change the signature of getGeneratedNameForNode to something like:

export function getGeneratedNameForNode(node: Node, flags?: GeneratedIdentifierFlags): Identifier

Also, the existing getGeneratedNameForNode actually defers reading the text of the node until emit. This is important so you could correctly handle a case where the node you are generating a name from is itself a generated name. What happens if you need to end up with <T>(x: T) => <T_1>(y: T_1) => T_1_1?

@@ -197,6 +197,15 @@ namespace ts {
return name;
}

export function getOptimisticScopedGeneratedNameForName(node: Identifier): Identifier {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather we just change the signature of getGeneratedNameForNode to something like:

export function getGeneratedNameForNode(node: Node, flags?: GeneratedIdentifierFlags): Identifier

Also, the existing getGeneratedNameForNode actually defers reading the text of the node until emit. This is important so you could correctly handle a case where the node you are generating a name from is itself a generated name. What happens if you need to end up with <T>(x: T) => <T_1>(y: T_1) => T_1_1?

@weswigham
Copy link
Member Author

@rbuckton Do you wanna update your status and give more comments?

@weswigham weswigham merged commit 6149b41 into microsoft:master May 10, 2018
@weswigham weswigham deleted the declarations-synthesized-typeparams branch May 10, 2018 18:16
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants