OperatorNode type should accept generics for 'op' and 'fn'

[mattvague]

The Problem

Right now all operator types share the same basic OperatorNode type. This has a few consequences:

• Nothing right now prevents creating invalid operators (like I did the other day) with new OperatorNode('*', 'mult')
• Right now you can't properly use multiple subsequent type guards that all check for a diff operator type

For example, say I've defined my own custom type guards like so

export function isAdditionNode(node: MathNode): node is OperatorNode {
  return isOperatorNode(node) && node.fn === 'add'
}

export function isSubtractionNode(node: MathNode): node is OperatorNode {
  return isOperatorNode(node) && node.fn === 'subtract'
}

If I try to then use them like this

function doStuff(node: MathNode) {
  if (isAdditionNode(node)) {
    return node.args[0]
  }
  if (isSubtractionNode(node)) {
    return node.args[0]
  }
}

Typescript is sad and upset because it thinks that the first type guard should have caught any operator node and therefore you could never have the second case (explaining the never type)

The Solution

If we added generic args to OperatorNode for op and fn typescript would warn us when making invalid operator nodes

and would allow doing this with type guards:

export function isAdditionNode(node: MathNode): node is OperatorNode<'+', 'add'> {
  return isOperatorNode(node) && node.fn === 'add'
}

export function isSubtractionNode(node: MathNode): node is OperatorNode<'-', 'subtract'> {
  return isOperatorNode(node) && node.fn === 'subtract'
}

Which then makes typescript very happy 🌈

[josdejong]

Thanks. It makes sense to refine the OperatorNode type with generics.

In your example demonstrating the issue I think TypeScript has a point, and you probably should not use isAdditionNode and isSubtractionNode to both type guard OperatorNode. I guess in the current setup you would have to use two checks: a type guard isOperatorNode(...): node is OperatorNode, and an additional function isAdditionNode(...): boolean.

Thinking aloud here, would the following work?

interface AdditionNode extends OperatorNode {
  op: '+'
  fn: 'add'
}

export function isAdditionNode(node: MathNode): node is AdditionNode {
  return isOperatorNode(node) && node.fn === 'add'
}

Thanks for implementing a generics based solution in #2576, that looks neat! I think it's a better solution than my idea here ☝️

[mattvague]

In your example demonstrating the issue I think TypeScript has a point, and you probably should not use isAdditionNode and isSubtractionNode to both type guard OperatorNode. I guess in the current setup you would have to use two checks: a type guard isOperatorNode(...): node is OperatorNode, and an additional function isAdditionNode(...): boolean.

Hmm, why do you think it's a bad plan to have isAdditionNode and isSubtractionNode as type guards for OperatorNode? Or are you just saying it's a bad plan when it's not generic like in my first example?

Thinking aloud here, would the following work?

I thought about that, but it would mean a fair bit of extra work to define all the subtypes for exist operators, plus adding an extra burden for new ones which is why I like the generics approach

Thanks for implementing a generics based solution in #2576, that looks neat! I think it's a better solution than my idea here ☝️

Oh ok cool, this sounds like you're agreeing with my approach 😄

[josdejong]

Hmm, why do you think it's a bad plan to have isAdditionNode and isSubtractionNode as type guards for OperatorNode? Or are you just saying it's a bad plan when it's not generic like in my first example?

It's not "wrong" perse, but since both isAdditionNode and isSubtractionNode return the same type OperatorNode, they are in conflict with each other, like you clearly demonstrate. It works better if they return a different result, like a generic type like OperatorNode<'+', 'add'> and OperatorNode<'-', 'subtract'> (like in your PR), or else AdditionNode and SubtractionNode (like in my small pseudo code example).

Oh ok cool, this sounds like you're agreeing with my approach 😄

Yes. Sorry if I confused you by thinking aloud too much 😂