Convert ParseNode to struct

[marcianx]

• This converts ParseNode to flat objects per Port Parser.js to @flow #892 and fixes additional type errors discovered by flow.
• Pro: This works better with the flow type system (I think the use of NodeValue<TYPE> used to constrain flow's reasoning sometimes).
• Con: It is less obvious/easily-greppable to find where ParseNodes are created in the source code.

NOTE: This PR has a higher chance of conflicts due to the unavoidable extensiveness of this change. So I'd like to get this in sooner rather than later. This does NOT flatten the objects yet! This reduces the area of merge conflict; and also, flattening is a rabbit hole since sometimes a handful of ParseNode types need to be converted together.

[codecov]

Codecov Report

Merging #1534 into master will decrease coverage by 0.02%.
The diff coverage is 90.74%.

@@            Coverage Diff             @@
##           master    #1534      +/-   ##
==========================================
- Coverage   84.14%   84.11%   -0.03%     
==========================================
  Files          79       79              
  Lines        4565     4558       -7     
  Branches      803      803              
==========================================
- Hits         3841     3834       -7     
  Misses        629      629              
  Partials       95       95

Impacted Files	Coverage Δ
src/functions/supsub.js	98% <ø> (ø)	⬆️
src/ParseError.js	100% <ø> (ø)	⬆️
src/stretchy.js	79.54% <ø> (ø)	⬆️
src/Settings.js	73.52% <ø> (ø)	⬆️
src/functions/symbolsOrd.js	100% <ø> (ø)	⬆️
src/functions/symbolsOp.js	100% <ø> (ø)	⬆️
src/buildMathML.js	98.73% <ø> (ø)	⬆️
src/defineEnvironment.js	100% <ø> (ø)	⬆️
src/buildTree.js	94.11% <ø> (ø)	⬆️
src/utils.js	66.66% <ø> (ø)	⬆️
... and 39 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2202aa7...a67dcd0. Read the comment docs.

[kevinbarabash]

Cool. This is going to take some time to review. I will attempt to do so tomorrow evening. Also, we can put other PRs that make code changes on hold until this get's merged.

[ylemkimon]

Is it possible to deduplicate mode and loc using intersection types(&) object spread? (facebook/flow#2626)

[ylemkimon]

diff --git a/src/ParseNode.js b/src/ParseNode.js
index 26b2bbe..7f8cc10 100644
--- a/src/ParseNode.js
+++ b/src/ParseNode.js
@@ -7,7 +7,14 @@ import type {Token} from "./Token";
 import type {Measurement} from "./units";

 export type NodeType = $Keys<ParseNodeTypes>;
-export type ParseNode<TYPE: NodeType> = $ElementType<ParseNodeTypes, TYPE>;
+type ParseNodeBase ={|
+    mode: Mode,
+    loc?: ?SourceLocation,
+|}
+export type ParseNode<TYPE: NodeType> = {|
+    ...$ElementType<ParseNodeTypes, TYPE>,
+    ...ParseNodeBase
+|};

 export type LeftRightDelimType = {|
     type: "leftright",
@@ -31,20 +38,19 @@ export type SymbolParseNode =
     ParseNode<"textord">;

 // Union of all possible `ParseNode<>` types.
-export type AnyParseNode = $Values<ParseNodeTypes>;
+export type AnyParseNode = {|
+    ...$Values<ParseNodeTypes>,
+    ...ParseNodeBase
+|};

seems to work, not sure it's correct.

[marcianx]

Nice find on the type spread operator!

Regarding the ParseNode definition above.

1. I would really like ParseNode to be covariant with its TYPE parameter. This means not looking up the value type within its definition. I think that might have been the cause of a lot of "___ is not compatible with ___" style errors which are completely unhelpful. Flow seems to be able to deal with an explicit union of explicitly-defined structs quite easily. An alternative is to replicate ...ParseNodeBase in every definition, which I can do.

2. As a minor nit: if we not make ParseNodeBase a strict object type, then every ParseNode<TYPE>: ParseNodeBase as well. Not sure if we would every use that fact, though.

3. The AnyParseNode as defined above is not technically a union of all the structs and the behavior of Flow's type checking (especially when it gives an error) might get very confusing in such cases. E.g. Flow will allow you to do the following with a union that type it won't with the type defined above.

   type AllTypes = {| type: "foo", foo: number |} |
                   {| type: "bar", bar: string |};
   function(val: AllTypes)  {
     let len;
     if (val.type === "foo") {
       len = val.foo + 1;
     } else {
       len = val.bar.length;
     }
   }

   Playground

   I was thus hoping to eventually change uses of checkNodeType to be simple type checks.

[marcianx]

@ylemkimon Actually, using

type ParseNodeBase = {
    mode: Mode,
    loc?: ?SourceLocation,
};

type ParseNodeTypes = {
    "array": {|
        type: "array",
        ...ParseNodeBase,
        value: ArrayEnvNodeData,
    |},
    ... // and same on the rest

seems to freeze the flow type checker very consistently (it's stuck at 24% progress at 100% CPU utilization). So let's skip that for now...

[ylemkimon]

As we don't have ParseNode class anymore, we should consider moving type declarations to types.js and functions to utils.js.

[ylemkimon]

The code LGTM.

[marcianx]

@ylemkimon IMO, ParseNode is central enough that it would really help to keep everything related to it -- it's functions and "methods" (free functions in this case) -- together for the sake of disciverability and learnability. Also, as it's immense, it may overshadow other standalone types in types.js, making them less discoverable.

But I'm open to any feedback you may have on any advantages of splitting it up.

[ylemkimon]

@marcianx I was thinking the PascalCase file name may give an impression that it default-exports a class named ParseNode, but I cannot think of better alternative.

[marcianx]

That's a valid concern. How about parsenode.js then?

[edemaine]

I think parseNode.js would be more consistent.

P.S. Sorry, I merged #1529 before seeing @kevinbarabash's suggestion to hold off on code changes.

[marcianx]

SGTM. Renamed to parseNode.js and also rebased to head, migrating the relevant code.
Bizarre thing: flow is strange ... renaming ParseNode.js to parseNode.js started to ignore one of my explicit $FlowFixMes in environment/array.js. I had to reorder code around to make it accept the code again!
Edit: Actually, I resolved the $FlowFixMes by updating the defineEnvironment type.

[kevinbarabash]

Sorry I had a social function this evening that I forgot about. @ylemkimon once the tests pass, feel free to approve the review if you feel comfortable.

[marcianx]

This was towards #1492.

[kevinbarabash]

@ylemkimon thanks for reviewing and approving.