Limit link def expansion

[notriddle]

Fixes #844

Before

After

[ollpu]

Simple enough solution. Looks good!

Similar quadratic growth could happen with the broken link callback, although it's highly context dependent. Should we warn about that in the docs?

[ollpu]

I guess I'd rather make this text.len() + 100_000 but replicating cmark is good.

[notriddle]

Similar quadratic growth could happen with the broken link callback, although it's highly context dependent. Should we warn about that in the docs?

This runs after the callback, so it should cover it, right?

[ollpu]

This runs after the callback, so it should cover it, right?

Ah, I see. But running the callback already potentially involves generating long strings, so quadratic work could be done anyway.

I guess the same applies for ref defs: If the url/title are not borrowed from the input, then them being fetched and cloned from the map is already quadratic. An input like

"[x]: " + "x\\." * N + "\n[x]" * N

is still quadratic with this PR (though only noticeably so with N > 2000 as it's just string cloning).

So the check for whether the limit has been exhausted should be before anything is cloned or the callback is called.

I'm also not sure that we should apply the same limit to the broken link callback. For ref defs, the url & title are included in the document itself once, so the limit will never be hit if each definition is only used once. This is not the case for links expanded by the callback. There may also be use cases where the generated links are significantly longer or contain a payload. Of course the 100K minimum size will be hard to reach in practice.

There is still quadratic behavior in some cases, as explained above

[notriddle]

That would be solved by making CowStr::Boxed use an Arc¹ instead of a Box, right?

Footnotes

1. with the caveat that we couldn't use String to build boxed cowstrs any more, and instead have to construct an Arc<str> in place, which means not getting to use a bunch of String helpers ↩

[ollpu]

That would be solved by making CowStr::Boxed use an Arc instead of a Box, right?

Yeah, that could be considered. For this issue though, there's probably a reasonable solution that checks the limit before cloning? Not sure if it's worth blocking a fix to this on switching to Arcs everywhere.

[notriddle]

Yeah, that's doable. Here's a commit that does it.

[ollpu]

Looks good now! (Assuming you're ok with the callback using the same limit)

[Martin1887]

I have to deep a bit more on this but it looks good, thanks!