Use generics for broken link callback

[ollpu]

As discussed in #508, #509 and #697, the current broken link callback API is a little clunky. This PR changes it to use generics instead of dynamic dispatch.

I haven't considered the performance or binary size implications of this. There should be no difference when not using the callback (all instances use DefaultBrokenLinkCallback), but when using it, all the parser's methods will be monomorphized somewhat unnecessarily. Ultimately it's up to you (the maintainers) to decide.

On the flip side, this solution provides the most flexible API. Parser will now transitively implement Send & Sync if the callback does. It can also accept 'static callbacks:

fn callback<'input>(link: BrokenLink<'input>) -> Option<(CowStr<'input>, CowStr<'input>)> {
    todo!()
}

fn parse(input: &str) -> impl Iterator<Item = Event<'_>> {
    Parser::new_with_broken_link_callback(input, Options::empty(), Some(callback))
}

Alternative solutions with less performance and binary size implications:

• Use Box<dyn FnMut> (doesn't allow Send & Sync)
• A hybrid solution, where the generic has a default of F = Box<dyn FnMut>, and only getting different monomorphizations by opting in with a separate method like new_with_generic_broken_link_callback.

Some details that I'm unsure of:

• new_with_broken_link_callback still takes an Option, though it could just accept the callback directly. Doing it like this reduces API breakage as the previous signature Option<&mut dyn FnMut> is still accepted.
• I chose for BrokenLinkCallback to be a trait with a method handle_broken_link. It could also be a "trait alias" with an FnMut bound so that the separate function name wouldn't be exposed. I feel having the method makes the no-op DefaultBrokenLinkCallback cleaner, but it could also be a function.
• BrokenLinkCallback could be more general – if it is decided to add other types of callbacks later, it would be convenient to add them to a broader "Callback" trait. In this case the blanket impl for T: FnMut would be questionable though.

Closes #508, #697

[Martin1887]

Thanks, I will try to review and analyze it in the next days.

[raphlinus]

Generally this sounds good to me. It's always a tradeoff, but I think if there is a concern over monomorphization, it's possible to use Box<dyn BrokenLinkCallback>. Do we need to impl that trait on the Box, though?

I was concerned about making the type of the parser more complex, but I see it was already generic on the callback lifetime, so I don't think making it generic on the callback itself is any worse.

[ollpu]

Right, I should add that impl.

[ollpu]

I added impl BrokenLinkCallback<'i> for Box<dyn BrokenLinkCallback<'i>> now.

I also considered replacing DefaultBrokenLinkCallback with this, so that if there are e.g. multiple crates linked together, but only some of them needing to define the callback, they would still automatically agree on a single monomorphization. Trouble is that the input lifetime needs to be carried around. This complicates signatures, so I didn't do that for now.

[ollpu]

Turns out my use case naturally evolved into having two different instantiations of Parser, one with a callback and one without.

pulldown-cmark is the only dependency, with default-features = false, features = ["html"]. I also have strip = "symbols" set.

Release binary size before this PR was 742 KB, and 766 KB after. I don't know if I can meaningfully measure performance with my setup.

[Martin1887]

Sorry for the delay, this seems good to me!

[justjavac]

After merging this PR, it no longer works

[Martin1887]

Thanks for the feedback, fixed in #746.