Strict variance and read-only versions of types

[mattmccutchen]

I feel like we should have an open "suggestion" issue with a meaningful title to track the work on stricter variance and read-only versions of types that the TypeScript team is considering, especially as other issues are being duped against it. The initial design sketch and a few comments are in #17502.

After a little more searching, should this be merged with #10725?

[mhegazy]

Here is a rough sketch to what we have discussed over the past years regarding variance checking. if we were to implement it we need at least:

• A new flag that disables the bivariance comparisons for all functions and methods. Strict function types #18654 does this for function/constructor types but not methods.
• A way to declare parameter variance for a function. We have existing suggestions in Covariance / Contravariance Annotations #1394 and Proposal: covariance and contravariance generic type arguments annotations #10717. Though looking at the issue so far, using a readonly/readwrite/writeonly annotations seem to be more fitting for a structural type system. A long with that we probably need a matching readonly type operator that allows you to get the readonly version of a type, and that is tracked by Support readonly type operator to allow correct definition of Object.freeze #10725.
• A way to declare a lower bound on generic type parameter (Enable type parameter lower-bound syntax #14520)

More discussions can be found in #18513 and #18339

[RReverser]

@mhegazy If I understand correctly, does the second suggestion cover function calls that may currently modify readonly types?

In particular, got bitten before by popular "immutable" Object.assign({}, obj, { ... }) pattern when person forgot the empty object and did Object.assign(obj, { ... }) and TS didn't complain despite obj being Readonly.

[mhegazy]

I think we can do that regardless of variance. the issue here is we do not check readonly flags in assignment compatibility.

type I = { p: number };

declare var i: I;
declare var roi: Readonly<I>;

i = roi; // Not an error!

roi.p = 0; // Error, p is readonly

This was tracked by #11481. The reason why we have shied from this check is it forces users to mark their parameters as readonly consistently everywhere once one of their libraries adopts a stricter readonly annotations. This might have been overblown concern, and maybe we should revisit our decision of a new --strictReadonlyChecks flag.

[RReverser]

it forces users to mark their parameters as readonly consistently everywhere once one of their libraries adopts a stricter readonly annotations

Hmm, true. But maybe TS could provide a simplified migration path for that concern, automatically "upgrading" any parameter types to readonly whenever it knows that function body doesn't modify the object? Then most of the code would be still compatible across such changes.

[mhegazy]

that is a possible solution. we have talked about code mods in the past..

[RReverser]

I thought rather about runtime type detection than codemod, but codemod would work as well.

[RReverser]

Basically:

// only reads `obj.prop`, so TS changes type of `f` to `({ readonly prop: number }) => ...`
function f(obj: { prop: number }) {
  return obj.prop;
}

[amir-arad]

I'm the maintainer of wix-react-tools
Considering this example:

type One = { one: 1 };
type OneTwo = { one: 1, two: 2 };
declare const useCase: (f: (f:One) => void) => void;
declare const fooOneTwo: (f:OneTwo) => void;

// this fails contravariance 
useCase(fooOneTwo);
/* Error: TS2345:Argument of type '(f: OneTwo) => void' is not assignable to parameter of type '(f: One) => void'.
   Types of parameters 'f' and 'f' are incompatible.
    Type 'One' is not assignable to type 'OneTwo'.
      Property 'two' is missing in type 'One'.
*/

This is a straightforward example for the new contravariance check introduced in 2.6, as it protects the user from a scenario where useCase callsfooOneTwo with {one:1}.

function useCase(f: (f:One) => void){
f({one:1}); 
}

However, in our case, the useCase is a decorator, and f is a class. useCase extends the f argument and returns a new version of it. It is only designed to work on classes that accept a constructor argument that extends One, so fooOneTwo is a valid argument.

Up until now We've given the users a type-safe experience, with full support in strict-mode checks. To support typescript 2.6 (wix-incubator/wix-react-tools#188) We now need to choose between asking them to use --strictFunctionTypes flag and opt out of this great new feature, or remove a lot of the type definition value we currently offer.

In all the proposals regarding contravariance checks there was some sort of in/out notation that would have allowed us to avoid this dilemma. I'm very much interested in any plans of supporting such notations.

[mattmccutchen]

However, in our case, the useCase is a decorator, and f is a class. useCase extends the f argument and returns a new version of it. It is only designed to work on classes that accept a constructor argument that extends One, so fooOneTwo is a valid argument.

Then shouldn't useCase have this type?

declare const useCase: <P extends One>(f: (f:P) => void) => (f:P) => void;

[amir-arad]

@mattmccutchen That could be a workaround , but I don't get to define useCase in my own terms, but rather get it from a generic API:

type Feature<T extends object> = <T1 extends T>(subj: T1) => T1;
type ReactFeature<P extends object> = Feature<ComponentType<P>>;
declare const useCase: ReactFeature<One>

@useCase // error
class X extends React.Component<OneTwo> {}

// error
const Y = useCase((p:OneTwo)=>{...})

(see original code here, here, here and here)
I could do it easily if I had extends as a generic wildcard:

type ReactFeature<P extends object> = Feature<ComponentType<extends P>>;

[mattmccutchen]

@amir-arad Now that I learned the basics of React for unrelated reasons, I took another look at this and didn't see an obvious solution, though I still don't fully understand what you're trying to do. I don't think discussing your case further in this thread is helpful; I suggest you start a new thread on one of the normal support channels. Linking to the original code is nice, but to make it easy for people to help you or evaluate a suggestion, you'll probably need to provide a brief explanation of why each type is set up the way it is and what the decorator actually does.

[Jessidhia]

A possible application of that readonly keyword semantic is with the type of React.createRef(); though I'm not entirely sure it's safe to just use it as a heuristic.

The result type of React.createRef<T>() is { readonly current: T | null }; that is, the object is only used as a way to get current as an additional output from a mount operation. This means this object should be usable as a ref for any component that is a subtype of T.

However, typescript thinks that, as this object looks like an input, that the ref parameter wants to read current (instead of write to it); so it's not actually possible to give it to subtypes of T.

A concrete example:

const htmlRef = React.createRef<HTMLElement>();

// type error; HTMLElement not assignable to HTMLDivElement
<div ref={htmlRef}/>;

However, the assignability is supposed to apply the other way around.

This works with function refs because of the bivarianceHack.

[Diggsey]

I'd like to suggest a solution here: for a lot of the javascript code I work with, the vast majority does not mutate javascript objects in place, and when it does so, it's mutating locally created objects rather than function arguments. This is particularly the case with react and other functional/reactive frameworks.

The good thing about Flow is that it (somewhat) correctly type-checks mutable vs readonly types, by making mutable types invariant, whereas Typescript is currently unsound in this regard, in order to be easier to use. The reason Flow is hard to use comes from a few reasons:

1. Read-only types are verbose, and only indicate that the callee cannot mutate the value: it says nothing about the value being changed elsewhere.
2. Types being invariant is consistently surprising and painful even when you're aware of this issue, as a lot of code deals with some kind of sub-typing relationships, especially in REST APIs.
3. $ReadOnly only operates shallowly, and so the intersection of nested types does not work as one might expect.

If Typescript is going to add a strictReadonlyChecks flag, then IMO it needs to add the following to avoid the above pitfalls:

1. A light-weight way to get deeply immutable versions of types. It's not enough to make something shallowly immutable, or deeply readonly. I will put an example below, but you should look at how Rust handles & vs &mut for why this is important.
2. A standard way to perform a "deep-copy" of an object, which type checks and produces a T from any Immutable<T>.

Example of why it is important to be able to construct a deeply immutable version of a type, and readonly is not sufficient (neither Typescript nor Flow have any "good" way of soundly type-checking this code without introducing a lot of intermediate types):

type Foo = {
    x: { y: number | undefined }
}

// Refine "Foo" for when we know "y" is present
type Bar = Foo & {
    x: { y: number }
}

function takesABar1(bar: Readonly<Bar>) {
    ...
}

function example1(foo: Readonly<Foo>) {
    if (foo.x.y !== undefined) {
        // This should not compile, because even though we've checked the constraint,
        // there's nothing stopping the constraint from being invalidated at a later time
        // by the caller, who has mutable access to the Foo
        takesABar1(foo);
    }
}


function takesABar2(bar: Immutable<Bar>) {
    ...
}

function example2(foo: Immutable<Foo>) {
    if (foo.x.y !== undefined) {
        // This is completely safe, because we are guaranteed by the caller that
        // "foo" will not be changed, and even that "foo.x" will not be changed.
        takesABar1(foo);
    }
}

Here I am manually annotating types as being Immutable, but there are a few different options:

1. Light-weight syntax for immutable versions of types
2. Make usage of types immutable by default, except when declared mutable
3. Infer whether a type is used mutably or immutably by examining the code: mutability or immutability could be specified in specific cases when it cannot be inferred.

The third option would be the hardest, but would also be the least breaking, as much code will just continue to work even with the new flag enabled. The compiler will need to perform some kind of escape analysis to allow going from mutable to immutable values.

Example:

function test(arg: Immutable<Array<number>>) { ... }

const x: Array<number> = []; // New array, compiler can see we have the only reference
x.push(4); // Compiler infers that in this line and the line above, the array should be mutable
test(x);  // OK, because we have the only reference to `x` so we can "convert it" without copying
x.push(3); // Not OK, "test" could have kept a reference to `x`, and we guaranteed that it would be immutable

Read-only support would still be useful in addition to immutability.

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

---

One of the projects has thousands of files and running the rule lead to fixing about 10 unsound assignments.

There was one problem that was with an external library. Luckily, it was a company internal library and we only had to fix one parameter's type to Readonly<T>. Not painful at all.

The other problem is that this is considered unsound,

declare const src : Buffer;
//Lint error
const dst : UInt8Array = src;

This is considered unsound even though Buffer extends UInt8Array because Buffer has some overloads for methods not on UInt8Array.

So, it is possible to so this,

dst.indexOf = someImplThatDoesNotHandleBufferOverloads;
src.indexOf(/*Buffer specific arguments*/); //Explosion, the impl does not handle Buffer overloads

This situation is highly unlikely, given that people usually do not replace method implementations like this.

So, I'll probably add a config option that checks if the source type explicitly extends or implements the destination type (via those keywords) and allow the assignment if it does

---

[Edit]

It now checks extends and implements and allows such unsafe assignments by default

---

Actually, never mind. The whole type checking thing is way above what my brain can handle. And ts-simple-type gets many simple cases right but falls flat on the not-so-simple ones.

Can't blame it, though.

It essentially has to rewrite all of TS' checking code if it wants to emulate it perfectly.

[kevinbarabash]

I'm curious if anyone's run into actual bugs in their code from the lack of strict variance checking on assignments and function calls where the source and params are variables.

[AnyhowStep]

It's happened to me a bunch of times when I used to write OOP-y code with a lot of state mutations in TS.

I don't do that so much anymore. I mostly treat objects as immutable nowadays. This bypasses a lot of variance problems.

If you look at the table here, #33234

You'll see that we currently don't have write-only properties, so we can ignore that row.

Readonly properties are considered covariant at the moment and this is sound. (Most of the time. The readonly-to-read-and-write assignment is allowed and that is unsound)

The read-and-write properties are the problem here. They're still considered covariant when they should be considered invariant.

You can see an example on #33234 for why covariant read-and-write properties are unsound.

However, if you start treating everything as immutable, even read-and-write properties, you've essentially also eliminated that row from the table in #33234 as well.

All that is left are readonly properties in the table and those are sound. So, by pretending everything is immutable, you side step many problems with variance.

---

It does make me a little nervous when I have to step back into the "mutating world" though, because I know the type system won't catch me if I trip up

[kevinbarabash]

So if we had a linter to ensure that all objects and arrays were immutable then we'd be safe. That's probably something worth exploring.

[aboyton]

In terms of linting for immutable arrays and objects I've been using https://github.com/jonaskello/tslint-immutable which I'm very happy with. It seems like they are migrating it to ESLint with https://github.com/jonaskello/eslint-plugin-functional.

[goatfryed]

I'm curious if anyone's run into actual bugs in their code from the lack of strict variance checking on assignments and function calls where the source and params are variables.

I've recently started a project to explore working with mobx and ran into a bug when working with a copy constructor in a child class. Took me a time to figure out that I must treat fields as invariant. See #35006
I would really like to a see the introduction of a stricter type checking rule.

[saostad]

I have another use case for strictReadonlyChecks

code below works without compiler error:

interface I {
  readonly a: string
}

class C implements I{
  a= ""
}
const D = new C
D.a = "something"

in order to make property 'a' really readonly I should make it readonly in class definition too!

in other word how can I make sure when I am creating a class by implementing interface I am creating it with right modifier?

[DeusProx]

I also ran against this another time.

My current use case is to identify several components by giving them a name. To easily use these components I created a union type of them and wanted to map to another union type with only their names as string literal types in it for other purposes. E.g.:

interface Component { readonly name: string }

class ComponentA implements Component { readonly name = 'a' }
class ComponentB implements Component { readonly name = 'b' }
export type Components = ComponentA | ComponentB

type GetComponentNames<T extends Component> = T["name"]
export type ComponentNames = GetComponentNames<Components> // "a" | "b" 

This works as long as the name property in these classes are really readonly which means that it uses the readonly keyword or is marked with as const. Sadly since the interface can't enforce the readonly property on the class implementation another unaware team member can create a new component ComponentC without the keyword, add it to Components type union which will collaps the type of ComponentNames to a simple string and this destroys our efforts.

class ComponentC implements Component { name = 'c' }
export type ComponentNames2 = GetComponentNames<Components | ComponentC> // string

Here is a link to a tsplayground for these examples.