Cc/discriminated unions #2

ChristopherChudzicki · 2022-10-22T20:58:57Z

I took a look at your work in regards to josdejong#2810. (Thanks for looking at that, by the way.)

See comments (which I am about to add) for more.

ChristopherChudzicki · 2022-10-22T21:01:14Z

.eslintrc.cjs

-      files: ['types/*.ts'],
+      files: ['types/*.ts', 'test/**/*.ts'],


This is unrelated, but I noticed that the the TS tests weren't being linted since they moved directories.

Nice find. We probably want to move that to a separate PR though?

It's a tiny enough change that I for one don't mind throwing it in here but of course if Jos objects...

Mostly added it in this PR because the lack of linting annoyed me 😄 I went ahead and opened a separate PR for this change just to (possibly) get it in sooner. Should have done that last week 🤷

ChristopherChudzicki · 2022-10-22T21:04:20Z

test/typescript-tests/testTypes.ts

-  expectTypeOf(instance1).toMatchTypeOf<MathNode>()
-  expectTypeOf(instance1).toMatchTypeOf<MathNodeCommon>()


If we're trying for a discriminated union on MathNode, then I don't think this test should pass: instance1 is a BaseNode, and I don't think a BaseNode should be assignable to a MathNode: BaseNodes. The type property on BaseNode is type: string, whereas on all other nodes it's a constant.

If BaseNode is assignmable to MathNode, then

let node: MathNode if (node.type === 'AssignmentNode) { // now node has type BaseNode | AssignmentNode }

ChristopherChudzicki · 2022-10-22T21:11:07Z

types/index.d.ts

-    TOp extends OperatorNodeMap[TFn] = never,
-    TFn extends OperatorNodeFn = never,
+    TFn extends OperatorNodeFn = OperatorNodeFn,


I should have isolated this issue more, but I am pretty sure that...

In the old setup, something like `new OperatorNode("+", "addition", ...)" was never going to be assignable to be assignable to a MathNode, because:

MathNode union looked like NodeType1 | NodeType2 ... | OperatorNode | ..., i.e., OperatorNode was using its defaults in MathNode union...

and OperatorNode default included op: never, so nothing was ever going to be assignable to OperatorNode.

To remedy this, I originally wanted:

interface OperatorNode< TOp extends OperatorNodeMap[TFn] = OperatorNodeMap[TFn], TFn extends OperatorNodeFn = OperatorNodeFn, ...>{...}

But TS complained later parameters TFn can't be used as defaults for earlier parameters. So originally I was going to swap the order of the two, but then I just got rid of TOp.

NOTE: Fixing this issue may not have been entirely necessary to get the discriminated union stuff to work, I'm not 100% sure, and I need to run soon so can't try to isolate it more right now.

In the old setup, something like `new OperatorNode("+", "addition", ...)" was never going to be assignable to be assignable to a MathNode, because:

Do you have a reproducible example of this? Maybe I'm misunderstanding but assigning new OperatorNode("+", "addition", ...) to MathNode seems to work for me (at least in test/typescript-tests/testTypes.ts):

But TS complained later parameters TFn can't be used as defaults for earlier parameters. So originally I was going to swap the order of the two, but then I just got rid of TOp.

I think it's going to be a fairly big breaking change to change the generics for OperatorNode so I'd suggest we try and instead find a way around whatever issue there is here.

tldr: I agree we should not change the OperatorNode signature in this PR. Instead we should use OperatorNode<OperatorNodeOp, OperatorNodeFn> in the MathNodes interface. I do think there's an issue with the type definition for OperatorNode, but best not to change it here.

The test you screenshotted above will fail on your branch; I agree it passes on master. Simpler version:

// passes on MathJs 11.2.1, (last version that used discriminated union) // passes on MathJs ^11.3.0 (when discriminated union was removed) // fails on goodproblems/types/revert-to-discr-union-for-nodes const node1: MathNode = new OperatorNode("+, "add", [new ConstantNode(1), new ConstantNode(1)])

Looking back at the old typedefs, the old discriminated union was:

type MathNode = ... | ... | OperatorNode<OperatorNodeOp, OperatorNodeFn> | ... ;

So I think we should go back to using OperatorNode<OperatorNodeOp, OperatorNodeFn> not OperatorNode (with its default arguments). That's definitely a more focused change than changing the type parameters.

But.. I do think that there is an issue with OperatorNode's type definition (a separate issue that needs not be handled here). Namely, its default arguments seem useless to me. No OperatorNode instance can ever be assignable OperatorNode type (with its default args): Currently on master:

Better to change the type signature or remove the defaults, imo. But, again, I agree: best to avoid that here.

The test you screenshotted above will fail on your branch; I agree it passes on master. Simpler version:

I was on that wrong branch, you're correct. 🙄

So I think we should go back to using OperatorNode<OperatorNodeOp, OperatorNodeFn> not OperatorNode (with its default arguments). That's definitely a more focused change than changing the type parameters.

Awesome, looks good 👍🏻

But.. I do think that there is an issue with OperatorNode's type definition (a separate issue that needs not be handled here). Namely, its default arguments seem useless to me. No OperatorNode instance can ever be assignable OperatorNode type (with its default args): Currently on master:

This was the reason for using never: josdejong#2576 (comment), not sure it's still a good reason. Happy to check out a PR which changes this.

ChristopherChudzicki · 2022-10-22T21:12:54Z

test/typescript-tests/nodeExtensions.ts

+interface MyNode extends BaseNode {
+  type: 'MyNode'
+  a: MathNode
+}
+declare module 'mathjs' {
+  interface MathNodeTypes {
+    MyNode: MyNode
+  }
+}


I didn't see this declaration in your branch...We would not expect some of the tests below to pass without it, right?

TS told me that ambient declarations need to be at the top level of nesting within a file, and I didn't want this to affect the other tests, so I moved this test to a separate file.

I didn't see this declaration in your branch...We would not expect some of the tests below to pass without it, right?

Yep, think you're right.

TS told me that ambient declarations need to be at the top level of nesting within a file, and I didn't want this to affect the other tests, so I moved this test to a separate file.

Yeah separate file seems fair. We are aiming to start splitting this up anyways.

mattvague · 2022-11-02T17:17:18Z

Nice work and thank you @ChristopherChudzicki! Sorry it took a while to respond. Left a few comments

gwhitney · 2022-11-02T18:37:09Z

Whenever you can get a consensus PR filed against mathjs itself that will be great :)

ChristopherChudzicki · 2022-11-03T02:08:53Z

@mattvague Good call about ~~minimizing~~ removing the OperatorNode change. I got it working without that, should be ready for another look.

mattvague · 2022-11-04T16:12:29Z

@ChristopherChudzicki Nice work, thank you! Will try and get this merged ASAP

ChristopherChudzicki added 4 commits October 22, 2022 16:22

lint ts files in test dir

0fb3bf7

change operatornode generic defaults

8928cb3

remove TOp type generic type

ef72237

add a test for discrimination

bdf864d

ChristopherChudzicki commented Oct 22, 2022

View reviewed changes

ChristopherChudzicki mentioned this pull request Oct 22, 2022

TS regression: Narrowing of MathNodes by type josdejong/mathjs#2810

Open

revert to old operatornode typeargs

899115d

mattvague merged commit 08eb25e into goodproblems:types/revert-to-discr-union-for-nodes Nov 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cc/discriminated unions #2

Cc/discriminated unions #2

ChristopherChudzicki commented Oct 22, 2022

ChristopherChudzicki Oct 22, 2022

mattvague Nov 2, 2022

gwhitney Nov 2, 2022

ChristopherChudzicki Nov 3, 2022

ChristopherChudzicki Oct 22, 2022

ChristopherChudzicki Oct 22, 2022

mattvague Nov 2, 2022

ChristopherChudzicki Nov 3, 2022 •

edited

Loading

mattvague Nov 4, 2022

ChristopherChudzicki Oct 22, 2022

mattvague Nov 2, 2022

mattvague commented Nov 2, 2022

gwhitney commented Nov 2, 2022

ChristopherChudzicki commented Nov 3, 2022

mattvague commented Nov 4, 2022

		files: ['types/*.ts'],
		files: ['types/.ts', 'test//.ts'],

		expectTypeOf(instance1).toMatchTypeOf<MathNode>()
		expectTypeOf(instance1).toMatchTypeOf<MathNodeCommon>()

Cc/discriminated unions #2

Cc/discriminated unions #2

Conversation

ChristopherChudzicki commented Oct 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristopherChudzicki Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattvague commented Nov 2, 2022

gwhitney commented Nov 2, 2022

ChristopherChudzicki commented Nov 3, 2022

mattvague commented Nov 4, 2022

ChristopherChudzicki Nov 3, 2022 •

edited

Loading