microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.07k stars 12.49k forks source link

Interface with readonly property is assignable to interface with mutable property #13347

Open danielearwicker opened 7 years ago

danielearwicker commented 7 years ago

TypeScript Version: 2.1.4

Code

interface MutableValue<T> {
    value: T;
}

interface ImmutableValue<T> {
    readonly value: T;
}

let i: ImmutableValue<string> = { value: "hi" };
i.value = "Excellent, I can't change it"; // compile-time error

let m: MutableValue<string> = i;
m.value = "Oh dear, I can change it";

Expected behavior: The assignment of i to m would fail, to stop us accidentally allowing value to be modified.

Actual behavior: The assignment is allowed.

The current behaviour was a deliberate choice so this is a breaking change (or strict flag) feature request rather than a bug report!

The Handbook has this snippet:

let a: number[] = [1, 2, 3, 4];
let ro: ReadonlyArray<number> = a;
ro[0] = 12; // error!
ro.push(5); // error!
ro.length = 100; // error!
a = ro; // error!

It notes that a = ro being an error is helpful. But this happens because ReadonlyArray has no push method, making it incompatible with Array.

My example above seems "morally equivalent" to modelling the input/output flow of values with separate methods:

interface MutableValue<T> {
    getValue(): T;
    setValue(v: T): void;
}

interface ImmutableValue<T> {
    getValue(): T;
}

declare let i: ImmutableValue<string>;
i.setValue("Excellent, I can't change it"); // compile-time error

let m: MutableValue<string> = i;
m.setValue("Oh dear, I can change it");

And sure enough, this stops the assignment of i to m.

Would be great if mutable and readonly properties had the same relationship as if they were modelled by separate get/set methods (which of course they might actually be, via property getter/setters).

danielearwicker commented 7 years ago

Judging from #6614 there was an idea to add a mutable modifier. If I'm correct in assuming that this would mean:

interface MutableValue<T> {
    mutable value: T;
}

interface ImmutableValue<T> {
    readonly value: T;
}

and that would stop i being assigned to m because I've explicitly said that value must be mutable, then this whole issue can be boiled down to "Please add the mutable modifier"! đź‘Ť

aluanhaddad commented 7 years ago

@danielearwicker I think you are channeling @aleksey-bykov and his issue readonly modifiers are a joke

danielearwicker commented 7 years ago

Sometimes I think I'm way too polite!

DavidKDeutsch commented 7 years ago

I don't see an obvious "vote for this" button, so count this as my +1. Just ran into a nasty bug of mine where I was incorrectly modifying a member of a class instance, so I changed the type using ReadOnly, hoping I would get a compile error to quickly point my lazy self to there I was assigning it to a "normal" T, but no luck. As far as I am concerned, there is no difference between allowing an assignment of a type ReadOnly to a T and allowing an assignment of an Object to, say, an Array.

Pauan commented 7 years ago

@DavidKDeutsch In the upper-right corner of every comment, there is a +:grinning: button. If you click that, you can choose the :+1: reaction. This counts as a plus vote. You can do this on any comment, but often it is done on the first comment.

dead-claudia commented 7 years ago

Here's a repro demonstrating the issue online.

dead-claudia commented 7 years ago

The correct behavior in that repro would have the fifth line (let roAsRw: RW = ro) be an error.

leoasis commented 7 years ago

Has adding this behavior behind a configuration flag been already discussed internally or externally? If so, what has been the outcome of that? I think adding this behavior as a feature flag would encourage people to try this and work with library definitions to add readonly modifiers as needed. Then at some point in the future in a major version of Typescript this could be set by default.

aluanhaddad commented 7 years ago

@leoasis https://github.com/Microsoft/TypeScript/issues/13002

Ailrun commented 6 years ago

@RyanCavanaugh Is there any progress?

EisenbergEffect commented 6 years ago

I'd love to see if there's any progress on this. I was just teaching some engineers about TypeScript today, showing different aspects of interfaces, optional and readonly properties. I went a little off my script and showed something like above....which didn't work how I would have expected.

I'd love to see the readonly constraint respected. It's particularly relevant for some of the work we're doing on the vNext of Aurelia, which is all TypeScript. We also have a lot of immutable scenarios in my day job's codebase where it would be nice to get some help from the compiler...

DavidSouther commented 6 years ago

I think this is more likely to come up when calling a mutating function:

interface Foo {
  id: string;
}

const foo: Readonly<Foo> = {
  id: 'original ID',
}

function mutateFoo(foo: Foo) {
  foo.id = 'new ID';
}

mutateFoo(foo);

As in the prior cases, it's "obvious" that a coercion is happening, here mutateFoo is possibly deep in some library I have no idea about.

If readonly were a narrowing modifier and that were prevented, I could have (in 2.8+) something like:

type Mutable<T> = {
  -readonly [k in typeof T]: T[k]
}

foo = mutateFoo(Mutable<foo>); // Hey look I know foo will be mutated and explicitly opted in!
Siegrift commented 5 years ago

Any update on this?

saviski commented 5 years ago

It would be useful to use some type testing like inherits, something like is readonly.

interface TypeWithReadOnlyProperties {
  a: any;
  readonly b: any;
}

type TypeWithOnlyMutableProperties<T> = {
  [P in keyof T]: T[P] is readonly ? never : T[P];
}

then

type OtherType = TypeWithOnlyMutableProperties<TypeWithReadOnlyProperties>;

is equivalent to 

interface OtherType {
  a: any;
  b: never;
}

It could be extended to allow checking for other modifiers in classes, like public, private, async.

dead-claudia commented 5 years ago

@saviski A much cleaner solution to that is propagating the readonly modifier with the key to the strings in keyof types, propagating it from the key strings to the resulting type in P in ..., and adding readonly PropertyKey as a type. So your TypeWithOnlyMutableProperties<T> would change to this:

type TypeWithOnlyMutableProperties<T> = Pick<T, Exclude<keyof T, readonly PropertyKey>>;

It'd also make mapped + indexed types less of a hack to work with in the face of readonly keys.

maxlk commented 5 years ago

@RyanCavanaugh back in 2017 you said (https://github.com/Microsoft/TypeScript/issues/13002#issuecomment-271384763) that adding --enforceReadonlyAssignability should be tracked here. But there is no any discussion/decisions/roadmap in this thread since that.

Is this feature planned to be implemented sometime? Is it abandoned? Is it waiting for something?

trusktr commented 4 years ago

Is it waiting for something?

My guess is it's waiting for someone who values this with high enough priority over other features to make a pull request. :smiley:

Does TypeScript have bounties on issues? Maybe people can donate bounties to encourage others to complete accepted feature requests?

peter-leonov commented 4 years ago

How do you people effectively use Readonly in real life? Are there any workarounds, like passing a readonly structure to any function and not breaking the promise of readonlyness?

danielnixon commented 4 years ago

An ESLint rule to help prevent this scenario: https://github.com/danielnixon/eslint-plugin-total-functions#total-functionsno-unsafe-readonly-mutable-assignment

remcohuijser commented 3 years ago

A gentle nudge in the hope this topic is not forgotten. The fact that you can accidentally change the value of something that is not supposed to be changed make readonly unusable. A real shame as it would be a nice way to expose something to the outside of a class in an effective way.

remcohuijser commented 3 years ago

Playing around a bit more with this a bit more shows this case:

interface EditableName{
    name: string;
}

function setNameToHi(e: EditableName){
    e.name = "hi"
}

class User{
    get name(){
        return "my name"
    }
}

const user = new User();

setNameToHi(user);

This actually compiles fine, but on runtime completely breaks as there is no set name on the User class. In my humble opinion, the compiler should treat readonly and editable fields structurally different. The documentation already states that getters are considered readonly, so this would also solve the issue above.

shicks commented 3 years ago

Context on the recently-linked #21152: this is about a readonly interface keyword, which might allow a way to fix this without breaking existing usages. Basically, if a readonly interface had an extra bit that prevented automatic downcasts to a non-readonly type, then folks can opt into the stricter checks.

jtbandes commented 3 years ago

Worth noting that Flow already handles this correctly:

/* @flow */

type State = { x: number; y: string; };

function mutate(state: State) {
  state.x = 1;
}

function definitelyDontMutate(state: $ReadOnly<State>) {
  mutate(state);
//       ^ Cannot call `mutate` with `state` bound to `state` because property `y` is read-only in `State` [1] but writable in `State` [2]. [incompatible-variance]
}

I'd be in favor of an optional flag like --strictReadonlyChecks to opt-in to stronger type checking for those who are willing to wade through some breaking changes.

adrian-gierakowski commented 2 years ago

I'd be in favor of an optional flag like --strictReadonlyChecks to opt-in to stronger type checking for those who are willing to wade through some breaking changes.

any feedback from the maintainer if this is something that would be possible to add in the near or longer term?

shicks commented 2 years ago

Not a maintainer, but I'll add that this is particularly relevant in light of the Records and Tuples proposal that's progressing through TC39 (#49243 already references this issue). It would be great to see some progress here.

lallenlowe commented 2 years ago

The cold hard fact is that TS allows you to assign an immutable type to a mutable one, which does not reflect the reality of what's happening in JS. Leading to object is not extensible errors that could have been protected against by the type system.

Offirmo commented 1 year ago

Another example here https://github.com/microsoft/TypeScript/issues/13923#issuecomment-1347610117

SimonAlling commented 1 year ago

This is an astonishingly unsound aspect of TypeScript's type system.

One need not even mention interfaces or classes at all:

function modify(x: { foo: boolean }) {
  x.foo = true;
}

const obj = { foo: false } as const;

modify(obj);

After the code above, obj.foo has the static type false, but its value is in fact true – and that's without using any explicit escape hatches (like type assertions or any) whatsoever.

What makes it particularly weird is that this equivalent bug is correctly detected:

function modify(x: [ boolean ]) {
  x[0] = true;
}

const arr = [ false ] as const;

modify(arr);
//     ^^^
// error TS2345: Argument of type 'readonly [false]' is not assignable to parameter of type '[boolean]'.
//   The type 'readonly [false]' is 'readonly' and cannot be assigned to the mutable type '[boolean]'.

Please consider @jtbandes's suggestion:

I'd be in favor of an optional flag like --strictReadonlyChecks to opt-in to stronger type checking for those who are willing to wade through some breaking changes.

cefn commented 1 year ago

I agree, @SimonAlling . From my ticket closed in favour of this, I was struck by the variance versus ReadonlyArray.

Assignment of a non-array Readonly<T> to a T should be a compile-time error in the same way that assignment of a ReadonlyArray<T> to a T[] type is a compiler error.

https://github.com/microsoft/TypeScript/issues/51864

I think this is in itself a reason to move towards a fix, not leaving Typescript with an inconsistent approach with unexpected variances between as const for lists vs. lookups that need tracking, discovering, explaining.

jlandrum commented 1 year ago

JavaScript cannot enforce this and as such it would break the notion that TypeScript is a superset of JavaScript. TypeScript doesn’t care what you assert an objects type to be, as long as it conforms. A field can be made immutable but it cannot be defined as immutable in JS, and it just as easily can be made mutable again. Just as explicitly making a field writable is easy, so is asserting an object is something else in TS, and in both cases you are given enough information to see that is happening.

It’s a common misconception that typescript created objects have types, they don’t. It feels like it at times only because typescript is very good at keeping track of what an object is asserted to be along its journey, but at the end of the day there’s only objects.

danielearwicker commented 1 year ago

@jlandrum following your line of reasoning, is TS ever able to produce type error messages?

e.g. If a variable says it can refer to an object with a property X that must be a number, of course at runtime in JS we could set x to be a string.

shicks commented 1 year ago

@jlandrum It's also incorrect that JavaScript cannot enforce this. You can write

Object.defineProperties(objectWithSomeReadonlyProp, {readonlyProp: {value: 42}});
Object.freeze(objectWithAllPropsFrozen);

In both cases, attempts to write to the readonly properties will fail at runtime (in strict mode) and those properties should rightly be typed as readonly in TypeScript. But as assignment to a same-shape object with mutable properties is currently allowed, which would then allow code that mutates these properties to typecheck.

cefn commented 1 year ago

This is a feature with broad community support and several ideas how to resolve it. It is a change which I would expect to have no effect on any existing bug-free Typescript code and doesn't change the language - just its conformance to expectations.

To move this issue along, below is a simple repro from my original issue which we could discuss concretely. What's the cost/obstacle/impossibility/mystery/indecision of making the Object case correct?

See compiler errors in playground

/** ARRAY */

// MUTATION CORRECTLY PREVENTED
const frozenPlanets = Object.freeze(["earth"]);
// correctly a compile-time error
frozenPlanets.push("mars");

// ASSIGNMENT CORRECTLY PREVENTED
let planets = ["earth"];
// prevented by compile time error (implicitly re-adds push) 
// this would otherwise allow error below
planets = frozenPlanets;

// would raise error at runtime in strict mode
// but was guarded by compile-time error above
planets.push("mars");

/** OBJECT */

// MUTATION CORRECTLY PREVENTED
const frozenPlanet = Object.freeze({
  name: "earth",
});
// correctly a compile-time error
frozenPlanet.name = "mars";

// ASSIGNMENT INCORRECTLY ALLOWED
let planet = {
  name: "earth",
};
// incorrectly allowed, (implicitly re-adds mutability) leading to the uncaught error below
planet = frozenPlanet;

// would raise error at runtime in strict mode
// AND NOT GUARDED by any compile time error above
planet.name = "mars";

Disclosure: It's significant to me personally as I am maintaining a React state store which expects state to be treated as immutable. This project would benefit hugely from compile-time immutability. Without this feature, even when they are flagged as recursively readonly, engineers elsewhere in the codebase may not realise values are immutable, when assigning them to a new name can cause them to suddenly regain mutability.

olfek commented 1 year ago

One option is to force ALL function parameters to be readonly with ESLint rule:

https://typescript-eslint.io/rules/prefer-readonly-parameter-types/

jlandrum commented 1 year ago

@jlandrum following your line of reasoning, is TS ever able to produce type error messages?

e.g. If a variable says it can refer to an object with a property X that must be a number, of course at runtime in JS we could set x to be a string.

Given the negative response, it's clear everyone is completely missing the point.

Typescript is absolutely able to produce type error messages because in scope, it has an idea of what things should be (and when it doesn't - any kicks in)

Typescript doesn't care if X is a number or a string at compile time. It will throw an error (or not, if told not to) - but it will compile just fine. People are expecting TypeScript to be far FAR more than it is.

As for the comment about freeze - that doesn't make the prop readonly. All it does is tell the runtime to block attempts to mutate. This is identical to Java in that it isn't the variable itself that's immutable, but something outside of the variable telling it to panic.

When you tell TypeScript "Hey, this thing is immutable" - in the scope of that thing, TypeScript will do its best to ensure immutability. Once that object leaves that scope however, all bets are off. Use of freezing is 100% absolutely outside of the scope of TypeScript - it introduces a side effect.

When people call TypeScript a "glorified linter" - it's misleading (optional chaining, for example, isn't something usually done with a linter) but not entirely inaccurate. Since JavaScript lacks the ability to - at time of definition - establish a variable as read only, it would then make TypeScript responsible of knowing when exactly a readonly property should in fact, be frozen.

All that to say - by the very nature of TypeScript - it still MUST support JavaScript interop, and the mere fact of passing something from TS to JS then back to TS shouldn't break things.

And considering most every other language behaves the same way - consts in C, C++, Java, can be mutated, it's ridiculous to expect TypeScript to somehow be more powerful than virtually every other language out there. The only difference with most of these other languages is types carry with the object through the lifecycle of the data, TypeScript isn't and was never intended to enforce that 100%.

Because - at the end of the day, this is still JavaScript - you MUST treat it like so. the most TypeScript can do is detect a cast and throw a warning such as "Casting from type X to type Y will make property X mutable", it can't change the nature of the property.

cefn commented 1 year ago

Many tracking this thread are Typescript experts. They do not oppose your view that Typescript transpiles to Javascript.

The negativity comes when this commentary seems out of place and presents an obstacle to others examining what I personally consider to be a well-defined and valuable potential typescript feature.

the most TypeScript can do is detect a cast and throw a warning such as "Casting from type X to type Y will make property X mutable

That's almost a statement of the feature we are discussing. A more precise one would be...

the most Typescript can do is detect an assignment (of an immutable reference to a mutable one) and throw a compiler error to reject it

If Typescript did that, we would already have our feature. It does not.

Please can we keep discussion focused on understanding if this is a well-defined feature, if it is valuable and should be a future feature of the language.

jlandrum commented 1 year ago

I once read an article that Adobe won't hire anyone who calls themselves an expert at any of their software, because it's too complex for any one person to understand but a tiny portion of the entire package. And knowing how to use something very well doesn't make someone an expert; you can call yourself an expert driver - and be a very good one - without knowing how to change your oil.

That said, you can call yourself an expert all you want - but being arrogant about your knowledge will only get in the way of your professional development.

The point still remains that TypeScript can't enforce something that isn't intrinsic to the data. I think the majority of people would agree adding checks to see if properties are read only before casting would be excessive. It also wouldn't be simple because a properties read only state only exists at runtime - which is the crux of this problem: there is no way for TypeScript to truly know if an object being cast with a readonly modifier is TRULY readonly. The only way to implement this would break a LOT of other things about TypeScript.

Edit: And this ignores that implementing it would take user choice away - what if you want something in one context to be readonly to prevent accidentally mutating it, but it needs to be mutable to something else? It's not an uncommon pattern at all in other languages where certain things including the object itself can mutate but what's given to the user of the API is immutable. In this case the roles are reversed, the library doesn't use readonly but what's exposed to the user does. If you then need to pass the now readonly object back to the library for mutation, this change wouldn't allow that pattern at all.

cefn commented 1 year ago

The point still remains that TypeScript can't enforce something that isn't intrinsic to the data.

Typescript routinely enforces things which are not 'intrinsic to the data' in JavaScript. This could be a summary definition of the Typescript language. That is why people find this line of argument so difficult to connect with.

Having said that, I think there is increasing clarity about what you think the proposed feature is, (somehow being aware of an object's runtime state), which might be impossible, and what the actual proposed feature is.

If you examine the case of Readonly arrays you can see the proposed feature is already implemented. Assignment of an immutable array to a mutable reference is an error. If it is already typed Readonly you can't implicitly make it mutable again by just assigning it to a mutable reference. You can do it with an explicit cast but not merely by assignment, which ought (normally) to be a type-safe operation.

However this is not implemented in a consistent way for e.g. object literals with keys, even when Typescript CAN infer that it is read-only. Examples of code that can infer a value is immutable are explicit declarations like Readonly or as const and invocations of Object.freeze.

No-one is suggesting Typescript tracks any runtime feature of the value. We simply ask that object reference assignment should be consistent in behaviour with array reference assignment when the value is typed Readonly hence allowing immutability to be a reliable behaviour.

Please allow the group pursuing this feature the use of this thread for advocacy and explanation so we can progress it.

jlandrum commented 1 year ago

That's the important distinction.

readonly a: string[]

Is not

a: readonly string[]

This is field access control versus type access control which is why the current implementation IS consistent. One is saying that what holds the data cannot be changed, the other is saying the data itself cannot be changed.

One IS a property of the value (albeit virtual to TS, not in reality) - the other is a property of the type.

And what's being said is "you're allowed to contribute to the conversation - but only if you agree with us"

cefn commented 1 year ago

I pasted both the example lines you shared into the Typescript playground and of course neither of the lines compile as Typescript.

That is to say, without the context of a more complete example, the intent of these fragments is hard to interpret, especially when it is the type of the containing object that this discussion relates to....

image

I feel discussions are more likely to progress with a https://en.m.wikipedia.org/wiki/Minimal_reproducible_example

You wrote:

This is field access control versus type access control

I couldn't find any matches on Google related to the Typescript language for the phrases "field access control" or "type access control". Are these concepts that are well-defined in some other language?

You wrote:

And what's being said is "you're allowed to contribute to the conversation - but only if you agree with us"

Not at all. I would welcome discussion of the reference shared at https://tsplay.dev/W4bY7N. This illustrates a concrete example with whole (compiling) typescript statements so that there's no ambiguity or confusion.

In that example, Object.freeze illustrates the worst possible outcome - a runtime error not guarded by Typescript, Cannot assign to read only property 'name' of object '#<Object>' even though it COULD guard from it.

If the use of Object.freeze is distracting us from the focus of this issue, you can see the same issue illustrated using as const at https://tsplay.dev/wRPnnm which leads to an error of intent (but not a runtime error).

In the reproducible examples, all statements compile, except those that illustrate Typescript's

I hope these minimal reproducible examples go some way to structuring the discussion.

In those examples the line planet = frozenPlanet; is not an error.

In my view it should be an error (or there should be a flag to make it so).

Are you saying

jlandrum commented 1 year ago

The fact you're trying to run obviously incomplete code to invalidate what's being said means either you don't have an understanding of the language or you're acting in bad faith.

The entire reason this "issue" exists is because TypeScript has the ability to mark an FIELD as readonly. A readonly FIELD is NOT a readonly TYPE.

Every single language that offers this pattern of making a read only field allows modification of data by asserting the data is something else.

You have to ask yourself why there's a ReadOnlyArray<> in the first place, and why there isn't a ReadOnly<>. You have to look to the fact that JavaScript allows mutating const objects. You have to look at the fact there's more ways to bypass readonly than there is to enforce it.

And given that TypeScript is a superset of JavaScript it MUST use JavaScript's rules. Const only works on primitives, Object.freeze only works on objects (and doesn't make the DATA immutable, it makes the ACCESSORS block mutation), Object.defineProperty is reversible.

For every argument to support this functionality there are multiple fundamental reasons in software engineering not to, and at this point it's safe to assume everyone who continues to push for this without trying to understand the problem it causes is acting in bad faith, and a discussion can't continue.

If you want to bring in that the people here are TypeScript "experts" - I've got 23 years of experience across 30 different programming languages. I wouldn't say I'm an expert of any of them, but I will say I've been around long enough varying concepts around immutability and that this push to make a field's attribute impact the field's data is unheard of.

adrian-gierakowski commented 1 year ago

@jlandrum I am sorry if this offends you but it seems to me that the only person acting in bad faith in this thread is you. Or maybe you are just misinformed and/or confused.

Please read this post to learn about why readonly modifier was initially implemented to not affect subtype and assignability type relationships of the containing type. It was purely a pragmatic decision to maximise backwards compatibly (which is good for adoption). It had nothing to do with what could or should be done in TypeScript due to it being a superset of JavaScript.

Unless you make an honest effort to understand what this issue is asking for, I would appreciate if you stopped posting further comments as they are detracting from a productive discussion and spam subscribers’ inboxes.

iliubinskii commented 1 year ago

Returning to the code sample in the first post... You are welcome to stop your Don Quixote windmill fighting and just use an eslint rule (misc/typescript/no-unsafe-object-assignment) that catches this and one more unsafe pattern: image

cefn commented 1 year ago

The fact you're trying to run obviously incomplete code to invalidate what's being said means either you don't have an understanding of the language or you're acting in bad faith.

I genuinely don't know what Typescript language features you're referring to in the examples and given the type of the containing object is unspecified in your example I wouldn't be able to realign them with actual typescript code for this reason.

The entire reason this "issue" exists is because TypeScript has the ability to mark an FIELD as readonly. A readonly FIELD is NOT a readonly TYPE...You have to ask yourself why there's a ReadOnlyArray<> in the first place, and why there isn't a ReadOnly<>.

Actually there IS a Readonly<> modifier for data structures. That's the feature we are discussing in this thread. You can read about it at https://www.typescriptlang.org/docs/handbook/utility-types.html#readonlytype

Every single language that offers this pattern of making a read only field allows modification of data by asserting the data is something else.

And you would be able to explicitly cast a Readonly<T> to T if the constraint was unwanted.

And given that TypeScript is a superset of JavaScript it MUST use JavaScript's rules.

Seems in direct contradiction with the nature of Typescript as a type-erasure language. https://en.m.wikipedia.org/wiki/Type_erasure

For every argument to support this functionality there are multiple fundamental reasons in software engineering not to and at this point it's safe to assume everyone who continues to push for this without trying to understand the problem it causes is acting in bad faith, and a discussion can't continue.

The discussion might continue if you would consider the specific code example and respond to the quite-specific questions I shared about it.

If you want to bring in that the people here are TypeScript "experts"

Some of those observing this thread are (probably) creators and maintainers of the language. I don't claim any expertise at that level.

I've got 23 years of experience across 30 different programming languages

That's more than me!

this push to make a field's attribute impact the field's data is unheard of.

I don't know how to relate this claim to any of the things we have discussed, sorry. The proposal concerns how to handle assignments of values that were composed using the readonly modifier like those defined with the Readonly<> utility type. You could say those language features are designed specifically to 'impact' statements which try to write field data. They cause those statements to be compile-time errors. OP wants to add another one to the list.

jlandrum commented 1 year ago

@adrian-gierakowski looking at that post only further supports my argument that this does not need to change. This change simply doesn't make sense. This discussion has been littered with attempts to justify such a decision that aren't based on anything other than wanting TypeScript to do something that isn't found anywhere else and adds nothing that good coding practices doesn't already accomplish - if you don't want a readonly field to be mutated, just don't cast. There's no reason to stop someone who should know what they're doing from doing what they want to do.

I'm absolutely not acting in bad faith, I've been trying to help others understand but have been met with nothing but harsh negativity and whataboutism. As someone who has constantly defended typescripts use, and understanding that community drives a languages support, this is a very bad look for the TypeScript community - nobody wants to be a part of something that's designed by popular opinion and not pragmatic understanding.

dead-claudia commented 1 year ago

Can you all please take this dramadiscussion elsewhere? It at this point barely has anything to do with the original feature request. And I'm getting tired of the needless pings here where I'm watching the issue.

jlandrum commented 1 year ago

@cefn You're correct that there is Readonly (which honestly I wasn't aware of) but it doesn't make the value readonly like ReadOnlyArray does - it makes the properties of T readonly. So it won't work on the object itself. And if the argument was that Readonly allows being cast to T, I would be in agreement that it shouldn't be possible (as it isn't with ReadOnlyArray. If that were the discussion I wouldn't have any issues with it. And it in fact does seem to provide the protections that are being asked for here, just not to the degree wanted for the reasons stated above - but understandably so - JavaScript doesn't have that ability: a value can NOT be read only in JavaScript, only it's accessors can be restricted.

The only reason I'm pushing hard against this is the unnecessary complexity this adds to a language that should remain flexible - and it doesn't give any example of provided value in doing so.

Looking at the given example:

interface MutableValue<T> {
    value: T;
}

interface ImmutableValue<T> {
    readonly value: T;
}

let i: ImmutableValue<string> = { value: "hi" };
i.value = "Excellent, I can't change it"; // compile-time error

let m: MutableValue<string> = i;
m.value = "Oh dear, I can change it";

There's already language features for cases where this would be an issue - if the intent is to extend off of ImmutableValue then it should use extends - at which point unless explicitly overridden, it will be readonly. There would be zero reason to use the example code unless the intent WAS to make it mutable; and this reflects vanilla JS:

const immutable = {};
Object.defineProperty(immutable, 'value', { value: 'hi', writable: false, configurable: true });
...
Object.defineProperty(immutable, 'value', { writable: true });
immutable.value = "Excellent, I can't change it";

But readonly doesn't do the same as Object.defineProperty - but it would have to in order to maintain TS being a subset of JS and thus work with JS - what it does is say "Objects of this type, throw an error if an attempt to mutate is made". And because the intent is erasure, the only way to ensure this is if readonly does the equivalent of:

interface MutableValue<T> {
    value: T;
}

interface ImmutableValue<T> {
    readonly value: T;
}

let i: ImmutableValue<string> = { value: "hi" };
Object.defineProperty(i, 'value', { writable: false });

i.value = "Excellent, I can't change it"; // compile-time error

let m: MutableValue<string> = i;
m.value = "Oh dear, I can change it"; // run-time error

Because the first compiles to:

"use strict";
let i = { value: "hi" };
i.value = "Excellent, I can't change it"; 
let m = i;
m.value = "Oh dear, I can change it";

Whereas the second compiles to:

"use strict";
let i = { value: "hi" };
Object.defineProperty(i, 'value', { writable: false });
i.value = "Excellent, I can't change it";
let m = i;
m.value = "Oh dear, I can change it";

And most importantly is the fact that by nature of TypeScript, this line: i.value = "Excellent, I can't change it"; // compile-time error does NOT prevent compilation; it compiles and runs just fine - just with errors presented to the author. And unfortunately - projects with many TS errors being deployed to production is unusually common, I see more projects failing automated tests for reasons like this than not.

And I think it comes down to misunderstanding what TypeScript is - it's not strictly typed in the sense that the code MUST conform, but that it helps ensure the code DOES conform (which is why it has gained the sentiment that it's a "glorified linter" which isn't entirely correct but not too far off either). Changes like these are people wanting TypeScript to be more than what it set out to be, at which point there already exists options like Kotlin - which understandably, setting up is a major pain point and the massive runtime is a concern, but if you NEED this strict level of conformity with types, JavaScript nor TypeScript are the answer to that.

Because at the end of the day, an object is only what you tell TypeScript it is at the time of compile-time checking, and readonly only modifies the shape of the object in the context of compile time checking, it doesn't change anything about the value itself (because that would go against TypeScript being a superset JavaScript - it's now generating code).

If it's that important the value not change, it should be up to the developer to take the extra steps to ensure such, it's why we have classes and Proxy and so on. If you want a good example of where this may be harmful:

interface InternalState<T> {
    property: T;
}

interface ExternalState<T> {
    readonly property: T;
}

function beginTransaction(): ExternalState<string> {
    const state: InternalState<string> = { property: "not ready" };
    return state;
}

function prepareTransaction(state: ExternalState<string>) {
    (state as InternalState<string>).property = "ready";
}

const transaction = beginTransaction();
transaction.property = "this won't work."; // Compile-time error.

prepareTransaction(transaction); // No errors

If readonly were enforced, this would not be possible - and while this example is too simple to showcase the true ability, it should illustrate why I'm so dead set on avoiding it; even making it a flag is still supporting working against potentially powerful patterns.

cefn commented 1 year ago

Thanks for clarifying.

You wrote:

if the argument was that Readonly allows being cast to T, I would be in agreement that it shouldn't be possible

In general there are few limits on casting between items of the same fundamental nature, so assigning a cast value is allowed, with or without this feature. This facilitates the pattern which is rightly needed where the developer 'knows best', very much like your transaction example.

const myRoVal: Readonly<T> = createThing();
const myRwVal: T = myRoVal as T; // here `as T` forces the issue with a cast

However, my wish is that Readonly<T> shouldn't be able to be assigned to T without a cast. If this is your view too then we are in agreement.

const myRoVal: Readonly<T> = createThing();
const myRwVal: T = myRoVal; // this should be an error because there's no no cast, but it isn't an error

Here's a version of the example you shared, which demonstrates how Readonly<T> therefore has surprising (and problematic) behaviour because it is too loose today...

interface State<T> {
  property: T
}

function beginTransaction(): Readonly<State<string>> {
    return { property: "not ready" };
}

function prepareTransaction(state: Readonly<State<string>>) {
  // @cefn: an explicit cast is always possible to make something mutable again if you need it
  (state as Mutable<typeof state>).property = "ready"; 
}

// @cefn: here the assignment of a Readonly<T> to a T should be an error - there is no explicit cast to Mutable
const readwriteTransaction: State<string> = beginTransaction();
readwriteTransaction.property = "this shouldn't work." // @cefn: because of the assignment loophole this is now implicitly allowed without error
prepareTransaction(transaction);

/** The 'opposite' of the Readonly<T> utility type - useful for casting. */
type Mutable<T> = { -readonly[P in keyof T]: T[P] };

And most importantly is the fact that by nature of TypeScript, this line: i.value = "Excellent, I can't change it"; // compile-time error does NOT prevent compilation; it compiles and runs just fine - just with errors presented to the author.

The use of strict compiler options and test pipelines is a must and it's true of every production context which has used Typescript in my experience. Compiler errors like these create a non-zero exit code and are treated as a pipeline failure. No code can therefore go to production with errors of this kind. If this feature landed in Typescript these mechanisms would prevent a number of real-world errors in production codebases, where otherwise immutability would be silently violated by this assignment loophole.

jlandrum commented 1 year ago

This is a slippery slope IMO - the developer must know the field exists and ideally that it's marked readonly if they're looking to cast it.

And you said Readonly - this I 100% fully agree with, I tested it and it allows casting far too liberally; it still does have the issue of transport between JS but that's always going to be an issue (and that's a whole other can of worms involving any vs unknown)

But this is about the readonly field modifier - not the type modifier. I simply can't think of a single language that the behavior being presented is normal - ducktyped or otherwise - except JVM languages and that's simply because everything is a class, so everything has a backing field that gets carried with it (probably not the most correct way to state that but hopefully understandable)

What this tells me is the proposed actions are still not good for the language but that Readonly should be expanded to work with native types. I love the idea of needing to cast a Readonly to a Mutable, I just still really don't like the idea of a field - which is a container for a value - being the arbiter of mutability when moving the data between types.

Field modifiers as opposed to type modifiers should only apply to the field itself, but blocking casting of the type would make it behave far too much like a type modifier; and as I brought up, defining a field as readonly that holds an array behaves differently than ReadonlyArray for this reason.

RyanCavanaugh commented 1 year ago

Hey folks, let's dial it down a notch. Remember the principles of the code of conduct

In no particular order:

I don't see any block-worthy behavior here but multiple people are quite close to crossing that line and I'd obviously prefer to not have to do that. Thanks!