TypeScript icon indicating copy to clipboard operation
TypeScript copied to clipboard

Interface with readonly property is assignable to interface with mutable property

Open danielearwicker opened this issue 8 years ago • 26 comments

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 avatar Jan 07 '17 23:01 danielearwicker

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"! 👍

danielearwicker avatar Jan 08 '17 00:01 danielearwicker

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

aluanhaddad avatar Jan 08 '17 00:01 aluanhaddad

Sometimes I think I'm way too polite!

danielearwicker avatar Jan 08 '17 08:01 danielearwicker

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<T>, 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<T> to a T and allowing an assignment of an Object to, say, an Array.

DavidKDeutsch avatar May 02 '17 15:05 DavidKDeutsch

@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.

Pauan avatar May 02 '17 20:05 Pauan

Here's a repro demonstrating the issue online.

dead-claudia avatar Jun 15 '17 07:06 dead-claudia

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

dead-claudia avatar Jun 15 '17 07:06 dead-claudia

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.

leoasis avatar Aug 25 '17 21:08 leoasis

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

aluanhaddad avatar Aug 25 '17 21:08 aluanhaddad

@RyanCavanaugh Is there any progress?

Ailrun avatar Feb 20 '18 20:02 Ailrun

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...

EisenbergEffect avatar May 09 '18 20:05 EisenbergEffect

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!

DavidSouther avatar Jun 01 '18 14:06 DavidSouther

Any update on this?

Siegrift avatar Jul 11 '19 08:07 Siegrift

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.

saviski avatar Jul 19 '19 10:07 saviski

@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.

dead-claudia avatar Jul 19 '19 15:07 dead-claudia

@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?

maxlk avatar Aug 11 '19 10:08 maxlk

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?

trusktr avatar Mar 21 '20 04:03 trusktr

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?

peter-leonov avatar Apr 03 '20 15:04 peter-leonov

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

danielnixon avatar Jun 27 '20 03:06 danielnixon

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 avatar Dec 22 '20 06:12 remcohuijser

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.

remcohuijser avatar Dec 22 '20 06:12 remcohuijser

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.

shicks avatar Jul 15 '21 16:07 shicks

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.

jtbandes avatar Aug 31 '21 00:08 jtbandes

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?

adrian-gierakowski avatar Oct 13 '22 13:10 adrian-gierakowski

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.

shicks avatar Oct 13 '22 17:10 shicks

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.

lallenlowe avatar Oct 18 '22 17:10 lallenlowe

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

Offirmo avatar Dec 15 '22 03:12 Offirmo

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.

SimonAlling avatar Jan 05 '23 10:01 SimonAlling

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.

cefn avatar Jan 05 '23 14:01 cefn

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.

jlandrum avatar Mar 06 '23 11:03 jlandrum