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

Insufficient type narrowing of object with a union property #33467

Closed
KevinWuWon opened this issue Sep 17, 2019 · 11 comments
Closed

Insufficient type narrowing of object with a union property #33467

KevinWuWon opened this issue Sep 17, 2019 · 11 comments
Labels
Duplicate An existing issue was already created

Comments

@KevinWuWon
Copy link

KevinWuWon commented Sep 17, 2019

TypeScript Version: [email protected] (typescript@next)

Search Terms: union property type narrowing

Note: this is two bugs in one but they look related. "Issue 2" might actually be more damning.

Issue 1

Code

// Compile with `tsc --strictNullChecks` maybe1.ts
type Base = { bar: number };
type MaybeFoo = Base & { foo: string | null };
type HasFoo = MaybeFoo & { foo: string };

export function useMaybeFoo(maybeFoo: MaybeFoo) {
  if (maybeFoo.foo !== null) {
    const hasFoo: HasFoo = maybeFoo;
  }
}

Expected behavior:
Should compile because the conditional should have narrowed the MaybeFoo to a HasFoo.

Actual behavior:

maybe1.ts:7:11 - error TS2322: Type 'MaybeFoo' is not assignable to type 'HasFoo'.
  Type 'MaybeFoo' is not assignable to type '{ foo: string; }'.
    Types of property 'foo' are incompatible.
      Type 'string | null' is not assignable to type 'string'.
        Type 'null' is not assignable to type 'string'.

7     const hasFoo: HasFoo = maybeFoo;
            ~~~~~~

Playground Link: click

Related Issues: had a quick search but couldn't anything

Workaround

This code is exactly the same, but compiles

// Compile with `tsc --strictNullChecks` maybe2.ts
type Base = { bar: number };
type NoFoo = Base & { foo: null };
type HasFoo = Base & { foo: string };
type MaybeFoo = NoFoo | HasFoo;

export function useMaybeFoo(maybeFoo: MaybeFoo) {
  if (maybeFoo.foo !== null) {
    const hasFoo: HasFoo = maybeFoo;
  }
}

Playground Link click

Issue 2

Now let's try using the same type definitions as maybe2.ts, but try creating a MaybeFoo:

// Compile with `tsc --strictNullChecks` maybe3.ts
type Base = { bar: number };
type NoFoo = Base & { foo: null };
type HasFoo = Base & { foo: string };
type MaybeFoo = NoFoo | HasFoo;

export function makeMaybeFoo(foo: string | null): void {
  const maybeFoo: MaybeFoo = {foo: foo, bar: 3};
}

Expected behavior:
Should compile.

Actual behavior:

maybe3.ts:7:9 - error TS2322: Type '{ foo: string | null; bar: number; }' is not assignable to type 'MaybeFoo'.
  Type '{ foo: string | null; bar: number; }' is not assignable to type 'HasFoo'.
    Type '{ foo: string | null; bar: number; }' is not assignable to type '{ foo: string; }'.
      Types of property 'foo' are incompatible.
        Type 'string | null' is not assignable to type 'string'.
          Type 'null' is not assignable to type 'string'.

7   const maybeFoo: MaybeFoo = {foo: foo, bar: 3};

Playground Link click

Issue 2 Workaround

Changing back to the original type definitions fixes it:

// Compile with `tsc --strictNullChecks` maybe4.ts
type Base = { bar: number };
type MaybeFoo = Base & { foo: string | null };
type HasFoo = MaybeFoo & { foo: string };

export function makeMaybeFoo(foo: string | null): void {
  const maybeFoo: MaybeFoo = {foo: foo, bar: 3};
}

Playground Link: click

@KevinWuWon KevinWuWon changed the title Insufficient type narrowing of object with a union field Insufficient type narrowing of object with a union property Sep 17, 2019
@jack-williams
Copy link
Collaborator

jack-williams commented Sep 17, 2019

Issue 1 is a duplicate of #30506, but #33205 might look closer to this issue.
Issue 2 is a duplicate of #33243, I think.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Sep 17, 2019
@KevinWuWon
Copy link
Author

Issue 1 is a duplicate of #30506

Yup.

Issue 2 is a duplicate of #33243, I think.

Yeah, seems related or the same. The workaround Expand workaround it describes works for my case.

Thanks! Feel free to dedupe, although I think my explanation of Issue 2 is more minimal than #33243

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 17, 2019

I will always be against having this narrowing implemented because of,

#33205 (comment)

//Assume this works
const hasFoo: HasFoo = maybeFoo;
maybeFoo.foo = null;
console.log(hasFoo.foo)//not a string

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 17, 2019

Your second example (alleged workaround of Issue 1) is saying a very different thing from the first example.

//In the second example
const hasFoo: HasFoo = maybeFoo;
maybeFoo.foo = null; //Error, cannot assign null to string

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 17, 2019

type MaybeFoo = NoFoo | HasFoo;

And

type MaybeFoo = Base & { foo: string | null };

Are not the same type and should not be treated the same


The narrowing you desire hasn't been implemented because it isn't safe. Not because of performance reasons, as pointed out in the other issues.

@KevinWuWon
Copy link
Author

Great point. So you're saying they're not equivalent because you can reassign foo.

Would the types be equivalent if all the properties were readonly? Sprinkling readonly everywhere doesn't make any of the errors go away.

@KevinWuWon
Copy link
Author

Ah, I just read the discussion on #33205 where you covered why readonly doesn't change anything (because it's not absolute). I agree now that Issue 1 is working as intended.

What do you think about Issue 2 though?

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 17, 2019

For issue 2 (this playground), it makes sense that the assignment should be sound.
It's an object literal that is not referenced elsewhere.

I'm not sure if it'll be marked as a duplicate of something else or "design limitation", though.


@jack-williams What do you think about that issue 2 thing?
Is it possible to make the assignment allowed if the RHS is a "fresh" object literal?

The other issues don't talk about fresh object literals on the RHS of the assignment

@jack-williams
Copy link
Collaborator

I’m pretty sure issue 2 is a bug as per my first comment. If you remove the intersections and just use a union of object literals it works

@AnyhowStep
Copy link
Contributor

Ah. It wasn't obvious to me the issues were indeed related because I was stuck on the fresh object literal part.

I didn't try to remove the intersection =x

@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

5 participants