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

Flow-analysis involving callbacks should consider readonly fields #37273

Closed
benny-medflyt opened this issue Mar 7, 2020 · 9 comments
Closed
Labels
Duplicate An existing issue was already created

Comments

@benny-medflyt
Copy link

I imagine that this surely has been discussed previously but I searched thoroughly and couldn't find anything.

I am using TypeScript 3.8.3, with all of the strict options enabled.

interface A {
    readonly x: number | null;
}

function printNum(num: number): void {
    console.log(num);
}

export function tst(a: A): void {
    if (a.x === null) {
        return;
    }

    printNum(a.x);

    setTimeout(() => {
        printNum(a.x);  // Error: Argument of type 'number | null' is not assignable
                        // to parameter of type 'number'.
    }, 1000);
}

I understand that if the "x" field is a mutable field, then the compiler is correct (it is possible for "x" to change to null at some point in the future). But I have declared it "readonly" and therefore "a.x" will always be non-null in the call to "printNum" and the code is always type-safe.

@MartinJohns
Copy link
Contributor

But I have declared it "readonly" and therefore "a.x" will always be non-null in the call to "printNum" and the code is always type-safe.

But that's not always the case. You could have just as easily have this code:

function changeNum(obj: { x: number | null }): void {
    obj.x = null;
}

changeNum(a);

Making sure this is not the case would involve following all code paths. (#9998)

@benny-medflyt
Copy link
Author

Interesting. I tried your changeNum example and you are correct.

I am surprised by this. I would expect typescript to reject the call to changeNum(a) since the type of "a" isn't the same as the type of "obj".

What it is doing is silently converting a {readonly x : number | null} to {x: number | null}. This basically makes the readonly keyword in general useless IMHO, since it can't be relied upon.

@MartinJohns
Copy link
Contributor

#10725 tracks support for deep readonly support. #14909 for support immutable types (they kinda overlap).

@RyanCavanaugh
Copy link
Member

readonly also doesn't imply immutability -- you could have a readonly view of a value that someone else has a writeable reference to.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Mar 9, 2020
@benny-medflyt
Copy link
Author

readonly also doesn't imply immutability -- you could have a readonly view of a value that someone else has a writeable reference to.

Right, but in this case x is a number primitive, so it is truly immutable

@RyanCavanaugh
Copy link
Member

The proposed behavior would make this crashing code legal:

function fn() {
   const obj = { x: 10 as number | undefined };
   const objView: Readonly<typeof obj> = obj;
   return {
      clear() {
         obj.x = undefined;
      },
      objView
   };
}

const a = fn();
if (a.objView.x) {
   window.setTimeout(() => {
      console.log(a.objView.x.toExponential());
   });
   a.clear();
}

@benny-medflyt
Copy link
Author

Thank you for the example, that makes sense.

I guess I would say that this line should cause an error:

   const objView: Readonly<typeof obj> = obj;

I understand that TypeScript has already decided that this is the way things should work. I would just like to voice my wish for a future where we can have an option for working with immutable types and values in TypeScript in a simple, intuitive and ergonomic fashion. I will be following #14909 and hope that progress is made.

@RyanCavanaugh
Copy link
Member

I think it should be unsurprising that Readonly creates a readonly thing instead of an immutable thing, because immutability and readonlyness are different. The size of a Map is readonly (you cannot set it) but it is not immutable (adding an entry to the map will change it). There are many other analogues to this; if you had some C#-like DateTime thing with a Now property, a ReadOnly<typeof DateTime> should be a no-op, not something that gives you back an "immutable" thing with an ever-changing Now property.

@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants