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

Variables type wrong unification / conditional inference #48714

Closed
HanabishiRecca opened this issue Apr 14, 2022 · 9 comments
Closed

Variables type wrong unification / conditional inference #48714

HanabishiRecca opened this issue Apr 14, 2022 · 9 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@HanabishiRecca
Copy link

Bug Report

πŸ”Ž Search Terms

unification, variable type, conditional inference

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

const
    foo = { a: 1 },
    bar = { a: 1, b: 2 };

const result = Math.random() > 0.5 ?
    foo : bar;

const resolve = () => {
    if (Math.random() > 0.5)
        return foo;
    else
        return bar;
};

πŸ™ Actual behavior

result type and resolve return type inferred as foo only ({ a: number; }).

πŸ™‚ Expected behavior

result type and resolve return type obviously should be inferred as foo | bar (in some form of { a: number; } and { a: number; b: number; } union).

More info

This only happens when:

  1. foo and bar share at least 1 property.
  2. Objects used as variables. Using inline objects, types are inferred as intended.

Example with inline objects:

const result = Math.random() > 0.5 ?
    { a: 1 } : { a: 1, b: 2 };

const resolve = () => {
    if (Math.random() > 0.5)
        return { a: 1 };
    else
        return { a: 1, b: 2 };
};

Here types are inferred as expected:

{
    a: number;
    b?: undefined;
} | {
    a: number;
    b: number;
}
@craigphicks
Copy link

craigphicks commented Apr 15, 2022

Without going into the reasons for, or reasonableness of, the behavior you note - please notice the behavior when foo and bar already have typical typescript (= objects with one or more fields serving as types discriminant):

type Foo = {a:1};
type Bar = {a:2,b:number};

const
    foo:Foo = { a: 1 },
    bar:Bar = { a: 2, b: 2 };

// UI shows - const result: Foo | Bar
const result = Math.random() > 0.5 ?
    foo : bar;

// / UI shows - const resolve: Foo | Bar
const resolve = () => {
    if (Math.random() > 0.5)
        return foo;
    else
        return bar;
};
// / UI shows - const resolve2: Foo | Bar
const resolve2 = () => {
    return (Math.random() > 0.5) ? foo : bar;
};

// / UI shows - property: 2|1
result.a

if (result.a===2){
    // / UI shows - property:  b: number
    result.b
}

As you can see it is tracking the return types as you probably expect.

@HanabishiRecca
Copy link
Author

@craigphicks yes, this example only works because 1 and 2 are different literal types, which leads to Foo and Bar not actually sharing the property type. But I'm talking about exact scenario. It doesn't work as expected when:

type Foo = { a: number; };
type Bar = { a: number; b: number };

Your example will also emit an error.

@craigphicks
Copy link

@HanabishiRecca -

The error you demonstrate can be resolved like this

if (result.a === 2) {
    // / UI shows - property:  b: number
    (result as Bar).b
}

The logic is that requiring explicit disambiguation prevents a mistaken assignment.
I also get the point you are making.
You stated the "this example only works because 1 and 2 are different literal types".
Right - but the qualifier "only" misses the point.
Typescript object types shine when using literal discriminants to discriminate.
Go with the flow and make the dough.

@HanabishiRecca
Copy link
Author

HanabishiRecca commented Apr 15, 2022

The logic is that requiring explicit disambiguation prevents a mistaken assignment.

I understand. But as mentioned earlier, the compiler can actually resolve this automatically to:

{
    a: number;
    b?: undefined;
} | {
    a: number;
    b: number;
}

(see example with inline objects)
Which eliminates the disambiguity.
Of course you can declare b?: undefined; for Foo explicitly. But the problem is, you are not always control the object source. And also this is not very convinient and clear way.

E.g. I faced this issue with 2 endpoints in the library. With declaration like:

declare const GetA: () => Promise<{ url: string; }>;
declare const GetB: () => Promise<{ url: string; shards: number; }>;

Obviously, I don't control the objects shape here.
In the worst case, such functions can be not connected to each other and just have such common property accidently. So you can't really cover all possible cases with explicit undefined.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Apr 15, 2022
@RyanCavanaugh
Copy link
Member

This is not an incorrect inference. Shoehorning the optional members back in can happen in cases where it's straightforward for TS to detect that it's possible, but this is not guaranteed to occur in all cases.

@HanabishiRecca
Copy link
Author

Ok.

@craigphicks
Copy link

craigphicks commented Apr 15, 2022

@HanabishiRecca

Type guards can make the code clearer

declare const GetA: () => Promise<{ url: string; }>;
declare const GetB: () => Promise<{ url: string; shards: number; }>;
type A = Awaited<ReturnType<typeof GetA>>;
type B = Awaited<ReturnType<typeof GetB>>;
// type guards
function isA(x:A|B): x is A {
  return typeof (x as B).shards==="undefined";
}
function isB(x:A|B): x is B {
  return !isA(x);
}
(async()=>{
  // UI shows const x: {url: string; kind: string; }}
  const x = Math.random() > 0.5 ? await GetB() : await GetA();
  if (isB(x)){
    x.shards // no error
  }
})()

Alternatively you can add a discriminant

declare const GetA: () => Promise<{ url: string; }>;
declare const GetB: () => Promise<{ url: string; shards: number; }>;
type A = Awaited<ReturnType<typeof GetA>>;
type B = Awaited<ReturnType<typeof GetB>>;
type XA = A & {kind:"A"}
type XB = B & {kind:"B"}
const getXA = async ():Promise<XA> => ({kind:"A", ...(await GetA()) });
const getXB = async ():Promise<XB> => ({kind:"B", ...(await GetB()) });
(async()=>{
  const xx = Math.random() > 0.5 ? await getXA() : await getXB();
  if (xx.kind==="B"){
    xx.shards; // no error 
  }
})()

@craigphicks
Copy link

craigphicks commented Apr 20, 2022

@HanabishiRecca - A fundamental observation - combining types with a logical-OR is an "obvious" approach, as you say. Adding types to a dictionary would be cheap. But as the number of types increase, (1) memory increases, and (2) type checking operations (which may be > O(dict size)) reading the dictionary becomes prohibitively expensive. The Typescript approach is a set of transformational rules that automatically "condense" the types, as the types are encountered, and the rules vary by situation. Unlike logical-OR, however, there are many conceivable approaches to "condensing" types. In fact the type operator | applied to Record types is not logical-OR (although in some cases it may be).

There are loads of discussions on this topic of combining types, e.g., "Proposal: Add an "exclusive or" (^) operator #14094 ".

Your case where you are faced with pre-existing types you can't or don't want to refactor - my prescriptive solution above of adding a fields (or wrapping in another type) is definitely heavy handed - it's against principle to add actual code for the sake of type checking when the types disappear in the actual JS output anyway. However, the type guards solution above doesn't scale when types are combined further, and to actually get the full value of type checking, adding actual coded properties seems like the best practical solution - I'd be happy to know better solutions - please instruct me when you find them.

@HanabishiRecca
Copy link
Author

@craigphicks, well, I ended up explicitly declaring "merged" type. Something like:

const result: {
    a: number;
    b?: number;
} = Math.random() > 0.5 ?
        foo : bar;

Not so convinient as inference, but works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

3 participants