Follow GFM spec on EM and STRONG delimiters

[calculuschild]

Starting toward better adherence to the GFM spec on Emphasis, specifically Left-flanking-delimiter-runs.

Marked version: 1.1.0

Markdown flavor: CommonMark|GitHub Flavored Markdown

Description

• Fixes Examples:
  (em) 341, 367, 368, 371, 372, 379, 390, 406, 417, 441, 444
  (strong) 391, 397, 399, 400, 401, 431, 443, 471, 475, 476, 479, 480

What was attempted

Applying the GFM spec for Left-flanking-delimiter-runs and right-flanking-delimiter runs more accurately for EM tags and STRONG tags.

Contributor

• Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
• no tests required for this PR.
• If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

• Draft GitHub release notes have been updated.
• CI is green (no forced merge required).
• Merge PR

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/markedjs/markedjs/qv2lelel1
✅ Preview: https://markedjs-git-fork-calculuschild-emphasisfixes.markedjs.vercel.app

[UziTech]

I don't have a lot of time to look at this right now but it could be a conflict with lists since * could be the start of a list as well as em and strong.

[calculuschild]

OK, so I figured out what the issue was. I now have it working quite well except for two test cases I could use some help on:

• Original strong_and_em_together
  This test passes fine normally, but when run with the 'pedantic' tag it breaks. I'm not totally sure what the pedantic tag is actually doing so I would like some feedback. Edit: Fixed!

• CommonMark Links example 519
  I have a pattern in there to skip any * inside \[ *stuff* \] to avoid conflict with links. This last example breaks on things between brackets that aren't actually links. I'm not sure how to handle this because it could be a reflink in certain cases but I can't tell without looking behind?

Note, I removed a line from the New em_2char test that was invalid according to the spec. I don't handle it properly here either, but my code isn't "breaking" it because it wasn't correct to begin with.

Also note, I haven't applied these changes to the _ version of em tags, but I expect several more tests will begin passing with that as well once these two issues are worked out.

[UziTech]

The pedantic option uses the pedantic rules to tokenize the markdown.

Basically making marked use the original spec instead of CommonMark.

[calculuschild]

OK gotcha. My change to the em tokenizer confused it. I can fix this pretty easily.
Edit: This has been fixed. Just this last bit I'm struggling on:

Any insight on detecting whether or not a set of square brackets is a reflink or not?

[calculuschild]

Can anyone help me out with detecting when square brackets are part of a reflink or not for commonmark Example 519?

@UziTech Would you have any suggestion?

[UziTech]

Can anyone help me out with detecting when square brackets are part of a reflink or not for commonmark Example 519?

You could try a lookahead or you might need to do some parsing in the em tokenizer.

[calculuschild]

Tada!! My solution was to inject known reflink labels into the em rules right after the block sequence in the lexer is finished. This way the em rule can properly skip over any links that might contain *, but still allow emphasis inside of [...] if it is not a link.

I'm hoping this tweaking of the lexer is alright. I'm not sure if there's some way people could inject malicious regex this way by giving their link labels some weird names, but I assume it would just require some further character escaping on the label names before injection.

If this looks good, I can move forward with the _ variant of the em tags as well as **strong** which uses a lot of the same logic.

[calculuschild]

Underscore em rules added. Fixes 3 more examples (371, 372, 406)

I would love some feedback on this PR!

[UziTech]

This is great work!

[calculuschild]

What do you guys think? Is this baby ready to go?

[davisjam]

LGTM for ReDoS

[UziTech]

Looks great. Thanks for all your hard work. 💯

[brainchild0]

So 424 and 425 are considered out of scope of this change set?

Or more simply:

$ git show -s
commit 6b729ed8cdb98ea75d4031f6218a1f58b9f02d8a (HEAD, EmphasisFixes)
Merge: e27e6f9 ad720c1
Author: Trevor Buckner <calculuschild@gmail.com>
Date:   Thu Jul 9 19:37:22 2020 -0400

    Merge branch 'EmphasisFixes' of https://github.com/calculuschild/marked into EmphasisFixes
$ 
$ echo '**a **b** c**' |  bin/marked 
<p><strong>a **b</strong> c**</p>

(Should be <p><strong>a <strong>b</strong> c</strong></p>.)

[UziTech]

@brainchild0 if you want to fix it I would be happy to review a PR 😁

[brainchild0]

For me the starting point is understanding what makes this particular case more challenging than the ones that have been resolved in this merge. Intuitively they all seem roughly equally complex. Obviously, I make the remark abstractly, having no familiarity with the design.

Or another way to ask the question, starting from the top and moving down: The general rule is to collect a stack of emphasis delimiters, which may be any of *, _, **, or __. As long as this rule may be somehow fully implemented, along with also dropping those same items from the stack, because of close (right flanking) delimiters, then all cases would be supported. What is missing, currently, that leaves out support for the remaining cases?

[calculuschild]

What is missing, currently, that leaves out support for the remaining cases?

The current implementation does not use a stack at all. It simply checks for existence of a left delimiter, then if found, find the first available matching right delimiter. Finally, ensure the text between the two is valid, meaning ignore any delimiters found inside links or code spans etc., and any other delimiters inside must occur in even pairs. If not valid, get the next possible end delimiter and check the middle again, until you run out of matching delimiters or you get a valid middle.

We already have regex for the left and right delimiters, so it would just be the extra effort of building up a stack in the tokenizer.

[fredck]

There is a good chance that this PR introduced #1754.