Fix CFA for with generic T (#62133)#62151
Fix CFA for with generic T (#62133)#62151kaushik-i-b wants to merge 8 commits intomicrosoft:mainfrom
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
@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! |
I have written some real code in https://github.com/zeng-y-l/xunhuan like this: type Maybe<Absent extends undefined> = { f: Absent | (() => void) }
const make = <Absent extends undefined>(f: Absent | (() => void)): Maybe<Absent> => ({ f })
const twice = <Absent extends undefined>({ f }: Maybe<Absent>): Maybe<Absent> => make(f && (() => { f(); f() }))Due to #62133 and #62248, By the way, I think |
|
Reopening the PR with reference to the above comment. |
kaushik-i-b
force-pushed
the
fix/62133-and-and-generic-
branch
from
5768bb7 to
3ee0907
Compare
|
As @zeng-y-l mentioned in the above example, I think we have enough motivation to fix the code. @RyanCavanaugh, please help me merge the code. |
kaushik-i-b
force-pushed
the
fix/62133-and-and-generic-
branch
from
5ed4476 to
b28c126
Compare
|
@typescript-bot pack this |
|
Hey @RyanCavanaugh, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
This PR doesn't make the code legal either. What's an example of code this PR fully addresses? |
4 participants
Fixes
Fixes #62133 –
x && xshould not narrow genericTtoNonNullable<T>.What changed
getDefinitelyFalsyPartOfTypeinsrc/compiler/checker.tsTypeParameterthat consults the type’s constraintNew behaviour