Iterator based API is very constraining

[dpc]

I need to take djot content, and replace certain types of links (to usernames) with other content (their display names).

To do that when I spot a certain event, of a certain format, I need to use it to look up user's information in the database. I can't do that in a .map(...) of an iterator, because I'm using async API.

I thought I could just use for event in events, but looks like the renderer expects a single iterator.

IMO, avoiding constraining API to iterator would work better here. A Renderer could have individual events pushed to it, and then the render_epilogue (fn finish might be a better name) could be called manually by the user. This would avoid async issues too. Iterator methods could be helpers.

[hellux]

An alternative can be to modify the events and collect them to a Vec first, then render them afterwards all at once? It would increase the memory usage for large inputs but might be okay otherwise.

Splitting it into e.g. render_event, render_epilogue would complicate the interface a bit:

• some renderers may need a render_prologue as well, so that must probably be added as well
• the current interface supports both unicode and byte sinks, so render_prologue, render_event, render_epilogue may need to be split into push_prologue, write_prologue, push_event, write_event, push_epilogue, write_epilogue in addition to the helpers push/write that take iterators.
• this would make the interface state based, so calling functions in a certain order may become illegal. E.g. calling render_event after render_epilogue, skipping prologue or epilogue may cause undefined behaviors.

[dpc]

An alternative can be to modify the events and collect them to a Vec first, then render them afterwards all at once? It would increase the memory usage for large inputs but might be okay otherwise.

Workable workaround, but kind of meh as a general solution.

this would make the interface state based

It already is, kind of? AFAICT the html rendered keeps some state. My event replacement routine already needs to keep some state as well, as I need to replace both begin and end for a container, so I need to track it.

so calling functions in a certain order may become illegal.

may cause undefined behaviors.

I can understand that it would lead to incorrect rendered text, but not UB. It's already possible to feed rendering completely nonsensical series of events - I already did it accidentally by misunderstanding how it is supposed to be used and got underflow panics etc.

some renderers may need (...)

the current interface supports both unicode and byte sinks

API surface is worthwhile minimizing, but this doesn't seems like an end of the world.

BTW. Can't render_prologue be done ... in a constructor? And render_epiloge in a finalizer?

The whole passing of out: W as an argument seemed odd.

struct HtmlRenderer<W>(W);

impl<W : fmt::Write> HtmlRenderer {

   fn new(w: W) -> Self {
        // .. emit prolog here
   }

   fn finalize(self) -> W {
     // .. emit epilogue here

     self.0
   }
}

impl Renderer for HtmlRenderer {

   fn emit(event: Event) {

   }

kind of thing.

Doesn't this remove the W from the Renderer, eliminating need for multiple APIs?

[hellux]

this would make the interface state based

It already is, kind of? AFAICT the html rendered keeps some state. My event replacement routine already needs to keep some state as well, as I need to replace both begin and end for a container, so I need to track it.

I mean no state is kept within the object between interface calls. Right now the state object is created and dropped within the push call. The only kept state is the written output.

I can understand that it would lead to incorrect rendered text, but not UB.

Incorrectly rendered output, yes, "undefined behavior" was a poor choice of words.

BTW. Can't render_prologue be done ... in a constructor? And render_epiloge in a finalizer?

The whole passing of out: W as an argument seemed odd.

struct HtmlRenderer<W>(W);

impl<W : fmt::Write> HtmlRenderer {

   fn new(w: W) -> Self {
        // .. emit prolog here
   }

   fn finalize(self) -> W {
     // .. emit epilogue here

     self.0
   }
}

impl Renderer for HtmlRenderer {

   fn emit(event: Event) {

   }

kind of thing.

Doesn't this remove the W from the Renderer, eliminating need for multiple APIs?

I think you still need multiple APIs, but in this case it would be two traits instead of two methods, but that might actually make it simpler, especially if the byte one can be implemented automatically when the unicode one is implemented. However, there is already a second RenderRef trait, so would probably want to merge that somehow to avoid 4 traits.

Moving the trait to the object with the writing state is an option (currently the trait is on the configuration object, and the writer object is created internally within each call), I don't see any immediate major problems with it but would have to experiment to see if the interface seems reasonable.

[dpc]

Oh. I haven't noticed RenderRef before. What's the purpose of it? So the owned one can avoid cloning by taking the things by value, and the RefRenderer can do the cloning? In that case why not take event: impl Into<Cow<Event>>? https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=ecd04881ddad9f9a629c07d9e7b13dfc

Or alternatively, have all the allocatable-things inside events be Cow ... waaaait ... I just checked and everything inside Events seems to be Cow already...

An now that I think about ... wouldn't most if not all renderers just want to take a reference and write it out into some kind of writer anyway, so seems like a reference would always do anyway?

[hellux]

Oh. I haven't noticed RenderRef before. What's the purpose of it? So the owned one can avoid cloning by taking the things by value, and the RefRenderer can do the cloning?

I think it was added to avoid having to use clone when rendering stored events multiple times, but I am not really sure it is a good enough reason to keep it.

An now that I think about ... wouldn't most if not all renderers just want to take a reference and write it out into some kind of writer anyway, so seems like a reference would always do anyway?

Converting an arbitrary iterator of owned items into an iterator of borrowed items is awkward. However, if the API is changed to allow single events at a time, this might not be a problem so that would be another reason to remove RenderRef.

In that case why not take event: impl Into<Cow<Event>>? https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=ecd04881ddad9f9a629c07d9e7b13dfc

Hmm, Iterator<Item=impl Into<Cow>> could potentially solve the borrowed/owned iterator problem also.

[dpc]

@hellux I have prepared a proof of concept: https://github.com/dpc/jotup

I have told LLM to take jotdown source code and convert it to use new interfaces. After a bit of back and forth (aka multiple sessions struggling to make LLM do what I wanted without taking shortcuts), I have slapped fuzzer on top to make sure jotup produces exactly same output as jotdown. Though now I think, the changes are actually minor enough, that now that I know what the end result needs to look like, I could just fork jotdown again and apply changes more manually and surgically, because at the end of the day I don't trust that LLM, even with fuzzing. Though it did save me a lot of time figuring out what needs to be done.

Notably the Renderer needs more advanced lifetimes to avoid Cloning everything, but it really comes down to Renderer not outliving the input, so it can hold references to it. Cloning to Cow::Owned is still possible, as long as Cow is used in the Events. I think if the Footnotes in the Writer would just store Event<'static> and just cloned Footnotes eagerly, then the whole extra lifetime stuff could be avoided completely, and it's probably an acceptable tradeoff as footnotes are not that big part of a typical markdown/djot document, I think.

Notably the new trait struct allows neatly composing rendering-modifing logic. E.g. in my app some links like myapp://foobar have special meaning, and need to be translated to custom html, or media content needs to be adjusted for privacy reasons, html sanitized, etc. Currently I'm doing some ugly looking stuff to achieve it, and I am hoping to replace it with modular Renderer-s, wrapping each other, implementing clearly defined djot-based functionality. Sanitizing stuff can be one Renderer, replacing external media link another, and so on.

I have also added async (tokio IO trait based) renderer. While the parser struct can be reused (though one could imagine Parser reading from io::Read, and io::AsyncRead, composing neatly with corresponding Renderers) the renderer had to be basically copy-pasted, as there is no way to do generic Rust over async. I think it's fine. I have put async stuff behind feature flag. My goal with Async one is to be able to do asynchronous lookup during rendering to e.g. replace certain HTML elements with stuff from the database (async IO) - e.g. rostra:<userid> is replaced with nicely looking rendered user handle, etc.

I also made some tweaks to how output is handled in the Renderer, and how minified etc. are working (builder pattern). Minor stuff, but I think it's an improvement.

In my app I made a decision to make djot a central point of social media content. So djot is load bearing for me and I really need to implement more features around it. If you'd like to make these changed part of jotdown than I would be very happy, try to help and be your best user. If not, than I will just keep going with a fork, and try to use an LLM + fuzzing to keep it in sync with jotdown, or re-attempt all the changes more manually.

[dpc]

Yeah, cloning to owned in footnotes is not a lot of changes.

[hellux]

@hellux I have prepared a proof of concept: https://github.com/dpc/jotup

...

Thanks for taking the time to show your findings!

So, there are two main drivers to your changes here:

1. composability of filters and
2. async filters?

If I understand correctly, the new Render trait solves 1 while 2 is solved with
a duplicate async implementation of the HTML renderer and a separate async
interface?

Some thoughts/ideas:

• The begin, emit and finish have more or less the same interface except the
  event argument. If the parser were to emit Start(Document)/End(Document)
  events, the emit function could handle begin/finish when those are
  encountered, removing the need of the begin/finish functions?

• The Render trait is quite general right now, it only takes &mut self and an
  Event, and then its up to the renderer to output to somewhere. The trait is
  then reused for filters by wrapping a renderer and calling its interface.

  I feel like there are two separate types of objects here:

  • renderers (which output an event to a byte sink)
  • filters (which convert each event to a new sequence of events)

  and perhaps there should be two separate traits to make it more
  straightforward how to use/implement them for each use case, something like:

  trait Render {
      fn emit<'s, W: Write>(&mut self, event: Event<'s>, out: W);
  }
  
  trait Filter {
      fn emit<'s, R: Render>(&mut self, event: Event<'s>, renderer: R);
  }

• I do like how you achieve composability of filters with the new interface in
  e.g.
  (https://github.com/dpc/jotup/blob/4609b1d2a5af32b79f8d3d958d823d8dfcbd8596/src/html/tests.rs#L17).

  Right now you need to manually recursively wrap filters around a renderer and
  then render with the outermost filter. Perhaps there could be some sort of
  helper that lets you specify a renderer and some filters and the composition
  is handled for you?

  • Could perhaps be achieved with a helper object, e.g. FilteredRenderer,
    which takes a renderer and a list of filter implementations:

    FilteredRenderer::with_renderer(html::Renderer::new())
      .with_filter(ReplaceImages::new())
      .with_filter(ReplaceLinks::new())
      .render(&mut output);

    However, this may require that each filter is accessed with dynamic
    dispatch which may affect performance.

  • Alternatively could be achieved with a macro that handles the wrapping for
    you at compile time:

    let renderer = filtered_renderer!(
        html::Renderer::new(),
        ReplaceImages::new(),
        ReplaceLinks::new(),
    );

    which could expand into roughly

    let renderer = FilteredRenderer {
        filter: ReplaceLinks::new(),
        renderer: FilteredRenderer {
            filter: ReplaceImages::new(),
            renderer: html::Renderer
        }
    };

    and FilteredRenderer<F, R> implements Render for any F: Filter, R: Render.

• It would be nice to allow for async with at least the same Renderer
  implementation. If the interface allows you to emit one event at a time, it
  may be possible to have an adapter between the non-async and async
  interfaces?

[dpc]

The begin, emit and finish have more or less the same interface except the
event argument. If the parser were to emit Start(Document)/End(Document)
events, the emit function could handle begin/finish when those are
encountered, removing the need of the begin/finish functions?

Yeah, I think these are equivalent.

should be two separate traits to make it more
straightforward how to use/implement them for each use case, something like:

I don't see a reason to duplicate these. The output/renderer being passed as an argument just unnecessarily constrains the interface and bolts it to writers, etc.. I think the emit-only abstraction neatly splits the job in two parts: parser that emits events, and kind of a "sink" that can consume them.

The Renderer here is a bit of a misnomer. It could be called "Sink" or a "Drain", though I imagine 99% of users will use these to render the events as a Html.
One could have Renderers that: do not drive any output. Or write to weird outputs that don't even implement any Write. Maybe they want to collect events as is, and render later and so on. It's regrettable that in Rust there's no way to abstract over async vs no-async, otherwise a single Parser + Renderer trait would work for everyone and everything.

Perhaps there could be some sort of
helper that lets you specify a renderer and some filters and the composition
is handled for you?

Crossed my mind too, but I think this is kind of a monad or something, and would require maybe another trait with a .map and `some associated type etc. It definitely can be added later.

Macro works too. I'll experiment with these when I'm using these in my project. But even doing the wrapping manually is just a bit tedious, not a big problem.

it
may be possible to have an adapter between the non-async and async
interfaces?

The adapter might be possible, but the amount of tiny block_on /block_in_place needed might be terrible for performance.

So what do I do? Should I start preparing PRs and we can try it out in some branch or something?

[dpc]

On the interfaces, worth noting, that most users should probably use this nice little way:

    let output = html::Renderer::new()
        .minified()
        .render_to_string(input);

The struct Renderer could e.g. have methods like:

    let output = html::Renderer::new()
        .minified()
        .sanitized()
        .render_from(input);

where .sanitized would wrap the html::Renderer itself inside some struct Sanitizer<R>.

So maybe there will be something like this:

/// Throwing dangerous html stuff away
struct Sanitize<R>(R);

/// Conveniance
trait RenderExt : Render {
  fn sanitized(self) -> Sanitized<Self> {
     Sanitized::new(self)
  }
}

/// Abstraction over composed renderers that produce output
trait RenderWithOutput : Render {
  type Output = O;
  
  fn to_output(self) -> Self::Output;


}

/// Convenience to get do the final rendering and get the result
trait RenderWithOutputExt {
  fn render_from(self, input: &str) -> Result<Self::Output, <Self as Render>::Error>;
}


impl RenderWithOutputExt for R where R: RenderWithOutput {
  fn render_from(self, input: &str) -> Result<Self::Output, <Self as Render>::Error> {
        for event in crate::Parser::new(djot) {
            self.emit(event)?;
        }
        Ok(self.to_output())
   }
}

/// Generic way to wrap renderers in each other (not sure about this one yet, need to play with real code and inference issues
///
/// but `renderer.wrap::<Sanitizer>()` should kind of work, I think.
trait RenderWrapper<Inner> : Render {
  fn wrap<R>(inner: R) -> Self where R: Render
}

impl RenderWithOutput for html::Renderer<W> where W: fmt::Write {
   type Output = W;

   fn to_output(self) -> W { self.output }
}

impl<Inner> RenderWrapper<Inner> for html::Sanitizer<R> where Inner: Render {
  fn wrap(r: R) -> Self {
     Sanitizer::new(r)
  }
}

impl<Inner> RenderWithOutput<Inner> for html::Sanitizer<R> where Inner: RenderWithOutput {
  fn to_output(r: R) -> Self {
     self.inner.to_output()
  }
}

[hellux]

So what do I do? Should I start preparing PRs and we can try it out in some branch or something?

Jotdown aims to be a quite low-level library that should allow you to build any abstractions on top of it. So I think we should prioritize to at least make it possible to achieve what you need without a fork. I.e. I want jotdown to be flexible enough to let downstream crates re-use its implementation as much as possible.

So, what do you think would be the minimum change to jotdown that would let you implement your functionality on top of it?

• The Parser could be reused from current jotdown as-is, right? Can your renderering changes be implemented outside the crate, or does it require anything non-public? (The goal is at least that the HTML renderer should only use the public interface, and the Render trait is completely optional to use).
• For you to be able to re-use the HTML renderer for composability, it should be enough to expose a method to render one event instead of the whole document?
• I am not sure exactly how to re-use the current HTML renderer for async, but if it is completely copied, it should at least be possible to implement it outside jotdown and not require to maintain a fork.

[dpc]

• The Parser could be reused from current jotdown as-is, right? Can your renderering changes be implemented outside the crate, or does it require anything non-public? (The goal is at least that the HTML renderer should only use the public interface, and the Render trait is completely optional to use).

There are some events that are using raw &'a str instead of Cow<'a, str>, otherwise I think everything needed is public. Since Parser is an iter, any code can just pull events one by one.

I am not sure exactly how to re-use the current HTML renderer for async, but if it is completely copied, it should at least be possible to implement it outside jotdown and not require to maintain a fork.

Yeah, that works.

[dpc]

BTW. I went ahead and made a fork of jotdown with everything how I wanted it to be. I'm using it for my own purposes and works perfectly for me. Thanks for your help, it did give me better understanding of nuances here and improved my own ideas. Consensus forming is time consuming, so I think it's just easier to fork sometimes than keep debating, especially in the age of LLMs. I'll probably pick changes (sweet bugfixes!) from jotdown, and keep utilizing fuzzing to ensure compatibility.

I'm not sure if I should keep this issue open, so feel free to close or not.

[hellux]

I think the issue can stay open as the original issue is not completely resolved yet.