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

Narrowing lost in polymorphic function application #27259

Closed
sledorze opened this issue Sep 21, 2018 · 15 comments
Closed

Narrowing lost in polymorphic function application #27259

sledorze opened this issue Sep 21, 2018 · 15 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue High Priority

Comments

@sledorze
Copy link

sledorze commented Sep 21, 2018

TypeScript Version: 3.1.0-dev.20180921 (next)

[email protected] <- works as expected.
[email protected] <- problem started to appear at that very version.

Search Terms:

generic inference regression

type XY = 'x' | 'y'
const x: XY = 'x'
interface Opt<T> {
  v: T
}
const foo = <T>(v: T): Opt<T> => ({ v })
const foox = foo(x) // Opt<string> instead of Opt<XY> (Regression)

const bar = <T>(v: T): Opt<typeof v> => ({ v })
const barx = bar(x) // Opt<string> instead of Opt<XY> (Regression)

const baz = <T>(v: T): T => v
const bazx = baz(x) // 'x' (OK)

Expected behavior:

Expected union type to be preserved

Actual behavior:

Union type is expanded to string

Playground Link:

This Playground Link demonstrate the CORRECT Behaviour (because previous typescript version is used in the playground)
http://www.typescriptlang.org/play/#src=type%20XY%20%3D%20'x'%20%7C%20'y'%0D%0Aconst%20x%3A%20XY%20%3D%20'x'%0D%0Ainterface%20Opt%3CT%3E%20%7B%0D%0A%20%20v%3A%20T%0D%0A%7D%0D%0Aconst%20foo%20%3D%20%3CT%3E(v%3A%20T)%3A%20Opt%3CT%3E%20%3D%3E%20(%7B%20v%20%7D)%0D%0Aconst%20foox%20%3D%20foo(x)%20%2F%2F%20Opt%3Cstring%3E%20instead%20of%20Opt%3CXY%3E%20(Regression)%0D%0A%0D%0Aconst%20bar%20%3D%20%3CT%3E(v%3A%20T)%3A%20Opt%3Ctypeof%20v%3E%20%3D%3E%20(%7B%20v%20%7D)%0D%0Aconst%20barx%20%3D%20bar(x)%20%2F%2F%20Opt%3Cstring%3E%20instead%20of%20Opt%3CXY%3E%20(Regression)%0D%0A%0D%0Aconst%20baz%20%3D%20%3CT%3E(v%3A%20T)%3A%20T%20%3D%3E%20v%0D%0Aconst%20bazx%20%3D%20baz(x)%20%2F%2F%20'x'%20(OK)%0D%0A

Related Issues:

I think this bug is very recent; I've not found anything relevant.

@sledorze sledorze changed the title Narrowing lost is polymorphic function application Narrowing lost in polymorphic function application Sep 21, 2018
@ahejlsberg
Copy link
Member

A smaller repro:

type XY = 'x' | 'y';
const x: XY = 'x';
let x2 = x;  // Has type string

We definitely shouldn't be widening the type of x2 to string.

@ahejlsberg
Copy link
Member

@weswigham @RyanCavanaugh We should look at this one right away.

@ahejlsberg
Copy link
Member

Timing seems to indicate this is an effect of #27042.

@weswigham
Copy link
Member

I can point exactly to what causes this, because (at least for booleans, which were the motivator) it was intentional. In

type XY = 'x' | 'y';
const x: XY = 'x';
let x2 = x;  // Has type string

x's declared type is narrowed by it's assigned type of a fresh 'x', resulting in a fresh 'x' (since the narrowing type was fresh) which then widens on assignment to let. Take the same example for boolean:

const x: boolean = true;
let x2 = x;  // Has type boolean

Without this rule, x2 would just be true. So do we want to remove the freshness-carries-through-narrowing change for all types? Keep it for just boolean?

@ahejlsberg
Copy link
Member

@weswigham See my comment here.

@ahejlsberg
Copy link
Member

Repeating my comment in #27042, the intuitive rule was always that once you add a type annotation to let, const or var declaration, values read from that variable are not fresh. But now they are unless the annotated type is a unit type.

const x1: 'x' = 'x';
let z1 = x1;  // 'x'

const x2: 'x' | 'y' = 'x';
let z2 = x2;  // Was 'x', now string

I find it hard to construct a rationale for that.

@weswigham
Copy link
Member

Hm, but the annotation actually wasn't why that was the case, strictly speaking. It was simply because the types we were narrowing by weren't fresh. If by chance you narrowed by a union of fresh types (because, eg, the union type is built up from conditional expressions) it would behave exactly like this already, even if it were annotated!

@weswigham
Copy link
Member

The reason I added the change was for consistency, because a union of fresh types like that is built for booleans all the time (while it's more rare for other literals) and consistiency between the declared and constructed forms of the types seemed desirable.

@ahejlsberg
Copy link
Member

ahejlsberg commented Sep 21, 2018

Hm, but the annotation actually wasn't why that was the case, strictly speaking.

As I said above, the core intuition was always that once a type annotation was present in the path that a value traveled, the value would loose its freshness. That's broken now in a way that I don't think I can explain.

I think we need to stick with the old rules. If they don't work for fresh booleans then maybe we don't need fresh booleans.

@weswigham
Copy link
Member

I think we need to stick with the old rules. If they don't work for fresh booleans then maybe we don't need fresh booleans.

It's more like the old rules are very unintuitive for booleans which previously widened everywhere.

As I said above, the core intuition was always that once a type annotation was present in the path that a value traveled, the value would loose its freshness. That's broken now in a way that I don't think I can explain.

We could choose preserve that intuition with a different change (ie, by actually explicitly having a rule governing it) - by explicitly stripping the freshness measured from an initializer of a statement with a declared type.

@npip99
Copy link

npip99 commented Sep 21, 2018

Ideally, when narrowing occurs, the type it narrowed from would be saved. Then, if it has to widen, it widens to the saved type.

const a : b = c; // a is of type c, but if it has to widen, it should widen to b
let d = a; // its fresh, so widen it, but instead widen it to b instead of the limited/arbitrary set of number/string/boolean/enum

I think disregarding ": b" when c is assigned to a, is rather confusing. In fact, if ": b" is completely ignored, it shouldn't even be allowed / it should be warned of by the linter, as writing code that becomes a noop is rarely expected behavior. Even in the original functionality d would be of type "c" - not what the user likely intended. Most think of constants by their type and value, and dont think of a constant's type being the value itself (As in C/C++ const int a = 5; where a is logically an integer. 5 can be used by the compiler and linting. auto b = a; still keeps b an integer). I haven't contributed to this repo but I'm sure my suggestion is a rather large overhaul anyway; at least it's food for thought.

let e : b = generateRandomB();
const a : b = c; // a is of type b with guaranteed value c
let d = a; // mutables inherit type and value as well, type cannot be widened, but value may be widened for mutables. Value may not be widened for consts, clearly.
somefunc(d) // can use the fact that d has value c for static analysis, and it will work if somefunc accepts type c but does not accept type b. Very useful if somefunc wants a special case of b, just like a narrowing if statement
let d = e // Valid, and 'd' now has type 'b' and no guaranteed value. Older version of TS mark this as invalid since 'd' is of type 'c', which doesnt fit what the user is thinking in his mind. Newest TS says its valid, because d is superwidened to the primitive of 'c', which is likely wider than 'b'; still not ideal.

I think this logic is a lot simpler than having literals being types, and causing an overload for what 'type' represents. On the same token,

const a = "hey"; // type string, value "hey"
let b = a; // b has type string, and value "hey"
b = randomString(); // b has type string, and unknown value

works perfectly (Which is what all this freshness was designed to achieve in the first place, as far as I understand - no guarantees Im understanding this well however).

@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.1 milestone Sep 21, 2018
@RyanCavanaugh
Copy link
Member

@ahejlsberg here's my summing of the situation. It feels like we're at a local maximum at least, but there's one example where it seems there's some disagreement about the desired behavior:

function f1() {
    let b = true;
    let obj = { b };
    // Desired: OK
    // 3.0: OK
    // 3.1 as-is: OK
    // 3.1 minus widening propagation: error
    obj.b = false;
}

function f2() {
    type Element = (string | false);
    type ElementOrArray = Element | Element[]; 
    let el: Element = null as any;
    let arr: Element[] = null as any;
    let elOrA: ElementOrArray = null as any;

    // Desired/actual: All OK
    let a1: ElementOrArray = el;
    let a2: ElementOrArray = arr;
    let a3: ElementOrArray = [el];
    let a4: ElementOrArray = Array.isArray(elOrA) ? elOrA : [elOrA];

    // Desired: OK
    // 3.0: Error
    // 3.1: OK
    let a5: ElementOrArray = [...Array.isArray(elOrA) ? elOrA : [elOrA]];
}

function f3() {
    type XY = 'x' | 'y';
    const x: XY = 'x';
    let x2 = x;
    // Desired: OK (up for debate?)
    // 3.0: Error
    // 3.1 as-is: OK
    x2 = 'y';

    // Desired/actual: All OK
    let x3: XY = x;
    x3 = 'y';
}

function f4() {
    const x: boolean = true;
    let x1 = x;
    // Desired: OK
    // 3.0: OK
    // 3.1: OK
    // 3.1 minus widening propagation: error
    x1 = false;
}

function f5() {
    type XY = 'x' | 'y';
    let arr: XY[] = ['x'];
    arr = ['y'];
    // Desired: OK
    // Error in all extant branches
    arr = [...['y']];
}

@sledorze
Copy link
Author

The widening approach breaks all efforts put into excluding union cases and prevent relying on the type checker for precise exhaustive cases checking.
It would hugely lowers the value of typescript.

@sledorze
Copy link
Author

thanks @weswigham !

@DanielRosenwasser DanielRosenwasser added the Fixed A PR has been merged for this issue label Oct 9, 2018
@ShanonJackson
Copy link

will this fix a huge issue in typing one of our tools?

class Box<A> {
    constructor(readonly value: A) { }
    orElse<B>(other: B): A | B {
        return !this.value ? other : this.value as any;
    }
}


// note the type here is "foo" | "bar" this is what i want.
const test = new Box("foo" as "foo").orElse("bar")  // "foo" | "bar"

// note the type here is never[] | string[] what i want is string[]
// so lets change box for better inference
const test2 = new Box(["foo"]).orElse([]) // never[] | string[]



class Box2<A> {
    constructor(readonly value: A) { }
    orElse<B>(other: B): B extends never[] ? A : A | B {
        return !this.value ? other : this.value as any;
    }
}
// string and not "foo" | "bar" its narrowed
const test3 = new Box2("foo" as "foo").orElse("bar")  // string
// this works now, just comes out as string[] but at the cost of inference of first example.
const test4 = new Box2(["foo"]).orElse([]) // string[]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue High Priority
Projects
None yet
Development

No branches or pull requests

7 participants