Skip to content

Narrowing types based on property access #46219

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
5 tasks done
trusktr opened this issue Oct 5, 2021 · 25 comments
Closed
5 tasks done

Narrowing types based on property access #46219

trusktr opened this issue Oct 5, 2021 · 25 comments
Labels
Duplicate An existing issue was already created

Comments

@trusktr
Copy link
Contributor

trusktr commented Oct 5, 2021

Suggestion

πŸ” Search Terms

type narrowing based on property checks, similar to in operator.

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code. I don't think so, because errors are currently produced, but now there would be narrowing instead of errors.
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

Lot's of JS code uses duck typing. For example:

πŸ“ƒ Motivating Example

class Foo {
    bar: object = {}
    meth() {}
}

declare const f: Foo | Foo[]

if (f.bar) { // Error, bar property does not exist on Foo | Foo[]
  f.meth()
}

playground

πŸ’» Use Cases

We can currently do something similar that actually works:

class Foo {
    bar: object = {}
    meth() {}
}

declare const f: Foo | Foo[]

if ('bar' in f) {
  f.meth()
}

playground

But using a property check would allow us to write code like we can (and like currently exists) in plain JS. The f.bar check would behave similar to the 'bar' in f check, and would narrow the type.

This would need to be limited to certain property types. For example, if Foo.bar were a number, then a value of 0 can make it result in a false even if the property exists. In this case TS would need to throw an error and would not be able to perform type narrowing in such cases.

If the type of Foo.bar is something that is always truthy (f.e. bar: Record<Foo, Bar> is always truthy) then type narrowing can be performed. The system would have to clearly report errors in cases when type narrowing can not be confidently performed based on the encountered type.

@MartinJohns
Copy link
Contributor

MartinJohns commented Oct 5, 2021

Duplicate of #39065.

You're wrongly assuming that just because a truthy bar property exists it must be a Foo object. Types are not sealed in TypeScript, additional properties are allowed. This issue already exists with the in operator and is unsound, but with your proposal it would get significantly worse.

@FilipeBeck
Copy link

You're wrongly assuming that just because a truthy bar property exists it must be a Foo object

declare const f: Foo | Foo[]

If f is Foo | Foo[] and bar exists in f, then it is a instance of Foo. I don't see problems using 'bar' in f, but the premise is true based on the type Foo | Foo[].

@fatcerberus
Copy link

Not necessarily:

interface Foo {
    foo: string;
}

interface Bar {
    foo: number;
    bar: string;
}

// not a valid `Bar` because `Bar` calls for `foo: number`.
// it IS a valid `Foo`, though!
const thing = { foo: "pig", bar: "cow" };
doIt(thing);

function doIt(foobar: Foo | Bar)
{
    if ('bar' in foobar) {
        foobar.foo;  // hover here - TS thinks it's a `Bar` and therefore that `foobar.foo` is a number
    }
}

@FilipeBeck
Copy link

Yes, but, in this case, f is Foo | Bar and Bar has foo too. Now, in Foo | Foo[], Array<Foo> does not have foo. Only Foo has foo. Look the playground link in issue description. The compiler sees f as Foo in 'bar' in f

@FilipeBeck
Copy link

As I said, based in type Foo | Foo[], the premise is true

@fatcerberus
Copy link

It's completely possible for Foo[] to have a foo property. One can make subtypes of Array.

@FilipeBeck
Copy link

For example, if Foo.bar were a number, then a value of 0...

This is reason why (if f.bar) doesn't always work

@FilipeBeck
Copy link

It's completely possible for Foo[] to have a foo property. One can make subtypes of Array.

Yes. Look the playground link and do this. You will get a compiler error

@FilipeBeck
Copy link

One can make subtypes of Array.

f is Foo | Foo[] and not Foo | ASubtypeOfArrayThatContainsAFooProperty

@MartinJohns
Copy link
Contributor

MartinJohns commented Oct 5, 2021

class Foo {
    bar: object = {}
    meth() {}
}

const f: Foo | Foo[] = Object.assign([], { bar: {} });

console.log(Array.isArray(f))
console.log('bar' in f)

Will print out true two times. Playground link


Also just pointing out that you can edit your comments, no need to add comment over comment.

@fatcerberus
Copy link

fatcerberus commented Oct 5, 2021

ASubtypeOfArrayThatContainsAFooProperty is assignable to Array. That was my point. Given a value of type Foo, you can't assume you don't have a subtype with unknown properties.

@FilipeBeck
Copy link

FilipeBeck commented Oct 5, 2021

const f: Foo | Foo[] = Object.assign([], { bar: {} });

You are assing bar to [] using Object.assign. In if('bar' in f), the compiler continue seeing f as Foo

@fatcerberus
Copy link

I give up.

@MartinJohns
Copy link
Contributor

MartinJohns commented Oct 5, 2021

the compiler continue seeing f as Foo

Yes, I mentioned this unsoundness with the in operator in my first comment already. See also #34975.

@FilipeBeck
Copy link

You can do Object.assign in any var (or (f as any).anything = 10). The error is declaring the type wrong in this case. The compiler is right on seeing f as Foo on if('bar' in f) when f is Foo | Foo[]

@fatcerberus
Copy link

fatcerberus commented Oct 5, 2021

Only if we had Exact Types. #12936. Spoiler alert: We don't.

@FilipeBeck
Copy link

You want a type-based system not to consider the type because you can create "patches" to circumvent the typing

@MartinJohns
Copy link
Contributor

The compiler is wrong seeing it, as my example demonstrated. It sees it as Foo, when in fact it is not a Foo, but a Foo[]. Trying to access meth() will result in an error. This unsoundness is also mentioned in #34975.

@MartinJohns
Copy link
Contributor

And here's an example without Object.assign:

type FooArrayWithBarProperty = Foo[] & { bar?: string }

const fooArrayWithBarProperty: FooArrayWithBarProperty = []
fooArrayWithBarProperty.bar = 'hello!'

const ff: Foo | Foo[] = fooArrayWithBarProperty;
if ('bar' in ff) {
    ff;
}

@FilipeBeck
Copy link

FilipeBeck commented Oct 5, 2021

Solution. Do right and don't use Object.assign (or any workaround) to assign a property that does not exists in the type. Do not do "gambiarras"

@FilipeBeck
Copy link

And here's an example without Object.assign:

Try to call ff.metch() inside if block

@MartinJohns
Copy link
Contributor

Try to call ff.metch() inside if block

Honestly, this is getting ridiculous. It's clear that you don't know what the heck you're talking about.

In the brief example you can't call meth() because TypeScript correctly narrows it to never. This is due to the behaviour of assignments narrowing the type of variables. TypeScript sees that a Foo[] is assigned, but no Foo. So trying to narrow it to Foo using 'bar' in ff results in never, because it can't be a Foo.

If you take this behaviour out of the equation (e.g. by returning the value from a function) you will get the issue mentioned:

class Foo {
    bar: object = {}
    meth() {}
}

type FooArrayWithBarProperty = Foo[] & { bar?: string }

const ff: Foo | Foo[] = getValue();
if ('bar' in ff) {
    // ff is narrowed to "Foo", but the value is actually a "Foo[]".
    // Calling meth() will result in an error.
    ff.meth();
}

function getValue(): Foo | Foo[] {
    const fooArrayWithBarProperty: FooArrayWithBarProperty = [];
    fooArrayWithBarProperty.bar = 'hello!';
    return fooArrayWithBarProperty;
}

Playground link

@DanielRosenwasser DanielRosenwasser added the Duplicate An existing issue was already created label Oct 5, 2021
@DanielRosenwasser
Copy link
Member

I understand these discussions can get frustrating - I encourage everyone to cool down for a bit. @MartinJohns and @fatcerberus are correct in their assessment. This is a duplicate of #39065, a duplicate of #38842, and probably more. We've made the design decision not to allow this code for the exact reasons discussed in this issue along with those issues. Object types are open (i.e. "width" subtyping) which means any object could have a property that it doesn't declare, and it's unsafe to narrow based on that.

@FilipeBeck
Copy link

FilipeBeck commented Oct 5, 2021

Honestly, this is getting ridiculous. It's clear that you don't know what the heck you're talking about.

I'm not saying that a type cannot have additional properties (I said the exact opposite here myself). I'm saying the compiler is correct when handling f as Foo due to Foo[] "not have" 'bar' because it already is the expected behaviour. The compiler is optimistic and handles as exact types with respect to the in operator. This is enough for the vast majority of cases if you use good programming practices. I never had any problems using in to determine the type, neither in my personal projects nor in my work.

This was explained here: #15256 (comment)

For example, if Foo.bar were a number, then a value of 0...

This is reason why (if f.bar) doesn't always work

Yes, it is a duplicate and, even if it wasn't, the issue is incoherent and must be closed

@FilipeBeck
Copy link

FilipeBeck commented Oct 5, 2021

Maybe I might not have expressed myself well before because my english is more google than me. Sorry by the possible language mistakes.

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