Remove incorrect handling of intersections in getStringMappingType #53383

jakebailey · 2023-03-20T16:13:14Z

This line is no longer needed; I believe it's a relic of an earlier version of #52836 which more eagerly evaluated these intersections, but that's not what the final intent of the PR was.

jakebailey · 2023-03-20T16:15:58Z

tests/baselines/reference/templateLiteralIntersection3.types

@@ -29,12 +29,12 @@ options1[`foo/${path}`] = false;

 // Lowercase<`foo/${Path}`> => `foo/${Lowercase<Path>}`
 declare const lowercasePath: Lowercase<`foo/${Path}`>;
->lowercasePath : `foo/${Lowercase<string> & { _pathBrand: any; }}`


I believe this was wrong in #52836; the intersection here should not be getting lifted out of the string mapping.

jakebailey · 2023-03-20T16:59:42Z

@typescript-bot test this
@typescript-bot test top100
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this faster

typescript-bot · 2023-03-20T17:00:05Z

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

Update: The results are in!

typescript-bot · 2023-03-20T17:00:05Z

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

typescript-bot · 2023-03-20T17:00:05Z

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

Update: The results are in!

typescript-bot · 2023-03-20T17:00:05Z

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

Update: The results are in!

typescript-bot · 2023-03-20T17:00:05Z

Heya @jakebailey, I've started to run the extended test suite on this PR at 8a6843f. You can monitor the build here.

typescript-bot · 2023-03-20T17:15:39Z

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

Everything looks good!

typescript-bot · 2023-03-20T17:15:44Z

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

Here they are:

Comparison Report - main..53383

_Metric	_main	₅₃₃₈₃	_Delta	_Best	_Worst	_p-value
_{Angular - node (v16.17.1, x64)}
_{Memory used}	_{361,045k (± 0.01%)}	_{361,057k (± 0.01%)}	_~	_361,017k	_361,092k	_{p=0.630 n=6}
_{Parse Time}	_{3.53s (± 0.49%)}	_{3.52s (± 0.38%)}	_~	_3.50s	_3.54s	_{p=0.210 n=6}
_{Bind Time}	_{1.18s (± 0.64%)}	_{1.19s (± 0.43%)}	_~	_1.18s	_1.19s	_{p=0.247 n=6}
_{Check Time}	_{9.45s (± 0.29%)}	_{9.42s (± 0.24%)}	_~	_9.39s	_9.45s	_{p=0.089 n=6}
_{Emit Time}	_{7.93s (± 0.54%)}	_{7.92s (± 0.98%)}	_~	_7.85s	_8.04s	_{p=0.688 n=6}
_{Total Time}	_{22.10s (± 0.22%)}	_{22.05s (± 0.33%)}	_~	_21.98s	_22.15s	_{p=0.228 n=6}
_{Compiler-Unions - node (v16.17.1, x64)}
_{Memory used}	_{193,088k (± 0.73%)}	_{193,081k (± 0.67%)}	_~	_192,496k	_195,739k	_{p=0.471 n=6}
_{Parse Time}	_{1.58s (± 1.60%)}	_{1.57s (± 1.42%)}	_~	_1.54s	_1.60s	_{p=0.512 n=6}
_{Bind Time}	_{0.82s (± 0.50%)}	_{0.82s (± 1.33%)}	_~	_0.81s	_0.84s	_{p=0.445 n=6}
_{Check Time}	_{10.08s (± 0.57%)}	_{10.09s (± 0.58%)}	_~	_9.99s	_10.16s	_{p=0.810 n=6}
_{Emit Time}	_{2.98s (± 0.86%)}	_{3.00s (± 0.88%)}	_~	_2.96s	_3.04s	_{p=0.292 n=6}
_{Total Time}	_{15.46s (± 0.45%)}	_{15.48s (± 0.53%)}	_~	_15.33s	_15.57s	_{p=0.687 n=6}
_{Monaco - node (v16.17.1, x64)}
_{Memory used}	_{345,573k (± 0.01%)}	_{345,573k (± 0.00%)}	_~	_345,547k	_345,601k	_{p=1.000 n=6}
_{Parse Time}	_{2.73s (± 0.38%)}	_{2.73s (± 0.46%)}	_~	_2.72s	_2.75s	_{p=0.615 n=6}
_{Bind Time}	_{1.08s (± 0.50%)}	_{1.09s (± 0.69%)}	_~	_1.08s	_1.10s	_{p=0.137 n=6}
_{Check Time}	_{7.70s (± 0.40%)}	_{7.70s (± 0.36%)}	_~	_7.67s	_7.75s	_{p=0.807 n=6}
_{Emit Time}	_{4.48s (± 0.73%)}	_{4.47s (± 1.01%)}	_~	_4.43s	_4.56s	_{p=0.624 n=6}
_{Total Time}	_{15.99s (± 0.40%)}	_{16.00s (± 0.42%)}	_~	_15.90s	_16.09s	_{p=1.000 n=6}
_{TFS - node (v16.17.1, x64)}
_{Memory used}	_{299,811k (± 0.01%)}	_{299,807k (± 0.01%)}	_~	_299,759k	_299,866k	_{p=0.810 n=6}
_{Parse Time}	_{2.18s (± 0.82%)}	_{2.17s (± 0.97%)}	_~	_2.15s	_2.21s	_{p=0.250 n=6}
_{Bind Time}	_{1.24s (± 0.66%)}	_{1.24s (± 0.51%)}	_~	_1.23s	_1.25s	_{p=0.432 n=6}
_{Check Time}	_{7.16s (± 0.29%)}	_{7.17s (± 0.33%)}	_~	_7.13s	_7.19s	_{p=0.627 n=6}
_{Emit Time}	_{4.34s (± 0.55%)}	_{4.36s (± 1.04%)}	_~	_4.31s	_4.43s	_{p=0.627 n=6}
_{Total Time}	_{14.92s (± 0.32%)}	_{14.94s (± 0.34%)}	_~	_14.86s	_14.99s	_{p=0.746 n=6}
_{material-ui - node (v16.17.1, x64)}
_{Memory used}	_{476,373k (± 0.00%)}	_{476,382k (± 0.00%)}	_~	_476,358k	_476,407k	_{p=0.422 n=6}
_{Parse Time}	_{3.22s (± 0.20%)}	_{3.22s (± 0.52%)}	_~	_3.20s	_3.24s	_{p=0.801 n=6}
_{Bind Time}	_{0.96s (± 0.43%)}	_{0.95s (± 0.54%)}	_~	_0.95s	_0.96s	_{p=0.112 n=6}
_{Check Time}	_{17.93s (± 0.35%)}	_{17.87s (± 0.23%)}	_~	_17.82s	_17.91s	_{p=0.108 n=6}
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_~	_0.00s	_0.00s	_{p=1.000 n=6}
_{Total Time}	_{22.10s (± 0.31%)}	_{22.04s (± 0.23%)}	_~	_21.99s	_22.10s	_{p=0.107 n=6}
_{xstate - node (v16.17.1, x64)}
_{Memory used}	_{550,626k (± 0.01%)}	_{550,638k (± 0.01%)}	_~	_550,583k	_550,757k	_{p=1.000 n=6}
_{Parse Time}	_{3.94s (± 0.30%)}	_{3.95s (± 0.27%)}	_~	_3.93s	_3.96s	_{p=0.419 n=6}
_{Bind Time}	_{1.80s (± 0.29%)}	_{1.79s (± 0.96%)}	_~	_1.77s	_1.82s	_{p=0.352 n=6}
_{Check Time}	_{3.04s (± 0.92%)}	_{3.04s (± 0.66%)}	_~	_3.01s	_3.07s	_{p=1.000 n=6}
_{Emit Time}	_{0.09s (± 0.00%)}	_{0.09s (± 0.00%)}	_~	_0.09s	_0.09s	_{p=1.000 n=6}
_{Total Time}	_{8.86s (± 0.41%)}	_{8.87s (± 0.38%)}	_~	_8.82s	_8.92s	_{p=0.687 n=6}

System

_{Machine Name}	_ts-ci-ubuntu
_Platform	_{linux 5.4.0-135-generic}
_Architecture	_x64
_{Available Memory}	_{16 GB}
_{Available Memory}	_{15 GB}
_CPUs	_{4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz}

Hosts

node (v16.17.1, x64)

Scenarios

Angular - node (v16.17.1, x64)
Compiler-Unions - node (v16.17.1, x64)
Monaco - node (v16.17.1, x64)
TFS - node (v16.17.1, x64)
material-ui - node (v16.17.1, x64)
xstate - node (v16.17.1, x64)

_Benchmark	_Name	_Iterations
_Current	₅₃₃₈₃	₆
_Baseline	_main	₆

Developer Information:

Download Benchmark

jakebailey · 2023-03-20T18:01:23Z

Reading the DT logs ahead of time, it hit another npm timeout so this is going to fail to show that the error is fixed, unfortunately, unless I run it again.

jakebailey · 2023-03-20T18:36:52Z

@typescript-bot run dt

typescript-bot · 2023-03-20T18:36:54Z

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

Update: The results are in!

typescript-bot · 2023-03-20T19:13:29Z

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

Everything looks good!

typescript-bot · 2023-03-20T20:41:09Z

Hey @jakebailey, the results of running the DT tests are ready.
There were interesting changes:

Main only errors:

Package: fragmented-store
Error:

Error: /home/vsts/work/1/s/DefinitelyTyped-tools/packages/dtslint-runner/DefinitelyTyped/types/fragmented-store/fragmented-store-tests.tsx:4:19
ERROR: 4:19   expect  TypeScript@local compile error: 
Property 'useUsername' does not exist on type 'StoreUtils<{ username: string; age: number; }>'.
ERROR: 4:32   expect  TypeScript@local compile error: 
Property 'useAge' does not exist on type 'StoreUtils<{ username: string; age: number; }>'.
ERROR: 30:41  expect  TypeScript@local compile error: 
Parameter 's' implicitly has an 'any' type.
ERROR: 42:38  expect  TypeScript@local compile error: 
Parameter 's' implicitly has an 'any' type.

    at testTypesVersion (/home/vsts/work/1/s/DefinitelyTyped-tools/packages/dtslint-runner/node_modules/@definitelytyped/dtslint/dist/index.js:194:15)
    at async runTests (/home/vsts/work/1/s/DefinitelyTyped-tools/packages/dtslint-runner/node_modules/@definitelytyped/dtslint/dist/index.js:151:9)

You can check the log here.

weswigham · 2023-03-20T20:43:43Z

Removes the intended main-only errors, nice.

jakebailey · 2023-03-20T20:44:51Z

Glad to know that the infra works! 😄

jakebailey added 2 commits March 20, 2023 09:03

Add bad test

7a744f7

Remove unneeded handling of intersections in getStringMappingType

8a6843f

jakebailey requested review from sandersn and weswigham March 20, 2023 16:13

typescript-bot assigned jakebailey Mar 20, 2023

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Mar 20, 2023

jakebailey commented Mar 20, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

weswigham approved these changes Mar 20, 2023

View reviewed changes

jakebailey changed the title ~~Remove unneeded handling of intersections in getStringMappingType~~ Remove incorrect handling of intersections in getStringMappingType Mar 20, 2023

jakebailey merged commit 3ba3ace into microsoft:main Mar 20, 2023

jakebailey deleted the fix-53382 branch March 20, 2023 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove incorrect handling of intersections in getStringMappingType #53383

Remove incorrect handling of intersections in getStringMappingType #53383

jakebailey commented Mar 20, 2023

jakebailey Mar 20, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

jakebailey commented Mar 20, 2023

typescript-bot commented Mar 20, 2023 •

edited

Loading

typescript-bot commented Mar 20, 2023 •

edited

Loading

typescript-bot commented Mar 20, 2023 •

edited

Loading

typescript-bot commented Mar 20, 2023 •

edited

Loading

typescript-bot commented Mar 20, 2023 •

edited

Loading

typescript-bot commented Mar 20, 2023

typescript-bot commented Mar 20, 2023

jakebailey commented Mar 20, 2023

jakebailey commented Mar 20, 2023

typescript-bot commented Mar 20, 2023 •

edited

Loading

typescript-bot commented Mar 20, 2023

typescript-bot commented Mar 20, 2023

weswigham commented Mar 20, 2023 •

edited

Loading

jakebailey commented Mar 20, 2023

Remove incorrect handling of intersections in getStringMappingType #53383

Remove incorrect handling of intersections in getStringMappingType #53383

Conversation

jakebailey commented Mar 20, 2023

jakebailey Mar 20, 2023

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

jakebailey commented Mar 20, 2023

typescript-bot commented Mar 20, 2023 • edited Loading

typescript-bot commented Mar 20, 2023 • edited Loading

typescript-bot commented Mar 20, 2023 • edited Loading

typescript-bot commented Mar 20, 2023 • edited Loading

typescript-bot commented Mar 20, 2023 • edited Loading

typescript-bot commented Mar 20, 2023

typescript-bot commented Mar 20, 2023

jakebailey commented Mar 20, 2023

jakebailey commented Mar 20, 2023

typescript-bot commented Mar 20, 2023 • edited Loading

typescript-bot commented Mar 20, 2023

typescript-bot commented Mar 20, 2023

weswigham commented Mar 20, 2023 • edited Loading

jakebailey commented Mar 20, 2023

typescript-bot commented Mar 20, 2023 •

edited

Loading

typescript-bot commented Mar 20, 2023 •

edited

Loading

typescript-bot commented Mar 20, 2023 •

edited

Loading

typescript-bot commented Mar 20, 2023 •

edited

Loading

typescript-bot commented Mar 20, 2023 •

edited

Loading

typescript-bot commented Mar 20, 2023 •

edited

Loading

weswigham commented Mar 20, 2023 •

edited

Loading