TS regression: Narrowing of MathNodes by type

[ChristopherChudzicki]

Describe the bug
Prior to v11.3 (not sure how far back, but certainly at least 10), MathNode would be narrowed based on its type property:

const doStuffWithMathNode(node: MathNode) => {
  if (node.type === 'FunctionAssignmentNode) {
    // now TS knows that node is a FunctionAssignmentNode and lets you access associated props
    // like node.params
  }
  // back to being a wider MathNode
}

To Reproduce

• working in v11.2 https://codesandbox.io/s/typescript-playground-export-forked-wnelgb?file=/index.ts
• erroring in v11.3 https://codesandbox.io/s/typescript-playground-export-forked-ke8srr?file=/index.ts

[ChristopherChudzicki]

I think this is a consequence of #2772. I haven't thought about it enough, but would be nice if the goals of that PR could be achieved while still allowing the narrowing.,

[josdejong]

Ai, good point.

I think we can solve this by defining functions like isFunctionAssignmentNode as typeguards, and then use

if (isFunctionAssignmentNode(node)) { 
 // ...
}

instead of

if (node.type === 'FunctionAssignmentNode) {
  // ... 
}

anyone interested in working out a fix? Probably by defining typeguards?

[ChristopherChudzicki]

@josdejong Those exist as typeguards already! Apparently I wrote the typedefs for them...not sure why I started using .type more recently instead.

Yes, I think that's a good solution. Feel free to close this.

Might be worth adding a note in the 11.3.0 release notes about this, though. Also, #2772 was missing its PR link in the release notes.

[gwhitney]

Right, #2772 removed the status of MathNode being a discriminated union.

Aren't all the custom type guards already provided? The PR explicitly changed node.type === 'SymbolNode' to isSymbolNode(node) for type narrowing in one test.

But note that providing a series of type guards isn't quite the same as a discriminated union. At least, you now can't handle a Node with a switch statement on its .type property and have it be narrowed in each case, and you can't do exhaustiveness checking on code that handles nodes (TypeScript no longer has any idea what "all the node types" are.)

So the question becomes which is more important: type narrowing just by checking .type, switch statements, and exhaustiveness checking (pre 11.3) or client extensibility of the node hierarchy while having to use custom type guards (post 11.3)? Unless there's some way to extend a discriminated union, which I doubt. @ChristopherChudzicki do the custom type guards cover your use cases?

[gwhitney]

Comments crossed. Sounds like the custom type guards are good enough.

[josdejong]

😂 that is good news!

I've updated the history to note this, good point

[gwhitney]

(And in some sense it means that 11.3 was inadvertently a breaking change, despite not being a major revision number.)

[josdejong]

yes indeed... not an intended one. Sometimes a new feature is a bug at the same time 😅

[gwhitney]

Hmmm, it turns out it would be possible to get both the discriminated union and client extensibility, see the answer to https://stackoverflow.com/questions/46392758/widen-tagged-discriminated-union-in-typescript-in-a-different-module, at the cost that clients that wanted to do so would need to use the declare module 'mathjs' { } syntax that I was previously unaware of. That seems like it would be the best of both worlds, with the only drawback that it would oblige clients that want to add Node types to use a somewhat unfamiliar TypeScript language facility. (We would have to provide an example of this, either in a test or in the examples directory.) Should we revert/replace #2772 with an alternate PR that sets up TypeScript to allow the extension of the MathNode union along the lines covered in the StackExchange answer? Or just leave be the new status quo with no discriminated union for nodes? Just wanted to raise the question since there is surprisingly actually an option that allows unions to be extended, so we could as ChrisopherChudzicki put it "achieve the goals of #2772 while still allowing narrowing" just by checking the .type property.

[josdejong]

Thanks for figuring this out, this is interesting to know.

What are you're opinions @ChristopherChudzicki @mattvague? I don't have a strong opinion on this.

[mattvague]

What are you're opinions @ChristopherChudzicki @mattvague? I don't have a strong opinion on this.

Interesting. Well, if we were forced to choose supporting either node.type checks with discriminated unions or client-extensible node types, I would say it's more important to support the latter since there is an alternative to the former in if-else statements with type guards (even if not always ideal). However, if it's possible to support both with something like @gwhitney's solution, and that didn't have significant costs associated with it then that's probably the best course of action.

Should we revert/replace #2772

If we revert 2772 we still have to do a little bit of work to make the declare module ... option workable right? I'd be more in favour of a new PR that reverts the changes from 2772 and adds support for declare module ... rather than just straight up reverting it.

[mattvague]

(I'm also happy to attack that since I was the culprit here 😆)

[gwhitney]

I'd be more in favour of a new PR that reverts the changes from 2772 and adds support for declare module ... rather than just straight up reverting it.

Oh yes, that's what I meant by "revert/replace", sorry I was speaking in too abbreviated a fashion. And yes, per that StackExchange answer, there is some extra stuff that needs doing to make both things work: you have to create an interface that has all of the node types, because interfaces are extensible in this manner but apparently unions cannot be directly extended with this declare module stuff. If you're interested and willing to take a stab at restoring the discriminated unions while allowing the extensibility, Matt, that would be great, especially as you can make certain that the extensibility with declare module 'mathjs' {...} really does work as you need it to, with the mathjs bundle resulting from the changes.

[ChristopherChudzicki]

I agree with @mattvague that

Well, if we were forced to choose supporting either node.type checks with discriminated unions or client-extensible node types, I would say it's more important to support the latter since there is an alternative to the former ...

Aside: I don't want to sidetrack us here, but is there a discussion anywhere about use-cases for custom node types? I am curious.

The SO post that @gwhitney found seems good: not too complicated to implement, and not too big a burden on people in userland extending the library. And there is value in the discriminated union for consumers as opposed to typeguards...e.g., I know it's a small thing, but I really appreciate the intellisense / code completion offered by the discriminated union:

So yeah, IMO, worth doing!

[gwhitney]

Aside: I don't want to sidetrack us here, but is there a discussion anywhere about use-cases for custom node types? I am curious.

@mattvague gives a brief description of a use case in his intro to #2775, so you could look there, and he might have more to say on this topic.

[josdejong]

Thanks for the feedbacks guys. Yes, it would be nice to be able to support both (in which case using .type requires some special notation declare module 'mathjs' {...} by the user when extending the collection of node types, which is acceptable). Let us also be pragmatic: if this leads to unforeseen complications, let's reconsider if we really want to go down some tricky TS rabbithole.

So @mattvague I understand you would like to give it a try?

[gwhitney]

in which case using .type requires some special notation declare module 'mathjs' {...} by the user,

To be clear, only extending the collection of node types would require this syntax...

[josdejong]

Yes indeed 👍 . I'll update my comment to prevent any future confusion

[mattvague]

So @mattvague I understand you would like to give it a try?

Yep! Going to spend a bit of time today on this.

[mattvague]

Got a start here, but still some stuff to figure out: #2820

[gwhitney]

I took a quick look at the first typescript error, and it seems to me that transform1 is declared to return an OperatorNode<'+', 'add'> which is missing the third generic template parameter, so that third parameter defaults to BaseNode[] if I am reading your new code right, but then it is typechecking the result against OperatorNode<'+', 'add', MathNode[]> but MathNode is not the same type as BaseNode, it's a union of types that extend BaseNode, so it's not too surprising to me that TypeScript is unhappy. I don't think in that TypeScript assertion its enough for the specified type to extend the actual type, I think it has to be the actual type. So it seems to me that either the typing of transform1 or the type it has to be checked against has to change so they match. I am not certain about this diagnosis as I am not that much of a TypeScript person, and I didn't look at the other errors, but maybe these remarks will help a little.

[ChristopherChudzicki]

@mattvague I just took a look at your branch and made a PR (goodproblems#2) that updates it to not have errors.I did this a bit quick and have to run now, so forgive my brevity!

[mattvague]

Status update: responded to @ChristopherChudzicki's PR on my PR.