Closed RyanCavanaugh closed 8 years ago
@joewood While bad practice, nothing prevents a user to implement a property getter that has side effects. (unless ofcourse you can also annotate its signature as readonly)
I think the compiler could warn on that. If the compiler auto promotes any function (including property getters) to read-only if they don't mutate then the compiler can warn if the readonly context modifier is applied and contradicts with what the compiler sees.
Of course, the problem with this approach will be like in C++ with const. One bad apple can spoil the whole thing.
Is it possible to add a check to compiler similar to "suppressImplicitAnyIndexErrors": true" that would check assignment to "get-only" properties? I think that is a good start that will cover 95% of its use cases. Immutability could be solved as a separate task. How about "checkReadonlyProperties"? I am probably reinventing hot water, but I think this would be extremely useful for many of TS programmers.
@tomitrescak That could just as easily be solved by marking getters as readonly.
@impinball And how can that be done? Specifying only getter and assigning value to such getter passes compilation.
Or at least a constant reference. Something to that effect.
@impinball you are being very confusing. How can you mark a getter as readonly so that assiging value to it will not pass compilation?
In another words, how do you make follwing fail compilation?
class MyClass {
private theCount: number;
private theOther: number;
public get count() {
return this.theCount;
}
public set other(value: number) {
this.theOther = value;
}
}
var c = new MyClass();
c.count = 5; // should fail
var e = c.other; // should fail
@tomitrescak I mean in the sense of it being a glorified constant.
const foo = 2;
foo = 1; // Error
class Foo {
get foo() { return 1; }
}
const obj = new Foo();
obj.foo = 2; // Error
Well, that's not what I was proposing :/, but thanks for your insight! Sorry for hijacking the discussion.
Defining an object as a constant is a hack @impinball, I agree with @tomitrescak that assignments to getters should fail compilation. Same for reading a getter. Moreover, an interface should be able to enforce these constraints.
@gustavderdrache nailed that in comment: https://github.com/Microsoft/TypeScript/issues/12#issuecomment-78297192
I think the const obj
creates confusion in the example @impinball. Instead, I imagine noone would disagree that this must hold true:
class Foo {
get foo() { return 1; }
}
var obj = new Foo();
obj.foo = 2; // Error
Read-only properties have been a matter of fact in javascript since the beginning of time, and it's unambiguous that type system support for read-only properties will be immensely helpful. Any other discussions about const objects and immutable collections IMHO are not relevant to this discussion.
Once we have read-only properties we could start discussing seriously stuff like modelling javascript "frozenness" and "sealedness" formally, but all our arguing about everyone's expectations of immutability and purity here are only detracting from the discussion. I seriously suggest we focus on getting type system support for read-only properties and think immutability next thing.
Okay... I'll admit the similarity with const
would be a little bit
hackish. But last time I checked, you can't assign to either one, const
or getter without setter. You can get the value of either one, but they
can't be assigned to. Because they behave identically on the left hand side
(excluding getter side effects), that's why I suggested it.
As for const obj
vs var obj
, I typed const
out of habit. Any of the
three declarations would suffice.
On Fri, Sep 18, 2015, 05:26 Hristo Dachev notifications@github.com wrote:
I think the const obj creates confusion in the example @impinball https://github.com/impinball. Instead, I imagine noone would disagree that this must hold true:
class Foo { get foo() { return 1; } } var obj = new Foo(); obj.foo = 2; // Error
Read-only properties have been a matter of fact in javascript since the beginning of time, and it's unambiguous that type system support for read-only properties will be immensely helpful. Any other discussions about const objects and immutable collections IMHO are not relevant to this discussion.
Once we have read-only properties we could start discussing seriously stuff like modelling javascript "frozenness" and "sealedness" formally, but all our arguing about everyone's expectations of immutability and purity here are only detracting from the discussion. I seriously suggest we focus on getting type system support for read-only properties and think immutability next thing.
— Reply to this email directly or view it on GitHub https://github.com/Microsoft/TypeScript/issues/12#issuecomment-141396042 .
This is now Typescript 1.6 available and still no support for this feature. You even implemented a support for third party library like the React in this version and still no long awaited and long discussed support for the compiler to check that the property has no setter. I had a bug in my project because the compiler did not give any hint while i assigned values to readonly properties.
Could you please make this feature in the compiler because it is needed for large projects like mine https://github.com/BBGONE/jRIAppTS
I suggest a quick and effective solution. As a compromise you can allow implicit casting immutable => mutable is OK. Just don't allow assignment to a readonly property when the variable has type of the interface with that property marked readonly. It is needed because developers who create frameworks define interfaces in it which can be consumed by clients and the clients should be aware what property is readonly and which is not. It is up to them to follow the contract what the developers provided. For example: an Entity can have calculated property and the clients should be aware that the property is readonly. It will cover 99% of usecases. Very litle developers care about the issues that a function can accept an argument and modify it internally. Most of the problem is when you work with properly typed variables (with readonly properties) and there's an assignment to it in the code.
interface Point { x: number; y: number; } interface ImmutablePoint { readonly x: number; readonly y: number; } var pt: ImmutablePoint = { x: 4, y: 5 }; // OK, can convert mutable to non-mutable pt.x = 5; // Error, 'pt.x' is not a valid target of assignment var pt2: Point = pt; // immutable => mutable ALLOW IT! pt2.x = 6; // ALLOW IT because it is now mutable pt= pt2 //IT is again immutable pt.x = 7; // Error, 'pt.x' is not a valid target of assignment
P.S. - reclassify this issue as a bug, because technically it is a bug : the compiler should check that a property can not be wriitten because a developer explicitly did not provide a setter.
@BBGONE
Only the part regarding getters without setters could remotely be considered a bug. The rest cannot, such as the readonly
modifier for other types. The feature didn't exist before, and it's not directly mis-typing the language. And in case you're curious, even the mis-typing of this
was considered a suggestion, not a bug. That problem came down to JavaScript being call-by-delegation in its object model, like Lua or Python, vs TypeScript typing it like a language with statically known methods like Java or C# (which isn't accurate at all).
@impinball "even the mis-typing of this was considered a suggestion, not a bug." It is considered a suggestion because it has an easy workaround and i use it often: instead of untyped 'this' use a typed 'self'
function f() { var self: SomeType = this; self.x = 3; }
But the issue with ReadOnly properties have no workaround. And my proposal can easily be implemented transparently to the existing codebase. Developers can then use interfaces with the new features, and their code will be protected from that bug if they use interfaces and typing consistently. Third party libs can be left as is.
@BBGONE
The thing with this
is that even Function.prototype.call is improperly typed. The standard library doesn't correctly take this into account.
As for readonly properties, it's still a feature. It's not a feature that was improperly/incorrectly implemented. Those constitute actual bugs.
I really don't understand why this conversation is still going in circles. The spirit of the issue as a requested feature has nothing whatsoever to do with deep immutability. This is specifically nothing more than having a compiler error when we know at build time that, given the implicit or explicit interface of an object, an assignment is absolutely guaranteed to always cause a runtime error.
I say this with full appreciation for the fact that syntax changes merit very serious scrutiny. I just propose that other immutability features should be forked to a different issue and considered OT here.
@joelday I agree with the entirety of your statement.
Back to the initial discussion, I do believe that there needs to be a way of marking whether something can be read or written. This would be an awesome thing to be able to have:
interface LodashArray {
readonly length: number;
}
There's no way I can possibly modify the length without something going wrong. Also, assigning to getters without setters should also fail in this case:
class Foo {
get foo() { return 2 }
}
new Foo().foo = 3 // error!
let foo = new Foo().foo // okay
Also, there should be a way to properly type setters without getters. I know it's not very common and not very elegant, but JS and TS both allow it, so it should be checked, anyways.
class Foo {
set foo(value) { /* ... */ }
}
let foo = new Foo().foo // error!
new Foo().foo = 2 // okay
@jbondc You mean s/writable/readable/g
and s/mutable/writable/g
? ;)
Also, what about get T
and set T
? There is no existing ambiguity AFAIK, since there's just whitespace between the two tokens.
Ok. It really confused me. To be honest, I don't like that naming for that very reason: it's non-obvious.
On Mon, Oct 19, 2015, 08:36 Jon notifications@github.com wrote:
@impinball https://github.com/impinball Yes well:
- !writable reads as Not Writable (readable).
- !mutable reads as Not Mutable (constant, immutable, ..?)
I explain it more here about attaching constraints to a type:
3076 (comment)
https://github.com/Microsoft/TypeScript/issues/3076#issuecomment-100675816
— Reply to this email directly or view it on GitHub https://github.com/Microsoft/TypeScript/issues/12#issuecomment-149201689 .
I would read number!writable as "number that is writable", and number!writable!enumerable as "number that is writable and enumerable". Is that what you meant?
Couple of thoughts:
writable
be better defined by simply the existence of the set property function? I realize that these are defined separately in JavaScript - but that makes more logical sense to me.configurable
and enumerable
), rather than creating a new syntax couldn't decorators be repurposed for this? I mean using some well recognized decorators defined internally to the compiler?Alternative I guess could be:
declare class Foo {
someValue: number;
foo: number{writable: false, enumerable: false}
}
@joewood Properties are writable in JS so not sure what you meant by the set property
:
class Foo {
someValue = 1
set foo() { this.someValue++ }
}
The emit in a declaration file would be:
declare class Foo {
someValue: number;
foo: number;
}
With decorators, look at the generated code: http://www.typescriptlang.org/Playground#src=%40A%0Aclass%20Clazz%20{%0A%20%20%20%20%40B%0A%20%20%20%20prop%20%3D%201%3B%0A%0A%20%20%20%20%40C%0A%20%20%20%20method%28%29%20{}%0A%0A%20%20%20%20%40D%0A%20%20%20%20get%20prop%28%29%20{return%201%3B}%0A%0A%20%20%20%20%40E%0A%20%20%20%20method2%28%29%20{}%0A%0A%20%20%20%20%40F%0A%20%20%20%20prop2%20%3D%201%3B%0A}
Not something I'm particularly excited about. They are a runtime thing and can't be applied to types/interfaces etc...
e.g.
function enumerable(proto, name) {
Object.defineProperty(proto, name, { value: undefined, enumerable: true });
}
class Foo {
@enumerable
a: number;
}
The emit in a declaration file is:
declare function enumerable(proto: any, name: any): void;
declare class Foo {
a: number;
}
So you lose the info.
@jbondc I mean if the emit included a declaration for the getter/setter. The compiler would then error if you were trying to use the property as an lvalue in an expression. This seems more natural to me, unless I'm missing something.
For decorators - I'm talking about reusing the syntax as a way of adding metadata, not as a runtime evaluation or generating any code. So this would just be used for metadata exposed to the compiler.
So you mean:
class Foo {
someValue = 1
get foo() { return this.someValue }
}
Declaration is:
declare class Foo {
someValue: number;
get foo: number;
}
Guess that can work too, syntax is more a game of perspective.
Right, seems to be more inline with the goal of declaration files being the header files of JavaScript.
And this is why I suggested get
and set
modifiers (I don't care if it's
prefixing the type or value name - it reads the same). I think it would
make perfect sense. The intent is very clear in that you can get
and/or
set
the property.
// Prefixing the name
declare class Foo {
someValue: number;
get foo: number;
}
// Prefixing the type
declare class Foo {
someValue: number;
foo: get number;
}
I'm indifferent to which is used, although I suspect most here would prefer the former.
On Mon, Oct 19, 2015 at 11:46 AM, Joe Wood notifications@github.com wrote:
Right, seems to be more inline with the goal of declaration files being the header files of JavaScript.
— Reply to this email directly or view it on GitHub https://github.com/Microsoft/TypeScript/issues/12#issuecomment-149254282 .
Isiah Meadows
Gotcha makes sense, like it.
@impinball Thanks! I think the issue with using "readonly" as a keyword in an interface is that it can be interpreted as declaring that the property cannot be writable on an object that implements that interface. Also, I think it would get confusing (or plain not work in some situations) when extending interfaces, and class declarations of get/set properties is also fundamentally additive rather than negative, so I think it would make sense for declaring it on interfaces to work the same way and use the same keywords.
Quick reminder that I prototyped what we're talking about a while ago: https://github.com/joelday/TypeScript/blob/getSetKeywordAssignment/tests/cases/compiler/assignmentToReadOnlyMembers.ts
:)
Any progress?
+1 for final
modifier.
Perhaps we can explore and borrow the idea of case classes
from Scala
i.e.
immutable class Point {
public x: number;
public y: number;
}
where x
and y
are automatically readonly
, all immutable
classes are provided a copy()
or clone()
methods for deep and shallow copying
if a particular property needs to be mutable for any reason, simply denote it with var
i.e. public var lastModified: Date
so that's +1 for immutable class
for me
This discussion not about constants in classes, but some issues marked as duplicate of this, and I wont to show my vision about this (it also gives an option for this https://github.com/Microsoft/TypeScript/issues/3743 ). We can do something like this:
class MyClass
{
static const MY_STATIC_CONST: string = 'my staic const';
const MY_CONST: string = 'my const';
}
And generated JS:
Object.defineProperty(
MyClass,
'MY_STATIC_CONST',
{
value: 'my staic const',
enumerable: true
}
);
Object.defineProperty(
MyClass.prototype,
'MY_CONST',
{
value: 'my const',
enumerable: true
}
);
It looks simple and useful for me.
Will we see a solution of this implemented in typescript compiler? P.S. I need a solution for the bug when you can assign a value to a property without setter and proper expression of this in the interfaces.
talk, talk , talk ....
TypeScript already generates defineProperty for ES5 getters/setters. And, as I have shown, in this case we can declare properties for prototype, that is also important.
I'm going to take a look at implementing something here. My current thoughts are:
readonly
modifier that can be applied to property and method declarations in interfaces and classes, and infer a read-only property when a class includes only a get
accessor declaration for a particular property.readonly
modifier to also be applied to interface and class declarations, making all members introduced in that interface or class declaration read-only.This effectively amounts to "shallow" immutability. With that in place, deeply immutable object types can be created by manually ensuring that all properties are read-only properties of primitive types (string, number, or boolean) or object types that are themselves deeply immutable (transitively).
I think this would cover the type checking side of things. I'm not sure we really want this to affect code generation (e.g. by using Object.defineProperty
) since there is overhead associated with that.
This effectively amounts to "shallow" immutability.
Isn't that shallow immutability logic the same logic as const
, therefore relatively straight forward to guard against?
Could readonly
also be applied to parameter types as a type modifier providing a read-only reference to an existing object? Only getters and readonly
members would then be available to that function. e.g.:
function doThis( someObject: readonly Foo ) {
someObject.prop = 123; // error
var v = someObject.prop; // ok
}
In a similar way to C++ const
. Without a type-modifier I suspect we'll end up writing interface definitions twice, once for immutability and again for mutability.
@kitsonk The difference between const
and readonly
is that it isn't possible to alias a const
and therefore you're guaranteed it will never be mutated. With readonly
you can fairly easily create a type with all read-only properties that aliases an object with read-write properties. In that scenario it is still possible for mutations to happen, it's just that you can't cause them.
@joewood The issue with readonly
as a type modifier is that it may not do what you expect. For example, a readonly string[]
would still have a push
method that mutates the array. All readonly
would do is keep you from assigning to the properties of the array.
@ahejlsberg maybe I not following, but wouldn't push
be inaccessible because it wouldn't be marked as readonly
because it mutates. Whereas functions like slice
would have a readonly
modifier, or doesn't it work like that.
It's probably worth breaking down several of the concepts of readonlyness that some languages have so we can clarify which would and would not exist:
Using C++ as the most direct analogy, we can understand the difference between 1 and 2 as the difference between const int* x
and int* const x
. Since C++ doesn't have property getters/setters, point 3 reduces to point 1 (modulo the mutable
keyword).
Since C++ doesn't have the ability to set e.g. std::vector.insert
on an arbitrary object reference, it doesn't have to make any subtle distinctions that a hypothetical JavaScript-based full understand of mutability would require. To clarify: there are two concepts you might want to express about Array#indexOf
or Array#push
-- you don't want to allow arbitrary setting of those properties, but only the latter causes mutation of the underlying object when invoked. It's worth thinking about how std::vector.size()
is a method, rather than a field, in that context (you can't set the size
of a mutable std::vector
via that member).
What's on the table (as far as I understand) is the following:
length
of a Function
does nothing and should be a compile-time error.someArray.slice()
(which does not modify the object) and someArray.reverse()
(which does) are indistinguishable.class
or interface
are read-only, but this is just a shortcut for declaring each property of that type with the readonly
modifier.const
vs. let
/var
cover reference immutability for variablesSince now I've written all this and now realize that there's a much simpler explanation, consider this symmetry:
const items: string[] = []; // OK, initializer
items.push('ok'); // OK
items = []; // Nope, const reference
and
readonly class MyClass {
items: string[]; // readonly because containing class is readonly
constructor() {
this.items = []; // OK, initializer
}
}
let m1 = new MyClass();
m1.items.push('hello'); // OK
m1.items = []; // Nope, readonly property
m1 = new MyClass(); // OK, not a const reference
const m2 = new MyClass();
m2.items = []; // Still an error
m2.items.push('world'); // Still OK
m2 = new MyClass(); // Nope, const reference
...and if the user wanted more run-time immutability they could:
readonly class MyClass {
items: string[]; // readonly because containing class is readonly
constructor() {
this.items = []; // OK, initializer
Object.freeze(this.items);
}
}
const m3 = new MyClass();
m3.items.push('world'); // Throws an error in strict mode (or noop otherwise)
TypeScript hasn't "tracked" that type of immutability to date and I suggest the proposal for readonly
also would just leave that behaviour to the runtime?
@ahejlsberg
Maybe your readonly
modifier is not the same as const
/final
modifier in classes, that me and some other people expect.
I'm looking for ability of creating constants in classes such as in other languages and as we can see in javascript standard objects: Node.ELEMENT_NODE
, XMLHttpRequest.DONE
and other (in js we also can have access to these constants from instances, because of prototypes, but it's not really needed).
And these constants in js is read-only — I just want to have ability to declare such constants in TypeScript easely.
But, maybe, your readonly
is OK, I have only one question — why you don't want to make class instance read-only properties assigned once for prototype and not for each instance?
Such a long discussion and finally things become clearer!
@RyanCavanaugh All that's on the table is correct but there's a small thing I want to mention about - the difference in getter/setter accessors visibility.
I agree that an object property can have no setter:
const foo = {
get bar() {
return 0;
}
}
foo.bar = 1; //error
I think this is clear. But here's a bit more complex example that describes the problem:
declare abstract class Emitter {
new (): Emitter;
on: (name:string, handler: (arg:any) => void) => void;
off: (name:string, handler: (arg:any) => void) => void;
protected emit: (name:string, arg:any) => void;
}
class Person extends Emitter {
constructor(name:string) {
super();
this.name = name;
}
private _name:string;
get name() {
return this._name;
}
set name(value) {
if (this._name !== value) {
this._name = value;
this.emit('change:name', value);
//this way is better
this.updatedAt = new Date();
//than this way
this.setUpdatedAt(new Date());
}
}
////
private _updatedAt:Date;
get updatedAt() {
return this._updatedAt;
}
private set updatedAt(value) { //Getter and setter do not agree in visibility
//some logic and a simple readonly (our absence of a setter) is not enough
if (this._updatedAt !== value) {
this._updatedAt = value;
this.emit('change:updatedAt', value);
}
}
//// method implementation - but what's the point in it?
private setUpdatedAt(value) {
if (this._updatedAt !== value) {
this._updatedAt = value;
this.emit('change:updatedAt', value);
}
}
}
const entity = new Person('Mike');
entity.on('change:updatedAt', console.log.bind(console));
entity.name = 'Thomas';
//but manually setting updatedAt should be forbidden
entity.updatedAt = new Date(); //restricted
Here, property updatedAt
actually can have a setter but it should not be accessable outside of Person
. Moreover this setter contains complex logic and a simple readonly or an absence of a setter is not enough.
I agree that private setter is just a syntax sugar for private methods with additional logic (like emitting) but I think it is inconsistent when part of logic related to a field is in a property (getter) and another in a method (actual private setter).
This way getters/setter are implemented in C#
, I think it would be usefull to allow different visibility at a compile time, though this contracts will be lost in runtime.
UPDATE I've found the duplication of my suggestion: #2845 but it is linked and closed
@RyanCavanaugh yes, understood. My two requests are really:
Type Modifier
Not having a type modifier on a reference (no. 2 above on your list) will mean that if you want to pass around an immutable reference to an object you will end up having to duplicate the interface definition with a simple readonly
before the declaration. That seems a little frustrating, especially given how popular immutability is becoming on the client side. So something like this (even though it's only shallow)
interface Foo {
x: number;
y: Bar;
}
function doSomething( foo: readonly Foo ) { // or maybe using the const keyword
foo.x = 10; // error
var x = foo.x; // ok
foo.y.bar = 10; // unfortunately ok because only shallow
}
Note, without this the developer would need to declare Foo
and also a duplicate interface ImmutableFoo
.
Context Type Modifier
Second request is to be able to decorate a function that acts as a type modifier for this
. Property getters have this set implicitly.
interface Foo {
readonly getState() : number;
resetState();
}
function readFoo( foo: readonly Foo ) {
let state = foo.getState(); // OK, works as readonly context function
resetState(); // error - mutating function
}
With a type modifier you could define a getter that returns a readonly type, so the immutability isn't just shallow. I think the interface definition would need to be extended to support that, or add some sort of immutable
directive to the type definition.
@Avol-V
Both Node.ELEMENT_NODE
, XMLHttpRequest.DONE
are not constants but enums which are immutable be design and you can just export them from the module along with the class using them - this is the proper way in ES6/TS.
If you need an immutable property on the class you can define it with only a getter:
export enum NodeType {
ELEMENT_NODE,
SOME_OTHER_NODE_TYPE
}
export class SomeElement {
private _type:NodeType;
get type() {
return this._type;
}
}
This works right now except of the ability to write to a property without a setter which was considered a bug eariler in the conversation. No readonly
/final
/immutable
etc.
@raveclassic
No, Node
and XMLHttpRequest
and others — is a classes, not enums. And ELEMENT_NODE and others — is constant in namespace of this class. Your example is wrong.
Correct example is:
class Node
{
…
}
namespace Node
{
export const ELEMENT_NODE: number = 3;
}
But in this case I have troubles with inheritance.
@Avol-V Yes, you are right, misunderstood. I can see now that your point is about inheriting class constants. The problem here is that this issue is bloated and should be split. We are all talking about different things.
Some properties in JavaScript are actually read-only, i.e. writes to them either fail silently or cause an exception. These should be modelable in TypeScript.
Previous attempts to design this have run into problems. A brief exploration:
Possible solutions?