Pr 50377 redo filter boolean overload #56357

craigphicks · 2023-11-09T22:10:06Z

It turns out a problem with this idea is that BooleanConstructor is not distinguishable from (x:any)=>boolean. Meaning TypeScript doesn't check the interface prototype when the prototype is a TypeScript Symbol for a user variable. I.e., in this case Boolean.

Oddly, it does check the interface prototype when the prototype is a unique ES Symbol.

If that gets sorted out, it might be worth considering.

Fixes #50377 - when Boolean is used a predicate to Array.filter falsie types are filtered in the result.
(However, this version doesn't use an overload in lib/es5.d.ts)

Source Files changed:

src/compiler/checker.ts

Added Test Files

tests/baselines/reference/unionOfArraysBooleanFilterCall.js
tests/cases/compiler/arrayFilterBooleanOverload.ts

Related issues

filter(Boolean)
#50377 Explore adding the Boolean to filter to narrow out null/undefined in array#filter (closed pull)
#50387 Add BooleanConstructor as an overload to .filter to allow for easy type predicate filtering (inDiscussion)
#30621 narrow array type with .filter(Boolean) (marked as duplicate of #16655, although #16655 is more broad than #30621)
#56013 Adding an overload to .filter breaks specific inference with nested functions (related)
#55777

Boolean as falsey check in general
#16655 Boolean() cannot be used to perform a null check (broader than filter(Boolean))
#29955 Allow Boolean() to be used to perform a null check (accepted pull, may have reverted)

#53489 Add fallback logic for generating signatures for unions of array members

Source code changes

checker.ts
In chooseOverload, after result is selected, when the argument is of type BooleanConstructor and the result signature is Array.filter, the result signature is cloned, and its resolvedReturnType property is set.

Regressions

None found with runtests

Discussion

In this revision, es5.d.ts is not modified at all. Instead, the return type is changed dynamically in chooseOverload in checker.ts.
Therefore, the user won't see a completion suggesting Boolean, unlike the last revision. Obviously that is a disadvantage.

The advantage is that there are less changes overall, so the pull is easier to review. (Previously there were ~60 files changes, and that's a lot to review).

fatcerberus · 2023-11-09T22:48:08Z

Adding special magic for this in the compiler internals for what is otherwise a userland method call (that isn’t reflected in the method signature) feels like it opens a can of worms that the TS maintainers may not want to touch…

craigphicks · 2023-11-09T23:02:55Z

Adding special magic for this in the compiler internals for what is otherwise a userland method call (that isn’t reflected in the method signature) feels like it opens a can of worms that the TS maintainers may not want to touch…

I hadn't initially thought of the js standard lib as a userland method. The compiler type processing internals do include tests for the js Array and Readonly array symbols, but not for the methods within those objects - so this would indeed be an outlier there.

fatcerberus · 2023-11-09T23:14:54Z

Yeah, the methods are declared in userland - I don’t think the built-in .d.ts files are treated any differently from user-authored ones in that regard.

I’m not a maintainer, so take what I say with a grain of salt, but this does feel like the kind of thing there’d be significant resistance to, based on past experience (I’ve been lurking around here for many years).

craigphicks · 2023-11-09T23:55:08Z

I believe you.

BTW - here is TypeScript compiler code where Array and Tuple methods are treated specially and rewritten:

            let memberName: __String;
            if (everyType(type, t => !!t.symbol?.parent && isArrayOrTupleSymbol(t.symbol.parent) && (!memberName ? (memberName = t.symbol.escapedName, true) : memberName === t.symbol.escapedName))) {
                // Transform the type from `(A[] | B[])["member"]` to `(A | B)[]["member"]` (since we pretend array is covariant anyway)
                const arrayArg = mapType(type, t => getMappedType((isReadonlyArraySymbol(t.symbol.parent) ? globalReadonlyArrayType : globalArrayType).typeParameters![0], (t as AnonymousType).mapper!));
                const arrayType = createArrayType(arrayArg, someType(type, t => isReadonlyArraySymbol(t.symbol.parent)));
                return (type as UnionType).arrayFallbackSignatures = getSignaturesOfType(getTypeOfPropertyOfType(arrayType, memberName!)!, kind);
            }
            (type as UnionType).arrayFallbackSignatures = result;

It is an example (and I think probably the only example) of the built-in .d.ts files being treated differently from user-authored ones. The result of being forced into a corner, I guess.

jakebailey · 2023-11-10T00:01:29Z

Yes, that was explicitly added in #53489.

@typescript-bot test top200
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot perf test this
@typescript-bot pack this

typescript-bot · 2023-11-10T00:01:32Z

Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 7afbdfa. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-11-10T00:01:32Z

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 7afbdfa. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-11-10T00:01:32Z

Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 7afbdfa. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-11-10T00:01:32Z

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 7afbdfa. You can monitor the build here.

typescript-bot · 2023-11-10T00:01:32Z

Heya @jakebailey, I've started to run the regular perf test suite on this PR at 7afbdfa. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-11-10T00:03:55Z

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/158543/artifacts?artifactName=tgz&fileId=E14CF5EE3A7D5B0DFF2D7B26E5C27FC04EBE738879CE4B753E3735E9CC233E5502&fileName=/typescript-5.4.0-insiders.20231110.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

typescript-bot · 2023-11-10T00:20:39Z

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/56357/merge:

There were infrastructure failures potentially unrelated to your change:

3 instances of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

webpack

tsconfig.types.json

[MISSING] error TS2345: Argument of type 'Comparator<Module>' is not assignable to parameter of type '(a: Module | null, b: Module | null) => number'.
- lib/ContextModule.js#L593
[MISSING] error TS18047: 'module' is possibly 'null'.
- lib/ContextModule.js#L596
[MISSING] error TS2345: Argument of type 'Module | null' is not assignable to parameter of type 'Module'.
- lib/ContextModule.js#L600

typescript-bot · 2023-11-10T00:24:02Z

@jakebailey
The results of the perf run you requested are in!

Here they are:

Compiler

Comparison Report - baseline..pr

_Metric	_baseline	_pr	_Delta	_Best	_Worst	_p-value
_{Angular - node (v18.15.0, x64)}
_{Memory used}	_{295,173k (± 0.01%)}	_{295,124k (± 0.01%)}	_{-49k (- 0.02%)}	_295,092k	_295,164k	_{p=0.037 n=6}
_{Parse Time}	_{2.63s (± 0.39%)}	_{2.62s (± 0.45%)}	_~	_2.60s	_2.63s	_{p=0.056 n=6}
_{Bind Time}	_{0.84s (± 1.23%)}	_{0.83s (± 1.00%)}	_~	_0.83s	_0.85s	_{p=0.923 n=6}
_{Check Time}	_{8.04s (± 0.32%)}	_{8.04s (± 0.19%)}	_~	_8.03s	_8.07s	_{p=0.808 n=6}
_{Emit Time}	_{7.09s (± 0.48%)}	_{7.07s (± 0.23%)}	_~	_7.06s	_7.10s	_{p=0.624 n=6}
_{Total Time}	_{18.60s (± 0.24%)}	_{18.57s (± 0.12%)}	_~	_18.55s	_18.61s	_{p=0.332 n=6}
_{Compiler-Unions - node (v18.15.0, x64)}
_{Memory used}	_{192,094k (± 1.24%)}	_{192,676k (± 1.58%)}	_~	_190,666k	_196,709k	_{p=0.378 n=6}
_{Parse Time}	_{1.36s (± 1.46%)}	_{1.35s (± 0.76%)}	_~	_1.34s	_1.37s	_{p=0.561 n=6}
_{Bind Time}	_{0.73s (± 0.00%)}	_{0.73s (± 0.56%)}	_~	_0.73s	_0.74s	_{p=0.405 n=6}
_{Check Time}	_{9.15s (± 0.23%)}	_{9.16s (± 0.24%)}	_~	_9.13s	_9.19s	_{p=0.627 n=6}
_{Emit Time}	_{2.62s (± 0.62%)}	_{2.63s (± 0.57%)}	_~	_2.61s	_2.65s	_{p=0.366 n=6}
_{Total Time}	_{13.86s (± 0.20%)}	_{13.88s (± 0.15%)}	_~	_13.86s	_13.91s	_{p=0.168 n=6}
_{Monaco - node (v18.15.0, x64)}
_{Memory used}	_{347,354k (± 0.01%)}	_{347,363k (± 0.00%)}	_~	_347,355k	_347,378k	_{p=0.688 n=6}
_{Parse Time}	_{2.45s (± 0.36%)}	_{2.46s (± 0.31%)}	_~	_2.45s	_2.47s	_{p=0.053 n=6}
_{Bind Time}	_{0.94s (± 0.00%)}	_{0.94s (± 0.43%)}	_~	_0.94s	_0.95s	_{p=0.405 n=6}
_{Check Time}	_{6.93s (± 0.67%)}	_{6.94s (± 0.38%)}	_~	_6.91s	_6.98s	_{p=0.629 n=6}
_{Emit Time}	_{4.05s (± 0.53%)}	_{4.05s (± 0.46%)}	_~	_4.03s	_4.08s	_{p=0.513 n=6}
_{Total Time}	_{14.38s (± 0.35%)}	_{14.40s (± 0.22%)}	_~	_14.35s	_14.45s	_{p=0.288 n=6}
_{TFS - node (v18.15.0, x64)}
_{Memory used}	_{302,608k (± 0.01%)}	_{302,607k (± 0.01%)}	_~	_302,579k	_302,674k	_{p=0.688 n=6}
_{Parse Time}	_{2.01s (± 0.80%)}	_{1.99s (± 0.38%)}	_~	_1.98s	_2.00s	_{p=0.071 n=6}
_{Bind Time}	_{1.01s (± 0.88%)}	_{1.01s (± 0.97%)}	_~	_1.00s	_1.02s	_{p=0.798 n=6}
_{Check Time}	_{6.25s (± 0.30%)}	_{6.26s (± 0.48%)}	_~	_6.22s	_6.29s	_{p=0.747 n=6}
_{Emit Time}	_{3.59s (± 0.49%)}	_{3.59s (± 0.50%)}	_~	_3.57s	_3.62s	_{p=0.806 n=6}
_{Total Time}	_{12.86s (± 0.36%)}	_{12.86s (± 0.23%)}	_~	_12.82s	_12.91s	_{p=0.294 n=6}
_{material-ui - node (v18.15.0, x64)}
_{Memory used}	_{470,573k (± 0.01%)}	_{470,593k (± 0.02%)}	_~	_470,527k	_470,750k	_{p=0.936 n=6}
_{Parse Time}	_{2.57s (± 0.20%)}	_{2.57s (± 0.41%)}	_~	_2.55s	_2.58s	_{p=0.794 n=6}
_{Bind Time}	_{0.99s (± 0.76%)}	_{1.00s (± 1.04%)}	_~	_0.98s	_1.01s	_{p=0.351 n=6}
_{Check Time}	_{16.70s (± 0.41%)}	_{16.65s (± 0.59%)}	_~	_16.54s	_16.79s	_{p=0.230 n=6}
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_~	_0.00s	_0.00s	_{p=1.000 n=6}
_{Total Time}	_{20.26s (± 0.34%)}	_{20.21s (± 0.52%)}	_~	_20.11s	_20.36s	_{p=0.298 n=6}
_{xstate - node (v18.15.0, x64)}
_{Memory used}	_{512,822k (± 0.01%)}	_{512,819k (± 0.01%)}	_~	_512,753k	_512,896k	_{p=0.936 n=6}
_{Parse Time}	_{3.28s (± 0.17%)}	_{3.28s (± 0.17%)}	_~	_3.27s	_3.28s	_{p=1.000 n=6}
_{Bind Time}	_{1.54s (± 0.68%)}	_{1.55s (± 0.54%)}	_~	_1.53s	_1.55s	_{p=1.000 n=6}
_{Check Time}	_{2.85s (± 0.34%)}	_{2.88s (± 0.14%)}	_{+0.03s (+ 1.05%)}	_2.87s	_2.88s	_{p=0.003 n=6}
_{Emit Time}	_{0.08s (± 0.00%)}	_{0.07s (± 5.69%)}	_{🟩-0.01s (-10.42%)}	_0.07s	_0.08s	_{p=0.007 n=6}
_{Total Time}	_{7.75s (± 0.13%)}	_{7.77s (± 0.15%)}	_{+0.02s (+ 0.32%)}	_7.76s	_7.79s	_{p=0.007 n=6}

System info unknown

Hosts

node (v18.15.0, x64)

Scenarios

Angular - node (v18.15.0, x64)
Compiler-Unions - node (v18.15.0, x64)
Monaco - node (v18.15.0, x64)
TFS - node (v18.15.0, x64)
material-ui - node (v18.15.0, x64)
xstate - node (v18.15.0, x64)

_Benchmark	_Name	_Iterations
_Current	_pr	₆
_Baseline	_baseline	₆

tsserver

Comparison Report - baseline..pr

_Metric	_baseline	_pr	_Delta	_Best	_Worst	_p-value
_{Compiler-UnionsTSServer - node (v18.15.0, x64)}
_{Req 1 - updateOpen}	_{2,399ms (± 0.75%)}	_{2,390ms (± 0.79%)}	_~	_2,366ms	_2,412ms	_{p=0.378 n=6}
_{Req 2 - geterr}	_{5,349ms (± 1.13%)}	_{5,366ms (± 1.38%)}	_~	_5,301ms	_5,478ms	_{p=0.936 n=6}
_{Req 3 - references}	_{326ms (± 1.44%)}	_{326ms (± 0.93%)}	_~	_322ms	_329ms	_{p=1.000 n=6}
_{Req 4 - navto}	_{279ms (± 0.98%)}	_{277ms (± 1.24%)}	_~	_273ms	_281ms	_{p=0.448 n=6}
_{Req 5 - completionInfo count}	_{1,356 (± 0.00%)}	_{1,356 (± 0.00%)}	_~	_1,356	_1,356	_{p=1.000 n=6}
_{Req 5 - completionInfo}	_{82ms (± 4.58%)}	_{83ms (± 7.10%)}	_~	_75ms	_90ms	_{p=0.787 n=6}
_{CompilerTSServer - node (v18.15.0, x64)}
_{Req 1 - updateOpen}	_{2,501ms (± 0.54%)}	_{2,495ms (± 1.14%)}	_~	_2,462ms	_2,525ms	_{p=1.000 n=6}
_{Req 2 - geterr}	_{4,082ms (± 2.13%)}	_{4,078ms (± 1.84%)}	_~	_4,020ms	_4,178ms	_{p=0.936 n=6}
_{Req 3 - references}	_{341ms (± 1.69%)}	_{340ms (± 1.76%)}	_~	_333ms	_347ms	_{p=0.686 n=6}
_{Req 4 - navto}	_{282ms (± 0.27%)}	_{282ms (± 0.39%)}	_~	_280ms	_283ms	_{p=1.000 n=6}
_{Req 5 - completionInfo count}	_{1,518 (± 0.00%)}	_{1,518 (± 0.00%)}	_~	_1,518	_1,518	_{p=1.000 n=6}
_{Req 5 - completionInfo}	_{86ms (± 7.20%)}	_{86ms (± 6.77%)}	_~	_78ms	_90ms	_{p=0.928 n=6}
_{xstateTSServer - node (v18.15.0, x64)}
_{Req 1 - updateOpen}	_{2,606ms (± 0.56%)}	_{2,602ms (± 0.39%)}	_~	_2,592ms	_2,618ms	_{p=0.688 n=6}
_{Req 2 - geterr}	_{1,733ms (± 2.08%)}	_{1,707ms (± 1.71%)}	_~	_1,673ms	_1,742ms	_{p=0.128 n=6}
_{Req 3 - references}	_{107ms (± 6.22%)}	_{112ms (± 9.94%)}	_~	_102ms	_123ms	_{p=0.686 n=6}
_{Req 4 - navto}	_{365ms (± 0.97%)}	_{366ms (± 0.75%)}	_~	_363ms	_370ms	_{p=0.936 n=6}
_{Req 5 - completionInfo count}	_{2,073 (± 0.00%)}	_{2,073 (± 0.00%)}	_~	_2,073	_2,073	_{p=1.000 n=6}
_{Req 5 - completionInfo}	_{313ms (± 1.98%)}	_{306ms (± 1.93%)}	_~	_301ms	_316ms	_{p=0.123 n=6}

System info unknown

Hosts

node (v18.15.0, x64)

Scenarios

CompilerTSServer - node (v18.15.0, x64)
Compiler-UnionsTSServer - node (v18.15.0, x64)
xstateTSServer - node (v18.15.0, x64)

_Benchmark	_Name	_Iterations
_Current	_pr	₆
_Baseline	_baseline	₆

Startup

Comparison Report - baseline..pr

_Metric	_baseline	_pr	_Delta	_Best	_Worst	_p-value
_{tsc-startup - node (v18.15.0, x64)}
_{Execution time}	_{152.84ms (± 0.18%)}	_{152.77ms (± 0.17%)}	_{-0.06ms (- 0.04%)}	_151.65ms	_154.60ms	_{p=0.042 n=600}
_{tsserver-startup - node (v18.15.0, x64)}
_{Execution time}	_{228.13ms (± 0.15%)}	_{228.03ms (± 0.16%)}	_{-0.10ms (- 0.04%)}	_226.94ms	_233.16ms	_{p=0.000 n=600}
_{tsserverlibrary-startup - node (v18.15.0, x64)}
_{Execution time}	_{229.50ms (± 0.17%)}	_{229.51ms (± 0.17%)}	_~	_228.05ms	_234.84ms	_{p=0.636 n=600}
_{typescript-startup - node (v18.15.0, x64)}
_{Execution time}	_{228.77ms (± 0.17%)}	_{228.82ms (± 0.17%)}	_~	_227.52ms	_235.08ms	_{p=0.255 n=600}

System info unknown

Hosts

node (v18.15.0, x64)

Scenarios

tsc-startup - node (v18.15.0, x64)
tsserver-startup - node (v18.15.0, x64)
tsserverlibrary-startup - node (v18.15.0, x64)
typescript-startup - node (v18.15.0, x64)

_Benchmark	_Name	_Iterations
_Current	_pr	₆
_Baseline	_baseline	₆

Developer Information:

Download Benchmarks

fatcerberus · 2023-11-10T00:27:15Z

Emit time improved 10+% from this? That has to be noise.

jakebailey · 2023-11-10T00:28:32Z

It is, note the time granularity is only 0.01s and that test has enough problems that it doesn't emit and so runs in 0.08.

typescript-bot · 2023-11-10T00:52:45Z

Hey @jakebailey, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

typescript-bot · 2023-11-10T02:27:56Z

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/56357/merge:

Everything looks good!

craigphicks · 2023-11-14T21:03:22Z

It turns out a problem with this idea is that BooleanConstructor is not distinguishable from (x:any)=>boolean. Meaning TypeScript doesn't check the interface prototype when the prototype is a TypeScript Symbol for a user variable. I.e., in this case Boolean.

Oddly, it does check the interface prototype when the prototype is a unique ES Symbol.

If that gets sorted out, it might be worth considering.

original checker and new tests

79f69d5

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 9, 2023

craigphicks force-pushed the PR-50377-redo-filterBooleanOverload branch from 6fb5fd3 to f56e8f0 Compare November 9, 2023 22:31

new checker.ts and updated test results

e1a72ff

craigphicks force-pushed the PR-50377-redo-filterBooleanOverload branch from f56e8f0 to e1a72ff Compare November 9, 2023 22:47

add ? after symbol

7afbdfa

craigphicks marked this pull request as draft November 11, 2023 16:03

craigphicks closed this Nov 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pr 50377 redo filter boolean overload #56357

Pr 50377 redo filter boolean overload #56357

craigphicks commented Nov 9, 2023 •

edited

Loading

fatcerberus commented Nov 9, 2023

craigphicks commented Nov 9, 2023 •

edited

Loading

fatcerberus commented Nov 9, 2023

craigphicks commented Nov 9, 2023

jakebailey commented Nov 10, 2023

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023

webpack

tsconfig.types.json

typescript-bot commented Nov 10, 2023

Compiler

tsserver

Startup

fatcerberus commented Nov 10, 2023

jakebailey commented Nov 10, 2023

typescript-bot commented Nov 10, 2023

typescript-bot commented Nov 10, 2023

craigphicks commented Nov 14, 2023

Pr 50377 redo filter boolean overload #56357

Pr 50377 redo filter boolean overload #56357

Conversation

craigphicks commented Nov 9, 2023 • edited Loading

Source Files changed:

Added Test Files

Related issues

Source code changes

Regressions

Discussion

fatcerberus commented Nov 9, 2023

craigphicks commented Nov 9, 2023 • edited Loading

fatcerberus commented Nov 9, 2023

craigphicks commented Nov 9, 2023

jakebailey commented Nov 10, 2023

typescript-bot commented Nov 10, 2023 • edited Loading

typescript-bot commented Nov 10, 2023 • edited Loading

typescript-bot commented Nov 10, 2023 • edited Loading

typescript-bot commented Nov 10, 2023 • edited Loading

typescript-bot commented Nov 10, 2023 • edited Loading

typescript-bot commented Nov 10, 2023 • edited Loading

typescript-bot commented Nov 10, 2023

webpack

tsconfig.types.json

typescript-bot commented Nov 10, 2023

Compiler

tsserver

Startup

fatcerberus commented Nov 10, 2023

jakebailey commented Nov 10, 2023

typescript-bot commented Nov 10, 2023

typescript-bot commented Nov 10, 2023

craigphicks commented Nov 14, 2023

craigphicks commented Nov 9, 2023 •

edited

Loading

craigphicks commented Nov 9, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading

typescript-bot commented Nov 10, 2023 •

edited

Loading