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

Better propagation of type predicate signatures #12798

Closed
zpdDG4gta8XKpMCd opened this issue Dec 9, 2016 · 19 comments · Fixed by #57465
Closed

Better propagation of type predicate signatures #12798

zpdDG4gta8XKpMCd opened this issue Dec 9, 2016 · 19 comments · Fixed by #57465
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@zpdDG4gta8XKpMCd
Copy link

declare function isNumber(value: {}): value is number;
declare const values: {}[];
declare function filterAs<a, b extends a>(values: a[], isIt: (value: a) => value is b): b[];
const yayNumbers = filterAs(values, isNumber); // number[]
const ohNoNotAgain = filterAs(values, x => isNumber(x)); /* <-- expected to work,
actual: Argument of type '(x: {}) => boolean' is not assignable to parameter of type '(value: {}) => value is {}'.
        Signature '(x: {}): boolean' must have a type predicate. */
@DanielRosenwasser
Copy link
Member

There actually is a way, but it's cumbersome.

const yayayayayayay = filterAs(values, (x): x is number => isNumber(x));

@aluanhaddad
Copy link
Contributor

Is there a way that this can be improved? It seems very counterintuitive for the type of the predicate to be lost when passed as a callback.

@DanielRosenwasser DanielRosenwasser changed the title there is no way to use a type guard in a callback position other than passing its reference Better propagation of type guards Dec 28, 2016
@DanielRosenwasser DanielRosenwasser changed the title Better propagation of type guards Better propagation of type predicate signatures Dec 28, 2016
@DanielRosenwasser DanielRosenwasser added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript labels Dec 28, 2016
@DanielRosenwasser
Copy link
Member

I'd like to think so, but there needs to be a proposal. I'd like to think that for the following:

declare function isNumber(x: any): x is number;
declare function isString(x: any): x is string;

function isSomething(x: any) {
    return isNumber(x) || isString(x);
}

isSomething becomes a predicate on x for string | number.

But for the following

declare function isNumber(x: any): x is number;
declare function isString(x: any): x is string;

function uhhhh(x: any, y: any) {
    return isNumber(x) || isString(y);
}

we'd like to potentially maintain the checks from each predicate, but we don't have a way to encode those guarantees on the signature of uhhhh. Maybe that's okay though.

@zpdDG4gta8XKpMCd
Copy link
Author

What I dont understand is that why 10% of corner cases prevent 80% of straightforward cases from being implemented. Can we error in these cases you mentioned? Or can we default to the current fall back to conventional boolean as it currently is?

@aluanhaddad
Copy link
Contributor

It seems like the return type of uhhhh would be (hypothetically)

x is number & y is any | x is any & y is string

But while this could be useful maybe it doesn't need to block inference that doesn't require new syntax to express explicitly. It seems like that would be an orthogonal feature, something like "Correlated User-Defined type Gards". I may be misunderstanding the issue however.

@gcnew
Copy link
Contributor

gcnew commented Dec 28, 2016

@Aleksey-Bykov Implementing features that are useful but not entirely thorough attracts a lot of criticism and is oftentimes even questionable. e.g. #13002

@zpdDG4gta8XKpMCd
Copy link
Author

Exactly, since we dont get to choose, the type guards don't work at certain positions, let's make them more thorough is my message here, and lets make readonly right by breaking some crappy code (#6532 (comment)) that is standing on the way, that sarcasm me gets too.

@aluanhaddad
Copy link
Contributor

I think it is just unexpected. I think it is surprising because type predicates are as rich and compositional as they already

if (true || isNumber(x) && isString(x) && isFunction(x)) {
  x // {}
}
if (false || isNumber(x) && isString(x) && isFunction(x)) {
  x // number & string & Function
}

so one may be surprised when they do not work

if (!true || isNumber(x) && isString(x) && isFunction(x)) {
  x // {}
}
if (!false || isNumber(x) && isString(x) && isFunction(x)) {
  x // {}
}

these are rather academic however.

@zpdDG4gta8XKpMCd
Copy link
Author

zpdDG4gta8XKpMCd commented Dec 28, 2016

I would like to add that && combinator can be replaced by successive application of different typeguards

As for || it doesn't make much sense if we keep in mind that typeguards are for narrowing (not widening), I might be wrong about it, so a real life example proving me wrong would be helpful. Narrowing from any to string | number looks made up, why not to narrow straight to number or string?

With this said there seem not to be a real situation that would require && and || combinators, ans if so it doesn't seem rational to invest in solving a nonexisting problem.

On the other hand the orifinal situation with type guards used for filtering is very real and needs some attention

@zpdDG4gta8XKpMCd
Copy link
Author

Situations that imply string | number are usually (in our code) explicitly stated: string | number | boolean | null | undefined | Function and then all it takes is to chip the cases off one type at a time:

if isNumber ... else if isString ... else if isBoolean

@aluanhaddad
Copy link
Contributor

@Aleksey-Bykov I agree that these are edge cases. I also like the chip them off one type at a time approach and use it myself.

@rotemdan
Copy link

rotemdan commented Dec 30, 2016

Edit: I got confused here between assertions on values and predicates between types, for some reason, please ignore this comment.

Just wanted to mention, on a somewhat unrelated note, that the way the T is U syntax uses the word is is quite ambiguous. Does it mean that type T is exactly U (in the sense of the types being an exact structural or nominal match), or it means that T is a subtype of U?

Common usage suggest the latter (in the sense that subclasses would pass instanceof checks for their super class) so it seems more appropriate that it would have been notated T extends U (and possibly the predicate is would be reserved for exact matches).

I feel it would be even more unfortunate that this misleading syntax is "abused" with composite formulas like T is U || V is W. Somewhat incidentally (almost entirely unrelated) I formulated a boolean syntax that used the more semantically accurate extends in #12942.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Dec 31, 2016

@rotemdan

Just wanted to mention, on a somewhat unrelated note, that the way the T is U syntax uses the word is is quite ambiguous. Does it mean that type T is exactly U (in the sense of the types being an exact structural or nominal match), or it means that T is a subtype of U?

the type predicate type annotation doesn't operate on two types it operates on a value and a type. A basic type predicate has the type

(x: any) => x is T

which means that if it returns true, the argument x is a subtype of T. It does not in any way mean that x is is exactly a T.

What do you mean by nominal match? private members create pseudo nominal-like assignability.

Common usage suggest the latter (in the sense that subclasses would pass instanceof checks for their super class) so it seems more appropriate that it would have been notated T extends U (and possibly the predicate is would be reserved for exact matches).

Type predicates take a value and a TypeScript (i.e. erased) type. The instanceof operator in JavaScript, surfaced in TypeScript, takes two values it doesn't take types at all...

@rotemdan
Copy link

rotemdan commented Dec 31, 2016

@aluanhaddad

I 100% agree that the extends syntax would not be appropriate for type assertions on values, thanks for correcting me, I had types in mind (as that was something I was thinking about for other, unrelated purposes) and got a bit confused.

Now that I sort out the confusion is seems better. The funny thing is that I suggested a syntax for exact types that would read like just T, and I specifically had in mind that val is just T would read well. Silly me, got completely confused there..

Edit: Maybe it was the title that used "type predicates", which probably intended to mean "value type guards", that got me confused.

@RyanCavanaugh
Copy link
Member

Bumping off my "stale" list because this is still a good idea

@ascott18
Copy link

ascott18 commented Feb 24, 2021

Another use case here that seems like it should work but doesn't - direct type assertions in a predicate (as opposed to a type assertion being done by a subroutine of a predicate):

Playground

class Extension { }
class ImageExtension extends Extension {
  imageUrl: string | null = null;
}

const extensions: Extension[] = [];
extensions.push(new ImageExtension());

// Doesn't work
const imageExtension: ImageExtension | undefined = extensions.find(e => e instanceof ImageExtension);

// Does work, but is unnecessarily verbose
const imageExtension2: ImageExtension | undefined = extensions.find((e): e is ImageExtension => e instanceof ImageExtension);

@0xdevalias
Copy link

0xdevalias commented Mar 17, 2021

This may not be the right place to raise this, and I can't guarantee that this is a limitation in the language and not just me coding something poorly.. but given the following type predicate:

export const isDefined = <T>(
  value: T | null | undefined
): value is NonNullable<T> => value !== null && value !== undefined;

It seems that I can't do something like the following, as it seems not to narrow the type (minimal contrived example based on an issue I just hit in my code):

const doSomethingWithFoo = (foo?: string) {
  const hasFoo = isDefined(foo);

  const someObject = { bar: "bar" };

  if (hasFoo && someObject.hasOwnProperty(foo) {
    // TS2345: Argument of type 'string | undefined' is not assignable to parameter of type 'string | number | symbol'.

    doSomething();
  }
}

Screenshot of the actual error case in my IDE:

image

With the variables defined as:

const hasSprites = isDefined(sprites);
const wantsSprites = isDefined(spriteName);

Yet when I use the type predicate functions directly within the if, it seems to work fine and has no type errors:

image

Is this another case of TypeScript not 'passing on' it's narrowed types? Or am I just making some kind of 'rookie error' here?

@neongreen
Copy link

neongreen commented Jan 22, 2022

This would be really great because currently it forces me to sacrifice the type safety.

E.g. I have the following predicate:

type Checked = ...

function check(smth, x: Blah): x is Blah & Checked { ... }

I'd like to filter by this predicate:

const filtered: (Blah & Checked)[] =
  xs.filter(x => check(smth, x))

For this to work, I have to add a type signature:

const filtered: (Blah & Checked)[] = 
  xs.filter((x): x is typeof x & Checked => check(smth, x))

However, TypeScript doesn't seem to check that check(smth, x) actually is a type-guarding predicate. The following compiles as well:

const filtered: (Blah & Checked)[] = 
  xs.filter((x): x is typeof x & Checked => true /* or anything equally wrong */)

So each of these lambda type guards is an extra line to potentially worry about.

@Jamesernator
Copy link

This issue makes (TS 5.0) decorators more difficult to write if they want to be able to be applied to potentially type predicates.

For example suppose we have:

function deprecatedMethod(reason: string) {
    function decorator<This, Args extends any[], Return>(
        method: (this: This, ...args: Args) => Return,
        context: ClassMethodDecoratorContext,
    ): (this: This, ...args: Args) => Return {
        return function decoratedMethod(this: This, ...args: Args): Return {
            // A proper implementation would only print once ever, rather than per call
            console.warn(reason);
            return method.call(this, ...args);
        };
    }

    return decorator;
}

class SomeClass {
    @deprecatedMethod(`SomeClass.isSomeClass is deprecated and will be replaced in future`)
    static isSomeClass(value: any): value is SomeClass {
        return "field" in value;
    }
    
    field = "blah";
}

Then we get the same problem as in the OP:

Decorator function return type '(this: unknown, value: any) => boolean' is not assignable to type 'void | ((value: any) => value is SomeClass)'.
  Type '(this: unknown, value: any) => boolean' is not assignable to type '(value: any) => value is SomeClass'.
    Signature '(this: unknown, value: any): boolean' must be a type predicate.(1270)

While this can be resolved by decorator authors by adding an overload:

function deprecatedMethod(reason: string) {
    function decorator<This, Kind>(
        method: (this: This, value: any) => value is Kind,
        context: ClassMethodDecoratorContext,
    ): (this: This, value: any) => value is Kind;
    function decorator<This, Args extends any[], Return>(
        method: (this: This, ...args: Args) => Return,
        context: ClassMethodDecoratorContext,
    ): (this: This, ...args: Args) => Return {
        return function decoratedMethod(this: This, ...args: Args): Return {
            // A proper implementation would only print once ever, rather than per call
            console.warn(reason);
            return method.call(this, ...args);
        };
    }

    return decorator;
}

It feels like it will be a large footgun for authors of fairly generic decorators (like deprecated, experimental, metadata, etc) who might not even be aware of the fact that type predicates require separate overloads.

I'm not sure if this could be more narrowly fixed for decorators, or if it would require a general solution like is proposed in the OP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants