fix(fmt): update markup_fmt to fix quadratic inline CSS formatting#34663
Conversation
Deno Individual Contributor License AgreementAll contributors have signed the CLA. Thank you! This is an automated message from CLA Assistant |
13a3e1a to
8e3aa49
Compare
`deno fmt` did not terminate on large HTML files containing many inline `style="..."` attributes. markup_fmt 0.27.0's `format_style_attr` prepended a whitespace-masked copy of the entire preceding document to every inline style before handing it to the CSS formatter, making formatting O(n^2) in the number of style attributes. markup_fmt 0.27.2 (g-plane/markup_fmt#223) removed that prefix and instead computes source line/column lazily only on error, restoring linear time. Bumping the pin picks up the fix. Adds a fmt spec test that formats an HTML file with many inline style attributes and asserts it completes. Closes denoland#30982
8e3aa49 to
91e0dfb
Compare
markup_fmt 0.27.2 requires the external formatter closure to return an error type that converts into anyhow::Error. The format_embedded_html closure still returned Infallible, which no longer satisfies the bound and broke the debug build. Use deno_core::anyhow::Error to match the other call site.
|
The failing The test asserts the syntax-error message includes correct line/column info: With 0.27.2 the columns become // before (correct):
ControlFlow::Break((line, offset)) => (line, pos - offset + 1),
// after (always 0):
ControlFlow::Break((line, offset)) => (line, pos - offset.max(pos)),On the I opened an upstream fix with a regression test: g-plane/markup_fmt#226. Once that's released (e.g. 0.27.3), this PR just needs to bump the pin to it and CI goes green. In the meantime a temporary |
|
Thanks for digging into this and finding the exact cause, that's a much better diagnosis than I had. Makes sense that the I've pinned |
…gression markup_fmt 0.27.2 introduced a regression in helpers::pos_to_line_col that reports column 0 (g-plane/markup_fmt#226). Pin to the fix commit via [patch.crates-io] to unblock CI; revert to a 0.27.3 version bump once released.
002b89a to
086fa95
Compare
markup_fmt 0.27.3 includes the upstream fix for the pos_to_line_col column-0 regression (g-plane/markup_fmt#226, now merged and released), so the temporary [patch.crates-io] git pin is no longer needed.
Signed-off-by: Bartek Iwańczuk <[email protected]>
What
Bumps the pinned
markup_fmtdependency from=0.27.0to=0.27.2(
Cargo.tomlandCargo.lock), plus a one-line type fix incli/tools/fmt.rsthat the new version requires.
Why
deno fmtdid not terminate on large HTML files with many inlinestyle="..."attributes (#30982; a 10 MB file ran for 15+ minutes with no output).
The cause is in
markup_fmt0.27.0.FormatCtx::format_style_attr(
src/ctx.rs) prepended a whitespace-masked copy of the entire precedingdocument (
self.source[0..start]) to the few bytes of inline CSS before handingit to the external CSS formatter (malva). Since
startgrows linearly down thefile, each of the N style attributes does O(start) work, which is O(n^2) overall.
That prefix only existed to keep byte offsets aligned for error reporting, and
the style-attr result is trimmed and uses
print_width = u16::MAX, so the prefixnever affects the output.
markup_fmt0.27.2 (g-plane/markup_fmt#223, merged upstream) drops the prefixand instead computes source line/column lazily, only on error, via
pos_to_line_col. That makes formatting linear again. The fix is alreadyreleased upstream, so this PR advances Deno's pin to pick it up.
How
Cargo.toml: changemarkup_fmt = "=0.27.0"to"=0.27.2".Cargo.lock: regenerated withcargo update -p markup_fmt --precise 0.27.2.0.27.2 also pulls in
memchr 2.7.5to2.8.1and addstiny_pretty 0.4.2;both are markup_fmt's own declared deps.
cli/tools/fmt.rs: change theformat_embedded_htmlclosure error type fromstd::convert::Infallibletodeno_core::anyhow::Error(see below).Source change required by 0.27.2
0.27.2 changed
format_textfromResult<_, FormatError<E>>to a concreteResult<_, FormatError>, where the external formatter closure must return anerror type that converts into
anyhow::Error. Deno has two call sites:format_html(line 451) already returnsAnyError(
deno_core::error::AnyError = anyhow::Error), so it needed no change.format_embedded_html(line 750) returnedOk::<_, std::convert::Infallible>,which no longer satisfies the bound. I changed it to
Ok::<_, deno_core::anyhow::Error>to match the other site.My first pass only checked the
format_htmlclosure and missed this second one,so the initial commit broke the debug build. CI caught it at
fmt.rs:751; thisis now fixed and
cargo check -p denocompiles clean.Verification / benchmark
I reproduced this end to end against Deno's exact pinned crate versions (stock
0.27.0 vs 0.27.2 via
[patch.crates-io]), formatting HTML with N<div style="...">lines:Stock is quadratic; the bumped version is linear (about 113x faster at N=8000),
and the formatted output is byte-identical.
Checklist (PR template)
fix(fmt): ...).Closes #30982)../x fmt:cargo fmt --checkpasses oncli/tools/fmt.rs(edition 2024),and
dprint check Cargo.tomlpasses../x lint:cargo check -p denocompiles clean after the closure fix.tests/specs/fmt/html_many_inline_styles, a fmt spec thatruns
fmt --checkon an HTML file with 150 inlinestyle="..."divs andasserts it completes. It passes in well under a second against a local
build; on 0.27.0 the same input does not terminate.
AI usage disclosure
This PR was prepared with help from an AI coding agent. I reviewed and verified
the diagnosis, the upstream root cause (g-plane/markup_fmt#223), the version
bump, the closure type fix, and the benchmark locally.