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

Improved support for read-only arrays and tuples #29435

Merged
merged 27 commits into from Jan 29, 2019

Conversation

@ahejlsberg
Copy link
Member

ahejlsberg commented Jan 15, 2019

This PR improves our support for read-only arrays and tuples:

  • readonly modifier on array types: readonly T[] corresponds to ReadonlyArray<T> (similar to how T[] corresponds to Array<T>).
  • readonly modifier on tuple types: readonly [T0, T1, ...] correponds to a tuple type that derives from ReadonlyArray<T0 | T1 | ...> and has read-only element positions.
  • Support for read-only arrays and tuples in mapped types. When a mapped type specifies a +readonly modifier, read-write array and tuple types are mapped to their read-only form, and when a mapped type specifies a -readonly modifier, read-only array types and tuples are mapped to their read-write form. This means that Readonly<T> now produces read-only forms of arrays and tuples.

Some examples:

function f1(mt: [number, number], rt: readonly [number, number]) {
    mt[0] = 1;  // Ok
    rt[0] = 1;  // Error, read-only element
}

function f2(ma: string[], ra: readonly string[], mt: [string, string], rt: readonly [string, string]) {
    ma = ra;  // Error
    ma = mt;  // Ok
    ma = rt;  // Error
    ra = ma;  // Ok
    ra = mt;  // Ok
    ra = rt;  // Ok
    mt = ma;  // Error
    mt = ra;  // Error
    mt = rt;  // Error
    rt = ma;  // Error
    rt = ra;  // Error
    rt = mt;  // Ok
}

type ReadWrite<T> = { -readonly [P in keyof T] : T[P] };

type T0 = Readonly<string[]>;  // readonly string[]
type T1 = Readonly<[number, number]>;  // readonly [number, number]
type T2 = Partial<Readonly<string[]>>;  // readonly (string | undefined)[]
type T3 = Readonly<Partial<string[]>>;  // readonly (string | undefined)[]
type T4 = ReadWrite<Required<T3>>;  // string[]

Fixes #26864.
Fixes #28540.
Fixes #28968.

@@ -1023,6 +1023,10 @@
"category": "Error",
"code": 1353
},
"'readonly' type modifier is only permitted on array and typle types.": {

This comment has been minimized.

@j-oliveras

j-oliveras Jan 16, 2019

Contributor

Typo: typle to tuple.

This comment has been minimized.

@ahejlsberg

ahejlsberg Jan 16, 2019

Author Member

@j-oliveras Oops. Thanks!

@@ -30516,6 +30522,11 @@ namespace ts {
return grammarErrorOnNode(node, Diagnostics.unique_symbol_types_are_not_allowed_here);
}
}
else if (node.operator === SyntaxKind.ReadonlyKeyword) {
if (node.type.kind !== SyntaxKind.ArrayType && node.type.kind !== SyntaxKind.TupleType) {
return grammarErrorOnFirstToken(node, Diagnostics.readonly_type_modifier_is_only_permitted_on_array_and_typle_types, tokenToString(SyntaxKind.SymbolKeyword));

This comment has been minimized.

@sheetalkamat

sheetalkamat Jan 16, 2019

Member

Add test for this?

@@ -3073,6 +3074,7 @@ namespace ts {
switch (operator) {
case SyntaxKind.KeyOfKeyword:
case SyntaxKind.UniqueKeyword:
case SyntaxKind.ReadonlyKeyword:

This comment has been minimized.

@sheetalkamat

sheetalkamat Jan 16, 2019

Member

Think we need test/handling for .d.ts generation and decorator for this new typeNode kind.

@Jessidhia

This comment has been minimized.

Copy link
Contributor

Jessidhia commented Jan 17, 2019

Does this improved support for readonly arrays mean it's now possible to have readonly array/tuples as the type of a rest argument?

function f(...args: ReadonlyArray<string>) {} is currently a type error.

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Jan 17, 2019

@Kovensky Isn't in the PR currently, but I see no reason why we couldn't support it. I will fix it, it's just a minor change.

sheetalkamat added a commit to Microsoft/TypeScript-TmLanguage that referenced this pull request Jan 18, 2019

@RyanCavanaugh
Copy link
Member

RyanCavanaugh left a comment

+1 for .d.ts emit coverage - the only .d.ts currently in this set has semantic errors in its originating file, which makes it shaky at best

@sheetalkamat

This comment has been minimized.

Copy link
Member

sheetalkamat commented Jan 25, 2019

I think having decorator test is good idea too.. since we normally forget that when we enable new kind of type annotation.

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Jan 25, 2019

// @emitDecoratorMetadata: true
// @experimentalDecorators: true
// @declaration: true

declare const someDec: any;
class A {
  @someDec
  j: readonly string[];
  @someDec
  k: readonly [string, number];
}
@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Jan 28, 2019

The latest commits provide a better fix for #28540.

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Jan 29, 2019

@typescript-bot test this

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

typescript-bot commented Jan 29, 2019

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 815f133. You can monitor the build here. It should now contribute to this PR's status checks.

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Jan 29, 2019

@RyanCavanaugh Only change in RWC is a single new error in the vscode project. Error is to be expected, Readonly<T> now produces a readonly X[] when given an X[]. The specific code causing the error is a call Object.freeze<XXX[]>(xxx) that explicitly specifies an array type to avoid the Object.freeze overload that otherwise would have returned a ReadonlyArray<T>. Definitely suspect code, it really should use a type assertion.

Issues have been addressed.

@ahejlsberg ahejlsberg merged commit cf7cd62 into master Jan 29, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@CvX CvX referenced this pull request Jan 29, 2019

Merged

Add TypeScript definition #68

@ahejlsberg ahejlsberg deleted the readonlyArrayTuple branch Jan 30, 2019

@zhuravlikjb

This comment has been minimized.

Copy link

zhuravlikjb commented Jan 31, 2019

Is this an intended change that a single 'readonly' is no longer parsed as a type?
Such code compiled fine before this change:

type readonly = string;
var q: readonly;

(This is not a sample from real code, just a test case.)

Seems that it is not a serious issue, but would be nice to know if this was planned, as you still may declare a type named 'readonly' but cannot use it anymore.

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Jan 31, 2019

@zhuravlikjb No, that was not intended. I will look at getting it fixed.

@Flarna Flarna referenced this pull request Jan 31, 2019

Merged

[node] correct SlowBuffer #32661

6 of 9 tasks complete
@ajafff

This comment has been minimized.

Copy link
Contributor

ajafff commented Feb 4, 2019

Using a version of TypeScript that contains this PR creates declaration files which are not compatible with older versions of TypeScript.

In case someone else has the same problem: I created a transformer to downlevel readonly array types in declaration files: https://github.com/ajafff/ts-transform-readonly-array

@falsandtru

This comment has been minimized.

Copy link
Contributor

falsandtru commented Feb 4, 2019

@ahejlsberg This feature leaved some large bugs (Maybe regression). Please fix them: #29442 #29702

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Feb 5, 2019

@falsandtru Those issues are not related to this PR, but I have a fix in #29740.

@mohsen1 mohsen1 referenced this pull request Feb 5, 2019

Closed

readonly function arguments #29742

5 of 5 tasks complete
@falsandtru

This comment has been minimized.

Copy link
Contributor

falsandtru commented Feb 5, 2019

Indeed, I thought another PR. Anyway, thanks for fixing.

@simonbuchan

This comment has been minimized.

Copy link

simonbuchan commented Feb 7, 2019

I assume the extension to readonly SomeObject or readonly T is not worth the native compiler support, now that Readonly<T> can work for everything as expected? React.Ref<readonly T> such that it's assignable from Ref<U> for U extends T would be kind of neat though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment