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

Local Meaning of Union Types (safe assignments) #17188

Closed
olegdunkan opened this issue Jul 14, 2017 · 10 comments
Closed

Local Meaning of Union Types (safe assignments) #17188

olegdunkan opened this issue Jul 14, 2017 · 10 comments
Labels
Needs More Info The issue still hasn't been fully clarified

Comments

@olegdunkan
Copy link

olegdunkan commented Jul 14, 2017

TypeScript Version: Playground

Code

class A {
    x: number;
}

class B extends A {
    y: number;
}

class C extends A {
    z: number;
}

declare function process(c:C);
function f(x:  B | C) {
    
    if (x instanceof C) {
        //we memorize x in c
        let c = x; //here x of type C        
      
        //restore x
        x = c;  //here x loses the narrowed type and x is now B | C again and is's by design    
        x.z = 1; //no error, what? 
        process(x);  //it have to be an error, what?
    }
}

Preamble:
A type guard for a variable x has no effect if the statements or expressions it guards contain assignments to x.

If compiler knows that the type of right hand expression is the same as narrowed type of parameter (in my case) why does compiler expand type of x to the initial type? Maybe it is possible to check for this type of assignments?

@kitsonk
Copy link
Contributor

kitsonk commented Jul 14, 2017

I assume, because tracking all those assignments are overhead for the compiler. While in abstract, it is clear that x is narrowed, but in real world applications, a re-assignment like that is unlikely to be that straight forward and trivial. Do you have a real world practical pattern that would demonstrate how this sort of makes it practical to track that sort of narrowing where re-assignment doesn't potentially re-widen the type?

Having built lots of TypeScript code over the last 2 years, I can't say I have ever run across a pattern where that would be valid.

@olegdunkan
Copy link
Author

olegdunkan commented Jul 14, 2017

@kitsonk Thanks for feedback, maybe I don't understand exactly what do you mean "tracking all those assignments". When compiler meets x=c it makes a decision to widen previously narrowed type, and from that line of code x will be of initial type. I can guess that all other assignments to x will be ignored by the compiler. If so then you are likely right. Do you think that the TS team has the same view as yours on this question? If so I will close the issue.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 14, 2017

Take for example this:

declare function process(c: B | C);

function f(x:  B | C) {
    
    if (x instanceof C) {
        let c = x;

        process(c);  // c is a reference, what happens in see cannot be easily tracked via CFA

        x = c;  // what is `x` here? not sure, so re-widen...
    }
}

There are all sorts of structural things that can occur at run-time which CFA has to sort of try to treat in the most reasonably type safe way. It is always easier to reset a narrowed type than to do complex tracking of narrowed types, and there will always be trade off of how far it can go.

Do you think that the TS team has the same view as yours on this question?

My opinions are mine. I would assume if you had a pattern that was a realistic and common real world pattern where the re-widening was causing usability issues with TypeScript, the team feel that there is a case. At the moment let x = c; c = x; does not seem like a practical real world use case to me, but again, that is my opinion. My personal champion though is the widening of types in callback functions because CFA cannot determine if they are immediately invoked or not (see: #11498). That seems like a much more real world use case that effects pratical type usage. 😉

@olegdunkan
Copy link
Author

olegdunkan commented Jul 14, 2017

@kitsonk things are getting more complicated )
Check this in playground:

class A {
    x: number;
}

class B extends A {
    y: number;
}

class C extends A {
    z: number;
}

declare function process(c:C);
function f(x:  B | C) {
     if (x instanceof C) {
        let c = x;       
      
        x = c; //hover on x it will be of type B | C
        x.z = 1; //hover on x it will be of type C and no error
        process(x); //no error 
    }
}

What is that? Now it seems to me that some bug exists or maybe rules have changed.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 14, 2017

Actually going back to your original code:

class A {
    x: number;
}

class B extends A {
    y: number;
}

class C extends A {
    z: number;
}

declare function process(c: C);

function f(x:  B | C) {
     if (x instanceof C) {
        let c = x;       
      
        x = c; //hover on x it will be of type B | C
        process(x); //no error 
    }
}

It was never an error anyways... the left hand assignment quick info was B | C but before and after the CFA tracked the assignment properly. Which makes sense, because x can always be assigned a value of B | C...

This examples shows that CFA tracks it perfectly fine:

class A {
    x: number;
}

class B extends A {
    y: number;
}

class C extends A {
    z: number;
}

declare function process(c: C);

declare const d: B;

function f(x:  B | C) {
     if (x instanceof C) {
        let c = x;       
        x;
        x = c; //hover on x it will be of type B | C
        x;
        process(x); //no error 

        x = d;
        x; // type = D
    }
}

@RyanCavanaugh
Copy link
Member

A type guard for a variable x has no effect if the statements or expressions it guards contain assignments to x.

This statement stopped being true when we added control flow analysis.

A complicating factor here is that a variable has two types at any point -- its declared type (for x that's B | C) and its control flow type (for x that's C everywhere inside the if). I'm not 100% clear on the rules for which gets shown in Quick Info, but the gold standard for figuring out the control flow type is trying to treat x as a C (e.g. as a function argument or use a C-specific method), not hovering in the editor.

@RyanCavanaugh RyanCavanaugh added the Needs More Info The issue still hasn't been fully clarified label Jul 14, 2017
@olegdunkan
Copy link
Author

@RyanCavanaugh thanks for notes, now I see.
Does TS team have any plans to update the specification with CFA description and more other latest issues?
I think a lot of questions and issues will disappear.

@kitsonk

This examples shows that CFA tracks it perfectly fine

Yes, my question was solved by CFA)

@RyanCavanaugh
Copy link
Member

We're pretty far behind on spec updates and I will admit that I don't expect that to change any time soon. A formal specification of CFA behavior is exceptionally difficult to write in prose; it's cliche but "the code is the spec" in terms of that feature. I expect when CFA does make it into the spec it will be a succinct but nonspecific description like "Conforming implementations may temporarily type an expression or declared property to a subtype of its declared type when a reachability analysis deduces that a type guard or type guard expression is in effect"

@olegdunkan
Copy link
Author

@RyanCavanaugh I understand that the main priority of development of TypeScript is new features rather then routine descriptions of it. And of course I don't expect that CFA requires a formal description in the specification but the notion of flow containers would be 👍 .
And it is really difficult to follow evolving of TypeScript by explore all PR and issues in this repo.
Thanks for support.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 17, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Aug 17, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Development

No branches or pull requests

4 participants