commonmark-hs icon indicating copy to clipboard operation
commonmark-hs copied to clipboard

`gfmExtensions` parser drops out of list when `|` is encountered

Open andreasabel opened this issue 3 years ago • 15 comments
trafficstars

( Lifted from

  • https://github.com/haskell/hackage-server/issues/1105 )

Rendering

- foo
- `a|b`
- bar

with gfmExtensions included in the parser gives this:

  • foo

- a|b

  • bar
rather than this:
  • foo
  • a|b
  • bar

Reproducer:

{-# LANGUAGE LambdaCase #-}
{-# LANGUAGE QuasiQuotes #-}
{-# LANGUAGE ScopedTypeVariables #-}

import Commonmark
import Commonmark.Extensions

import Data.String.QQ (s)
import Data.Text.Lazy.IO as TLIO

main :: IO ()
main = do
  commonmarkWith (gfmExtensions <> defaultSyntaxSpec) "inline" input >>= \case
    Left e                  -> error (show e)
    Right (html :: Html ()) -> TLIO.putStr $ renderHtml html
  where
  input = [s|
- foo
- `a|b`
- bar
|]

{- Produces:

<ul>
<li>foo
</li>
</ul>
<p>- <code>a|b</code></p>
<ul>
<li>bar
</li>
</ul>

Seems like the gfmExtensions parser cannot deal with `|`.
-}

andreasabel avatar Jul 19 '22 14:07 andreasabel

I think what's happening is that the table extension is kicking in, because of the |. I think the current behavior of this extension is to regress and parse the lines as paragraph content if a table heading line doesn't follow -- obviously, this isn't what is wanted in the current case. In fact, this case may interact badly with the commonmark parser's goal of parsing line by line with no backtracking.

jgm avatar Jul 19 '22 17:07 jgm

The relevant part of the source is in commonmark-extensions, src/Commonmark/Extensions/PipeTable.hs, lines 176-193.

jgm avatar Jul 19 '22 17:07 jgm

You may be able to work around this by moving the pipeTableSpec after defaultSyntaxSpec, i.e.

allTheGfmExtensionsExceptPipeTable <> defaultSyntaxSpec <> pipeTableSpec

jgm avatar Jul 19 '22 17:07 jgm

I think what's happening is that the table extension is kicking in, because of the |. I think the current behavior of this extension is to regress and parse the lines as paragraph content if a table heading line doesn't follow

https://github.com/jgm/commonmark-hs/blob/714a1b651a5a6dc0fa9e0a2eba9848ca898be383/commonmark-extensions/src/Commonmark/Extensions/PipeTable.hs#L183-L195

andreasabel avatar Jul 21 '22 06:07 andreasabel

Judging from the examples given with https://github.github.com/gfm/#table, and its present implementation on github.com, a table header needs to be followed by a delimiter row, otherwise it is not a table.

i|am|not|a|table

|i|am|also|not|a|table|

i am a table
i am also a table

In fact, this case may interact badly with the commonmark parser's goal of parsing line by line with no backtracking.

Apparently, you need two lines to start a table. So yes, noble goal, but reality is harsh... ;-)

andreasabel avatar Jul 21 '22 06:07 andreasabel

You may be able to work around this by moving the pipeTableSpec after defaultSyntaxSpec, i.e.

allTheGfmExtensionsExceptPipeTable <> defaultSyntaxSpec <> pipeTableSpec

Thanks for the hint!

I am a bit hesitant to follow this path, because I cannot judge the consequences. How do I know this isn't a whack-a-mole-game? We haven't secured desired behaviors of our markdown parser instance with a testsuite, so I might break something else. I'd rather wait for a fix of the table parser...

andreasabel avatar Jul 21 '22 07:07 andreasabel

a table header needs to be followed by a delimiter row, otherwise it is not a table.

Yes, and that's what this parser expects. The difficulty is that this parser tries to do line-by-line block parsing with no backtracking, so it can't look ahead to the next line. The current approach is to retroactively change the block from a table to a paragraph if that second line isn't encountered. But that won't work for the kind of example you give. The fix, with the current parsing strategy, isn't obvious, unless it's the thing I mention above. Note that the monoidal composition of syntax specs is order-sensitive. So the problem here is that we're checking for a table before checking for a list item.

jgm avatar Jul 21 '22 08:07 jgm

Ok, I try the workaround then!

andreasabel avatar Jul 21 '22 10:07 andreasabel

The problem with the suggested workaround (pipeTableSpec after defaultSyntaxSpec) is that tables do not parse anymore:

{-# LANGUAGE LambdaCase #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE ScopedTypeVariables #-}

import Commonmark
import Commonmark.Extensions

import Data.Text.Lazy.IO as TLIO

main :: IO ()
main = do
  commonmarkWith spec "inline" input >>= \case
    Left e                  -> error (show e)
    Right (html :: Html ()) -> TLIO.putStr $ renderHtml html
  where
  spec = mconcat
    [ emojiSpec
    , strikethroughSpec
    , autolinkSpec
    , autoIdentifiersSpec
    , taskListSpec
    , footnoteSpec
    , defaultSyntaxSpec
    , pipeTableSpec
    ]
  input = table
  table = "|a|table|\n|---|---|"

Gives:

<p>|a|table|
|---|---|</p>

andreasabel avatar Jul 21 '22 10:07 andreasabel

OK, back to the drawing board.

Here's another case to keep in mind:

- `a|b`
- | -

This does parse -- as a table. Hm, I wonder how GitHub's parser renders it? Let's try:

  • a|b
  • | -

Let's also try your original case:

  • foo
  • a|b
  • bar

jgm avatar Jul 22 '22 08:07 jgm

Literal pipes in GitHub's pipe tables formerly needed to be escaped, even inside code backticks; let's see if that's still the case:

test table
` `

Answer: yes.

jgm avatar Jul 22 '22 08:07 jgm

Aha. You need a trailing newline in input in your program above. That's why it's not parsing as a table.

jgm avatar Jul 22 '22 09:07 jgm

Original case also works with the workaround; you just need to ensure that all lines are terminated with newline characters.

jgm avatar Jul 22 '22 09:07 jgm

I don't think the workaround will have any bad consequences: it just means that nothing will be interpreted as a table if it can be interpreted as any other kind of block-level element (aside from a paragraph), and I think that's desirable.

We can leave this issue open, though, because this is an awkward workaround and it makes it impossible to use gfmExtensions.

jgm avatar Jul 22 '22 09:07 jgm

Thanks for the further research, @jgm. I now applied your workaround to hackage-server:

  • https://github.com/haskell/hackage-server/pull/1114

andreasabel avatar Jul 24 '22 15:07 andreasabel