Unify logic in typeof narrowing #33434
Conversation
|
@typescript-bot test this because bots don't get weekends. |
|
Heya @jack-williams, I've started to run the extended test suite on this PR at 8be155d. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @jack-williams, I've started to run the perf test suite on this PR at 8be155d. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
Heya @jack-williams, I've started to run the parallelized community code test suite on this PR at 8be155d. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@jack-williams Here they are:Comparison Report - master..33434
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
User tests look like noise, I think. |
jack-williams
force-pushed
the
refactor-typeof-narrowing
branch
from
8be155d to
17ed7e2
Compare
While this narrowing does match the reality at runtime, I'm not sure it's sound semantically, given that IMO, any construct that allows a value of type declare let callback: () => void = () => true;
declare let knownToBeBool;
declare function foo(b: boolean): void;
let x = Math.random() > 0.5 ? callback() : knownToBeBool;
if (typeof x === 'boolean') {
// 'x' should NOT be narrowed here because it could leak the return value of 'callback'
// (which we've already promised to ignore by declaring it 'void')
foo(x);
} |
|
The Care needs to be taken with empty intersection reduction of void, which I’m not sure is entirely correct right now, but the principle of narrowing void in general does not seem unsound - at least in relation to the typical understanding of soundness. |
|
Quibbles about theoretical details aside (suffice to say there's no practical reason to disallow |
|
In other words: declare function doSomething(callback: () => void);
doSomething(someRandoFunction);Since |
Not related to this issue, but this just got me thinking, how much real-world code would break if this were actually the case... because if that change were made, Function types returning |
|
The
|
|
If the proposed behaviour does not introduce significant real-world breaks I think it could be justified, though I don't know the landscape well enough. The behaviour feels slightly draconian though, given that even unconstrained type parameters can be inspected and narrowed. The priority should be addressing the unsound behaviour that narrows In modern TypeScript with |
|
IMO Put another way, We can think of |
|
To cut through the noise and be clear (and get back on-topic): I only mean to argue that narrowing |
If this really is a desirable and bug-reducing feature I think the correct thing is to introduce non-narrowable type parameters and use generics.
There may be similarities when interpreting the contract of Narrowing The former is clearly unsound under any reasonably definition and should be fixed. The latter is open to interpretation. |
|
The way I see Therefore, Obviously, yes, if I’ll see if I can come up with a practical example later, so we’re not dealing entirely with intangible theory. If so I’ll also open a separate issue for discussion and stop spamming this one. 😄 |
|
Possibly relevant: #32809 |
|
I'll get to merging this in the next week (for anyone planning to review this). |
Will do! |
|
@jack-williams @weswigham should we close this now? I wasn't sure from the discussion whether it's completely obsoleted by #38311 but it sounds like it. |
|
@sandersn The only objective here is to unify the logic of #38311 changes the root type for discriminant narrowing, so I think it solves a different problem. I did have another PR which was correctly replaced. I've merged master and moved things around to reduce the diff, hopefully this makes it clearer. Feel free to close or merge, whatever you see fit. I think we should get this out of the open PR's either way! Tests fail because of n/n-1 with linter in master. |
jack-williams
force-pushed
the
refactor-typeof-narrowing
branch
from
913cd6d to
76e9b26
Compare
In looking to resolve #33180 I want something like
if (typeof x === "boolean") { ... }to narrowxtobooleanwhenxis of typeboolean | void, such behaviour is consistent with empty intersection reduction.Doing so requires changing both
ifandswitchlogic. It also feels more robust moving forward to unify the logic inif (typeof x === "boolean") { ... }andswitch (typeof x) { ... }, so I'm looking to merge this first and then fix thevoidcase---hoping to avoid conflating any regressions.