Typescript: Checking is slow in project consuming xstate types

[amcasey]

The original project is here: https://github.com/AlCalzone/node-zwave-js
The actual owner is @AlCalzone

1. Clone https://github.com/amcasey/ZwavePerfRepro
2. npm ci
3. tsc -p .

The portion of the code below // ACASEY boils down to a single function call that takes the vast majority of the checking time. The portion of the code above is enough scaffolding to make it compile.

I've framed the problem in terms of tsc because it's easier to investigate, but the editor experience is also terrible.

[amcasey]

At a high level, the issue is that the many types in the 600-line object literal depend on the parameter type of the function to which it is passed, which depends on the type of the argument (it's not quite a cycle, but it's close).

[amcasey]

Looks like it can be reduced quite a bit. You can replace the whole createSendThreadMachine function with this and the performance is basically the same.

const _on: TransitionsConfig<SendThreadContext, SendThreadEvent> = {
    add: [
        {
            cond: "isPreTransmitHandshakeForCurrentTransaction",
            actions: [
                assign({
                    preTransmitHandshakeTransaction: (_, evt) =>
                        evt.transaction,
                }),
                raise("preTransmitHandshake"),
            ],
        },
    ],
};

[amcasey]

I've pushed a Simplified branch with a greatly reduced repro. It takes about half as long as the full repro, but a lot of that came out of program time. The Type and Instantiation counts are still very high (and depend entirely on the declaration of _on, mentioned above).

[amcasey]

I'm seeing a slow comparison (roughly half of check time) between two instantiations of State.

Edit: specifically, it's a variance check of the first type parameter of State (i.e. with marker types substituted for the first type parameter).

Edit 2: If I hard code the variances for that type as [], the problem goes away (fast checking, few types, few instantiations).

[amcasey]

I've updated the Simplified branch with this further reduced repro:

import { ActionObject, AssignAction, EventObject } from "xstate";

interface DummyContext {
}

interface Event1 {
    type: "evt1";
}

interface Event2 {
    type: "evt2";
}

type Action<TContext, TEvent extends EventObject> =
    | ActionObject<TContext, TEvent>
    | AssignAction<TContext, TEvent>; // NB: extends ActionObject<TContext, TEvent>

declare const action1: AssignAction<DummyContext, Event1>;
const action2: Action<DummyContext, Event1 | Event2> = action1;

(That's it! You could copy-paste, but cloning conveniently pulls down package-lock.json, etc.)

[amcasey]

@davidkpiano It seems strange that Action contains both AssignAction and its base type ActionObject. Was it intentional and, if so, what was the goal? I'm pretty sure it's contributing to the perf issues some consumers are seeing (because the compiler has to compare two very similar types).

p.s. Please let me know if you'd rather have an issue in the xstate repo - I started here to spare you the clutter.

[davidkpiano]

@amcasey I believe that was part of the effort to get inference right for something like this:

{
  actions: assign({/* ... */})
}

Where we wanted that to "just work" without having to specify generic types for assign every time, e.g.,:

{
  actions: assign<SomeContext, SomeEvent>({/* ... */})
}

Hence actions being typed as:

type Action =
  | ...
  | AssignAction<...>
  | ...

For proper inference. I will look into simplifying that type.

[amcasey]

@davidkpiano I'm not super-familiar with xstate, but the customer repro I'm working with produces the same completion items in that position if I delete either AssignAction or ActionObject (just much more quickly). It looks like they're just based on the context type argument, which should be inferred the same, regardless of which is removed.

[amcasey]

I believe @davidkpiano's change resolved this issue. Thanks!

p.s. I'm still investigating how we could have made the problem easier to identify.

[AlCalzone]

Thanks everyone, I will test it out after my vacation 👍🏻

[AlCalzone]

@davidkpiano @amcasey
Just did a quick test using today's TypeScript and xstate from https://github.com/davidkpiano/xstate/issues/1350#issuecomment-668852857.
The original repro I've sent @amcasey via mail now takes 8s on my NAS instead of over 30. On my desktop, we're talking 1-2 seconds instead of 5. Although I'm not sure how much of that is the request for completions.

[davidkpiano]

Nice, that's quite the improvement. Please let me know if there are other optimizations I can make in XState and I will prioritize those.

[amcasey]

@AlCalzone it should be about 50/50 completion and squiggles - they're doing the same work (separate discussion :|).

I think I see another hotspot and I'm investigating now.

[amcasey]

@davidkpiano Does this intentionally not pass through the fourth type argument (TTypestate)?

https://github.com/davidkpiano/xstate/blob/ab7329eaf849b613b94cb815a7e1575e9c9f7ea9/packages/core/src/StateNode.ts#L222

[davidkpiano]

@amcasey It's not intentional; it should.

[amcasey]

@davidkpiano FWIW, I don't think that's the problem - I just noticed it as I was digging.

[amcasey]

@AlCalzone as a mitigation, you can add this type annotation:

const incrementSendDataAttempts: AssignAction<SendThreadContext, SendThreadEvent> 

[amcasey]

@davidkpiano Changing TEvent extends { type: K } ? TEvent : never to TEvent at https://github.com/davidkpiano/xstate/blob/ab7329eaf849b613b94cb815a7e1575e9c9f7ea9/packages/core/src/types.ts#L340 saves me another ~10% of check time. I kind of wanted to ask about that condition anyway, since it looks like it just rules it event types that are unions of string literals. Assuming you're happy with the change, it should probably apply to the Array variant as well (smaller benefit, since less consumed).

Edit: actually, if you like the change, you can get rid of the intersection too, for another speedup: [K in TEvent['type'] | '' | '*']?: TransitionConfigOrTarget<TContext, TEvent>.

[davidkpiano]

I kind of wanted to ask about that condition anyway, since it looks like it just rules it event types that are unions of string literals.

The purpose for that rule is that we want to limit what kinds of events are in the transitions:

type SomeEvents =
  | { type: 'count'; value: number; }
  | { type: 'foo' };

// ...
on: {
  count: {
    //       inferred: { type: 'count'; value: number; }
    //                 vvvvv
    actions: (context, event) => {
      console.log(event.value);
    }
  }
}

Is there a more performant way to do this? Changing that to TEvent won't provide that safety.

[amcasey]

Sorry, I'm not sufficiently familiar with xstate to understand your example. What's being ruled out? Also, how's it being ruled out? Does the never make the resulting object unusable or something?

I'll also point out that there's a value judgement to be made here (which I don't have enough information to do): if this code is representative and the more precise type adds 10% to check time for a large percentage of users, is it still worthwhile? If it catches a common or subtle bug, then it probably is. If it catches something rare/hypothetical or something that will be caught by the first test, then a faster build/editor might be preferable.

Anecdotally, the project motivating this issue was hitting this case (i.e. the event type became never) and it was thoroughly non-obvious how to fix it (as I recall, inference for the whole state machine object failed and the invalid event type wasn't flagged in any way).

Edit: Oh, maybe you meant TEvent extends { type: K } ? TransitionConfigOrTarget<TContext, TEvent> : never?

In any case, I think the remaining slowdown is largely due to the slightly different types in TransitionsConfigMap and TransitionsConfigArray. (Since this repro isn't using TransitionsConfigArray, I commented it out and the check time dropped by 2/3.)

[davidkpiano]

It catches a common bug. Expanding on the above example:

type SomeEvents =
  | { type: "COUNT"; value: number }
  | { type: "MESSAGE"; value: string };

interface SomeContext {
  count: number;
}

const machine = createMachine<SomeContext, SomeEvents>({
  context: { count: 3 },
  // ...
  on: {
    MESSAGE: {
      actions: (context, event) => {
        console.log(context.count * event.value);
      }
    }
  }
});

The event is { type: 'MESSAGE'; value: string; } and properly inferred here; whereas if it were SomeEvents, the developer would have to specify event as SomeEvents & { type: 'MESSAGE' } (or something similar) instead to get proper inference.

[mikecann]

Unfortunately I dont currently have a spare minute to help with this issue but I just want to throw in my words of encouragement.

This is really important work for all us TS users and im glad you guys @amcasey and @davidkpiano are working on it. It sadly sounds like theres going to be some tradeoffs of performance vs type safety here.

Im also wondering about regressions as new versions of XState are released. Would it be worth having some sort of "TS Performance Test Suite" that runs on the CI?

[davidkpiano]

Im also wondering about regressions as new versions of XState are released. Would it be worth having some sort of "TS Performance Test Suite" that runs on the CI?

That would be awesome. New versions of XState will be largely compatible with the current version, and typings will be similar. @mattpocock and @Andarist are also working on a TypeGen which will assist greatly with typing machines without compromising performance.

[amcasey]

@davidkpiano I get the same error when I use this (faster) declaration:

declare type TransitionsConfigMap<TContext, TEvent extends EventObject> = {
    [K in TEvent['type'] | '' | '*']?: TransitionConfigOrTarget<TContext, TEvent>;
};

I'm pretty sure it will only make a difference when you have events like:

type SomeEvents =
  | { type: 'count' | 'offset'; value: number; }
  | { type: 'foo' };

[Andarist]

I've prepared a simplified playground showcasing why this matters - look at the declared config and the cond property in it.

The current version (working):
https://www.typescriptlang.org/play?#code/C4TwDgpgBAyglgOwOYBsIHkBOBBTmCGIAPACoB8UAvFCQNoC6UAPjQNwBQ7iwEmAZvgDG0AKIA3CAmDoARgCsIg4FADe7KFFCQAXFADOwTIiQcAvpy3QSBBHrjA4Ae1sBhZ3zhIAsvjCk3UhAAHsAANDTiksrBPAgAJnpQkVKyCkoU1GoatADSUIgRElK0AESWJfT0APy61vi29k4IAR5IWCT4mEgQwETqGjQBPCGh-RokydEhkgmqmuAQunmmUFWFUVC6CBASmP1kZhzsljQ2dg7OLZ4dXT1U+obGzFAArvEQHttxR9y8AsKnernJpXJD+ZzDMLrKRQGIzRKTVKKYAULJQQTOOI1KAACiUQVqQxi4R2tUmAEoqBQZI5HGh6hwNMBOt1gLUzo1Lu5riyemYLAtAQ0Ls1uW1MDdWX1xkSRv0JkUprFZoj5Mj2BlYMY0FhcARiHVhSCxZK7ixDcCuQhWuDAiNoSiDgLIFBQbbIeEFRs4fEEYqkel7mjnNiLZzXGKfH4SLKoV6pE7TEcMbZlII+EhdG7+iooHF8MzdCUALaOOIQFAlKCmUYaXOWIsAMXQ6BKrCgYEwjjAWxexZkvGrz3rCyLACFsAAlNsdrs9ubbAwQOK6AxGZBD8ya4MIXRojQTyd7sYDZm3NlQEoyfAAL2M+BKtYGGhTK9xAH18SSxJTKKiT8+mA9C8mAILCYgAHSdt2EGLjwcRUJQ1AlECADuvBtgB1ZPhoNb9HhSbsEAA

Proposed version (not working):
https://www.typescriptlang.org/play?#code/C4TwDgpgBAyglgOwOYBsIHkBOBBTmCGIAPACoB8UAvFCQNoC6UAPjQNwBQ7iwEmAZvgDG0AKIA3CAmDoARgCsIg4FADe7KFFCQAXFADOwTIiQcAvpy3QSBBHrjA4Ae1sBhZ3zhIAsvjCk3UhAAHsAANDTiksrBPAgAJnpQkVKyCkoU1GoatADSUIgRElK0AESWJfT0APy61vi29k4IAR5IWCT4mEgQwP7OPCHhJMnAZGYc7JY0NnYOzi2eHV09VPqGxsxQAK7xEB4IEHET3LwCwtP1s00LSH2Bg4VRUDGSCUlF0vKKo6rqUILOOI1KAACiUQVqAQGYWeYlqIwAlFQKDJHI40PUOBpgJ1usBajNGvN3ItcT0zBZwFZCXNmiS2pglniiH8SFCYqFWSNniFXokRqlvuwMrBjGgsLgCMQ6g1aTcmSsWDKrsSEK07tChiMyGNKZAoDcNRzHlIebE3gKvulVlkoM5gcqia56T4-Gz+sbhh9daYJgDbMpBHwkLpDX8VFA4vgcboSgBbRxxCAoEpQUycjQRyyxgBi6HQJVYUDAmEcYF0CC2cZkvDTmyzVNjACFsAAlQvF0vl1RQA4GQ66AxGZB18wi23OXS2jQt1tTv4abFk-FQEoyfAAL2M+BKGcXGn9cV0IIA+uDwhAxEjKBRp-uoJgeltMAhYQA6Etlt99nhxKiUagSkuAB3XhCwXRd0wgqCNCg312CAA

[amcasey]

Thanks, @Andarist, examples like that are very helpful! I always forget that conditionals distribute across unions. If that's what you're going for, IMHO (as someone who totally failed to understand this, so grain of salt...) is that it would be more straightforward to have the conditional outside the generic type: TEvent extends { type: K } ? TransitionConfigOrTarget<TContext, TEvent> : never.

I'll ask around in case there's a more direct way to express that.

Edit: no, what you have seems good. And 10% seems like a fair price to pay for that kind of inference improvement.

Edit 2: I haven't dug into it, but my version doesn't actually work. Stick with what you've got. :smile:

[amcasey]

Purely for your amusement, I thought this might help, but it makes checking 4X as slow. 😆

TransitionConfigOrTarget<TContext, TEvent & { type: K }>

[amcasey]

I think I've convinced myself that this fragment of TransitionsConfigArray

{
    [K in TEvent['type']]: TransitionConfig<TContext, TEvent extends {
        type: K;
    } ? TEvent : never> & {
        event: K;
    };
}[TEvent['type']]

is trying to form the union of TransitionConfig<TContext, K_part_of_TEvent> & { event: K } for each type of event, K. If that's the case, I think I've convinced myself that we can use more conditional-distribution to write it as

TEvent extends EventObject ? TransitionConfig<TContext, TEvent> & { event: TEvent["type"]; } : never

which is noticeably faster (10-20%) on my box.

Note that I'm not recommending this as a general pattern - I only suggest sneaky perf changes in code that has been _measured_ to be slow.

[amcasey]

I believe a bunch of time is going into distinguishing TransitionsConfigMap from TransitionsConfigArray. I'm still looking for a way to speed up the check.

Edit: I can no longer repro the speedup from tweaking these - maybe it depended on my incorrect tweak to TransitionsConfigMap.

[amcasey]

If I am correct in believing TransitionsConfigMap will never have an "event" property, you can prepend {event?:never} & to the beginning of the existing union. On my box, it's a substantial speedup.

To summarize, I now have

declare type TransitionsConfigMap<TContext, TEvent extends EventObject> =
    { event?: never }
    & { [K in TEvent['type']]?: TransitionConfigOrTarget<TContext, TEvent extends { type: K; } ? TEvent : never> }
    & {
        ''?: TransitionConfigOrTarget<TContext, TEvent>;
        '*'?: TransitionConfigOrTarget<TContext, TEvent>;
    };
declare type TransitionsConfigArray<TContext, TEvent extends EventObject> = Array<
    (TEvent extends EventObject ? TransitionConfig<TContext, TEvent> & { event: TEvent["type"]; } : never)
    | (TransitionConfig<TContext, TEvent> & { event: ''; })
    | (TransitionConfig<TContext, TEvent> & { event: '*'; })>;

Sorry for mucking with your line wrapping. :smile:

Edit: I added the { event?: never } based on a misreading of the code. It does seem to account for most of the improvement I measured below, but I don't know why. Investigating...

Edit 2: @rbuckton pointed out that I was missing some parens in TransitionsConfigArray. It doesn't affect the numbers, but they are important.

[amcasey]

In the original (i.e. unreduced) repro, I'm seeing

Before

Types:                       59271
Instantiations:             218383
Check time:                  4.41s

After

Types:                       42758
Instantiations:             149793
Check time:                  3.50s

[Andarist]

I'm not sure if there is any recommendation for us here now 😅 Some of your posts seem to refer to the next ones? Or I have just misunderstood your intention. Could you provide a final version of your recommendation? I could then try to assess if it's correct from our POV.

Also a note - I'm also a little bit surprised that this (what we currently have) works:

type TransitionsConfigMap<TContext, TEvent extends EventObject> = {
  [K in TEvent["type"]]?: TransitionConfigOrTarget<
    TContext,
    TEvent extends { type: K } ? TEvent : never
  >;
};

Like - it's not super clear to me that TEvent gets narrowed down by this conditional.

Something like:

TEvent & { type: K }

or

Extract<TEvent, { type: K }> 

would be more clear to me. Are you aware of any perf characteristics of all of those options? You have already kinda observed that using & decreases the perf, so there is that - probably best to avoid this pattern for extracting a subset of a union. My guess (based on type hints displayed in tooltips) is that this doesn't eagerly narrow things down but rather expand on the union, creating things like (U1 & T) | (U2 & T) | ... and this probably hurts perf later down the road.

[amcasey]

@Andarist sorry, my investigation notes tend to be pretty stream-of-consciousness. :smile: I tried to indicate the final state in my "to summarize" comment and the numbers reflect the performance impact of those changes.

Having said that, I'd hold off a bit longer because we can't actually figure out _why_ that change helps so much (or just drop { event?: never } - the rest of the change seems sound).

Regarding your current code, it took me a while to figure that out too, because I always forget that conditionals distribute over unions. My mental model (which probably isn't precisely correct) is that (T1 | T2) extends TCond ? TTrue : TFalse is treated as (T1 extends TCond ? TTrue : TFalse) | (T2 extends TCond ? TTrue : TFalse).

I did try TEvent & { type: K } since that seemed most natural to me too, but it caused a huge slowdown. I'm planning to isolate that and file a fresh bug (against us, not you).

I would agree that it's generally best to avoid intersection types as much as possible.

[AlCalzone]

@amcasey Do you know if the changes in Typescript that helped speed up my project are included in 4.0.2?

[Andarist]

@Andarist sorry, my investigation notes tend to be pretty stream-of-consciousness. 😄

Not a problem, I definitely tend to stream my consciousness on others as well as my thoughts race 😅

My mental model (which probably isn't precisely correct) is that (T1 | T2) extends TCond ? TTrue : TFalse is treated as (T1 extends TCond ? TTrue : TFalse) | (T2 extends TCond ? TTrue : TFalse)

Also not sure if precisely correct, but my mental model is exactly the same for this 😉

I would agree that it's generally best to avoid intersection types as much as possible.

Interesting, generally I wouldn't have guessed that this might affect things that much but clearly it might - given this issue.

---

As to the final proposed improvement - TransitionsConfigMap seems to be exactly the same as we currently have, apart from the { event?: never } part. Adding this { event?: never } is unfortunately not technically correct. TEvent can be parametrized with { type: 'event' } after all. If that would improve our perf dramatically we could add this not-so-correct change, but if we could avoid it - then this would be preferred.

Looking at the proposed TransitionsConfigArray: so this trick uses conditional types distribution to handle this rather than mapped types. Nice trick. Gonna prepare a PR with this change in a moment as the current version is incorrect anyway from what I'm seeing.

---

Question - would you be open for chatting a little bit about TS performance and guidelines? We'd love to improve the situation in this regard but, quite frankly, there are nearly no resources about the subject. We are also working on a typegen tool that potentially could help with this, but it would also be very helpful for us if we would know what patterns should we stick to there, what patterns should we avoid, etc.

[amcasey]

@AlCalzone I only remember making xstate changes to speed up your project. Having said that, I'm not aware of important perf improvements that slipped from 4.0 so, if we previously discussed some, yes, they probably landed.

[amcasey]

@Andarist

As to the final proposed improvement - TransitionsConfigMap seems to be exactly the same as we currently have, apart from the { event?: never } part

Oops, didn't even notice. Kudos on your optimization. :smile:

Regarding { event?: never } - you should definitely ignore that part for now. We have yet to figure out what that's doing or why it speeds up checking so much (bizarrely, the name doesn't seem to matter - you can use "definitelyNotAProperty" and it has the same effect). I'll update when we've figured it out.

would you be open for chatting a little bit about TS performance and guidelines?

Definitely, but I should warn you that the reason there aren't any resources is that we're still figuring it out for ourselves. Working through problems like this one is an important step towards being able to publish tools and best practices for the community.

[amcasey]

Finding # 1: @AlCalzone, you (and other consumers of xstate) probably want "strictFunctionTypes": true, because there are so many variance checks when using xstate types. Forcing the variance checks will probably also speed things up (because they're cheaper than structural checks).

[amcasey]

Finding # 2: It sure looks like something is going wrong: #40247. At the very least, the behavior is surprising. (Note that the issue is about correctness, rather than perf, but I conjecture that the behavior change is resulting in short-circuiting of expensive logic, affecting the perf.)

[AlCalzone]

probably want "strictFunctionTypes": true, because there are so many variance checks when using xstate types

I think I'll pass 😨 (although the speedup is immense)

Aside from that, this breaks a bunch of typed function declarations (or at least makes them much less elegant).

[amcasey]

@AlCalzone I agree that that error text is intimidating, but - in the absence of evidence to the contrary - I'm inclined to believe that it indicates a real problem with the code.

Having said that, if you feel it's making your code worse, I'd be interested to know more.

[AlCalzone]

I'd be interested to know more.

I'm pretty sure this is intended behavior, but it makes enforcing assignability constraints cumbersome.

Basically I need to enforce that the parameters of predicates passed to a function are subclasses of the predicate argument types:
https://www.typescriptlang.org/play?strictFunctionTypes=false&noImplicitReturns=false#code/CYUwxgNghgTiAEkoGdnwLIlVA5gg3vAA4wCWAblAC4IC2AjAFzwCuAdgNZsD2A7mwG54AXwCwAKFBI4iaKngBlIuFJQImbHnggAHjTbA0G5LgLEylGvFoAmZuy59BIiVOgyk8gHLcqAQWNTeEISCmo6AGZ7Th5+ITFxCSoAT2VzEGBSMHCAHgAVbT0QAyMsEy0AXgwy0wA+eCqAClpkHGY8gEoG+oAjbm4IEChBV3B3BAAzdjAqUm42eAn+xqJmEgys8I7mPoGhkcTxAHoj+F5SKgALbhYqeGQqMhmAMWnZ+bzUrHgr0jQ-+DcDgSMDzB7EehQNZwTLZKxNFptRTKMCqdQ1PBdCr1fASeD4+BwKgsGALR4sEACCTCKniJbcFaQjq0iQnM4Xa63e6PLJUV5sGZzNifZRoX7-NCgmBwGYQZLwUA0GYZeAoVULEDS7gwVmnHz+QJaHh3FDIUg4NhQHqDH7caqaEAgsF3Ij0HrQjZwhAI1rMfUBDEgLE4vEEokkskwCm0mkSemMnrMoA

With strictFunctionTypes, the generics must be passed through and are decoupled from the implementation params:
https://www.typescriptlang.org/play?noImplicitReturns=false#code/CYUwxgNghgTiAEkoGdnwLIlVA5gg3vAA4wCWAblAC4IC2AjAFzwCuAdgNZsD2A7mwG54AXwCwAKFBI4iaKngBlIuFJQImbHnggAHjTbA0G5LgLEylGvFoAmZuy59BIiVOgyk8gHLcqAQWNTeEISCmo6AGZ7Th5+ITFxCSoAT2VzEGBSMHCAHgAVbT0QAyMsEy0AXgwy0wA+eCqAClpkHGY8gEoG+oAjbm4IEChBCQB6UfheUioAC3hkKjIwKgAxdmXSbjY81Kx4AEl4NhAM+CpuYhQ0PGOltFmYbhYcOZByEBhk3hmPkFdwdwIABm6yomzY8CB-XyhX0hmqmhAtUaRGYJAyWVyeVqHWYfQGQxGiXE43gw2AZx+yWsLAW8B6CGQyjApCBpFOpAhswQKTSblg1HBEjAWzpRBsUDRcEy2RoOSUKjUgTw9SaLRwXQq9XwEngevgcCoLBgXJgLBAAgkwkt4ih3BREo6NrGExFMDgywg1NANGWpxQZIhH0eMGFoqoxBsPSlGNlIByPn8yqRDXgzVamu1uv1huNpvNNutEjtDp6TokLoOiGGrGQwNB4LQnIWQ2AABp6SwI7FKQhg9wYGSiMpYGhqL3a0FkNMQB2eBHufBQGy2NMhbaG1tIRL021FMzVOoangujrxPqDSAjSazmaLVbi-1GkDHc6N2wNluXz1d8xEwFjxAU9sz1XMb0WAsH1tJ9vydIA

[AlCalzone]

@amcasey with a bunch of changes I could build the project again. However there seems to be a problem with raise actions now (which are the source of the wall of errors).

You can play with this tree: https://github.com/AlCalzone/node-zwave-js/tree/e10cb2c6d706bfc9dda0b5039ec38c651a2fbbd7

In https://github.com/AlCalzone/node-zwave-js/blob/e10cb2c6d706bfc9dda0b5039ec38c651a2fbbd7/packages/zwave-js/src/lib/driver/SendThreadMachine.ts, you'll see I had to any some of the raise actions, e.g. line 394 or 405.

I'm not sure why these would cause an issue. @davidkpiano I assume those should just work?

---

by the way, intellisense no longer has any noticeable delays now :)
But there's still a discrepancy between lerna run build, npm run watch and the language server in reported errors.

[Andarist]

@AlCalzone I've forked your second playground here, you can look at the playground or read through it here as I'm going to basically copy-paste stuff here

My comments are purely a comparison to your first playground - I have discarded other examples that you have included in the second one but not in the first one.

You have included this:

// and they must be specified in the type declaration
const p2a: predicate<SpecialMessage> = (msg) => {
    return true;
};
foo(p2a);

but if I remove the explicit type param this works OK, so I'm not sure what you are referring to when saying that "they must be specified in the type declaration".

case1: "without strictFunctionTypes this is ok" from the other playground

const p1a: predicate = (msg: SpecialMessage) => {
    return true;
};
foo(p1a);

const p2a: predicate<Message> = (msg: SpecialMessage) => {
    return true;
};
foo(p2a)

Those are both errors, but why exactly? Let's look at an example that is somewhat easier to understand:

class Animal {
    walk() {}
}
class Dog extends Animal {
    bark() {}
}
function acceptDog(dog: Dog) {
  dog.bark()
}
const acceptAnimal: (anim: Animal) => void = acceptDog
acceptDog(new Animal()) // ERROR!

This makes total sense because acceptDog could use dog-specific properties (and it does - it calls .bark()) and an instance of Dog's super-class just doesn't satisfy this constraint. Tt's basically what happens here, this is 100% correct, maybe you have already known this but I felt it might be worth reiterating through this example to demonstrate that this behavior is preventing bugs.

case2: without strictFunctionTypes this is correctly detected as an error

const p1b: predicate = (msg: NotAMessage) => {
    return true;
};
foo(p1b);

This one is still detected as an error, even without an explicit type param

raise issue

Could you raise an issue about this in XState? I haven't looked into this yet at all - so maybe it's just working as intended but maybe it is not. It will be easier for us to track it when reported in our issue tracker

[AlCalzone]

but if I remove the explicit type param this works OK, so I'm not sure what you are referring to when saying that "they must be specified in the type declaration".

Maybe I was unclear. These predicate functions are taylored to specific subclasses of the Message class, so I need to specify the type somewhere. Doing it for the function argument improves readability IMO.

This makes total sense because acceptDog could use dog-specific properties

True, I haven't thought about it this way. I guess my use case aims a lot lower. Here's an example:

function testResponseForBinarySensorGet(sent: BinarySensorCCGet, received: BinarySensorCCReport) {...} 

@expectedCCResponse(BinarySensorCCReport, testResponseForBinarySensorGet)
export class BinarySensorCCGet extends BinarySensorCC { ... }

The logic that uses this decorator internally makes sure that the first predicate argument is an instance of the class the decorator is on (BinarySensorCCGet). The decorator itself makes sure that the second predicate argument is an instance of the first decorator argument (BinarySensorCCReport).
So basically the check was supposed to check if the predicate argument types are both subclasses of the common super class of all these CC classes. If not, I probably messed up while auto-importing something (it has happened before!).

[amcasey]

@AlCalzone

by the way, intellisense no longer has any noticeable delays now :)

Excellent!

But there's still a discrepancy between lerna run build, npm run watch and the language server in reported errors.

https://github.com/microsoft/TypeScript/issues/39811

I'm pretty sure this is intended behavior, but it makes enforcing assignability constraints cumbersome.

Variance is incredibly confusion, but I think I might see what's going wrong in your examples. (This is basically just a rehash of @Andarist's excellent explanation, but maybe another framing will be helpful.)

Suppose I have a dog-watcher - a Watcher<Dog>. Obviously, my dog-watcher can watch any sort of dog. If I ask it to watch a cat, no good. What happens if all the dog-watchers I know are sick and I need someone else to watch my dog. In this case, we have the opposite behavior from an array - to replace a Watcher<Dog>, I need someone who can watch a supertype of Dog - maybe a Watcher<Animal>, because someone who can watch any animal can certainly watch any dog. (What if I went the other way? I might end up with a chihuahua-watcher for my pitbull and everyone would be unhappy.)

A predicate is like a watcher. When I say I'm willing to accept any function that validates Dogs (i.e. predicate<Dog>), I mean that I would accept any function that is at least capable of validating any Dog (e.g. predicate<Animal>, which can validate any animal). Having the type parameter in an input position (i.e. as the type of the predicate's parameter) means that I want to accept supertypes, rather than subtypes.

So, as far as I can tell, the compiler is correctly detecting that your expectation was backwards - I can assign a predicate<Message> to a predicate<SpecialMessage> because it's more general, but not the other way around (otherwise, someone might test a SpecialMessage2 with the predicate and it wouldn't know how to handle it.)

Unfortunately, there's really no way we could fit an explanation of variance into a compiler error message. 😢

you'll see I had to any some of the raise actions, e.g. line 394 or 405.

I'm not sure what's going on with raise (since I haven't played with your branch yet), but it sounds like you're going to discuss it on the xstate repro. Obviously, feel free to bring it back here or loop me in if the compiler appears to be behaving incorrectly. :smile:

Edit: I dropped a part of my explanation that was botched (sigh, variance). I thought about correcting it, but it wasn't relevant to the particular issue we're discussion. I'll summarize it briefly here: you can put a Dog in an Animal[], but that's not variance - that's just subtyping. However, you can also (depending on the language) treat a readonly Dog[] as a readonly Animal[] (the readonly matters - it's reversed for write-only arrays), because any access will return an Animal. I meant to contrast this with Watcher, which has the opposite behavior, but the contrast isn't particularly necessary.