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

Fix declaration emit for instantiation expressions. #60796

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dragomirtitian
Copy link
Contributor

Fixes #60758

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Dec 17, 2024
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

This seems like the wrong approach. The issue doesn't occur with this change, but not for any reasons I'd expect - this makes us bail on node reuse when we descend into the type predicate and see that the parameter name referenced by the predicate isn't in scope (But it probably should be! It being missing is probably causing us to skip reusing type query nodes we could otherwise reuse!). The underlying issue really is that we're not tracking predicate node matching-ness correctly in the first place - in fact, we're not really recognizing predicate nodes in the serializer at all, except for the aforementioned entity name check. We really need to check if the annotation is a predicate, and, if so, break it down and compare the predicated types instead of the raw return types (which are just boolean for is predicates and never for asserts predicates, and will always match across instantiations, producing false-positives under the current node reuse check because of that!) serializeInferredReturnTypeForSignature handles predicates - the non-inferred branch of serializeReturnTypeForSignature does not and needs to (which seems to be because canReuseTypeNodeAnnotation is not type predicate aware and needs to be).

It might be easier to push the logic out one layer and create a serializeReturnTypePredicateForSignature on the host that's used by callers when the signature is known to have a predicate, rather than integrating it directly into the existing annotation codepath, but I'm not picky about which direction is better - I just know the one in this PR's gonna regress with more improvements to node reuse/synthetic scoping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Status: Waiting on author
Development

Successfully merging this pull request may close these issues.

instantiation expression usage leading to an invalid d.ts file generation
3 participants