Throw, if BlockSyntax.parseLines loops indefinitely

[jonasfj]

We throw an AssertionError if BlockSyntax.parseLines enters an infinite loop. This should have no effect, if there are no bugs. But if there is a bug, it's easier to discover it in production if the failure mode isn't an infinite loop eating up CPU, but just an unhandled exception.

---

Context: We hit #531 in production, which caused an infinite loop. It would be much easier to detect what went wrong in cases such as this, if it just threw an error. That will have a stack trace :D

I also think it's hard to promise that this won't happen again, it looks as though all of these BlockSyntax objects are very interwoven, and their behavior might even be affected by the order in which they are given. And users can make subset of them, if they want to, possibly even subclass and customize syntaxes to make their own. All of which makes it a bit hard to promise there won't be any infinite loops or other bugs.

I'm not sure how well customing a BlockSyntax subclass works, maybe if you mostly customize the output it'll mostly work -- I can definitely see cases where the result is hard to predict, and where any changes in package:markdown is a breaking change 🤣

[coveralls]

Pull Request Test Coverage Report for Build 4753859523

• 4 of 5 (80.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.05%) to 96.374%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/src/block_parser.dart	4	5	80.0%

Totals
Change from base Build 4744952408:	-0.05%
Covered Lines:	1515
Relevant Lines:	1572

---

💛 - Coveralls

[jonasfj]

I wonder if we should do a similar thing in InlineParser, it's unclear if all code-paths from an iteration there will advance the position -- or, maybe I just didn't loop close enough.

It's also possible we could make neverMatch a set or list, instead of a single value. But this proposal is probably cheaper (no allocations) and super robust. Kind of attractive to do whenever we have while (!isDone) for any reason.

[sigurdm]

LGTM

[chenzhiguang]

https://github.com/tagnote-app/dart_markdown/blob/master/lib/src/parsers/block_parser.dart#L97

@jonasfj This neverMatch is a list, I forgot the reason why I simplified it as a single value in this package.

[srawlins]

Love it!

[jonasfj]

@chenzhiguang, btw, I'm curious, why do syntax.canParse not just return null?

It seems like:

• syntax.parse(); if (_pos == positionBefore) might as well have been syntax.canParse() == false,
• syntax.parse() != null might as well have been syntax.canParse() == false

Is there some logic to why BlockSyntax.canParse() == true only means "maybe", instead of meaning that it can parse? It might be worth while explaining it in comments, because it looks a bit confusing.

[chenzhiguang]

@jonasfj

You're right. canParse means maybe here. Perhaps we should make canParse real means can parse, and make parse must return a Node value.

[jonasfj]

@chenzhiguang I think that would be worth while.

Or you just remove BlockSyntax.canParse() entirely, and make BlockSyntax.parse() return null whenever BlockSyntax.canParse() would have returned false.

I would also make BlockSyntax.parse() return null whenever it doesn't advance _pos. And make an assert(_pos > positionBefore) in the loop iteration -- so that the invariants are maintained.