Skip to content

Check field with optional chaining operator in union types #35542

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

Closed
reforms opened this issue Dec 6, 2019 · 5 comments
Closed

Check field with optional chaining operator in union types #35542

reforms opened this issue Dec 6, 2019 · 5 comments
Labels
Duplicate An existing issue was already created

Comments

@reforms
Copy link

reforms commented Dec 6, 2019

TypeScript Version: 3.7.2

I'm seeing incorrect behavior when I trying to use optional chaining operator in union type. Instead of writing boilerplate code for using field with optional chaining operator I'm seeing use short construction. But...

Code


type OneField = {
    id: string;
}
type TwoField = {
    id: string;
    data: string;
}
function someWork(owner: OneField | TwoField) {
    if (owner.data?.length) { // Property 'data' does not exist on type 'OneField | TwoField'.
        console.log(owner.data);
    }
    // instead of
    // if ("data" in owner && owner.data?.length) {
    //     console.log(owner.data);
    // }
}

Search Terms

chaining operator, use field

Suggestion

Short construction for use field with optional chaining operator in union is available

@dragomirtitian
Copy link
Contributor

dragomirtitian commented Dec 6, 2019

While this definitely would be useful, I would expand it out to union access in more contexts if it were implemented. For example, following the same logic, the compiler could allow access to non-common members in conditionals and use it as a type guard :

function someWork(owner: OneField | TwoField) {
    if (owner.data) { 
        console.log(owner.data);
    }
}

Playground Link

One could go further and allow access in any context to non-common members by adding an | undefiend to such uncommon properties. (ie owner.data is always a valid access, but owner.data is of type string | undefined)

Just one note, creating a type to transform a union in such a way to allow the code in the original example, is trivial, and the original union is even still compatible with this new type:

type OneField = {
    id: string;
}
type TwoField = {
    id: string;
    data: string;
}
type MakeAllUnionConstituentsHaveAllFieldsHelper<T, KAll extends PropertyKey> =
    T extends T ? T & Partial<Record<Exclude<KAll, keyof T>, undefined>> :
    never;

type AllKeys<T> = T extends T ? keyof T : never;
type MakeAllUnionConstituentsHaveAllFields<T> = MakeAllUnionConstituentsHaveAllFieldsHelper<T, AllKeys<T>>

function someWork(owner: MakeAllUnionConstituentsHaveAllFields<OneField | TwoField>) {
    if (owner.data?.length) { 
        console.log(owner.data);
    }
}

declare let o: OneField | TwoField;
someWork(o)/// Comaptible

Playground Link

@jcalz
Copy link
Contributor

jcalz commented Dec 6, 2019

Duplicate of (the declined 😿) #33736

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Dec 9, 2019
@RyanCavanaugh
Copy link
Member

This is a correct error because the code is unsound; you have to account for the possibility of aliasing:

type OneField = {
    id: string;
}
type TwoField = {
    id: string;
    data: string;
}
function someWork(owner: OneField | TwoField) {
    if (owner.data?.length) { // Property 'data' does not exist on type 'OneField | TwoField'.
        // alleged to be OK
        console.log(owner.data.toLowerCase());
    }
    // instead of
    // if ("data" in owner && owner.data?.length) {
    //     console.log(owner.data);
    // }
}

const obj = { id: "", data: 42 };
const alias: OneField = obj;
// Crashes
someWork(alias);

@trusktr
Copy link
Contributor

trusktr commented Jan 5, 2021

@RyanCavanaugh Doesn't TypeScript have enough information there to know that data is a number and not a string at the call site? It could throw an error for that case, then optional chaining would be totally useful!

These are the sorts of discussions Hegel.js is having. And I think it would be worthwhile to investigate such concepts (f.e. type checking based on usage sites, not just static information from the type definitions).

Your previous example would do this:

function someWork(owner: discriminated OneField | TwoField) {
    if (owner.data?.length) {
        // This is totally OK
        console.log(owner.data.toLowerCase());
    }
}

const obj = { id: "", data: 42 };
const alias = obj;

// Type error: alias is not assignable to owner because data is a number but owner expects string | undefined. or similar.
someWork(alias);

Then we can simplify code in many places. Instead of

// thing is type Foo | Bar, and only Bar has method()
if (foo instanceof Bar) {
  foo.method()
}

we can just write

foo.method?.()

A lot cleaner!


Straying a little, maybe TypeScript can investigate a discriminated operator (or built-in generic type) or similar thing:

type Foo = discriminated OneField | TwoField

function foo(foo: discriminated OneField | TwoField) {}

where discriminated causes things to behave more nominalish than duckish. New strict options could enforce these things. I think many people would prefer more strictness in various ways.

Then the "smarter" type checker would see metadata for the example's alias that holds the original type, not just the definition type, and can throw an type error when it is passed to the discriminated type.

Not sure if I'm using the correct wording, discriminated, but I think you get the idea.

@RyanCavanaugh
Copy link
Member

Doesn't TypeScript have enough information there to know that data is a number and not a string at the call site? It could throw an error for that case, then optional chaining would be totally useful!

No, it doesn't. It only knows that alias has an id: number property.

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