Convert ParseNode to struct #1534

marcianx · 2018-07-31T03:04:35Z

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 · 2018-07-31T03:07:27Z

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 · 2018-07-31T03:53:18Z

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 · 2018-07-31T08:39:29Z

src/ParseNode.js

-    ParseNode<"sqrt"> |
-    ParseNode<"underline"> |
-    ParseNode<"xArrow">;
+export type AnyParseNode = $Values<ParseNodeTypes>;


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

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.

Nice find on the type spread operator!

Regarding the ParseNode definition above.

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.

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.

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.

@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 · 2018-07-31T16:41:00Z

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 · 2018-07-31T17:38:20Z

@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 · 2018-07-31T17:44:07Z

@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 · 2018-07-31T22:49:24Z

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

edemaine · 2018-07-31T22:56:05Z

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.

Passes test:jest, but fails test:flow.

marcianx · 2018-08-01T02:16:45Z

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 · 2018-08-01T02:20:17Z

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.

…nment/array.js.

marcianx · 2018-08-02T03:38:18Z

This was towards #1492.

kevinbarabash · 2018-08-03T14:46:34Z

@ylemkimon thanks for reviewing and approving.

marcianx requested review from ylemkimon and kevinbarabash July 31, 2018 03:04

marcianx force-pushed the parsenode-start branch from d23be88 to 7fdc103 Compare July 31, 2018 03:18

ylemkimon reviewed Jul 31, 2018

View reviewed changes

ronkok mentioned this pull request Jul 31, 2018

Improve Unicode docs #1533

Merged

ylemkimon reviewed Jul 31, 2018

View reviewed changes

marcianx added 4 commits July 31, 2018 21:39

Define the nested version of ParseNodes structs explicitly.

6869648

Passes test:jest, but fails test:flow.

Fix additional type errors reported by flow.

89485ae

Migrate rebased code from master.

9e5a9ff

Rename ParseNode.js to parseNode.js.

0bf2606

marcianx force-pushed the parsenode-start branch from 7fdc103 to 49da6e0 Compare August 1, 2018 02:15

Update defineEnvironment output type to fix the flow errors in enviro…

a67dcd0

…nment/array.js.

marcianx force-pushed the parsenode-start branch from 49da6e0 to a67dcd0 Compare August 1, 2018 02:56

ylemkimon approved these changes Aug 1, 2018

View reviewed changes

ylemkimon merged commit 0ac4b6e into KaTeX:master Aug 1, 2018

marcianx deleted the parsenode-start branch August 1, 2018 10:46

ylemkimon mentioned this pull request Aug 18, 2018

Release v0.10.0-rc #1634

Merged

1 task

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade katex from 0.9.0 to 0.11.1 saurabharch/Rocket.Chat#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert ParseNode to struct #1534

Convert ParseNode to struct #1534

marcianx commented Jul 31, 2018

codecov bot commented Jul 31, 2018 •

edited

Loading

kevinbarabash commented Jul 31, 2018

ylemkimon Jul 31, 2018 •

edited

Loading

ylemkimon Jul 31, 2018

marcianx Jul 31, 2018

marcianx Jul 31, 2018

ylemkimon commented Jul 31, 2018 •

edited

Loading

ylemkimon left a comment

marcianx commented Jul 31, 2018

ylemkimon commented Jul 31, 2018

marcianx commented Jul 31, 2018

edemaine commented Jul 31, 2018 •

edited

Loading

marcianx commented Aug 1, 2018 •

edited

Loading

kevinbarabash commented Aug 1, 2018

marcianx commented Aug 2, 2018

kevinbarabash commented Aug 3, 2018

Convert ParseNode to struct #1534

Convert ParseNode to struct #1534

Conversation

marcianx commented Jul 31, 2018

codecov bot commented Jul 31, 2018 • edited Loading

Codecov Report

kevinbarabash commented Jul 31, 2018

ylemkimon Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

ylemkimon Jul 31, 2018

Choose a reason for hiding this comment

marcianx Jul 31, 2018

Choose a reason for hiding this comment

marcianx Jul 31, 2018

Choose a reason for hiding this comment

ylemkimon commented Jul 31, 2018 • edited Loading

ylemkimon left a comment

Choose a reason for hiding this comment

marcianx commented Jul 31, 2018

ylemkimon commented Jul 31, 2018

marcianx commented Jul 31, 2018

edemaine commented Jul 31, 2018 • edited Loading

marcianx commented Aug 1, 2018 • edited Loading

kevinbarabash commented Aug 1, 2018

marcianx commented Aug 2, 2018

kevinbarabash commented Aug 3, 2018

codecov bot commented Jul 31, 2018 •

edited

Loading

ylemkimon Jul 31, 2018 •

edited

Loading

ylemkimon commented Jul 31, 2018 •

edited

Loading

edemaine commented Jul 31, 2018 •

edited

Loading

marcianx commented Aug 1, 2018 •

edited

Loading