AST validation

[yannham]

This draft PR implements a simple validation procedure for CommonMark ASTs, following the discussions in #371. The idea is to perform the additional checks that the nature of a block and the nature of its children (or the absence thereof) match, which isn't currently possible to enforce (or at least not in a agreable way) using static types.

@kivikakk The logic is pretty simple, but the tests seem to loop forever, so it sounds like there's an infinite loop somewhere. However the function is a pretty dumb tree traversal, so I wonder if I'm missing something (like children() is actually returning descendants, but given the API and the existence of descendants(), this doesn't seem to be the case).

[kivikakk]

I think there's quite a few places in the new validate function where self should be node; the infinite loop happens where we stack.extend(self.children()) on every iteration, thus making it inexhaustible.

[kivikakk]

After that, I think you'll find most checks raised by the tests are of the InlineNodeWithChildren kind, but it's not true that inline nodes can't have children — they certainly can (and most do). (Basic examples of each: Emph and Strong are inlines, and they can contain Text, other Emph and Strong, Link, most (all?) other inlines. Text is an inline which cannot contain any other node.) nodes::can_contain_type should enumerate all the valid nestings, and should be used by the validate function.

The commonmark_avoids_spurious_backslash test constructs an invalid AST (that LineBreak should go on the end of p1, not as a child of the document) and is correctly flagged!

[yannham]

I think there's quite a few places in the new validate function where self should be node; the infinite loop happens where we stack.extend(self.children()) on every iteration, thus making it inexhaustible.

Oh, right. I started with a recursive function but it blew the stack and there's a specific test case to avoid that, so I switched to an iterative version which introduced node but I must have forgotten to change self to node. Thanks

[yannham]

It seems that a few dozen remaining tests still don't pass. Most of them have a Document node with Text or variants (like SpoileredText) as direct children, but this isn't allowed according to can_contain_type and the documentation (and the markdown spec I believe, which agrees that documents should only have blocks as children: https://github.com/commonmark/commonmark-spec/blob/9103e341a973013013bb1a80e13567007c5cef6f/CommonMark.dtd#L8).

What do you think? Should we relax can_contain_type, or should we fix the fact that such ASTs are ever produced?

[yannham]

For the record, failing tests are:

failures:
    tests::commonmark::commonmark_avoids_spurious_backslash
    tests::footnotes::footnote_in_table
    tests::footnotes::footnote_with_superscript
    tests::spoiler::mismatched_spoilers
    tests::spoiler::spoiler
    tests::spoiler::spoiler_in_table
    tests::spoiler::spoiler_regressions
    tests::strikethrough::strikethrough
    tests::superscript::superscript
    tests::underline::underline

[kivikakk]

Nice, thank you so much!

Of the remaining, some of them are legit (like FootnoteReference in a TableCell) and need can_contain_type to be adjusted, and some of them (like Text in Document) aren't. If it's all good with you, I'll make the remaining adjustments myself?

[yannham]

Sure!

[kivikakk]

Thanks! The spoiler ones were particularly fun: half of the issues raised were legit, but half unearthed a weirdness in what gets added to the AST when a spoiler doesn't fully match. I've left a hack in can_contain_type for that for now since fixing that is out of scope for this PR. If both you and CI like where it's at, it should be good to go!

[yannham]

I did a bit of clean up, and simplified further the validation function: the first test that some nodes don't have children at all was superfluous, because it's also subsumed by can_contain_node which would return false on such an unexpected child. So now, validate mostly just maps can_contain_child on each pair (child, parent) of the tree.

It's good to go on my end 🙂

[kivikakk]

Really clean now. Thank you so much!

[kivikakk]

This just unearthed some AST mistakes I was making! Thanks again! :)