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

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

Open
danielearwicker opened this issue Jan 7, 2017 · 56 comments · May be fixed by #58296
Open

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

danielearwicker opened this issue Jan 7, 2017 · 56 comments · May be fixed by #58296
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@danielearwicker
Copy link

danielearwicker commented Jan 7, 2017

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
Copy link
Author

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
Copy link
Contributor

@danielearwicker I think you are channeling @Aleksey-Bykov and his issue readonly modifiers are a joke

@danielearwicker
Copy link
Author

Sometimes I think I'm way too polite!

@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Jan 8, 2017
@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature and removed In Discussion Not yet reached consensus labels Jan 9, 2017
@DavidKDeutsch
Copy link

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
Copy link

Pauan commented May 2, 2017

@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
Copy link

Here's a repro demonstrating the issue online.

@dead-claudia
Copy link

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

@leoasis
Copy link

leoasis commented Aug 25, 2017

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
Copy link
Contributor

@leoasis #13002

@Ailrun
Copy link

Ailrun commented Feb 20, 2018

@RyanCavanaugh Is there any progress?

@EisenbergEffect
Copy link

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
Copy link
Contributor

DavidSouther commented Jun 1, 2018

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!

@jlandrum

This comment was marked as off-topic.

@cefn

This comment was marked as off-topic.

@jlandrum

This comment was marked as off-topic.

@cefn

This comment was marked as off-topic.

@jlandrum

This comment was marked as off-topic.

@cefn

This comment was marked as off-topic.

@jlandrum

This comment was marked as off-topic.

@adrian-gierakowski

This comment was marked as resolved.

@iliubinskii
Copy link

iliubinskii commented Oct 7, 2023

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

This comment was marked as off-topic.

@jlandrum

This comment was marked as off-topic.

@dead-claudia

This comment was marked as resolved.

@jlandrum

This comment was marked as off-topic.

@cefn

This comment was marked as off-topic.

@jlandrum

This comment was marked as off-topic.

@RyanCavanaugh
Copy link
Member

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

In no particular order:

  • If this feature request were ill-defined or beyond the scope of the language, rest assured I would have closed it already. It isn't.
  • Accusations of bad faith engagement should not be tossed around lightly
  • That said, if you can't engage with the substance of someone's comment without nitpicking (picking at the use of certain words, purposefully taking code substrings out of context, etc), that is bad faith engagement and I'll be asking you to refrain from participation

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!

@aboodman
Copy link

@RyanCavanaugh - can you please comment on the status of this bug from the project's perspective? I don't see any official comment from the ts team.

@RyanCavanaugh
Copy link
Member

It's in the long list of features which are plausible, but aren't currently a priority based on the overall tradeoff of what would be gained here (write safety under aliasing, which is already a soundness hole as relates to the types of the properties themselves) vs what would be required for anyone to be able to turn the option on (generally speaking, "everyone" would have to properly document readonly for it to be usable in practice).

@adrian-gierakowski
Copy link

generally speaking, "everyone" would have to properly document readonly for it to be usable in practice

It’s a chicken and egg problem

@cedw032
Copy link

cedw032 commented Apr 4, 2024

This is a pretty massive hole.

What do we have to do to get some movement on this?

Does anyone know why this keyword was added when it provides next to no protection?

EDIT: To be clear, I am asking if someone could point me towards the PR or issue that led to the addition of this feature so I can establish what the intention was, and how it ended up in the state that it's in. I am sure it was done with good intentions, and it seems unfortunate that it is stuck half way.

@MartinJohns
Copy link
Contributor

Relevant comment by RyanCavanaugh: #58236 (comment)

We're considering closing the { readonly x: number } -> { x: number } soundness hole under a flag, [..]

@ahejlsberg
Copy link
Member

ahejlsberg commented Apr 23, 2024

Now implemented in #58296.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet