Yet another Math implementation #734

notriddle · 2023-10-19T00:38:58Z

This feature is loosely based on what 63a29a1 #733 described, but copies commonmark-hs more closely (the balanced braces feature is added).

It largely ignores GitHub, because its math parsing is very buggy.

ollpu

This looks fairly solid! I dismissed the curly brace matching feature initially because it seemed too complex to implement compared to its utility. Your approach here makes it look quite straightforward though.

In my implementation (rhysd#1), I opted to emit each dollar as its own item in the first pass, instead of eagerly combining pairs as display tags. This seemed to make parsing simpler. It avoids the "parity avalanche" where a closing inline tag requires all subsequent dollars in the same run to be reconsidered. Our implementations differ in these corner cases:

$x$$$$$
yours: <p><span class="math inline">x</span>$$$$</p>
mine:  <p><span class="math inline">x</span><span class="math display"></span></p>

$x$$$$$$$y$$
yours: <p><span class="math inline">x</span>$<span class="math display"></span>
          <span class="math inline">y</span>$</p>
mine:  <p><span class="math inline">x</span><span class="math display"></span>
          <span class="math display">y</span></p>

Although I've just noticed commonmark-hs does not accept empty display math either, so it disagrees with both of us:

$x$$$$$$$y$$
<p><span class="math inline">x</span>$$$<span class="math display">$y</span></p>

src/firstpass.rs

src/parse.rs

- Display math can no longer be empty (aligns with commonmark-hs) - Add test case $x$$$$$$$y$$ from raphlinus#734 - Add description about how content is handled compared to code spans: newlines, surrounding spaces, and pipes inside tables.

- Display math can no longer be empty (aligns with commonmark-hs) - Add test case $x$$$$$$$y$$ from raphlinus#734 - Add description about how content is handled compared to code spans: newlines, surrounding spaces, and pipes inside tables. (Almost) implemented in https://github.com/ollpu/pulldown-cmark/tree/alt-math

ollpu · 2023-11-04T11:10:06Z

Heads up: I changed #733 to also require display math to be non-empty. The behavior I described for escaped pipes in tables should be the same as in your implementation as far as I understand.

src/main.rs

- Display math can no longer be empty (aligns with commonmark-hs) - Add test case $x$$$$$$$y$$ from raphlinus#734 - Add description about how content is handled compared to code spans: newlines, surrounding spaces, and pipes inside tables.

notriddle · 2024-03-05T18:23:06Z

I've rebased this PR onto the current mainline, and started fuzz testing it against commonmark-hs (and found a couple of problems).

ollpu · 2024-03-07T18:51:06Z

I've rebased this PR onto the current mainline, and started fuzz testing it against commonmark-hs (and found a couple of problems).

Great! I'll clean up the spec PR and add brace handling to it hopefully next week, so we can move forward with this.

Martin1887 · 2024-03-11T17:52:15Z

Hi!, and sorry for the long time without a response about this.

I still have to review the code, but I have reviewed the possible specs and #733 and this are the way to go in my opinion.

The only thing I would change is the target of the pull request, so for major changes it is better to use the branch https://github.com/pulldown-cmark/pulldown-cmark/tree/branch_0.11 I created last week.

I will review the code of both pull requests and this will be merged soon, finally! The spec is mandatory in the crate docs though, I guess that we can convert #733 complete description into RustDoc comments.

Thanks to both for this great work!

Martin1887 · 2024-03-11T18:01:53Z

The only thing in the spec I would change is using math math-inline and math math-display classes instead math inline and math display, to avoid possible collisions with user-defined or CSS framework classes.

Based on pulldown-cmark#622 and copied from https://github.com/ollpu/pulldown-cmark/tree/alt-math. Co-authored-by: rhysd <lin90162@yahoo.co.jp>

This feature is loosely based on what 63a29a1 described, but copies [commonmark-hs] more closely (the balanced braces feature is added). [commonmark-hs]: https://github.com/nschloe/github-math-bugs It largely ignores GitHub, because its math parsing [is very buggy]. [is very buggy]: https://github.com/nschloe/github-math-bugs

@ollpu

This approach, based on @ollpu's suggestion, tracks single `$`s in the inline tree, and merges them later. It avoids having to merge and unmerge them in some corner cases.

The essential problem is: every time you write `$$x$}`, you get another entry added to a hash table. Even if it's not [theoretically] *quadratic*, it's still slow. Hard limiting it to 255 entries makes this not a problem. Interestingly enough, when I tried to write an analogous torture test for code spans, I couldn't find a way to do it because code spans are keyed by their *length* instead of their *position*. In order to get N entries in the hash table, I basically had to write N `` ` `` in a row, forcing me to write quadratic amounts of input text. Comparison: ``` michaelhowell@Michael-Howells-Macbook-Pro pulldown-cmark % python3 -c 'print("$$x$}"*5000)' | time target/release/pulldown-cmark.old -M > /dev/null target/release/pulldown-cmark.old -M > /dev/null 2.63s user 0.02s system 99% cpu 2.673 total michaelhowell@Michael-Howells-Macbook-Pro pulldown-cmark % python3 -c 'print("$$x$}"*5000)' | time target/release/pulldown-cmark.new -M > /dev/null target/release/pulldown-cmark.new -M > /dev/null 0.01s user 0.00s system 6% cpu 0.109 total ``` [theoretically]: http://www.ilikebigbits.com/2014_04_21_myth_of_ram_1.html

Martin1887

Some minor comments. Thanks for the great work!

pulldown-cmark/src/firstpass.rs

pulldown-cmark/src/lib.rs

pulldown-cmark/src/parse.rs

ollpu · 2024-03-12T18:10:34Z

@Martin1887

The spec is mandatory in the crate docs though, I guess that we can convert #733 complete description into RustDoc comments.

So are you thinking this would be a brief explainer in an Options::ENABLE_MATH doc comment or something else / more elaborate?

Currently the math.txt spec is kind of long and it's hard to read as a guide. It could use some rearranging (clearly separate the pathological test cases to the end) and a pass to make sure that the important parts are concise and understandable.

I could work on these – maybe in a new PR after this is merged.

pulldown-cmark/specs/math.txt

Martin1887 · 2024-03-13T09:08:37Z

@Martin1887

The spec is mandatory in the crate docs though, I guess that we can convert #733 complete description into RustDoc comments.

So are you thinking this would be a brief explainer in an Options::ENABLE_MATH doc comment or something else / more elaborate?

Currently the math.txt spec is kind of long and it's hard to read as a guide. It could use some rearranging (clearly separate the pathological test cases to the end) and a pass to make sure that the important parts are concise and understandable.

I could work on these – maybe in a new PR after this is merged.

@ollpu, I think the best option is writing a small summary with a link to the full documentation, and creating a mdBook with all specs and other tips and guides, as the issue #863 I created yesterday. Thanks!

Martin1887 · 2024-03-23T11:06:20Z

I think that to merge this pull request and to close the math issue there are only two pending tasks:

Making clear the heading of the spec in this pull request.
Updating the spec pull request and search a place for it. Maybe we can create the mdBook now although it is not completed yet, many things can be already included.

ollpu · 2024-03-23T11:50:13Z

One remaining divergence compared to #733 is that this PR accepts empty display math items:

$ cargo run -- -m
$$$$
<p><span class="math math-display"></span></p>

I changed this to be disallowed in d583a8d ~~to align with commonmark-hs~~ (not true actually?).

Otherwise I think #733 can be closed in favor of this, and small edits can be done later. No need to merge both.

Martin1887 · 2024-03-23T11:56:29Z

I think that the high level description of #733 can be a good start point for the math spec in the documentation, but as it is true it will not be merged, I have closed it. Thanks!

Martin1887 · 2024-03-23T17:39:53Z

So let's merge it. Thanks!

ollpu · 2024-03-25T12:52:43Z

Bug:

$ cargo run -- -m
$x {$ $ } $x$
<p>$x {$ $ } $x$</p>

I'm working on a small refactor which should fix this as well.

notriddle force-pushed the math branch 3 times, most recently from 8a59500 to ebbcf99 Compare October 19, 2023 18:00

ollpu reviewed Oct 21, 2023

View reviewed changes

src/firstpass.rs Outdated Show resolved Hide resolved

src/parse.rs Outdated Show resolved Hide resolved

ollpu reviewed Oct 21, 2023

View reviewed changes

src/parse.rs Outdated Show resolved Hide resolved

notriddle force-pushed the math branch from 413befa to d541aeb Compare October 22, 2023 00:23

notriddle mentioned this pull request Oct 22, 2023

Simplify and parse math more like code spans rhysd/pulldown-cmark#1

Merged

ollpu mentioned this pull request Oct 23, 2023

Math spec #733

Closed

2 tasks

rhysd reviewed Oct 25, 2023

View reviewed changes

src/parse.rs Outdated Show resolved Hide resolved

src/parse.rs Outdated Show resolved Hide resolved

notriddle force-pushed the math branch from 154543c to c4388f7 Compare October 27, 2023 17:55

ollpu reviewed Nov 6, 2023

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

Martin1887 force-pushed the master branch from cc49653 to 2c0aebf Compare March 1, 2024 17:11

notriddle force-pushed the math branch 3 times, most recently from 8172f8f to 2d0091f Compare March 5, 2024 03:07

notriddle force-pushed the math branch from 2d0091f to 0567202 Compare March 7, 2024 18:45

notriddle changed the base branch from master to branch_0.11 March 11, 2024 22:41

ollpu and others added 5 commits March 11, 2024 15:43

Initial math spec

b0b524a

Based on pulldown-cmark#622 and copied from https://github.com/ollpu/pulldown-cmark/tree/alt-math. Co-authored-by: rhysd <lin90162@yahoo.co.jp>

Clean up some minor nits from code review

f331b06

Use a better parsing strategy for display

9f8d9b5

This approach, based on @ollpu's suggestion, tracks single `$`s in the inline tree, and merges them later. It avoids having to merge and unmerge them in some corner cases.

notriddle added 3 commits March 11, 2024 15:43

Avoid some incorrect treatment of display math lookups

25b5659

Improve brace overflow heuristics

5f55960

Fix bugs in handling of block structure

f5b33a1

notriddle force-pushed the math branch from 0567202 to f5b33a1 Compare March 11, 2024 22:43

Add math- to HTML classes inline and display

b456480

rhysd mentioned this pull request Mar 12, 2024

Support math extension #622

Closed

Martin1887 mentioned this pull request Mar 12, 2024

Add MathJax Support #477

Closed

Martin1887 requested changes Mar 12, 2024

View reviewed changes

pulldown-cmark/src/firstpass.rs Show resolved Hide resolved

pulldown-cmark/src/lib.rs Outdated Show resolved Hide resolved

pulldown-cmark/src/parse.rs Outdated Show resolved Hide resolved

ollpu reviewed Mar 12, 2024

View reviewed changes

pulldown-cmark/specs/math.txt Outdated Show resolved Hide resolved

Martin1887 added 3 commits March 23, 2024 11:37

docs: minor docstring error in display math variant

bd44686

chore: minor comment fix

890bb8e

fix(clippy): <= 0 comparison in a usize

d078efb

Martin1887 approved these changes Mar 23, 2024

View reviewed changes

Fix headers in math.txt

1825373

Martin1887 merged commit a51a362 into pulldown-cmark:branch_0.11 Mar 23, 2024
1 check passed

Martin1887 mentioned this pull request Mar 23, 2024

math support #6

Closed

vilhelmgray mentioned this pull request Mar 23, 2024

[Feature Request] about katex getzola/zola#1695

Open

notriddle deleted the math branch March 23, 2024 19:59

elijah-potter mentioned this pull request Mar 25, 2024

bug: Harper lints inline math elijah-potter/harper#68

Open

HU90m mentioned this pull request Mar 29, 2024

Lychee trys to parse math equation leading to erroneous error message. lycheeverse/lychee#1197

Open

zeozeozeo mentioned this pull request Mar 30, 2024

LaTeX support lampsitter/egui_commonmark#25

Open

SichangHe mentioned this pull request May 30, 2024

Call for PR: use Pulldown-CMark to extract match expressions lzanini/mdbook-katex#116

Open

maneesh29s mentioned this pull request Jun 1, 2024

Broken escaping of square brackets in math zoni/obsidian-export#14

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yet another Math implementation #734

Yet another Math implementation #734

notriddle commented Oct 19, 2023 •

edited

ollpu left a comment •

edited

ollpu commented Nov 4, 2023

notriddle commented Mar 5, 2024

ollpu commented Mar 7, 2024

Martin1887 commented Mar 11, 2024 •

edited

Martin1887 commented Mar 11, 2024

Martin1887 left a comment

ollpu commented Mar 12, 2024 •

edited

Martin1887 commented Mar 13, 2024 •

edited

Martin1887 commented Mar 23, 2024

ollpu commented Mar 23, 2024 •

edited

Martin1887 commented Mar 23, 2024

Martin1887 commented Mar 23, 2024

ollpu commented Mar 25, 2024

Yet another Math implementation #734

Yet another Math implementation #734

Conversation

notriddle commented Oct 19, 2023 • edited

ollpu left a comment • edited

Choose a reason for hiding this comment

ollpu commented Nov 4, 2023

notriddle commented Mar 5, 2024

ollpu commented Mar 7, 2024

Martin1887 commented Mar 11, 2024 • edited

Martin1887 commented Mar 11, 2024

Martin1887 left a comment

Choose a reason for hiding this comment

ollpu commented Mar 12, 2024 • edited

Martin1887 commented Mar 13, 2024 • edited

Martin1887 commented Mar 23, 2024

ollpu commented Mar 23, 2024 • edited

Martin1887 commented Mar 23, 2024

Martin1887 commented Mar 23, 2024

ollpu commented Mar 25, 2024

notriddle commented Oct 19, 2023 •

edited

ollpu left a comment •

edited

Martin1887 commented Mar 11, 2024 •

edited

ollpu commented Mar 12, 2024 •

edited

Martin1887 commented Mar 13, 2024 •

edited

ollpu commented Mar 23, 2024 •

edited