-
Notifications
You must be signed in to change notification settings - Fork 13k
Fix CFA for with generic T (#62133) #62151
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 CFA for with generic T (#62133) #62151
Conversation
@microsoft-github-policy-service agree |
@typescript-bot test it |
Hey @RyanCavanaugh, the results of running the DT tests are ready. Everything looks the same! |
@RyanCavanaugh Here are the results of running the user tests with tsc comparing Everything looks good! |
@RyanCavanaugh Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@RyanCavanaugh Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Will my PR be merged.? 😅 |
What's the non-degenerate repro where this matters? |
I pushed commit that removes the new test file and instead adds a minimal, non-degenerate test into an existing file:
// Generic type parameter keeps falsy constituents (#62133)
declare function id<T>(x: T): T;
function f<T extends string | null>(x: T) {
const y = x && x; // should be T, not NonNullable<T>
return y;
}
const t1 = f("a"); // "a"
const t2 = f(null); // null (was incorrectly never) The baseline diff now shows only the expected new lines, and the |
There's no reason to write |
Sorry for the confusion...😅 What the old code did if (type.flags & TypeFlags.TypeParameter) {
return neverType; // “the falsy part of T is nothing”
} Result: in control-flow analysis the expression What the new code does if (type.flags & TypeFlags.TypeParameter) {
const constraint = getBaseConstraintOfType(type);
if (!constraint || constraint === type) return neverType;
return getDefinitelyFalsyPartOfType(constraint);
}
|
What user code is affected by this in a situation where the code makes sense in the first place? How can I observe if this PR is present or not without writing nonsense code like If there's not an answer to that question, I don't think we should merge this -- code changes need to be well-motivated by actual examples. |
After the discussion above I agree the real-world impact is too small to justify the extra complexity. I’m closing the PR—thanks for the thorough review! |
Fixes
Fixes #62133 –
x && x
should not narrow genericT
toNonNullable<T>
.What changed
getDefinitelyFalsyPartOfType
insrc/compiler/checker.ts
TypeParameter
that consults the type’s constraintNew behaviour