Skip to content
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

Attempt to reuse type parameter constraint nodes #58539

Merged

Conversation

weswigham
Copy link
Member

Fixes #58511

We should probably also do defaults, but one thing at a time.

@weswigham weswigham requested a review from jakebailey May 14, 2024 22:34
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 14, 2024
@@ -8274,7 +8278,6 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
sym = resolveEntityName(leftmost, meaning, /*ignoreErrors*/ true, /*dontResolveAlias*/ true);
if (
context.enclosingDeclaration &&
(getNodeLinks(context.enclosingDeclaration).fakeScopeForSignatureDeclaration || !findAncestor(node, n => n === context.enclosingDeclaration)) &&
Copy link
Member Author

Choose a reason for hiding this comment

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

A fourslash test was affected by the code change and made it clear this small change to #58516 was needed - checking that the entity name resolves within the enclosing context is just always required to ensure the name's actually available, not just when there's a fake scope.

@@ -8431,6 +8434,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function visitExistingNodeTreeSymbolsWorker(node: Node): Node | undefined {
if (isJSDocTypeExpression(node)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The constraint type node for a type parameter declared in jsdoc is a JSDocTypeExpression which, as it turns out, is not actually a type node (according to isTypeNode, though the typesystem thinks it is - inconsistent much?), so it needs to get unwrapped; you also definitely don't want the {} from jsdoc copied into a .d.ts file in the first place! So the assertion triggering off the core of this change found this bug, which was nice.

@weswigham weswigham merged commit 79a8514 into microsoft:main May 16, 2024
28 checks passed
@weswigham weswigham deleted the reuse-type-parameter-constraint-nodes branch May 16, 2024 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[transpileDeclaration API][5.5] More missing type-only imports
3 participants