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.

Sign up for GitHub

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

Cc/discriminated unions #2

Merged
merged 5 commits into from
Nov 4, 2022
Merged

Cc/discriminated unions #2

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0fb3bf7
lint ts files in test dir
ChristopherChudzicki Oct 22, 2022
8928cb3
change operatornode generic defaults
ChristopherChudzicki Oct 22, 2022
ef72237
remove TOp type generic type
ChristopherChudzicki Oct 22, 2022
bdf864d
add a test for discrimination
ChristopherChudzicki Oct 22, 2022
899115d
revert to old operatornode typeargs
ChristopherChudzicki Nov 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintrc.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ module.exports = {
}
},
{
files: ['types/*.ts'],
files: ['types/*.ts', 'test/**/*.ts'],
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷

extends: [
'plugin:@typescript-eslint/recommended',
'plugin:prettier/recommended' // this should come last
Expand Down
52 changes: 52 additions & 0 deletions test/typescript-tests/nodeExtensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { expectTypeOf } from 'expect-type'
import {
MathNode,
BaseNode,
MathNodeCommon,
ConstantNode,
Node,
FunctionAssignmentNode,
} from 'mathjs'

interface MyNode extends BaseNode {
type: 'MyNode'
a: MathNode
}
declare module 'mathjs' {
interface MathNodeTypes {
MyNode: MyNode
}
}
Comment on lines +11 to +19
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/*
MathNode examples
*/
{
class CustomNode extends Node implements MyNode {
a: MathNode
type: 'MyNode'
constructor(a: MathNode) {
super()
this.a = a
}
}

// Built-in subclass of Node
const instance1 = new ConstantNode(2)

// Custom subclass of node
const instance2 = new CustomNode(new ConstantNode(2))

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

expectTypeOf(instance2).toMatchTypeOf<MathNode>()
expectTypeOf(instance2).toMatchTypeOf<MathNodeCommon>()
expectTypeOf(instance2).toMatchTypeOf<CustomNode>()

let instance3: MathNode
if (instance3.type === 'FunctionAssignmentNode') {
expectTypeOf(instance3).toMatchTypeOf<FunctionAssignmentNode>()
}
}
62 changes: 10 additions & 52 deletions test/typescript-tests/testTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,16 @@ import {
Matrix,
ObjectNode,
OperatorNode,
OperatorNodeFn,
OperatorNodeOp,
ParenthesisNode,
PolarCoordinates,
QRDecomposition,
RangeNode,
SimplifyRule,
SLUDecomposition,
SymbolNode,
MathNodeCommon,
Unit,
Node,
isSymbolNode,
BaseNode,
} from 'mathjs'
import * as assert from 'assert'
import { expectTypeOf } from 'expect-type'
Expand Down Expand Up @@ -1089,16 +1086,16 @@ Transform examples
{
const math = create(all, {})
{
const myTransform1 = (node: MathNode): OperatorNode<'+', 'add'> =>
const myTransform1 = (node: MathNode): OperatorNode<'add'> =>
new OperatorNode('+', 'add', [node, new ConstantNode(1)])
const myTransform2 = (
node: OperatorNode<'+', 'add'>
): OperatorNode<'-', 'subtract'> =>
node: OperatorNode<'add'>
): OperatorNode<'subtract'> =>
new OperatorNode('-', 'subtract', [node, new ConstantNode(5)])

expectTypeOf(
math.parse('sqrt(3^2 + 4^2)').transform(myTransform1)
).toMatchTypeOf<OperatorNode<'+', 'add', MathNode[]>>()
).toMatchTypeOf<OperatorNode<'add', BaseNode[]>>()

assert.deepStrictEqual(
math.parse('sqrt(3^2 + 4^2)').transform(myTransform1).toString(),
Expand All @@ -1110,7 +1107,7 @@ Transform examples
.parse('sqrt(3^2 + 4^2)')
.transform(myTransform1)
.transform(myTransform2)
).toMatchTypeOf<OperatorNode<'-', 'subtract', MathNode[]>>()
).toMatchTypeOf<OperatorNode<'subtract', BaseNode[]>>()

assert.deepStrictEqual(
math
Expand Down Expand Up @@ -1845,17 +1842,15 @@ Function round examples
new math.ConstantNode(3),
new math.SymbolNode('x'),
])
).toMatchTypeOf<OperatorNode<'/', 'divide', (ConstantNode | SymbolNode)[]>>()
).toMatchTypeOf<OperatorNode<'divide', (ConstantNode | SymbolNode)[]>>()

expectTypeOf(new math.ConstantNode(1).clone()).toMatchTypeOf<ConstantNode>()
expectTypeOf(
new math.OperatorNode('*', 'multiply', [
new math.ConstantNode(3),
new math.SymbolNode('x'),
]).clone()
).toMatchTypeOf<
OperatorNode<'*', 'multiply', (ConstantNode | SymbolNode)[]>
>()
).toMatchTypeOf<OperatorNode<'multiply', (ConstantNode | SymbolNode)[]>>()

expectTypeOf(
new math.ConstantNode(1).cloneDeep()
Expand All @@ -1865,9 +1860,7 @@ Function round examples
new math.ConstantNode(3),
new math.SymbolNode('x'),
]).cloneDeep()
).toMatchTypeOf<
OperatorNode<'+', 'unaryPlus', (ConstantNode | SymbolNode)[]>
>()
).toMatchTypeOf<OperatorNode<'unaryPlus', (ConstantNode | SymbolNode)[]>>()

expectTypeOf(
math.clone(new math.ConstantNode(1))
Expand Down Expand Up @@ -2225,9 +2218,7 @@ Factory Test
expectTypeOf(x).toMatchTypeOf<ObjectNode>()
}
if (math.isOperatorNode(x)) {
expectTypeOf(x).toMatchTypeOf<
OperatorNode<OperatorNodeOp, OperatorNodeFn, MathNode[]>
>()
expectTypeOf(x).toMatchTypeOf<OperatorNode>()
}
if (math.isParenthesisNode(x)) {
expectTypeOf(x).toMatchTypeOf<ParenthesisNode>()
Expand Down Expand Up @@ -2308,36 +2299,3 @@ Random examples
MathJsChain<number[]>
>()
}

/*
MathNode examples
*/
{
class CustomNode extends Node {
a: MathNode
constructor(a: MathNode) {
super()
this.a = a
}
}

// Basic node
const instance1 = new Node()

// Built-in subclass of Node
const instance2 = new ConstantNode(2)

// Custom subclass of node
const instance3 = new CustomNode(new ConstantNode(2))

expectTypeOf(instance1).toMatchTypeOf<MathNode>()
expectTypeOf(instance1).toMatchTypeOf<MathNodeCommon>()
Comment on lines -2333 to -2334
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}


expectTypeOf(instance2).toMatchTypeOf<MathNode>()
expectTypeOf(instance2).toMatchTypeOf<MathNodeCommon>()
expectTypeOf(instance2).toMatchTypeOf<ConstantNode>()

expectTypeOf(instance3).toMatchTypeOf<MathNode>()
expectTypeOf(instance3).toMatchTypeOf<MathNodeCommon>()
expectTypeOf(instance3).toMatchTypeOf<CustomNode>()
}
11 changes: 4 additions & 7 deletions types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,13 +319,12 @@ declare namespace math {
type OperatorNodeFn = keyof OperatorNodeMap

interface OperatorNode<
TOp extends OperatorNodeMap[TFn] = never,
TFn extends OperatorNodeFn = never,
TFn extends OperatorNodeFn = OperatorNodeFn,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

image

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.

Copy link
Author

@ChristopherChudzicki ChristopherChudzicki Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Screen Shot 2022-11-02 at 9 26 36 PM

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

TArgs extends BaseNode[] = BaseNode[]
> extends BaseNode {
type: 'OperatorNode'
isOperatorNode: true
op: TOp
op: OperatorNodeMap[TFn]
fn: TFn
args: TArgs
implicit: boolean
Expand All @@ -343,7 +342,7 @@ declare namespace math {
fn: TFn,
args: TArgs,
implicit?: boolean
): OperatorNode<TOp, TFn, TArgs>
): OperatorNode<TFn, TArgs>
}
interface ParenthesisNode<TContent extends BaseNode = BaseNode>
extends BaseNode {
Expand Down Expand Up @@ -3185,9 +3184,7 @@ declare namespace math {

isObjectNode(x: unknown): x is ObjectNode

isOperatorNode(
x: unknown
): x is OperatorNode<OperatorNodeOp, OperatorNodeFn>
isOperatorNode(x: unknown): x is OperatorNode<OperatorNodeFn>

isParenthesisNode(x: unknown): x is ParenthesisNode

Expand Down