Ensure rest type for source parameter is readonly in relations #53258

jakebailey · 2023-03-14T21:41:12Z

Fixes #53255
See also DefinitelyTyped/DefinitelyTyped#64739

Given:

declare function each<T extends ReadonlyArray<any>>(cases: ReadonlyArray<T>): (fn: (...args: T) => any) => void;

const cases = [
    [1, '1'],
    [2, '2'],
] as const;

const eacher = each(cases);

// Error.
eacher((a, b) => {
});

// No error?
eacher((...args) => {
    const [a, b] = args;
});

We used to say:

Argument of type '(a: 1 | 2, b: "1" | "2") => void' is not assignable to parameter of type '(...args: readonly [1, "1"] | readonly [2, "2"]) => any'.
  Types of parameters 'a' and 'args' are incompatible.
    Type 'readonly [1, "1"] | readonly [2, "2"]' is not assignable to type '[a: 1 | 2, b: "1" | "2"]'.
      The type 'readonly [1, "1"]' is 'readonly' and cannot be assigned to the mutable type '[a: 1 | 2, b: "1" | "2"]'.(2345)

Here, the "source" type is [a: 1 | 2, b: "1" | "2"], but that can't be assigned to the target as the target is readonly. But, these are the rest arguments, so there's no way for this function to observe the read-only ness of the parameter. (Note: these are parameters, so directionality is swapped.)

I'm not sure if this is a perfect fix, but one way to "solve" this is to just ensure the tuple/array we cook up for the source is readonly, that way it's always compatible. This fixes the issue, but does change some error messages because they do observe the intermediary type here.

Maybe the "better" thing to do here would be to strip readonly-ness from both sourceType and targetType if they were produced via getRestTypeAtPosition? I haven't exactly found "the way" to do that yet, e.g. no helper named "remove readonly from tuple or array".

jakebailey · 2023-03-14T21:41:30Z

@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 pack this

typescript-bot · 2023-03-14T21:41:34Z

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

Update: The results are in!

typescript-bot · 2023-03-14T21:41:34Z

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

Update: The results are in!

typescript-bot · 2023-03-14T21:41:34Z

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

Update: The results are in!

typescript-bot · 2023-03-14T21:41:34Z

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

typescript-bot · 2023-03-14T21:41:34Z

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

Update: The results are in!

typescript-bot · 2023-03-14T21:41:34Z

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

typescript-bot · 2023-03-14T21:43:12Z

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/149439/artifacts?artifactName=tgz&fileId=E33B7102478AE1E2DBEF19B4743FF90C23816DB6BF2A8FC73E70ED7940D0E74302&fileName=/typescript-5.1.0-insiders.20230314.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-03-14T21:55:18Z

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

Everything looks good!

typescript-bot · 2023-03-14T22:30:47Z

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

Here they are:

Comparison Report - main..53258

_Metric	_main	₅₃₂₅₈	_Delta	_Best	_Worst	_p-value
_{Angular - node (v16.17.1, x64)}
_{Memory used}	_{361,467k (± 0.01%)}	_{361,469k (± 0.01%)}	_~	_361,424k	_361,494k	_{p=0.687 n=6}
_{Parse Time}	_{3.52s (± 0.30%)}	_{3.51s (± 0.46%)}	_~	_3.49s	_3.53s	_{p=0.250 n=6}
_{Bind Time}	_{1.17s (± 1.17%)}	_{1.18s (± 0.54%)}	_~	_1.17s	_1.19s	_{p=0.654 n=6}
_{Check Time}	_{9.48s (± 0.68%)}	_{9.44s (± 0.31%)}	_~	_9.41s	_9.48s	_{p=0.227 n=6}
_{Emit Time}	_{7.96s (± 1.22%)}	_{7.93s (± 0.61%)}	_~	_7.89s	_8.02s	_{p=0.935 n=6}
_{Total Time}	_{22.14s (± 0.58%)}	_{22.06s (± 0.22%)}	_~	_21.99s	_22.12s	_{p=0.296 n=6}
_{Compiler-Unions - node (v16.17.1, x64)}
_{Memory used}	_{194,247k (± 0.96%)}	_{192,516k (± 0.07%)}	_~	_192,282k	_192,632k	_{p=0.173 n=6}
_{Parse Time}	_{1.58s (± 1.91%)}	_{1.57s (± 0.87%)}	_~	_1.55s	_1.59s	_{p=0.461 n=6}
_{Bind Time}	_{0.82s (± 0.50%)}	_{0.82s (± 0.50%)}	_~	_0.81s	_0.82s	_{p=0.218 n=6}
_{Check Time}	_{10.03s (± 0.70%)}	_{10.06s (± 0.36%)}	_~	_10.02s	_10.12s	_{p=0.421 n=6}
_{Emit Time}	_{2.98s (± 0.60%)}	_{3.04s (± 4.15%)}	_~	_2.98s	_3.30s	_{p=0.142 n=6}
_{Total Time}	_{15.41s (± 0.47%)}	_{15.49s (± 0.79%)}	_{+0.09s (+ 0.56%)}	_15.43s	_15.74s	_{p=0.043 n=6}
_{Monaco - node (v16.17.1, x64)}
_{Memory used}	_{346,711k (± 0.00%)}	_{346,721k (± 0.00%)}	_~	_346,703k	_346,734k	_{p=0.335 n=6}
_{Parse Time}	_{2.72s (± 0.28%)}	_{2.73s (± 0.80%)}	_~	_2.71s	_2.77s	_{p=0.315 n=6}
_{Bind Time}	_{1.09s (± 0.47%)}	_{1.08s (± 0.75%)}	_~	_1.07s	_1.09s	_{p=0.523 n=6}
_{Check Time}	_{7.75s (± 0.70%)}	_{7.75s (± 0.28%)}	_~	_7.71s	_7.77s	_{p=0.870 n=6}
_{Emit Time}	_{4.46s (± 1.21%)}	_{4.46s (± 0.90%)}	_~	_4.42s	_4.53s	_{p=0.871 n=6}
_{Total Time}	_{16.02s (± 0.59%)}	_{16.03s (± 0.24%)}	_~	_15.99s	_16.09s	_{p=0.573 n=6}
_{TFS - node (v16.17.1, x64)}
_{Memory used}	_{300,127k (± 0.01%)}	_{300,140k (± 0.01%)}	_~	_300,115k	_300,182k	_{p=0.471 n=6}
_{Parse Time}	_{2.16s (± 0.45%)}	_{2.16s (± 0.35%)}	_~	_2.15s	_2.17s	_{p=0.554 n=6}
_{Bind Time}	_{1.24s (± 1.11%)}	_{1.24s (± 0.85%)}	_~	_1.22s	_1.25s	_{p=0.867 n=6}
_{Check Time}	_{7.19s (± 0.44%)}	_{7.16s (± 0.65%)}	_~	_7.10s	_7.22s	_{p=0.261 n=6}
_{Emit Time}	_{4.35s (± 0.36%)}	_{4.34s (± 0.70%)}	_~	_4.30s	_4.38s	_{p=0.568 n=6}
_{Total Time}	_{14.94s (± 0.35%)}	_{14.89s (± 0.34%)}	_~	_14.80s	_14.95s	_{p=0.195 n=6}
_{material-ui - node (v16.17.1, x64)}
_{Memory used}	_{477,742k (± 0.01%)}	_{477,747k (± 0.00%)}	_~	_477,718k	_477,776k	_{p=0.575 n=6}
_{Parse Time}	_{3.22s (± 0.17%)}	_{3.22s (± 0.47%)}	_~	_3.19s	_3.23s	_{p=0.498 n=6}
_{Bind Time}	_{0.95s (± 1.10%)}	_{0.96s (± 0.54%)}	_~	_0.95s	_0.96s	_{p=0.794 n=6}
_{Check Time}	_{18.06s (± 0.56%)}	_{18.19s (± 0.98%)}	_~	_18.03s	_18.52s	_{p=0.173 n=6}
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_~	_0.00s	_0.00s	_{p=1.000 n=6}
_{Total Time}	_{22.22s (± 0.49%)}	_{22.36s (± 0.81%)}	_~	_22.20s	_22.71s	_{p=0.128 n=6}
_{xstate - node (v16.17.1, x64)}
_{Memory used}	_{550,725k (± 0.02%)}	_{550,740k (± 0.02%)}	_~	_550,631k	_550,935k	_{p=1.000 n=6}
_{Parse Time}	_{3.93s (± 0.37%)}	_{3.94s (± 0.25%)}	_~	_3.92s	_3.95s	_{p=0.624 n=6}
_{Bind Time}	_{1.80s (± 0.65%)}	_{1.78s (± 0.46%)}	_{-0.01s (- 0.83%)}	_1.77s	_1.79s	_{p=0.048 n=6}
_{Check Time}	_{3.04s (± 0.92%)}	_{3.02s (± 0.64%)}	_~	_3.00s	_3.05s	_{p=0.332 n=6}
_{Emit Time}	_{0.09s (± 0.00%)}	_{0.09s (± 0.00%)}	_~	_0.09s	_0.09s	_{p=1.000 n=6}
_{Total Time}	_{8.85s (± 0.35%)}	_{8.84s (± 0.39%)}	_~	_8.80s	_8.88s	_{p=0.571 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

typescript-bot · 2023-03-14T23:16:44Z

Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

typescript-bot · 2023-03-14T23:45:25Z

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

Everything looks good!

weswigham

Since this doesn't change applyToParameterTypes, this doesn't change inference, so won't be observable to a Parameters conditional call - you'll only see the difference in relationship error messages bewteen signatures. And maybe that's OK. It's s little weird to say that variadic rests are sometimes-readonly-sometimes-not, but... function parameters are pretty wonky on the edges as-is.

jakebailey · 2023-03-15T21:33:24Z

Yeah, this is just a case where inference is already right (the insides don't change).

Would my alternative suggestion of stripping readonly from rest types work better, if I can find the way to do that?

jakebailey · 2023-03-15T21:43:36Z

In general, I'm also having trouble seeing where anyone would ever want the result of Parameters to be readonly itself; it never matters what you pass into a function, the actual variadic parameters themselves are copied, so only individual parameters need to maintain readonly-ness (e.g. arrays).

jakebailey · 2023-03-15T23:20:23Z

Actually, I'm even more weirded out, because in my example above args is readonly [1, '1'] | readonly [2, '2'], but clearly mutating it will not mutate the input...

> const fn = (...args) => { args[0] = "oops" }
undefined
> const arr = ["some", "values"]
undefined
> fn(...arr)
undefined
> arr
[ 'some', 'values' ]

So, shouldn't we be stripping readonly in every case from rest arrays? That seems like the right fix...

jakebailey · 2023-03-15T23:28:43Z

I mean, maybe I guess this fix is okay just to get rid of the spurious relation error, and something that addresses my followup comments can come later?

jakebailey · 2023-03-15T23:30:35Z

@typescript-bot run dt

typescript-bot · 2023-03-15T23:30:37Z

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

Update: The results are in!

typescript-bot · 2023-03-16T01:13:36Z

Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

weswigham · 2023-03-20T20:29:28Z

I mean, maybe I guess this fix is okay just to get rid of the spurious relation error, and something that addresses my followup comments can come later?

Seems reasonable.

jakebailey · 2023-03-20T21:03:03Z

Followup is here: #53398

sandersn · 2023-03-23T15:30:08Z

Breaks ckeditor__ckeditor5-upload on DT. Didn't notice it until today because the overnight tests were blocked by the --legacy-peer-deps problem.

Filed #53459 with a 5-line, 2-type repro.

jakebailey · 2023-03-23T15:37:20Z

My fault for not retrying DT until I saw the results...

jakebailey added 2 commits March 14, 2023 14:10

Add baselines

5eec44a

Ensure rest type for source parameter is readonly

5f7155d

typescript-bot assigned jakebailey Mar 14, 2023

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

jakebailey requested a review from weswigham March 15, 2023 17:47

weswigham approved these changes Mar 15, 2023

View reviewed changes

jakebailey marked this pull request as draft March 15, 2023 23:21

jakebailey marked this pull request as ready for review March 15, 2023 23:28

jakebailey mentioned this pull request Mar 20, 2023

...args should not be readonly within function body #53398

Open

jakebailey merged commit e9836a4 into microsoft:main Mar 20, 2023

jakebailey deleted the fix-53255 branch March 20, 2023 23:18

jakebailey mentioned this pull request Jul 18, 2023

Fixed inferred const rest types in signatures #55034

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure rest type for source parameter is readonly in relations #53258

Ensure rest type for source parameter is readonly in relations #53258

jakebailey commented Mar 14, 2023 •

edited

Loading

jakebailey commented Mar 14, 2023

typescript-bot commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023

typescript-bot commented Mar 14, 2023

typescript-bot commented Mar 14, 2023

typescript-bot commented Mar 14, 2023

weswigham left a comment

jakebailey commented Mar 15, 2023

jakebailey commented Mar 15, 2023

jakebailey commented Mar 15, 2023 •

edited

Loading

jakebailey commented Mar 15, 2023

jakebailey commented Mar 15, 2023

typescript-bot commented Mar 15, 2023 •

edited

Loading

typescript-bot commented Mar 16, 2023

weswigham commented Mar 20, 2023

jakebailey commented Mar 20, 2023

sandersn commented Mar 23, 2023 •

edited

Loading

jakebailey commented Mar 23, 2023

Ensure rest type for source parameter is readonly in relations #53258

Ensure rest type for source parameter is readonly in relations #53258

Conversation

jakebailey commented Mar 14, 2023 • edited Loading

jakebailey commented Mar 14, 2023

typescript-bot commented Mar 14, 2023 • edited Loading

typescript-bot commented Mar 14, 2023 • edited Loading

typescript-bot commented Mar 14, 2023 • edited Loading

typescript-bot commented Mar 14, 2023 • edited Loading

typescript-bot commented Mar 14, 2023 • edited Loading

typescript-bot commented Mar 14, 2023 • edited Loading

typescript-bot commented Mar 14, 2023 • edited Loading

typescript-bot commented Mar 14, 2023

typescript-bot commented Mar 14, 2023

typescript-bot commented Mar 14, 2023

typescript-bot commented Mar 14, 2023

weswigham left a comment

Choose a reason for hiding this comment

jakebailey commented Mar 15, 2023

jakebailey commented Mar 15, 2023

jakebailey commented Mar 15, 2023 • edited Loading

jakebailey commented Mar 15, 2023

jakebailey commented Mar 15, 2023

typescript-bot commented Mar 15, 2023 • edited Loading

typescript-bot commented Mar 16, 2023

weswigham commented Mar 20, 2023

jakebailey commented Mar 20, 2023

sandersn commented Mar 23, 2023 • edited Loading

jakebailey commented Mar 23, 2023

jakebailey commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023 •

edited

Loading

typescript-bot commented Mar 14, 2023 •

edited

Loading

jakebailey commented Mar 15, 2023 •

edited

Loading

typescript-bot commented Mar 15, 2023 •

edited

Loading

sandersn commented Mar 23, 2023 •

edited

Loading