Refactor ASTTransformer #1497

nojaf · 2021-02-27T18:43:56Z

Refactored ASTTransformer:

optimized for tail recursion, heavily inspired by https://www.gresearch.co.uk/article/advanced-recursion-techniques-in-f/
Threw out ASTNodes that we never use.
Threw out properties and FsAstNode as they are not used.

Please review if I missed something.

nojaf · 2021-02-27T19:34:49Z

PR

Ubuntu

Method	Mean	Error	StdDev	Rank	Gen 0	Gen 1	Gen 2	Allocated
Format	2.513 s	0.0383 s	0.0358 s	1	123000.0000	44000.0000	5000.0000	1.76 GB

Windows

Method	Mean	Error	StdDev	Rank	Gen 0	Gen 1	Gen 2	Allocated
Format	3.071 s	0.0488 s	0.0433 s	1	102000.0000	32000.0000	3000.0000	1.76 GB

Mac

Method	Mean	Error	StdDev	Rank	Gen 0	Gen 1	Gen 2	Allocated
Format	2.765 s	0.0412 s	0.0385 s	1	300000.0000	121000.0000	5000.0000	1.76 GB

Master

Ubuntu

Method	Mean	Error	StdDev	Rank	Gen 0	Gen 1	Gen 2	Allocated
Format	3.228 s	0.0469 s	0.0416 s	1	77000.0000	21000.0000	5000.0000	1.79 GB

Windows

Method	Mean	Error	StdDev	Rank	Gen 0	Gen 1	Gen 2	Allocated
Format	2.676 s	0.0526 s	0.0541 s	1	77000.0000	21000.0000	5000.0000	1.79 GB

Mac

Method	Mean	Error	StdDev	Rank	Gen 0	Gen 1	Gen 2	Allocated
Format	3.039 s	0.0599 s	0.0641 s	1	304000.0000	121000.0000	4000.0000	1.79 GB

Smaug123 · 2021-02-28T19:58:44Z

I'll try and review this tomorrow (Monday) - although I'm only just back from a period of leave, so I might be swamped.

Smaug123 · 2021-03-07T20:02:07Z

I've sat down to look at this a few times, and… it's just too big for me to meaningfully review. How confident are you in the change, and/or is there any particular bit you'd like me to look at?

Smaug123 · 2021-03-01T09:04:55Z

src/Fantomas/Utils.fs

+    let rec sequence<'a, 'ret> (recursions: (('a -> 'ret) -> 'ret) list) (finalContinuation: 'a list -> 'ret) : 'ret =
+        match recursions with
+        | [] -> [] |> finalContinuation
+        | recurse :: recurses -> recurse (fun ret -> sequence recurses (fun rets -> ret :: rets |> finalContinuation))


There's actually a slightly different phrasing of this that I prefer, but I'm still fighting to get the blog post through review :P I don't think there's anything wrong with this version, it just has a very odd order of evaluation.

For reference (though I don't think you need to change what you have), my preferred version is:

let rec sequence (results : (('result -> 'ret) -> 'ret) list) (andThen : 'result list -> 'ret) : 'ret = match results with | [] -> andThen [] | andThenInner :: andThenInners -> fun (results : 'result list) -> fun (result : 'result) -> result :: results |> andThen |> andThenInner |> sequence andThenInner

Smaug123 · 2021-03-07T20:57:58Z

One thing that would help a lot, I think, is if you broke this out into two different reviews: one for "Threw out ASTNodes that we never use" and "Threw out properties and FsAstNode as they are not used.", and one for the CPS transform. I understand how painful this will be to extract, though :(

nojaf · 2021-03-08T07:14:11Z

Hey Patrick, good feedback. This is indeed a too large PR that does multiple things.
I'd mostly like a review of the CPS stuff. I'll annotate one function and ask some specific questions.
The answers will apply to all of them.
I'm actually very confident about the entire PR.

nojaf · 2021-03-08T07:16:53Z

src/Fantomas/AstTransformer.fs

+                          yield! nodes ]
+                        |> finalContinuation)
+            | SynExpr.Quote (operator, _, quotedSynExpr, _, range) ->
+                let continuations : ((TriviaNodeAssigner list -> TriviaNodeAssigner list) -> TriviaNodeAssigner list) list =


Does it makes sense to use the sequence helper function when the list is fixed and small (2 elements in this case)?

I think there's nothing wrong with it, although the situation comes up so infrequently that I would certainly not call myself an expert on style. If you wanted to do it by hand, maybe it would be a bit easier to read - the sequence operator is a bit arcane, and continuation-passing style is already arcane, so maybe being explicit is better than using fancy machinery here. I think it's up to you, really.

nojaf · 2021-03-08T07:18:17Z

src/Fantomas/AstTransformer.fs

+                    [ visit operator; visit quotedSynExpr ]
+
+                let finalContinuation (nodes: TriviaNodeAssigner list list) : TriviaNodeAssigner list =
+                    [ mkNode SynExpr_Quote range


I used List.collect a lot of times, any problems or improvements there?

I can't think of an alternative.

nojaf · 2021-03-08T07:19:13Z

src/Fantomas/AstTransformer.fs

+                    exprs |> List.map visit
+
+                let finalContinuation (nodes: TriviaNodeAssigner list list) : TriviaNodeAssigner list =
+                    [ mkNode SynExpr_Tuple range


Would mkNode SynExpr_Tuple range :: (List.collect id nodes) be better? I seem to switch styles here and there.

Pff, I really don't mind. Personally I prefer to be explicit if I use the yield keyword - I'm not a fan of implicit yields, because if you omit the yields then you have the chance in principle to get confused between [if true then yield 3 else yield 4] and [yield (if true then 3 else 4)].

I suspect mkNode SynExpr_Tuple range :: (List.collect id nodes) might be more efficient - it would certainly rely less on the compiler's magic ability to optimise. It's not obvious to me that the computation expression avoids copying whatever is yield!'ed.

Smaug123

I certainly haven't read the whole thing, but I think it looks basically good. I have no fundamental comments other than "is there anywhere you could be more generic?".

Smaug123 · 2021-03-12T20:58:35Z

src/Fantomas/AstTransformer.fs

+                          yield! nodes ]
+                        |> finalContinuation)
+            | SynExpr.Quote (operator, _, quotedSynExpr, _, range) ->
+                let continuations : ((TriviaNodeAssigner list -> TriviaNodeAssigner list) -> TriviaNodeAssigner list) list =


I think there's nothing wrong with it, although the situation comes up so infrequently that I would certainly not call myself an expert on style. If you wanted to do it by hand, maybe it would be a bit easier to read - the sequence operator is a bit arcane, and continuation-passing style is already arcane, so maybe being explicit is better than using fancy machinery here. I think it's up to you, really.

Smaug123 · 2021-03-12T21:01:02Z

src/Fantomas/AstTransformer.fs

+    and visitSynExpr (synExpr: SynExpr) : TriviaNodeAssigner list =
+        let rec visit
+            (synExpr: SynExpr)
+            (finalContinuation: TriviaNodeAssigner list -> TriviaNodeAssigner list)


By the way, I find these things easier to write correctly if I stay generic as long as possible. It makes it harder when you can't put generic type parameters on, like in an inner function, but if finalContinuation returned a 'ret then the types constrain you further and it's harder to get it wrong. I'm not sure whether that's possible here.

Smaug123 · 2021-03-12T21:01:44Z

src/Fantomas/AstTransformer.fs

+                    [ visit operator; visit quotedSynExpr ]
+
+                let finalContinuation (nodes: TriviaNodeAssigner list list) : TriviaNodeAssigner list =
+                    [ mkNode SynExpr_Quote range


I can't think of an alternative.

Smaug123 · 2021-03-12T21:04:26Z

src/Fantomas/AstTransformer.fs

+                    exprs |> List.map visit
+
+                let finalContinuation (nodes: TriviaNodeAssigner list list) : TriviaNodeAssigner list =
+                    [ mkNode SynExpr_Tuple range


Pff, I really don't mind. Personally I prefer to be explicit if I use the yield keyword - I'm not a fan of implicit yields, because if you omit the yields then you have the chance in principle to get confused between [if true then yield 3 else yield 4] and [yield (if true then 3 else 4)].

I suspect mkNode SynExpr_Tuple range :: (List.collect id nodes) might be more efficient - it would certainly rely less on the compiler's magic ability to optimise. It's not obvious to me that the computation expression avoids copying whatever is yield!'ed.

Smaug123 · 2021-03-12T21:05:53Z

src/Fantomas/AstTransformer.fs

+            | SynExpr.New (_, typeName, expr, range) ->
+                visit
+                    expr
+                    (fun nodes ->


I've got a post coming out soon where I talk about continuation-passing style all over again, and in the course of that, I ended up much preferring the following layout just on aesthetic grounds:

(fun nodes -> blah ) |> visit expr

Saves a layer of indentation, although it is quite weird from an imperative-programming point of view.

nojaf · 2021-03-13T09:04:47Z

Thanks for the review @Smaug123!

nojaf requested review from jindraivanek and Smaug123 February 27, 2021 18:43

nojaf changed the title ~~Refactor ast viewer~~ Refactor ASTTransformer Feb 27, 2021

nojaf force-pushed the refactor-ast-viewer branch 3 times, most recently from 949049c to 5437946 Compare March 6, 2021 14:02

Smaug123 reviewed Mar 7, 2021

View reviewed changes

nojaf commented Mar 8, 2021

View reviewed changes

nojaf force-pushed the refactor-ast-viewer branch 2 times, most recently from 004e886 to 05c56c7 Compare March 12, 2021 12:57

Smaug123 approved these changes Mar 12, 2021

View reviewed changes

nojaf added 8 commits March 13, 2021 09:29

Return Node list from AstTransformer.fs.

5988cb9

Add Continuation.sequence to AstTransformer.fs.

69939af

Add Continuation.sequence to AstTransformer.fs, part 2

88ca56e

Remove unused properties and filter out unused nodes in AstTransformer.

7ec9aa8

Make TriviaNodeAssigner directly in AstTransformer.fs.

3e6b6b5

Remove unused code

37db876

Remove SynExpr.Const and SynPat.Const from AstTransformer.fs.

cc2c35e

Add more explicit yield in lists.

53ae465

nojaf force-pushed the refactor-ast-viewer branch from 784b0dc to 53ae465 Compare March 13, 2021 08:54

nojaf merged commit 75f0dfc into fsprojects:master Mar 13, 2021

nojaf deleted the refactor-ast-viewer branch March 13, 2021 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ASTTransformer #1497

Refactor ASTTransformer #1497

nojaf commented Feb 27, 2021

nojaf commented Feb 27, 2021

Smaug123 commented Feb 28, 2021

Smaug123 commented Mar 7, 2021

Smaug123 Mar 1, 2021

Smaug123 commented Mar 7, 2021

nojaf commented Mar 8, 2021 •

edited

Loading

nojaf Mar 8, 2021

Smaug123 Mar 12, 2021

nojaf Mar 8, 2021

Smaug123 Mar 12, 2021

nojaf Mar 8, 2021

Smaug123 Mar 12, 2021

Smaug123 left a comment

Smaug123 Mar 12, 2021

Smaug123 Mar 12, 2021

Smaug123 Mar 12, 2021

Smaug123 Mar 12, 2021

Smaug123 Mar 12, 2021

nojaf commented Mar 13, 2021

Refactor ASTTransformer #1497

Refactor ASTTransformer #1497

Conversation

nojaf commented Feb 27, 2021

nojaf commented Feb 27, 2021

PR

Ubuntu

Windows

Mac

Master

Ubuntu

Windows

Mac

Smaug123 commented Feb 28, 2021

Smaug123 commented Mar 7, 2021

Choose a reason for hiding this comment

Smaug123 commented Mar 7, 2021

nojaf commented Mar 8, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Smaug123 left a 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

nojaf commented Mar 13, 2021

nojaf commented Mar 8, 2021 •

edited

Loading