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 properties and index signatures #6532

Merged
merged 34 commits into from
Jan 27, 2016
Merged

Readonly properties and index signatures #6532

merged 34 commits into from
Jan 27, 2016

Conversation

ahejlsberg
Copy link
Member

This PR adds support for readonly properties and index signatures as suggested in #12. Changes include:

  • A new readonly modifier that can applied to property and index signature declarations.
  • A property or index signature declared with the readonly modifier is considered read-only.
  • Read-only properties may have initializers and may be assigned to in constructors within the same class declaration, but otherwise assignments to read-only properties are disallowed.
  • Assignments to read-only index signatures are always disallowed.

Entities are implicitly read-only in several situations:

  • A property declared with a get accessor and no set accessor is considered read-only.
  • In the type of an enum object, enum members are considered read-only properties.
  • In the type of a module object, exported const variables are considered read-only properties.
  • An entity declared in an import statement is considered read-only.
  • An entity accessed through an ES2015 namespace import is considered read-only (e.g. foo.x is read-only when foo is declared as import * as foo from "foo").

Some examples:

interface Point {
    readonly x: number;
    readonly y: number;
}

var p1: Point = { x: 10, y: 20 };
p1.x = 5;  // Error, p1.x is read-only

var p2 = { x: 1, y: 1 };
var p3: Point = p2;  // Ok, read-only alias for p2
p3.x = 5;  // Error, p3.x is read-only
p2.x = 5;  // Ok, but also changes p3.x because of aliasing
class Foo {
    readonly a = 1;
    readonly b: string;
    constructor() {
        this.b = "hello";  // Assignment permitted in constructor
    }
}
// Read-only view of Array<T> with all mutating methods removed
interface ReadonlyArray<T> {
    readonly [x: number]: T;
    readonly length: number;
    toString(): string;
    toLocaleString(): string;
    concat<U extends ReadonlyArray<T>>(...items: U[]): T[];
    concat(...items: T[]): T[];
    join(separator?: string): string;
    slice(start?: number, end?: number): T[];
    indexOf(searchElement: T, fromIndex?: number): number;
    lastIndexOf(searchElement: T, fromIndex?: number): number;
    every(callbackfn: (value: T, index: number) => boolean, thisArg?: any): boolean;
    some(callbackfn: (value: T, index: number) => boolean, thisArg?: any): boolean;
    forEach(callbackfn: (value: T, index: number) => void, thisArg?: any): void;
    map<U>(callbackfn: (value: T, index: number) => U, thisArg?: any): U[];
    filter(callbackfn: (value: T, index: number) => boolean, thisArg?: any): T[];
    reduce(callbackfn: (previousValue: T, currentValue: T, currentIndex: number) => T, initialValue?: T): T;
    reduce<U>(callbackfn: (previousValue: U, currentValue: T, currentIndex: number) => U, initialValue: U): U;
    reduceRight(callbackfn: (previousValue: T, currentValue: T, currentIndex: number) => T, initialValue?: T): T;
    reduceRight<U>(callbackfn: (previousValue: U, currentValue: T, currentIndex: number) => U, initialValue: U): U;
}

let a: Array<number> = [0, 1, 2, 3, 4];
let b: ReadonlyArray<number> = a;
b[5] = 5;      // Error, elements are read-only
b.push(5);     // Error, no push method (because it mutates array)
b.length = 3;  // Error, length is read-only
a = b;         // Error, mutating methods are missing

Note that the readonly modifier does not affect subtype and assignability relationships of the containing type for the reasons described here. Also note that this PR specifically doesn't attempt to formalize immutable objects. However, with the readonly modifier it becomes a lot more convenient to implement such objects.

The PR still needs tests, but I thought I'd get it up there for folks to take a look at.

Conflicts:
	src/compiler/parser.ts
	src/compiler/types.ts
@JsonFreeman
Copy link
Contributor

Yay!

@@ -134,6 +134,8 @@ namespace ts {
const anySignature = createSignature(undefined, undefined, emptyArray, anyType, 0, /*hasRestParameter*/ false, /*hasStringLiterals*/ false);
const unknownSignature = createSignature(undefined, undefined, emptyArray, unknownType, 0, /*hasRestParameter*/ false, /*hasStringLiterals*/ false);

const enumNumberIndexInfo = createIndexInfo(stringType, /*isReadonly*/ false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this indexer not readonly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe because we don't want to break this code:

enum SyntaxKind { Node, OtherNode }
function dbgPrint(value: number, yourEnum: { [n: number]: string }) {
    console.log(yourEnum[value]);
}
// Would-be error, dbgPrint wants mutable index signature but enum has readonly signature
dbgPrint(SyntaxKind.OtherNode, SyntaxKind);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, because of the assignability rule. Makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. It's bit of a puzzle to ensure backwards compatibility, so there are actually two forms of read-only-ness in the new implementation:

  • readonly properties, readonly index signatures, and get only accessors are always considered read-only, in both assignments and type compatibility checks.
  • const variables, functions, classes, namespaces, enums, and enum members are considered read-only in assignments, but not in type compatibility checks. This ensures backwards compatibility. For example, it is reasonably common to implement an interface contract using a module or namespace, and strict read-only-ness would break that pattern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. That sounds like a good distinction.

// -- operator on enum type

enum ENUM1 { A, B, "" };

// expression
var ResultIsNumber1 = --ENUM1["A"];
~~~~~~~~~~
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As clarification, increment/decrement operator and any assignment to enum isn't allowed as enum type is read-only upon assignment? if that is the case, I think the error message should be updated.

@joelday
Copy link
Contributor

joelday commented Jan 22, 2016

Just stumbled upon this. Don't think I could be any happier than I am right now.

@malibuzios
Copy link

@ahejlsberg These questions in #8496 (and the issue in general) are highly relevant to the topic here. Consider providing your comments. I have only closed that issue because I felt that your (unbelievably stubborn and righteous) team members would somehow find it more 'respectful' that I show my complete pessimism at having any influence here than trying to 'confront' them in any way. You have already won the 'battle', congratulations, now feel free to join the discussion.

@joelday
Copy link
Contributor

joelday commented May 8, 2016

Unbelievably stubborn and righteous, eh?

@malibuzios
Copy link

malibuzios commented May 8, 2016

[I apologize for this being very off-topic but since I was asked I felt I had no choice but answering here, the person who asked doesn't have a public email so there is no way for me to contact them personally]

@joelday

Yes, my personal impression is that they are very 'difficult' people to deal with, in the sense that they are putting as much energy into 'justifying' (sometimes dishonestly or irrationally) their actions and decisions to actually considering things openly and objectively when presented to them. They even do this on some very trivial subjects (e.g. you tell them they don't conform to the ES6 spec by practically disallowing inheritance of static accessors for ES5 targets and instead of listening and acknowledging the problem like a normal person would do, a group of team members just 'jumps' on you and starts to throw some strange and arbitrary ('canonical') justifications about performance consideration and dubious statistics there is '0%' anyone would ever want to do this.. well I did! and so did others who reported this problem, and probably many who didn't.. :( ).

They put you in a situation where you have to be so eloquent and rational, to the point you have to literally spend dozens of hours into very carefully and rationally crafting a 'thesis' for something otherwise relatively trivial, just for them. And they on the other side are allowed to simply 'push' their opinions and twisted patterns of 'justification' arbitrarily without anyone saying anything. When you start challenging those patterns they would either ignore you or tell you you are intentionally 'wasting their time' with things that aren't important. They would also use their 'internal' understanding of the language and compiler by mentioning intricate things or concepts you don't know or completely understand and dishonestly use that against you.

They take submitted ideas they like but they don't actually partake in real two-way conversations about it. They don't really involve the submitter of their personal ideas and insights, at least before they decide on their actions. And when/if they do inform back, they would condense it to a set of a few sentences that are not very useful or enlightening by themselves (or direct you to a set of design 'notes' that are very hard to extract anything useful from). You can't ask a (non-trivial) question because that's just 'wasting their time' (they don't do 'one on one').

If they don't like an idea, they would usually 'tank' and attack it mercilessly to the point of almost (or perhaps actually) insulting the person who submitted it. They almost never 'improve' on things suggested to them, at least not publicly. They would rather spend their time and 'smarts' in crafting careful arguments for justifying why they think someone are 'wrong' and they must be 'right' than taking a step back to seriously and objectively reconsider things, and eventually thank the participant for contributing something useful to the discussion.

This has been my experience so far, anyway. I'm mostly referring here to the 2-3 members who reply to GitHub issues.

@RReverser
Copy link
Contributor

Is there any reason / chance to change that the generated code contains regular property assignment and not defineProperty with writable: false? That would help a lot to protect against runtime assignment to properties designed as readonly.

@Robert923
Copy link

I agree with @RReverser - on my team we frequently use TypeScript compiled code from (legacy)JavaScript applications - so keeping as many of those protections as possible would be very helpful.

@ScallyGames
Copy link

Is this already available or only for TypeScript 2.0 (neither VS nor TypeScript Playground seem to understand it)

@RReverser
Copy link
Contributor

@Aides359 As usual with all the fresh stuff, you need to use npm i typescript@next to get the latest compiler with experimental features included.

@aluanhaddad
Copy link
Contributor

@Aides359 I think the playground hosts the latest stable release. I recommend doing what @RReverser suggests and additionally installing vscode and setting then "typescript.tsdk" property of your user/workspace settings to the install location's lib directory.

@paleo
Copy link

paleo commented Jul 25, 2016

Read-only properties may have initializers and may be assigned to in constructors within the same class declaration, but otherwise assignments to read-only properties are disallowed.

Why disallow assignments in the other methods of a class?

A readonly property would be useful to implement a getter with a true property (better for performance, readability):

class Foo {
    readonly a = 1;
    inc() {
        ++this.a;  // Why not allow to do this?
    }
}

Does the limitation exist for a technical reason?

@RyanCavanaugh
Copy link
Member

If that were allowed we'd have daily reports about "Bug: Why does TS let me mutate readonly fields?" 😉

We discussed the notion of "public readonly, protected mutable" but opted not to simply due to complexity. You can already have a getter-only wrapper around a private variable, which achieves the same semantics at the cost of slightly increased class size.

@paleo
Copy link

paleo commented Jul 25, 2016

OK. You use the term readonly as a synonym for const. I hoped an accessor readonly. :(

@ghost
Copy link

ghost commented Dec 10, 2016

I use this great feature for mutable preventing by the syntax highlighting. Thanks a lot for this!

+++

@zpdDG4gta8XKpMCd
Copy link

This change isn't as bad as it might sound because we'll always error on direct assignments to readonly properties and that's where the majority of the errors occur.

this is a horrible change, rendering this feature usless

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Dec 17, 2016

simple use case that fails, I have my app about 1500 TS files thick, in this app I have an interface named Data that was intended to be immutable historically could not be forced being so

With the readonly feature announced it was immediately clear that such enforcment can be imposed.

However it turned out that there were a few places where Data was meant to be mutable.

To address it the original Data interface was split into ReadOnlyData and WitableData, however it didn't help much due to the said decision. Details can bee seen in #13002.

Bottom line is we are still not sure if Data is effectively mutable or not, despite applibg readonly to all its properties.

@masaeedu
Copy link
Contributor

masaeedu commented Dec 19, 2016

@ahejlsberg Correct me if I'm wrong, but the only reason interface compatibility checking on readonly is a breaking change is because of readonly being implicitly applied to everything. The tradeoff of worse soundness in return for saving the user some typing, especially when you now have mapped types, is not a good one IMO.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.