Skip to content

Simplify isDistributionDependent to only check dependance within trueType/falseType #52034

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

Closed

Conversation

Andarist
Copy link
Contributor

While looking into #52021 I noticed that isDistributionDependent checks more than if a type parameter is referenced by the conditional's type trueType/falseType. What has been checked is mentioned in this comment (and in the implementation following this comment).

I might be missing something but it seems to me that the only relevant parts of this check should be the trueType and falseType. Tests pass - so that's a good sign as well 😉

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Dec 27, 2022
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Comment on lines 17502 to 17505
function isDistributionDependent(root: ConditionalRoot) {
return root.isDistributive && (
isTypeParameterPossiblyReferenced(root.checkType as TypeParameter, root.node.trueType) ||
isTypeParameterPossiblyReferenced(root.checkType as TypeParameter, root.node.falseType));
containsReference(root.checkType as TypeParameter, root.node.trueType) ||
containsReference(root.checkType as TypeParameter, root.node.falseType));
Copy link
Contributor Author

@Andarist Andarist Dec 27, 2022

Choose a reason for hiding this comment

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

The new logic more closely matches the comment at the only call site of isDistributionDependent:

// We check for a relationship to a conditional type target only when the conditional type has no
// 'infer' positions and is not distributive or is distributive but doesn't reference the check type
// parameter in either of the result types.

Copy link
Member

@ahejlsberg ahejlsberg left a comment

Choose a reason for hiding this comment

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

I don't think we need this change. The additional check mentioned in the comment doesn't come into play in the isDistributionDependent scenario because there are never any intervening statement blocks between the infer declarations and the true and false branches of the conditional type. So we're doing a little bit of (harmless) extra work, but that's all.

@Andarist
Copy link
Contributor Author

Andarist commented Jan 3, 2023

@ahejlsberg when investigating the referenced issue , I noticed that this conditional type is actually determined to be distribution dependent and that surprised me:

function test<T extends {}>() {
    type T1 = T extends {} ? true : false;
}

The change in this PR actually changes the behavior, it's not only an "optimization". I'm not sure how to write a test case that would actually showcase this change though - the case showed in the referenced issue doesn't pass even with this change (just for other reasons, I described the overall issue in more detail here).

So let's take a look at what happens in isTypeParameterPossiblyReferenced for this very case:

node // true
container // function test<T extends {}>() { type T1 = T extends {} ? true : false; }
n // (1st iter): true
n // (2nd iter): T extends {} ? true : false
n // (3rd iter): type T1 = T extends {} ? true : false;
n // (4th iter): { type T1 = T extends {} ? true : false; }

So we return true from isTypeParameterPossiblyReferenced as we encountered a block - a block that is the function's body.

@Andarist
Copy link
Contributor Author

@sandersn I think this has the wrong status assigned to it right now. It should have "waiting on reviewers" and not "waiting on author".

@sandersn
Copy link
Member

@ahejlsberg Mind giving a second opinion based on @Andarist 's explanation?

…onDependent

# Conflicts:
#	src/compiler/checker.ts
@sandersn
Copy link
Member

It's been a couple of years and this hasn't been high enough priority to reconsider, so I think it's better to close the PR. We can re-open it later (or rewrite it in Go even later) if needed.

@sandersn sandersn closed this Apr 30, 2025
@github-project-automation github-project-automation bot moved this from Waiting on reviewers to Done in PR Backlog Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants