Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Readonly<> does not work on tuple types in 3.1.0-dev #26864

Closed
UselessPickles opened this issue Sep 4, 2018 · 4 comments
Closed

Readonly<> does not work on tuple types in 3.1.0-dev #26864

UselessPickles opened this issue Sep 4, 2018 · 4 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@UselessPickles
Copy link

TypeScript Version: 3.1.0-dev.20180901

Search Terms: Readonly tuple

Code

type Foo = Readonly<[string, number]>;
const foo: Foo = ["hello", 42];
foo[0] = "blah";

Expected behavior:
Compile error at foo[0] = "blah" because the types of all indexes of the tuple should be readonly (this is the behavior in previous versions of Typescript, from at least 2.3 up to and including 3.0).

Actual behavior:
No compiler error. The type of foo is incorrectly reported as simply [string, number] rather than Readonly<[string, number]>, and the compiler allows the assignment.

@ahejlsberg
Copy link
Member

Readonly tuple types never really worked correctly. Previously, when applying a mapped type (such as Readonly<T>) to a tuple type, we'd generate a type that is structurally similar to a tuple, but not actually assignable. For example:

const foo: Readonly<[string, number]> = ["hello", 42];
const bar: [string, number] = foo;  // Now ok, previously would error

We now preserve the "tuple-ness" across mapped type applications (e.g. Partial<T> applied to a tuple actually does the right thing), but it means that the readonly-ness of the properties is lost because we have no inherent notion of tuples with readonly properties. To fix this issue we'd need to add a proper notion of readonly tuples--so, I'm going to label this a suggestion.

@ahejlsberg ahejlsberg added the Suggestion An idea for TypeScript label Sep 4, 2018
@UselessPickles
Copy link
Author

Thanks for the explanation. That makes sense, but I still feel that this is a breaking change. It is causing "incorrect" behavior and failing dtslint tests on a project that has worked "correctly" and passed dtslint tests since TypeScript 2.3.

Even though Readonly tuples never really worked correctly, there was a subset of functionality that did work correctly (as one would expect in terms of observable behavior) prior to 3.1.0-dev. Now that mapped types are applied to tuples "correctly", some previous "observably correct" behavior with Readonly tuples has now been lost in exchange.

Please consider treating this more like a bug rather than an arbitrary suggestion or feature request.

@ahejlsberg
Copy link
Member

@RyanCavanaugh @DanielRosenwasser Let's discuss how we want to prioritize this.

@UselessPickles
Copy link
Author

I had a complete failure of the brain when trying to come up with the right terminology while writing my previous comment. I think the correct term to describe this issue is not just a "breaking change", but more specifically a "regression".

It may also be worth pointing out that if a version of TypeScript is released with the current behavior of 3.1.0-dev, then dtslint tests of types involving Readonly tuples will become complicated.

Sample setup code (quite a bit oversimplified compared to my real world examples just to focus on
examples of types involved, so ignore whether or not it would make sense for this function to return a readonly tuple, etc):

export type Foo<K extends string, T> = Readonly<[K, T]>;

export function foo<T, R extends Record<Extract<keyof R, string>, T>>(
  data: R
): Foo<Extract<keyof R, string>, T> {
   ...
}

And the corresponding dtslint test example that works in TS 3.0 an earlier:

interface Blah {
  a: number,
  b: number
}

const blah: Blah = {
  a: 10,
  b: 42
};

// $ExpectType Readonly<["a" | "b", number]>
const fooTestResult = foo(blah);

// $ExpectError
fooTestResult[1] = 9;

With TS 3.1.0-dev, the reported return value type of foo(blah) is ["a" | "b", number] instead of Readonly<["a" | "b", number]>.

The only way I can write dtslint tests that work with both 3.1.0-dev AND previous versions is to give up on dtslint type assertions, and fall back to less precise assignability tests, in addition to removing the test that attempts to assign a value to one of the tuple's indexes.

const fooTestResult: Readonly<["a" | "b", number]> = foo(blah);

Even if a future version of TS re-introduces Readonly tuple behavior, I (and anyone else making use of Readonly tuple types) will be stuck with the less reliable assignability tests in dtslint in order to support validating previous versions of TS, and will need to write separate tests for this future version and beyond in order to precisely validate the type with an $ExpectType assertion.

@weswigham weswigham added the Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature label Nov 6, 2018
@ahejlsberg ahejlsberg added the Fixed A PR has been merged for this issue label Jan 29, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants
@weswigham @ahejlsberg @UselessPickles and others