Extensions redux

[kivikakk]

Hi there!

I've taken the work in #123 and rejigged it a bit. At GitHub we're currently using a Sundown-based parser/renderer, but it's not super extensible. So, we've decided to roll out CommonMark to replace it.

Here are some of the changes to #123 in this PR:

• Took out the shared object searcher. I don't think library code should searching and loading objects dynamically at runtime. Instead, you as the user register whatever plugins you might have linked in yourself. (Maybe you loaded it dynamically — that's not something for a Markdown library to do, imo.)
• Expanded the extension interface enough such that the two existing plugins (table, strikethrough) can be implemented without any changes to the core code. Implement parsing extensions #123 had table specific code in the core (outside of the shared object), which limited its usefulness. This branch means no change to core code for implementing said. Extensions can register their own node types, their own renderer functions for said node types, etc.
• Fixes the Windows build.
• Adds tests.
• Adds an autolink extension.
• Adds a whitelist extension for the HTML renderer.

This functionality has all been exposed as opt-in in the Ruby gem commonmarker, which is our primary interface.

[jgm]

The object-loading code and the extension-specific changes in core were two things I didn't like about #123, so this seems a step in the right direction. However, I'd be interested in comments from @MathieuDuponchelle and @nwellnhof.

This is a big code revision. It's hard to see the forest for the trees. To ease review, could you please rebase it so that (a) all of the changes are on top of current master, (b) commits are reordered and consolidated into (a smaller number of) logically coherent commits? Part of the difficulty with #123 was that it was hard to survey.

Also note that travis builds are failing due to a warning.

[kivikakk]

Travis was failing due to an older Valgrind on the Linux instances. That's been fixed up.

I'll look into cleaning up the PR. It is rather large; I probably won't touch commits from #123.

[nwellnhof]

We should really try to split this up into smaller chunks. First of all, I'd suggest to tackle block and inline extensions separately. Then, before writing any new code, we should start by discussing the internals.

Discuss changes to core data structures

How to identify extension nodes

The new PR improves upon the previous approach by generating numeric ids for new node types dynamically. Personally, I'd still prefer to have two single node types CMARK_NODE_EXTENSION_BLOCK and CMARK_NODE_EXTENSION_INLINE for all extension nodes and to identify nodes by name (for example using URIs like in XML namespaces). This makes comparisons a little slower but avoids a couple of problems with generating ids dynamically (thread safety, registering the same extension for multiple users, identifying nodes without having access to the generated id).

Render hooks and extension-specific node data

I'd propose to add two new fields to the cmark_node struct for extension nodes. An opaque void * like in the new PR for extension-specific data and a pointer to a struct with the following elements:

struct cmark_ext_node_type {
    // A unique name for the node type, possibly a URI to avoid clashes.
    char *name;
    // Hooks for the HTML renderer.
    void (*html_on_enter)(cmark_node *node, ...);
    void (*html_on_exit)(cmark_node *node, ...);
    // Destructor.
    void (*destroy)(cmark_node *node);
};

The PR adds a separate field for the destructor and stores other callbacks in the extension struct. I think it's cleaner to have separate struct for each node type which can contain callbacks and other static data .

Discuss public extension API

I think the best approach is to look at the core node types and restructure the code in blocks.c and inlines.c as if core nodes were extension nodes themselves. I recently did this for block nodes and realized that we only need a single hook in open_new_blocks. If a new (extension or core) block is found, a new node can be created and three callbacks are needed for further processing:

• check_open which checks whether the block is still open. This callback is invoked from check_open_blocks and replaces the switch statement with a call to a function pointer.
• add_line which adds the end of the current line to the block's content. This callback is invoked from add_text_to_container and replaces the switch statement with a call to a function pointer.
• finalize which finalizes a block. This callback is invoked from finalize and replaces the switch statement with a call to a function pointer.

These callbacks only need access to the current line and offset in cmark_parser. They don't have to be registered globally and can be passed to the parser for each new block.

This isn't too different from the PR, but it seems a bit simpler to me. This also makes it straightforward to support core blocks inside extensions blocks. I'm not sure how this would be handled in the PR. I can publish my changes to the block parser for review if anyone is interested.

I haven't looked at the inline extension API yet. It will probably be a lot more complicated than the API for extension blocks. I only noted that the PR exposes the delimiter struct.

Agree on the design

As I already mentioned in #123, I'm still not happy with some design decisions. See "Discuss changes to core data structures" above where I reiterate my criticism.

Implement

This should really be the last step.

[vmg]

These are all very good suggestions, @nwellnhof! Unfortunately we're short on time as we're rolling out these changes to the website in the upcoming weeks, so we can't afford to sit down and discuss the extensions API for the hundredth time. Making progress on this stuff has been extremely painful so far.

The truth is that CMark is not gaining traction because nobody is using it on production, and nobody is using it on production because it doesn't support any of the features that the large websites (like us at GitHub) need to use it.

So, to speed up things, we'll be publishing soon our own fork of CMark. Hopefully you can pick up from there all the bits and pieces until this CMark implementation becomes more usable, or maybe other websites will just start using our fork -- either way we all win because it means the CMark standard is getting adoption. Thank you and everybody involved for the fantastic work so far.

[MathieuDuponchelle]

woot :) I'll take some time to look at it later today, thanks a bunch :)

[jgm]

I have to admit, adding an extension mechanism hasn't been a big priority for me. The primary aim of this library is to be a reference implementation for the CommonMark spec and a demonstration of algorithms for efficiently parsing the syntax defined by the spec. It was never intended to be the only implementation. (See Markdig and markdown-it for extensible CommonMark implementations in C# and JavaScript.) In principle I like the idea of an extension mechanism, but everything I've seen so far has been too complex for my taste, and I haven't had time to try to come up with anything I like better. (Maybe it's not possible.) Publishing a fork that explores ideas for extensibility is a reasonable thing to do.

That said, I completely agree with the things @nwellnhof has said here, and I'm a bit taken aback by @vmg's response. As a user of GitHub who has often been plagued by Markdown-related problems, I think that GitHub ought to be interested in thinking through the details of the extension architecture and API very carefully before rushing code into production. It seems foolish to give up the chance to discuss this with people who have thought quite a lot about it.

[MathieuDuponchelle]

OK, I'll take the time to formulate a more constructive answer tomorrow, but I'm not so enthusiastic anymore, @vmg saying you're gonna use your own fork is fine and a good thing to do, and I'm happy if my code can be useful in some manner to you folks, but seriously this pull request is a joke, and stating 13 hours after you made it "maybe other websites will start using our fork" is just ruthlessly bad-mannered in my very humble opinion, especially coming from someone representing a company that does have the power to carry out this kind of aggressive fork, which is what that would be, no matter how you may sugar-coat it.

More constructively, the reason why I didn't complain about the perceived lack of reactivity from upstream is that the reduction of scope needed to be done, I would have indeed liked a bit more guidance from @jgm, but we all have our time constraints and I respect that faced with 20+ large commits it's hard to take the time to sit down and go through them.

However the code dump that's proposed there is the exact opposite of "redux", with partial manual reverts, obviously WIP patches such as "Super hack to move table state out of HTML", I don't know how you guys even have the nerve to propose this seriously!

Important the main reason why @jgm was unhappy with my request was a very meaningful design decision that I made, which was to not allow registration of new types of nodes from the outside, because then mixing and matching extensions from different providers that wouldn't know about each other, and thus couldn't specify correctly containing rules, was not possible.

That use case might have been a bit too extreme, and imposing severe restrictions on the design, as a side note that's for this use case that I half-implemented the plugin loading mechanism.

As I don't even need this for my use case, as I want to expose a specific set of extensions, over which I have complete control, much like I suppose you guys at github do, I decided some time ago I'd undertake a rework, and provide an actual redux version which upstream would be happy with.

You guys should have come discuss on #123 to figure out how to approach this task which you visibly agree needed doing, I'd have been more than happy to help and we would have ended up with something more mergeable than at the beginning, in its state that branch is, in my opinion, way less mergeable than the original one, especially as it entirely contains it then partly deconstructs it ...

[MathieuDuponchelle]

Please "super-squash" what I actually did in a commit authored by me, and split out your own additions in a separate one, that would be way more polite

[kivikakk]

@MathieuDuponchelle: can you suggest a solution you'd be happy with? I'm not really interested in authorship itself as a concept, so if retagging the commit as yours would be sufficient, I'd most like to do that.

[MathieuDuponchelle]

@kivikakk , see above, thanks, you don't get to decide whether your "lack of interest for authorship as a concept" extends to other people's work as well ;)

[MathieuDuponchelle]

Same for the strikethrough extension, and the table extension, you seem to have reworked the latter but it would have been way more practical to actually see what your changes were if you had committed them separately, in any case I do have authorship on the original work, you didn't even mention it in the commit messages there ...

[kivikakk]

@MathieuDuponchelle: I've split the commits up accordingly. It's not possible to separate the table and strikethrough extensions into separate commits because they're in entirely different files to begin with; the diff is simply minus core-extensions.c, plus table.c.

I hope this helps.

[MathieuDuponchelle]

@kivikakk thanks.

I'd really like to know what your intentions are with this fork in the long term?

[jeroen]

Thanks everyone, this is cool. You guys mention that this is already in use to render github comments. Is the syntax highlighting in github flavored markdown done as a preprocessing step or so?

[vmg]

@MathieuDuponchelle: We don't have a specific goal with the fork. It's the code we use to render Markdown in GitHub and we wanted to OSS it so users can refer to it.

Of course, we would love it if @jgm were interested on reviewing the extensions we've implemented and consider merging those upstream! But he's already busy enough as it is, so we're happy to just start rendering all the documents we currently have as CommonMark + our unofficial extensions.

@jeroenooms: It's actually a post-processing step on the generated HTML, just like all the other small GitHub-specific features, like autolinking commits and user names. Check out http://github.com/jch/html-pipeline for an example of how to do this yourself.

[jeroen]

OK. One final question: if no extensions are enabled in the parser, the output of this fork will be exactly identical to the original jgm/cmark code, correct? If we default to not enabling extensions, I should be able to use this fork as a drop-in replacement without any side effects?

[vmg]

@jeroenooms: Yes! That's been the intention since day one and we've been trying really hard to stick to that. Just this morning we've upstreamed the only slight modification we had performed on the CommonMark spec (a small change on the way links are parsed) and @jgm has merged it. So we should be 100% compatible.

By the way, I was going to followup by email, but yeah... There's the fork. Hopefully it'll be useful for you. :)

[jgm]

+++ Vicent Martí [Dec 01 16 02:53 ]:

***@***.***: It's actually a post-processing step on the generated HTML, just like all the other small GitHub-specific features, like autolinking commits and user names. Check out [4]http://github.com/jch/html-pipeline for an example of how to do this yourself.

It's pretty easy to use cmark's iterator interface to do this kind of thing prior to rendering, instead of postprocessing. Here's an example of doing this in lcmark, my lua cmark wrapper: https://github.com/jgm/lcmark/blob/master/filters/highlight.lua I would guess this approach would be more efficient than postprocessing HTML, because you're crawling an efficient representation of the document's nodes, and you don't need to parse HTML.

[jeroen]

OK this is all very cool stuff. I am going to release R bindings soon. Many R users have been waiting for a complete and portable markdown renderer which does not depend on haskell ;)

I do hope that the split will be temporary and we will eventually converge to combine efforts into a single "cmark" implementation.

[jeroen]

Another question: in an embedded application, should cmark_register_plugin(core_extensions_registration); only be called once or each time a new parser is created?

[jeroen]

@vmg I am getting the following compiler warnings on Windows 64 bit:

c:/Rtools/mingw_64/bin/gcc  -I"C:/PROGRA~1/R/R-33~1.2/include" -DNDEBUG -Icmark -I.    -I"d:/Compiler/gcc-4.9.3/local330/include"     -O2 -Wall  -std=gnu99 -mtune=core2 -c cmark/iterator.c -o cmark/iterator.o
c:/Rtools/mingw_64/bin/gcc  -I"C:/PROGRA~1/R/R-33~1.2/include" -DNDEBUG -Icmark -I.    -I"d:/Compiler/gcc-4.9.3/local330/include"     -O2 -Wall  -std=gnu99 -mtune=core2 -c cmark/blocks.c -o cmark/blocks.o
cmark/blocks.c: In function 'cmark_manage_extensions_special_characters':
cmark/blocks.c:395:41: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
       unsigned char c = (unsigned char) (unsigned long) tmp_char->data;
                                         ^
c:/Rtools/mingw_64/bin/gcc  -I"C:/PROGRA~1/R/R-33~1.2/include" -DNDEBUG -Icmark -I.    -I"d:/Compiler/gcc-4.9.3/local330/include"     -O2 -Wall  -std=gnu99 -mtune=core2 -c cmark/inlines.c -o cmark/inlines.o
cmark/inlines.c: In function 'add_extensions_openers_bottom':
cmark/inlines.c:511:41: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
       unsigned char c = (unsigned char) (unsigned long) tmp_char->data;
                                         ^
cmark/inlines.c: In function 'get_extension_for_special_char':
cmark/inlines.c:525:45: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
       unsigned char tmp_c = (unsigned char) (unsigned long) tmp_char->data;
                                             ^

@MathieuDuponchelle what is the point of these double casts?

unsigned char c = (unsigned char) (unsigned long) tmp_char->data;

Windows doesn't like that. Can I just take out the (unsigned long) part?

[MathieuDuponchelle]

@jeroenooms , I don't know why they even kept this API, it's absolutely useless given there's no plugin loading mechanism anymore. What you want to do is instantiate extensions, and attach them on a per-parser basis. That way you can have multiple parsers with different extensions enabled in the same program. Here is an example of how that worked with a previous version of the extensions API:

https://github.com/hotdoc/hotdoc/blob/master/hotdoc/parsers/cmark_module.c#L462 and https://github.com/hotdoc/hotdoc/blob/master/hotdoc/parsers/cmark_include_extension.c

[jeroen]

@MathieuDuponchelle hmmm OK. Well I need to get rid of these warnings to publish my bindings:

Found the following significant warnings:
  cmark/blocks.c:395:41: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  cmark/inlines.c:511:41: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  cmark/inlines.c:525:45: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]

So it looks like on Win64, the pointer is not the same size as unsigned long. What would be the best way to fix this warning? Thanks for your amazing work on this btw!

[MathieuDuponchelle]

Hm, the problem is similar to this: https://developer.gnome.org/glib/stable/glib-Type-Conversion-Macros.html and should be handled in the same way. I would suggest going with a solution along these lines as well.

[MathieuDuponchelle]

Unfortunately, the code for this is a bit obfuscated in the configure.ac file of the glib, not readily copypastable I'm afraid @jeroenooms . The API could also be reworked to take a null-terminated char array fwiw.

[nwellnhof]

@jeroenooms I'd either stop using linked lists which are unnecessarily complex, or make the payload a union of pointer and integer types. But all of this this doesn't belong here. @vmg Can you allow to open issues on github/cmark?

[MathieuDuponchelle]

@jeroenooms I'd either stop using linked lists which are unnecessarily complex

That's my second suggestion, it shouldn't be very hard to implement

or make the payload a union of pointer and integer types

There shouldn't be any reason to do that, I don't think cmark is intended to be compilable on platforms with < 32 bits pointer sizes?

[jeroen]

@nwellnhof @kivikakk @vmg it would be great to open issues / pr's for the fork. I'd like to add support for aligning tables (example).

[kivikakk]

I opened issues on the github/cmark fork; I forgot they might not be enabled by default.

We kept the plugin registration mechanism so e.g. the cmark binary can list extensions that are available, so the user can then enable them at their leisure:

$ build/src/cmark --list-extensions
Available extensions:
table
strikethrough
autolink
tagfilter
$

(Per @MathieuDuponchelle's original design!) Without a registration mechanism, there's no way for the library to list them ahead of time. This is important for wrapping the library.

@jeroenooms: here's how we've done it in the Ruby commonmarker extension: https://github.com/gjtorikian/commonmarker/blob/d5c50a9238f5df210783ae6cbc41b66c02848be5/ext/commonmarker/commonmarker.c

We register the known core extensions once, on initialisation of the gem, and then provide the method CommonMarker.extensions to get a list of known extension names. The Ruby gem user can then supply an array of extension names when rendering, which are attached to the parser or supplied to cmark_render_html.

As for table alignment, it's already added! See the relevant part of the extensions tests, and this example:

| longer table | headers to | demonstrate alignment |
| :-- | :-: | --: |
| a | b | c |

longer table	headers to	demonstrate alignment
a	b	c

[kivikakk]

@jeroenooms: could you try building this branch for your R bindings? https://github.com/github/cmark/tree/kivikakk/win32

Unfortunately I'm unable to get an exact build environment up that simulates yours (or perhaps that's CRAN's), but this might clear all the warnings.

[kivikakk]

If there are any more issues with our fork, please feel free to open them at https://github.com/github/cmark/issues/new!

[MathieuDuponchelle]

(Per @MathieuDuponchelle's original design!) Without a registration mechanism, there's no way for the library to list them ahead of time. This is important for wrapping the library.

Given the reduction of scope in your branch, and the removal of plugins, the whole cmark_plugin API is simply useless. You could simply (and should, in order to provide an API that makes a minimal amount of sense), at cmark_init time, directly instantiate the extensions, as you compile them along the rest of your sources, instead of going through convoluted maneuvers in the cmark executable, involving (sic), a #include "../extensions/core-extensions.h". Anyway ...

[jeroen]

For those following this thread, I have posted an announcement of the R bindings here: https://ropensci.org/blog/blog/2016/12/02/commonmark

[kivikakk]

@MathieuDuponchelle: (a) there is no more cmark_init in our fork, (b) it is designed so that the core cmark code can be compiled once and not further changed: it can be distributed as a library, and then you can compile or discover extensions however you like and then register them. You're right that I can ditch cmark_plugin_* functions; we should just directly allow addition of syntax extensions to the known registry.

#include "../extensions/core-extensions.h" is in the main.c, which is not part of the library. The program sources are only "main.c" itself, which is then linked with libcmark and libcmarkextensions. This works quite well.

I hope this has helped elucidate some of the design factors for you. Seeing as we've published our fork, I'll close this issue — please feel free to open issues on github/cmark if you have questions about our fork specifically!

@jeroenooms: the R package release notes look great! :)

[MathieuDuponchelle]

Last remark, the design that was implemented by this pull request does not make it possible for a third party to implement a new inline extension and have it containable in, for example a table cell as implemented by the table extension. So much for compiling the core once ;)