TypeScript: Unsafe type-incompatible assignments should not be allowed

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

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.

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.

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 (https://github.com/Microsoft/TypeScript/issues/11481), in which all unannotated properties would be considered “mutable”.

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.

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.

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 https://github.com/kevinbarabash/TypeScript/pull/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.

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.

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

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

@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