Unsafe type-incompatible assignments should not be allowed

[jaredru]

TypeScript Version: 2.1.6

Code

interface StringOnly {
  val: string;
}

interface StringOrNull {
  val: string | null;
}

const obj: StringOnly = { val: "str" };

// should not be allowed
const nullable: StringOrNull = obj;

nullable.val = null;

// obj is now in a bad state

Expected behavior:
The sample should not compile, as it's not safe. Type casts/assertions should be required to override type incompatibility errors here. (Or, of course, cloning the object itself const nullable: StringOrNull = { ...str };).

For comparison, Flow does not allow the sample code.

Actual behavior:
The sample compiles and further accesses of str.val will likely result in exceptions.

EDIT: Removed all references to readonly, as discussion of that modifier is overshadowing the issue.

[mhegazy]

Duplicate of #13347

[jaredru]

@mhegazy This issue overlaps with #13347, certainly, but it's not a duplicate. There are more issues here than simply readonly. Union and optional/nullable types have similar problems:

type A = { val: string; };
type B = { val: string | number; };

const a: A = { val: "string" };
const b: B = a;

b.val = 3;

console.log(a.val.toUpperCase()); // throws

[mhegazy]

A literal is in the domain of string, similarly a string is in the domain or string or null. These work as intended.

[jaredru]

I'm sorry I'm not making myself clear, but that's not the point. The string value type is not the problem.

The reference type containing those value types is the problem. Again, I'll point out that Flow does not allow these assignments, because they are unsafe.

In my last example, type A is "in the domain of" type B, but the converse is not. However, by allowing the assignment of the variable a to b without copying it, the compiler is allowing us to act on A as if it were a B. That is bad behavior, intended or not.

[mhegazy]

Thanks for the explanation. TypeScript originally did not have readonly. so we opted to error on the side of usability, and considered properties to as "unknown" instead strictly read/write.

When readonly modifier was added, we did not want to make all existing code suddenly have different semantics, so properties not annotated with readonly modifier, are "unknown" instead of "mutable".

We have talked about --strictReadonlyChecks flag (#11481), in which all unannotated properties would be considered "mutable".

[jaredru]

I understand, but ignore readonly for now. I want to emphasize that this is still a problem:

const a: { val: string } = { val: "str" };
const b: { val: string | number } = a;

The second assignment should not be allowed. If b.val is changed to a number, a.val is now a number, as well. This is not safe.

[mhegazy]

This is unsafe if b.val is "mutable" . it is safe if b.val is "readonly". since we treat properties with no readonly as "unknown", i.e. possibly readonly, the check is skipped.

[jaredru]

As I said, please ignore readonly. This is not a duplicate.

The problem is fundamentally about allowing incompatible type assignments. This is a problem regardless of whether readonly existed in the language or not. Flow does not have readonly, and Flow does not allow these assignments. (I'm not suggesting TypeScript should blindly copy Flow; I'm pointing out that this is a safety problem they recognize and prevent).

I'm not suggesting that the value should be immutable, simply that allowing it to be assigned a number breaks the original reference's type. It's unsafe because we're treating val as string and string | number after the second assignment. The exact same object is referenced by two variables of two different, incompatible types.

Here's my example again, annotated further to explain the problem in more depth:

const a: { val: string } = { val: "str" };
// There is now one reference to the object.
// The object's type is `{ val: string }`.

const b: { val: string | number } = a;
// There are now two references to the **same** object.  We did not copy the
// object, we simply added another reference.
//
// However, our two references treat the object as different, incompatible
// types.  Flow does not have `readonly`, but Flow does not allow the previous
// assignment.

b.val = "a different string";
// This is OK.  The object being mutable is fine, so long as the types match.
// This does not break `a`.  `a` expects `val` to be a `string`, and it is.

console.log(a.val.toUpperCase());
// Yay, `a.val` is the type we expected, so this logs correctly.

b.val = 3;
// Since `b` references the same object as `a`, this breaks `a`.  The inner
// `val` is now a number, and not the expected `string`.

console.log(a.val.toUpperCase());
// This throws an Error, because `a.val` is no longer a `string` like the type
// specifies it should be.

[jaredru]

@mhegazy I've updated the original issue description to remove all references to readonly. That modifier was a simple example and largely irrelevant, yet the discussion has focused on it for some reason.

[mhegazy]

const a: { val: string } = { val: "str" };
const b: { val: string | number } = a;

in this example. the assignment is safe if you never write to b.val. if b.val is readonly, then there is nothing wrong with this assignment. it is OK for a reader that expects string | number to get a string.

The problem is, as you noted, if you were to write to b.val. e.g.

b.val = 2;
a.val.indexOf(""); // Error

Since TS did not have a way to designate a property as readonly, what does a property mean? is it readonly or is it readwrite. If you say readwrite, then yes, this is unsafe. if you say readonly then it is.

Again since we did not have a way to track accuratelly what is readonly and what is not, we opted to error on the side of usability. We opted to make it unknown, i.e. it can be readonly or readwrite. and thus the assignment is possibly safe.

If we have a mode (again --strictReadonlyChecks described above), we can say properties are always mutable unless marked with readonly modifier, and thus error on this assignment, but not on:

const c: { readonly val: string | number } = a;

[jaredru]

Right, but even if we supported --strictReadonlyChecks, I don't want it to be readonly is my point.

in this example. the assignment is safe if you never write to b.val.

Yes, but since we don't have a mechanism in the language for preventing that, my contention is that the assignment itself should be disallowed (as it is in Flow). If I want to ignore the error, I should have to assign it with a type assertion const b = a as { val: string | number }. If I don't want to ignore it, I should have to copy the object const b: { val: string | number } = { ...a }.

[mhegazy]

Yes, but since we don't have a mechanism in the language for preventing that, my contention is that the assignment itself should be disallowed (as it is in Flow). If I want to ignore the error, I should have to assign it with a type assertion const b = a as { val: string | number }. If I don't want to ignore it, I should have to copy the object const b: { val: string | number } = { ...a }.

That is what i meant by "we opted to error on the side of usability.". and that is what i was proposing with --strictReadonlyChecks.
Enforcing the rule regardless would mean a breaking change to existing code bases.

[jaredru]

I should have clarified that I expected such a change would need to live behind a flag. I apologize for leaving that out.

However, I maintain that this is a separate issue from readonly. Even if --strictReadonlyChecks existed and was enabled, there's no reason (based on its name alone, at least) to believe that it would prevent the const b: { val: string | number } = a assignment. Neither a nor b's types have readonly values. Further, I don't want either type to be immutable; I want to be prevented from making an unsafe assignment while maintaining mutability.

So, --strictReadonlyChecks would be insufficient. @RyanCavanaugh closed #11481 with the comment:

We're thinking about a future with some kind of variance / mutability annotations but won't be doing this as a one-off feature.

My concern is that --strictReadonlyChecks only covers part of the problem, and treating this (broader) issue as closed on the basis of a subset of it doesn't give it the consideration it deserves.

[karldray]

Would --strictReadonlyChecks prevent the unsafe assignment in this example?

interface A { kind: "A"; foo(): void; }
interface B { kind: "B"; bar(): void; }

function f(x: A | B): void {
    switch (x.kind) {
        case "A": x.foo(); break;
        case "B": x.bar(); break;
    }
}

function setKindToB(x: A | B): void {
    x.kind = "B"; // clearly unsafe
}

const a: A = { kind: "A", foo() {} };
setKindToB(a); // uh oh...
f(a); // crash: x.bar is not a function

[mhegazy]

I would say so. the call setKindToB would be flagged. but function setKindToB(x: Readonly<A | B>): void would not.

[jaredru]

For clarity, do we agree that there could be a separate --strictTypeChecks flag distinct from --strictReadonlyChecks wherein mutability would be retained but the type checking of assignments would be more strict? And that this hypothetical flag would be valuable even if readonly as a concept didn't exist at all?

Whether or not such a flag would get prioritized and added is a separate issue, but I'd like to confirm that we're on the same page regarding the distinctness of this issue as compared to #13347.

[karldray]

I agree that readonly is too coarse in general because the safety of an assignment operation can depend on the type of the value being assigned. Example:

type AB = { prop: "A" | "B" };
type BC = { prop: "B" | "C" };

function f(x: AB | BC) {
    x.prop = "B"; // safe, should compile
    x.prop = "C"; // unsafe, should not compile
}

Maybe this can be solved by giving properties distinct "readable" and "writable" types. In this example, x.prop would be [readable: "A"|"B"|"C", writable: "B"]. Then readonly would just be a feature for explicitly making the "writable" type empty.

[mhegazy]

I would say it should be --srtictVarianceChecks, but now we are in the 🚲 🏡 zone.

[jaredru]

but now we are in the 🚲 🏡 zone.

Fair enough. 😄

Would it be reasonable to remove the "Duplicate" label and judge this on its own as a suggestion to add a --strictVarianceChecks flag? I understand that the suggestion may or may not gain any ground, but it would be nice to have it considered. (I'd be happy to open a new issue for that and simply close this, as well).

[jaredru]

Hi @RyanCavanaugh, I noticed that this suggestion was discussed in #14490. I was hoping you could go into a bit more detail about this comment:

Could anyone actually get this to work?

I'm curious if that refers to implementing support within TypeScript? Or if it refers to using the flag were it to be implemented?

The projects of course have different approaches, but I feel it's worth mentioning that Flow made object property types invariant by default a number of months ago.

[RyanCavanaugh]

Sorry, forgot to flush the notes to this issue.

Or if it refers to using the flag were it to be implemented?

This. Anyone who wants to use this flag would have to go through every definition file they reach and add property variance annotations. They may even have to split up types into separate "input" and "output" types. A similar exercise is to try to add const correctness to just one class in a nontrivial C++ program. You quickly realize that the entire program has to be converted for it to work.

I think the Flow type system is heading in a very complex direction by enforcing property variance before implementing array covariance - it's going to be a major headache either way at some point in their near future. For example, this program does not have an error in Flow (flowtype.org/try at v0.41), but has the same problem as the aliased-object situation in TypeScript:

function addANumber(arr: Array<string | number>): void {
  arr.push(10);
}

let x = ['hello'];
addANumber(x);
let j = x.pop();
j.substr(2); // No error, but fails at runtime

[jaredru]

Thanks for following up, and for the additional details. I appreciate it. I just have a few comments.

Anyone who wants to use this flag would have to go through every definition file they reach and add property variance annotations.

That's only true where the property types of the objects varied. If you're dealing with a fairly regular set of interfaces, I don't believe this would change much at all.

You quickly realize that the entire program has to be converted for it to work.

I'd agree with that, but I think the tradeoff would be worthwhile. We found this to mostly be true with --noImplicitAny and --strictNullChecks, as well, but we have more guarantees of correctness as a result (which is what we wanted in the first place).

If there's further discussion to be had here, I'd love to contribute however I can. In either case, thanks again for the notes and for considering the suggestion.

[kevinbarabash]

I'm interested in adding strict variance to TypeScript. I started working on this a few weeks ago and have it work for a few cases, see kevinbarabash#4. In that PR the flag is called --strictAssignment but it's the same thing that's being discussed here.

A couple of notes:

• it works with arrays as well as objects
• it allows covariant assignment of literals while enforcing invariance when the source is a variable

You quickly realize that the entire program has to be converted for it to work.

I think providing a way to enable/disable various strict flags would be useful for this. Beyond just having to update an entire program, I think this particular feature will probably also cause issue when using library definitions that haven't been upgraded yet. In the PR I linked to above I was experimenting with requiring pragmas in .d.ts files in order for the feature to work with the functions from that library. For the feature to actually work the following must be true:

• --strictAssignment must be enabled in the files where both the source type and target type are declared
• --strictAssignment is disabled by default in .d.ts files

I think some variation of this solution make work for rolling out new strict flags that break existing code and/or library definitions. I think having pragmas to selectively enable/disable the strict variance within source code well as library code would be useful too. I wonder if people would find these kinds of pragmas useful in adopting existing strict flags.

[AnyhowStep]

Hello everyone,

Just thought I'd drop this here,
https://github.com/anyhowstep/ts-eslint-invariant-mutable-properties

It's a prototype typescript-eslint rule that attempts to make all mutable properties invariant.

I've tested it on some projects at work and it's not 100% accurate but it does the job for the most part.

While waiting for full typescript support, this might be a good interim solution.

There's no npm package yet because this is still a prototype. You can make it a custom rule for your projects, though.

[castarco]

Hi, what's the state of this discussion? it seems it stalled for some time...

I know introducing new keywords and syntax changes is usually disliked and discouraged, and for good reasons, but maybe we could improve a little bit the situation (without having to rely on flags that change the code semantics) by introducing a "sister" keyword for readonly, could be mutable, or mut.

It's true that introducing this keyword would not fix old bugs in old codebases, but would allow us to do a smoother transition into safer (and more explicit) code for people who need it or care about.

Just as an example:

function messUpTheArray(mutable arr: (string | number)[]): void {
    arr.push(3);
}

const strings: string[] = ['foo', 'bar'];

// This wouldn't be allowed because of the `mutable` qualifier in the function signature
messUpTheArray(strings);

const a: { val: string } = { val: "str" };

// Not allowed, as we know in advance that we'll mutate the object referenced by b
const b: mutable { val: string | number } = a;

// Not allowed, as we know in advance that we'll mutate the object referenced by c.val
const c: { mutable val: string | number } = a

// Allowed
const d: { val: string, mutable foo?: number } = a

Of course this has its own problems, as it does not seem to be very useful beyond function signatures and reference duplications (given that almost everything is mutable by default in JS & TS).

P.S.: I see that there was a discussion about introducing mutable in 2016 ( #6614 ), and it was closed, although the discussion is completely locked and we can't see why the proposal was closed. Any clue on that?