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.

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

"Functored" AST #98

Open
Ericson2314 opened this issue May 19, 2022 · 18 comments · May be fixed by #99
Open

"Functored" AST #98

Ericson2314 opened this issue May 19, 2022 · 18 comments · May be fixed by #99

Comments

@Ericson2314
Copy link

Instead of doing e.g.

data Block = ...Block...

what if we did

data BlockF block = ...block...
   deriving Functor
newtype Block = MkBlock (BlockF Block)

This trick allows ASTs to be extensible, among other benefits.

I generally like this approach, but I had a specific problem in mind that would benefit which I would like to share. https://github.com/obsidiansystems/dombuilder-pandoc/blob/master/src/Reflex/Dom/Builder/Pandoc.hs is some code to translate the Pandoc AST to reflex-dom "dom builder action" order to use within a website built with reflex dom.

I would like to have my own custom handling of pandoc the pandoc AST --- e.g. parsing relative URLs in links into a Route AST --- without having to copy and paste that function. But if we do the above "functored AST" approach, I can replace

block :: DomBuilder t m => Block -> m ()

with

blockF :: DomBuilder t m => BlockF a -> m ()

or

blockF :: DomBuilder t m => BlockF (m ()) -> m ()

These can be thought of as specialized versions of traverse_ or sequence_, and they are very nice to work with!

@jgm
Copy link
Owner

jgm commented May 19, 2022

I'd like to understand this better. Maybe you could give a small example, with a very abbreviated version of the pandoc types (e.g. with just Str, Emph, Para, and BlockQuote).

As for the application, it's hard for me to see what you're trying to accomplish just from the types you give. I assume you're familiar with our existing Walk module?

@Ericson2314
Copy link
Author

Yeah sure, I would be happy to make the example.

Ericson2314 added a commit to obsidiansystems/pandoc-types that referenced this issue May 19, 2022
Fixes jgm#98

See the issue for details of why this is useful
@Ericson2314 Ericson2314 linked a pull request May 19, 2022 that will close this issue
@Ericson2314
Copy link
Author

OK see #99

I frequent concern when this sort of thing comes up is breaking downstream code. If that is a concern for you, I am happy to rename the variants and create a bunch of compatibility patterns

@Ericson2314
Copy link
Author

Ericson2314 commented May 20, 2022

obsidiansystems/dombuilder-pandoc#1 shows how I would change the DOM builder to use this. The "prime" functions are not recursive, and thus can be reused to render something like

data MyInline 
  = Regular (InlineF MyInline) -- fixed typo was `Inline` before
  | ReatliveLink Attr [MyInline] MyRoute Text

which would accurately represent a document with checked relative URLs.

@jgm
Copy link
Owner

jgm commented May 20, 2022

Should that be Regular (InlineF MyInline)?

One question is how many changes would be needed to the pandoc code base if pandoc-types were to change in this way.
A lot, it seems to me -- but maybe pattern synonyms could be defined that would allow current code to compile?

@Ericson2314
Copy link
Author

Ericson2314 commented May 20, 2022

Should that be Regular (InlineF MyInline)?

Yes, thanks!

One question is how many changes would be needed to the pandoc code base if pandoc-types were to change in this way.
A lot, it seems to me -- but maybe pattern synonyms could be defined that would allow current code to compile?

Indeed. I wanted to wait to hear your thoughts first, but I am happy to write those pattern synonyms and then make the (far more minimal) changes in pandoc too.

@madeline-os
Copy link

Here is a report from another project that saw great utility out of structuring their AST in this way: http://newartisans.com/2018/04/win-for-recursion-schemes/ I thought it would be useful to note here.

@Ericson2314
Copy link
Author

Thanks @madeline-os, I was completely blanking on where the existing literature was on this technique.

@tarleb
Copy link
Contributor

tarleb commented May 20, 2022

The one downside I can think off is that the change would make pandoc less accessible to newcomers. However, I believe that this is not as big a problem anymore. Pandoc's code base reflects more and more of the complexity that's inherent to the problems it solves, and this would be just another step in that direction.

I note that the "functorization" could potentially be used to solve jgm/pandoc#684.

@Ericson2314
Copy link
Author

OK I created pattern synonyms such that that the PR should be almost breakage free. (Exception is cannot associate data constructors to type synonyms for aliases, which are used for tables.)

@jgm
Copy link
Owner

jgm commented May 22, 2022

Have you tried compiling pandoc against the altered pandoc-types, to see where breakage might happen?

@Ericson2314
Copy link
Author

@jgm trying that now.

@Ericson2314
Copy link
Author

pandoc/pandoc-lua-marshal#5 jgm/pandoc#8084 are the changes. As you can see are quite minimal and just making some imports uglier because of the GHC deficiency.

@jgm
Copy link
Owner

jgm commented May 23, 2022

Wow, that's quite impressive. I will try to have a closer look at this before long, but it might be a couple weeks.

@Ericson2314
Copy link
Author

Sounds good. Just let me know about any and all questions once you do :).

Ericson2314 added a commit to obsidiansystems/pandoc-types that referenced this issue Jun 16, 2022
Fixes jgm#98

See the issue for details of why this is useful
Ericson2314 added a commit to obsidiansystems/pandoc-types that referenced this issue Jun 16, 2022
Fixes jgm#98

See the issue for details of why this is useful
@Ericson2314
Copy link
Author

@jgm thanks for giving this another look and repeatedly enabling CI for me.

All 3 PRs:

are now passing, anything more you need from me?

@jgm
Copy link
Owner

jgm commented Jun 24, 2022

Many thanks! I was just waiting for CI to pass before taking a closer look. I'll do that next, but it may be a little while before I can find the time. Don't worry, I haven't forgotten about this.

@Ericson2314
Copy link
Author

Sure, sounds good. Take the time you need and thanks for reviewing. Just want to make sure I've gotten everything done on my end, to avoid any "I am waiting on you, you wait on me" deadlock. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants