Repository navigation
Conversation
ZogStriP
approved these changes
Sep 18, 2026
gschlager
force-pushed
the
fix-roundtrip-bugs
branch
3 times, most recently
from
September 21, 2026 15:41
a8930da to
c7a8cde
Compare
The newline in front of the closing fence belongs to the fence syntax, not to the code. `<pre><code>x\n</code></pre>` and `[code]x\n[/code]` both carry a final newline in their content, and CodeTag added one more, so the cooked block ended with a blank line. CodeTag now chomps one trailing newline before it writes the closing fence. Found by the CommonMark round-trip spec (70 examples).
The postprocessor collapsed runs of newlines and cleared whitespace-only lines over the whole document, code fences included. Two blank lines inside a code block became one, and a line of spaces inside it became empty. The postprocessor now splits the text at fence lines and cleans only the text between fenced blocks. A fence may be indented or follow a list marker on the same line, a backtick fence cannot have a backtick after its run, and a fence closes only on a run of the same character that is at least as long. The newline that ends a closing fence counts towards the blank lines after the block. Text without any fence run takes the old path with no line loop. The structure round-trip generator now puts blank lines and lines of spaces into code blocks, and the CommonMark round-trip spec lost the five examples that this defect kept on its known-bug list.
Inline code was always written between single backticks. A backtick in the content ended the span early, and content that started with a backtick, or with a space on both sides, came out changed. The delimiter is now one backtick longer than the longest backtick run in the content, and the content gets a space of padding on each side when it starts or ends with a backtick, or when it starts and ends with a space (CommonMark strips one space from each end of such a span, the padding is what gets stripped). The structure round-trip generator now puts backticks and edge spaces into inline code, and the CommonMark round-trip spec lost five known bugs.
`AST::Image` had no place for the alt text, so the HTML, BBCode and TextFormatter parsers dropped it and ImageTag always wrote ``. The node has an `alt:` keyword now, the three parsers fill it (an empty HTML alt attribute counts as none), and ImageTag writes ``. The alt text goes through the renderer as a Text node under the Image, so it is escaped like a link label in Markdown and like an attribute in html_mode. Found by the CommonMark round-trip spec (21 examples).
A `)` in an href ended the Markdown link destination early, and the rest of the URL showed up as text after the link. UrlTag now writes a backslash in front of every parenthesis in the destination. Balanced pairs would not need it, but a uniform rule is simpler than a balance check, and the escaped form reads back the same. Found by the CommonMark round-trip spec (4 examples).
`### foo ###` cooks to a heading that says "foo": a run of # after whitespace at the end of an ATX heading is the optional closing sequence. The escaper only knows about # at the start of a line, so HeadingTag now puts a backslash in front of such a run. Found by the CommonMark round-trip spec (example 76).
`- ---` is one run of dashes and spaces, which CommonMark reads as a thematic break, so a rule at the start of a list item ended the list. `- * * *` is a list item that holds a thematic break (CommonMark example 61). HorizontalRuleTag now uses stars under a ListItem. Found by the CommonMark round-trip spec (example 61).
The escaper passes a lone `!` through, because it cannot see the node after it. When that node is a link, `` is an image. The renderer sees both sides where it joins the outputs of siblings, so the check moved there, next to the emphasis-boundary rule: a buffer that ends with an unescaped `!` gets a backslash when the next part starts with `[`. Found by the CommonMark round-trip spec (example 593).
Two lists of the same kind that follow each other rendered as one list with a blank line between the items, and CommonMark read that as one loose list. The renderer now puts an HTML comment between blank lines (`<!---->`) between them where it joins the outputs of siblings. A change of list kind needs no separator, CommonMark starts a new list on its own. A list without items renders to nothing now, like the other containers do. Its blank lines alone would have counted as a list for the separator. The structure round-trip generator may put two lists next to each other again. Found by the CommonMark round-trip spec (examples 301, 302 and 308).
…nctuation `item*\#*` cooks to the literal text `item*#*`. CommonMark's flanking rules do not let a `*` open when a word character stands in front of it and punctuation follows it, and the same holds the other way round for closing. After escaping, every tricky character at the edge of an emphasis starts with a backslash, which is punctuation, so this hit about one seed in seven in the structure round-trip. The renderer now puts the `<!---->` boundary comment between the word and the delimiter run where it joins sibling outputs. The comment is punctuation, so the run can open or close again. A `_` run never opens or closes next to a word character, so it gets the comment whatever follows. cmark-gfm does not count a tilde as punctuation next to an emphasis delimiter, so strikethrough right next to such an emphasis gets the comment too. The structure round-trip generator may put tricky tokens at the edges of emphasis again.
ListTag dropped the blank lines around a list whenever a List or a ListItem was anywhere in the ancestor chain. Text after the nested list then sat at the indentation of the last nested item, and CommonMark read it as a continuation line of that item: `[list][*]a[list=1][*]b[/list]c[/list]` put `c` into the item `b`. The same happened through a quote or an aligned block inside an item. Only the direct parent decides now. A list right inside a list item starts on the line after the item text, so the list stays tight, and gets one blank line after it. Everywhere else the list is a block with blank lines on both sides. The structure round-trip generator may put blocks after a nested list again.
`BULLET_LIST` and `ATX_HEADING` accept the end of the line after the marker, `ORDERED_LIST` asked for a space or a tab. A line that was just `1.` therefore passed through and cooked to an empty `<ol><li></li></ol>`. The lookahead now accepts the end of the line too. The structure round-trip generator uses the bare `1.` token again. Found by that generator (first at seed 30).
A bare URL renders as its plain href, so Discourse can link and onebox it. A Markdown parser links a plain URL only when whitespace, or the start of the line, stands in front of it and nothing sticks to its end, so `see<a href="u">u</a>` came out as one word of text. UrlTag now looks at the siblings of the link. When the text next to it has no whitespace at that edge, the URL is written as a Markdown link with the URL as its text, `[href](href)`, which links everywhere, also for a relative href. Such a URL could not onebox anyway. RenderingInterface gained `previous_sibling` and `next_sibling`, which look at the children of the element on top of the parent chain. An element without a tag, the Document above all, is now on that chain for its children, the same way Tag::PASSTHROUGH puts it there. Found by the CommonMark round-trip spec (examples 480 and 481). Its known-bug list is empty now.
…ples Mutation testing on the fixes found gaps: the byte ranges of the word and punctuation checks had no probes at their ends, the List checks had no subclass example, inline code with a backtick only at the start or a space only at the end was untested, and the fence handling of the postprocessor had no example with an indented or space-padded closing fence, nor a tilde fence with a backtick in its info string.
- HeadingTag escaped a trailing run of # only when the content ended with the #. A closing sequence may be followed by spaces, so the guard was wrong as well as untestable; the sub runs always now. - UrlTag escaped parentheses behind an include? guard. gsub returns a copy also without a match, so the guard only saved that copy. - ImageTag rendered a missing alt through an early return. The escapers turn nil into an empty string anyway. - CodeTag's padding check compared strip/lstrip/rstrip results that are all the same for content that starts and ends with a space; it counts the spaces instead. The padding is nil instead of "". - Renderer#join is one if/elsif chain without an early return, the `!=` on the byte before a `!` became `unless ==`, and the two run scans use byteindex/byterindex with a pattern per delimiter instead of index loops whose start value had an equivalent mutation. - Postprocessor#closes_fence? compares squeezed runs, and the fence guard in #call moved into #clean_document, which is on the mutant ignore list with the reason: both branches produce the same text.
The fixes before this commit made rendering 36 to 41 percent slower per post (bench/corpus_bench.rb render_only), while parsing and the escaper stayed the same. Three things cost, measured one at a time on the `simple` report: - The join between siblings ran several method calls and two `is_a?` checks per child. The byte classes (word, punctuation, delimiter) now come from a 256-byte table read with getbyte, so the common case is decided from the two edge bytes with no method call, and the list separator moved into ListTag, which asks for its previous sibling only when a list is rendered. - The Document sat on the parent chain, so every chain walk was one step longer, and a text node walks the chain several times. The Document is now the context's root element instead: not on the chain, but available to previous_sibling and next_sibling when the chain is empty. - A text node walked the chain four times (Code, Url, Email, Image). It walks once now and collects both answers. With this the `simple`, `list`, `table` and `url` reports are back at the level of main.
The postprocessor's line loop made a document with a code block about 30 percent slower to convert (`code` report), and an anchored pattern searched over the whole text costs about 14 microseconds per post, because the engine tries it at every byte. The scan now finds candidate runs of three backticks or tildes with a plain string search and checks only the line around a candidate with the anchored patterns, for the opening and the closing fence alike. The two gsubs in `clean` run only when a cheap check says there is something to replace, which also makes documents without a fence cheaper than before. UrlTag escapes the parentheses of a destination only when it has any; gsub would copy every href otherwise. The guards are equivalent for mutant, so they are on the ignore list with their reasons: the clean_document gate, the two guards in clean (scoped to the local `prose`) and the parentheses check (scoped to `href.match?`).
The buffer ends with `!`, so a negative index wraps around to a byte that is not a backslash and the loop stops on its own. A flag that flips per backslash replaces the counter; mutation testing showed the guard and the counter changes as equivalent.
A bare URL glued to text is written as a Markdown link with the URL as its text. The label was the raw href, so an href with a `]` in it ended the label early and the whole link cooked as literal text. The rendered text of the link is already escaped for a link label, so UrlTag uses that. A link without text, or with a label that renders to nothing, renders the href as a Text node the same way. The common paths do not change: a labelled link and a bare URL with whitespace around it take the same code as before. Only a glued link without any label text renders one extra Text node. The unit tests for the glued form used hrefs without Markdown characters, the structure round-trip generator never builds a bare URL, and the CommonMark spec has no such example. The new cooked-output spec goes through the HTML parser, because the BBCode option syntax cannot carry a `]`.
A backslash at the end of an href, or right before a `)` in it, escaped the parenthesis that closes the destination, so the link or image cooked as literal text. Now every backslash in a destination is doubled in UrlTag and ImageTag, the same way the parentheses get their escape. The structure round-trip generator gets three hrefs with a parenthesis, a `]` and a trailing backslash. The cooked href is percent-decoded before the comparison, commonmarker encodes `]` and `\` there.
…check The ignore pattern for the guard in UrlTag#markdown_destination was keyed on `href.match?` under an `if`. The scheme check in UrlTag#linkable? has the same shape, so mutant never mutated it, and a mutation that lets `javascript:` through would have gone unnoticed. The parameter of markdown_destination is now named `destination`, which no other `if … .match?` in lib uses, and the pattern is keyed on that name.
The examples for a glued link without text used an href with a `_`, which the escaper escapes in any context. A `]` is escaped only for text under a link, so these examples now use one: mutant showed that rendering the href outside the link's context passed them.
Reusing the rendered text as the label of a glued bare URL gives the same Markdown as rendering the href as text again; the branch saves a Text node and an escape, a quarter of the time of a glued link. ImageTag#markdown_destination's match? guard saves the copy that gsub makes without a match, the same as the one in UrlTag. Neither can be told apart through the public API, so both go on the ignore list.
gschlager
force-pushed
the
fix-roundtrip-bugs
branch
from
September 23, 2026 18:10
471a984 to
7cbe50b
Compare
The scan loop still used `/```|~~~/` while the gate in front of it already worked with include?. A string search runs as a byte search, a regex with an alternation walks the text with the engine. Two searches and the smaller offset win: about 2% on the code report, measured with alternating runs because a single pass is inside the noise of this machine. Picking the smaller of the two offsets only shows up in a document that holds both kinds of fence, so there are now examples for both orders.
A fenced document with nothing to clean has to come back unchanged, and a whitespace-only line next to a fence has to be cleared even when the document holds no run of three newlines. Both pass against the current code; they hold the ground for the guard that follows.
A document with a code block went through the whole fence scan even when none of the three cleanups had anything to do in it. The scan then put the same bytes into a new string: the opening line, the closing line and every segment around the blocks cost a slice, a match and a copy, for a result equal to the input. That is the common shape — a post with a fence and no run of three newlines and no line of spaces. The scan now asks first whether `clean` would change anything anywhere in the text and hands the text back as it is when it would not. The check is sound because a segment the scan passes to `clean` starts at the beginning of a line and ends at the end of one, so a line of a segment is a line of the document at the same place: a document with no run of three newlines and no whitespace-only line has none in any of its segments either. An instance that strips invisibles says yes without looking, that option is off by default. The closing-line check builds its slice from an offset and a length instead of a Range, which saves the Range object per candidate line. For the `code` benchmark document the postprocessor now allocates one object instead of thirteen — one below the four that main allocates, because the two gsub copies are gone as well. Over the ascii corpus it is 6.86 objects per post against 7.05. I checked the two implementations against each other on 600k random documents built from fence pieces, list markers, blank lines, lines of spaces and invisibles, with the option both on and off: no difference. The guard is a pure speed guard, so the mutations that make it fire less often are all equivalent; they are on the ignore list with their reason. The mutations that make it fire too often change the output and are killed by the specs.
The join between two sibling outputs ran up to four method calls per child (join, blocked_flanking?, blocking_neighbour?, punctuation?), and it ran for every adjacent pair in every document. All the rules except three look only at the byte in front of the join and the byte after it, so they now come out of a 2304-byte action table: nine rows of 256, one row per class of the byte in front. render_children reads the action itself and calls into the renderer only when there is something to do. On the bench inputs that means no call at all: `simple` went from 17 join calls per conversion to 0, `quote_nested` from 16, `nested` and `url` from 9. The five bytes the rules name (`!`, `[`, `` ` ``, `*`, `_`, `~`) get a byte class of their own, so the table can tell them apart without a second comparison. They stay above PUNCTUATION in the class order, which keeps the punctuation check a single `>=`. render_text walks the parent chain itself now instead of collecting the two answers in an array: in html_mode the chain does not matter at all, and under a Code ancestor nothing after it does, so the walk can stop early. That drops an array per text node — 149 of them per 20 posts, the third-largest allocation site in the renderer. Allocations per post (bench/alloc_profile.rb): render_only 81 -> 73 (ascii) and 95 -> 87 (multi), fresh 210 -> 203 and 234 -> 226. The old and the new join agree for all 10,485,760 combinations of the two edge bytes with ten buffer shapes and eight part shapes around them.
The renderer packs the byte classes and the join actions into two strings while the class body runs. Mutation testing cannot reach the rules behind them: a mutant redefines a method, and by then the tables are already built, so every rule mutation would survive as equivalent. These examples take that job instead. They walk all 256 byte classes and all 9 * 256 join actions and compare each entry with the rules written out longhand in the spec — the same if/elsif chain, spelled in characters rather than in class numbers. A change to a rule, to a byte class, or to the order of the class numbers now fails a spec. The order matters on its own: the renderer asks whether a byte is punctuation with a single `>=` against the class table, so every class CommonMark counts as punctuation has to sit at or above PUNCTUATION. That is its own example.
…loop Three things on the escaper's hot path, none of them changing output: - `escape_block_level` and its helpers answered with a `[line, skip_inline]` pair, so every line allocated an Array. They hand the content back unchanged now when the line starts no block construct, and `escape_line` compares identity. - The `when DIGIT_0..DIGIT_9` arm is not a literal range, so Ruby built a new Range for every line that fell through to it. It is a frozen constant now. - The inline loop called `dispatch_inline_byte` for every byte, and an ordinary byte walked all eleven `when` arms before it landed in the `else`. A 256-entry table says now whether a byte needs the dispatch, and the loop copies the byte itself otherwise. That leaves the dispatch `else` with multi-byte characters only, so `escape_regular_char` became `copy_multibyte_char`. Its `min` clamp is gone with it: `byteslice` copies only what is left anyway, and a position past the end ends the loop. Allocations per `escape` call on the two isolated benchmark inputs: escape_plain 2 -> 0, escape_mixed 5 -> 3. Traced calls per escape_mixed call: 51690 -> 18891. Over the corpus, `bench/alloc_profile.rb render_only` goes from 81 to 74 objects per post (ascii) and from 95 to 87 (multi). I ran 1.6 million escapes of generated strings through the four escaper configurations before and after the change: same bytes, same encodings. The `if byte < 128` entry in mutant.yml's ignore patterns went with `escape_regular_char`; no `if byte < ...` is left in lib.
Every emphasis in the corpus had a space on both side of it and content that ended on a word. CommonMark's flanking rules only get interesting when a delimiter sits directly against a word and the content ends in punctuation, so the corpus never reached the code that handles it: the two run probes in the renderer were called zero times over 200 posts, while the spec suite calls them 266 times. All four post builders now also produce the glued form. The probes go from 0 to 200 calls per 50 posts, for BBCode, MediaWiki, HTML and TextFormatter, on both the ascii and the multibyte words. This matters because a post with glued emphasis costs about 25% more than one without, and none of that showed up in any benchmark. The published corpus numbers were measured on the easier corpus, so the docs page now says they do not compare with a run of the current one.
The published numbers were taken before the corpus gained glued emphasis and before the performance work in this branch, so they no longer describe anything that exists. Every table is re-measured on the same laptop, all five engines in one sitting, with the documented warmup and measure settings. The engine versions moved too: ruby 3.3.12, 3.4.10 and 4.0.7, jruby 10.1.1.0 on OpenJDK 27-ea+35, and truffleruby 40.0.0. The TruffleRuby build on this machine is the Oracle GraalVM native one, not the Community build the page claimed, so the caveat about the two is the other way round now. Two things to know about the numbers: - `escape_plain` has no TruffleRuby entry any more. That engine reports 6.7M i/s for it, which is 149 ns for a 4,200 character string. The benchmark drops the return value and the escaper allocates nothing for plain text, so the call is compiled away instead of run. The same number comes out on main, so it is the engine and the benchmark, not a change in the escaper. - The corpus numbers are lower across the board because the corpus is harder now, not because anything got slower. The JRuby effect where a second corpus in the same process stays at half speed still reproduces: 9.7k posts/s in a fresh process, and 4.7k in two of three runs that converted the ASCII corpus first.
The page made the reader work through two tables and the warmup section before the practical question was answered. The numbers do point at clear advice, so it is now at the top in five lines: what to run a long migration on, what to run a short script on, which CRuby to take, when JRuby makes sense, and what multibyte input costs. Everything in it comes from the tables below, so there is nothing new to keep in sync beyond the numbers themselves.
The shell is wired for the default Ruby, so rv exports its GEM_HOME, GEM_PATH and RUBY_ROOT. TruffleRuby reads GEM_HOME and then installs into, and loads from, the default Ruby's gems instead of its own. `bundle exec` adds to that: it puts the default Ruby's gem bin directory in front of PATH, so a nested `ruby` is the default one again, whatever engine started the command. Nothing fails when this happens, which is the problem. The documented benchmark command for TruffleRuby ran CRuby and reported it under the TruffleRuby heading. The TruffleRuby column of the benchmark page was CRuby for a while because of it. bin/with-ruby drops those variables, puts the engine's bin directory first and points bundler at this project. `gem` and `bundle` go through the engine's own `ruby` rather than their bash launchers, which answer with the default Ruby's gem directory even when GEM_HOME says otherwise. Provisioning installs the bundle once per engine now. A single `bundle install` only ever served the default one, which is why the other engines had no gems to fall back on except the default Ruby's. The benchmark page uses the wrapper in its run instructions and says to check the engine line a run prints before trusting it.
gschlager
marked this pull request as draft
September 24, 2026 10:08
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes all 14 defects that the round-trip specs from #87 and #88 recorded. The known-bug list of the CommonMark round-trip is empty now, and the structure generator has no constructs left out any more. One commit per fix, the commit messages carry the details.
Code and lists:
<!---->between blank lines between them.* * *.1.on a line cooked to an empty list item, the escaper only knew the marker followed by a space.Inline:
<!---->boundary comment there, the same mechanism it already used for two emphasis runs that would merge. cmark-gfm also does not count a tilde as punctuation next to emphasis, so strikethrough next to such emphasis gets the comment too.!right in front of a link made it an image. It gets a backslash.)in a link destination broke the link. Parentheses in destinations are escaped.#in a heading was eaten as the closing sequence.[url](url), when the text next to it has no whitespace at that edge. That also works for a relative href, which an autolink would not. For thisRenderingInterfacegotprevious_siblingandnext_sibling. They look at the children of the element on top of the parent chain, or of the context's newrootelement when the chain is empty (the Document, which has no tag and stays off the chain).AST::Imagegot analt:keyword, the three parsers fill it, and it renders as.Performance: the first version of these fixes made rendering 36 to 41 percent slower per post (bench/corpus_bench.rb, render_only), which the escaper benchmark alone did not show. Two PERF commits take the work off the hot paths again: the sibling join decides the common case from a 256-byte class table, the list separator moved into ListTag, a text node walks the parent chain once instead of four times, the Document is the context's root instead of a chain element, the postprocessor finds fences with string searches and checks only the candidate lines, and the gsub copies in the postprocessor and UrlTag run only when there is something to replace. Measured back to back against main under the same machine state: end-to-end conversion of the corpus posts is within 1 to 2 percent of main, render_only is 4 to 5 percent slower, and the small bench.rb reports are 7 to 9 percent faster than main except
code, a document that is nothing but one code block, which is 12 percent slower because of the fence scan.The DEV commits come from mutation testing: more examples for the new join rules and the fence handling, and a few branches dropped that mutant showed to be equivalent.
bin/mutant run --since origin/mainis at 100% with one new ignore for the postprocessor's fence guard, with the reason in mutant.yml.The upgrade notes for 0.4.3 list the output changes. I ran the escaper benchmarks before and after the
ORDERED_LISTchange, escape_plain and escape_mixed are unchanged within the run-to-run noise.One thing I noticed but left alone:
[url]https://example.com[/url]without an option produces a Url with no href, so it renders as plain text and the autolink handling above cannot apply to it. That is a BBCode parser question for another PR.