kivikakk / comrak

CommonMark + GFM compatible Markdown parser and renderer
Other
1.17k stars 140 forks source link

Add math support #366

Closed digitalmoksha closed 6 months ago

digitalmoksha commented 7 months ago

See https://github.com/kivikakk/comrak/pull/366#issuecomment-2016316394 for updated details.


This will add support for math syntax, both the dollar math and the newer GitLab/GFM syntax. Relates to https://github.com/kivikakk/comrak/issues/23

Display math is mostly working, $$ and ```math syntax. Inline not at all yet.

A couple notes:

I'm flexible on these decisions, this is just where it's at right now.

digitalmoksha commented 6 months ago

@kivikakk I think I have this finished 🤞

It's similar to pandoc and commonmark-hs for dollar math, including using <span>. But it uses a data-math-style attribute to determine whether it's inline or display.

Code math (the GitLab syntax), uses <code> instead of <span>, which I think is correct given the markdown syntax. And it gives a way to tell the two syntaxes apart if the HTML needs to be converted back into markdown (which we do).

If you get some time, would you mind reviewing?

cargo run -- -e math-dollars enables dollar math.

cargo run -- -e math-code enables code/GitLab math.

kivikakk commented 6 months ago

heya @digitalmoksha, thank you so much for this! i'm currently preparing for an imminent overseas move, but i'll try to squeeze this in in the next couple days :)

digitalmoksha commented 6 months ago

And good luck on your move! Exciting!

digitalmoksha commented 6 months ago

Oh I'm sorry to hear that. I hope you feel better.

Thank you for allowing me to move forward on this 🙇. I will definitely run fuzzing and take another look at the commented out tests.

kivikakk commented 6 months ago

Just fwiw, this branch did hit a crash on cargo fuzz run all_options -j4 within a minute or two, so it'd be worth going through a few fuzz–fix cycles!

kivikakk commented 6 months ago

Minimised repro of the one I hit first:

> printf '$`$' | RUST_BACKTRACE=1 cargo run -- -e math-dollars -e math-code
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/comrak -e math-dollars -e math-code`
thread 'main' panicked at src/parser/inlines.rs:726:42:
slice index starts at 2 but ends at 1
stack backtrace:
   0: rust_begin_unwind
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/std/src/panicking.rs:597:5
   1: core::panicking::panic_fmt
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/core/src/panicking.rs:72:14
   2: core::slice::index::slice_index_order_fail_rt
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/core/src/slice/index.rs:98:5
   3: core::slice::index::slice_index_order_fail
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/core/src/slice/index.rs:91:14
   4: <core::ops::range::Range<usize> as core::slice::index::SliceIndex<[T]>>::index
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/core/src/slice/index.rs:406:13
   5: core::slice::index::<impl core::ops::index::Index<I> for [T]>::index
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/core/src/slice/index.rs:18:9
   6: comrak::parser::inlines::Subject::handle_dollars
             at ./src/parser/inlines.rs:726:42
   7: comrak::parser::inlines::Subject::parse_inline
             at ./src/parser/inlines.rs:219:25
   8: comrak::parser::Parser::parse_inlines
             at ./src/parser/mod.rs:2014:15
   9: comrak::parser::Parser::process_inlines_node
             at ./src/parser/mod.rs:1994:17
  10: comrak::parser::Parser::process_inlines
             at ./src/parser/mod.rs:1988:9
  11: comrak::parser::Parser::finalize_document
             at ./src/parser/mod.rs:1827:9
  12: comrak::parser::Parser::finish
             at ./src/parser/mod.rs:1809:9
  13: comrak::parser::parse_document_with_broken_link_callback
             at ./src/parser/mod.rs:123:5
  14: comrak::parser::parse_document
             at ./src/parser/mod.rs:62:5
  15: comrak::main
             at ./src/main.rs:286:16
  16: core::ops::function::FnOnce::call_once
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/core/src/ops/function.rs:250:5
digitalmoksha commented 6 months ago

@kivikakk I've been running fuzzing for about 5 hours without any problems, and will probably let it run overnight. I feel this is ready to go.

Let me know if you feel like giving it another once-over before I merge it.

digitalmoksha commented 6 months ago

I've been running fuzzing for about 15 hours without problems.

I'm going to go ahead and merge this.

digitalmoksha commented 6 months ago

@kivikakk before you become completely unavailable with your move, would you have time to cut a new release? I'm particularly hoping to get the escaped chars option released.

kivikakk commented 6 months ago

Yes, of course! Will do so in the next day or two! Nice work on the release 🥳El 30 mar 2024, a la(s) 01:28, digitalMoksha @.***> escribió: @kivikakk before you become completely unavailable with your move, would you have time to cut a new release? I'm particularly hoping to get the escaped chars option released.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

kivikakk commented 6 months ago

Release underway …

kivikakk commented 6 months ago

Done! https://crates.io/crates/comrak/0.22.0 / https://github.com/kivikakk/comrak/releases/tag/0.22.0

Thank you all. :blush:

gjtorikian commented 6 months ago

BTW @kivikakk if it is at all helpful I can help cut releases too—I just don’t know how or whether I have the access 😅

kivikakk commented 6 months ago

@gjtorikian Thank you very much! Invite sent; bus factor halved! ✅

gjtorikian commented 6 months ago

Thanks for that! Do you just cargo publish locally or is there any automation for git tags and GitHub releases?

kivikakk commented 6 months ago

No automation, just RELEASE_CHECKLIST.md in the root! (It'd be lovely to automate but frankly it's never become worth doing.)

digitalmoksha commented 6 months ago

I updated the dingus to allow the enabling of math (and the enabling of most other options, though I'm still missing a couple)

gjtorikian commented 6 months ago

No automation, just RELEASE_CHECKLIST.md in the root! (It'd be lovely to automate but frankly it's never become worth doing.)

I am a lazy, lazy person, so I will see how much a robot can do for us. 😆