✨ NEW: stable heading anchors: shared slug module, presets, HTML ids, explicit-id priority#1158
Conversation
… explicit-id priority
Core changes for the heading-anchor overhaul (tests/CLI/docs follow):
- New `myst_parser.slugs` module: the single source of truth for slug
generation (github + gitlab presets, fixed unique-suffix algorithm),
pure/stdlib-only so alternative implementations can replicate it.
- Duplicate headings now suffix as `x, x-1, x-2` (GitHub behaviour, as
documented), not `x, x-1, x-1-2`.
- Anchor slugs are additionally emitted as secondary HTML ids, so
GitHub-style fragments actually exist in published output; the
docutils id stays first, keeping all previously published fragments
and resolved references unchanged. Escape hatch:
`myst_heading_anchors_html_ids = False`.
- `myst_heading_slug_func` accepts preset names ("github", "gitlab") as
the canonical, declarative form; dotted import paths remain as a
legacy escape hatch, and dotless unknown strings now error clearly.
- New `PrioritiseExplicitIds` transform: an explicitly named target
(`(name)=`) propagated onto a section becomes its primary id, so
toc/permalinks/objects.inv use the stable explicit id; the implicit
id is kept as a secondary anchor.
- `[](#target)` now falls back to implicit docutils names/ids in the
same document (e.g. headings beyond the `heading_anchors` depth),
and cross-doc `[](./doc.md#id)` accepts section ids and std-domain
labels that demonstrably exist, instead of warning on working links.
All regress fixture changes reviewed: explicit-first id reordering and
added slug ids only.
- tests/fixtures/slugs.json: language-agnostic slugify/uniqueness corpus (expected values generated from the implementation), consumed by the new tests/test_slugs.py - myst-anchors CLI: gains --slug-func github|gitlab and now shares the renderer's slugify (previously drifted: the anchors-plugin default strips whitespace) - tests/test_anchors.py: CLI duplicate/preset cases + a renderer/CLI cross-check over a 13-heading corpus (the single-source-of-truth regression guard) - config/renderer tests: heading_anchors_html_ids escape hatch, unknown preset error, cross-doc link to a std label id resolving silently - docs: presets as the primary heading_slug_func form, secondary HTML ids + escape hatch, fixed duplicate suffixing, CLI example
- Slug ids are now emitted by a dedicated transform (`AddSlugIds`, priority 700) that runs after ALL other id assignment, instead of at parse time. This fixes two silent-change classes: a slug can no longer claim an id that docutils/the user would assign to a later element (previously renaming published and even explicitly chosen ids), and sphinx's SortIds can no longer rotate the slug into being a section's primary id for auto-`idN` headings (previously changing toc/searchindex anchors for non-ASCII/digit titles). - The local implicit-anchor fallback for `[](#target)` is now docutils-mode-only at resolve time; under sphinx it is recorded on the pending_xref (`reflocalid`) and used only after project-wide and intersphinx resolution both fail, so links that previously resolved to another document's explicit target keep doing so. - Cross-doc `[](./doc.md#name)` referencing a std label by *name* now resolves to the label's actual anchor id, instead of silently emitting the (broken) name as a fragment. - The documented `#`-destination resolution order now matches actual behaviour, with the new last-resort local step; the explicit-target primary-id change is documented with a versionchanged note. Each finding has a regression test; plus corpus edge rows and the doubled "(default:" in the docutils CLI help fixed.
for more information, see https://pre-commit.ci
chrisjsewell
left a comment
There was a problem hiding this comment.
Review: stable heading anchors
I reviewed the full diff and exercised it locally (Sphinx 9.0.4 / docutils 0.22.4): the whole suite passes (1156 passed, 11 skipped), and all 21 CI checks are green across the py3.11–3.14 × Sphinx 8/9 × {ubuntu, windows} matrix. This is a well-architected, carefully-tested change and I'd be comfortable merging it after considering the notes below (none are blocking).
Overview
Consolidates heading-anchor slugging into one stdlib-only module (slugs.py), fixes the duplicate-suffix contract (x, x-1, x-2), emits slugs as secondary HTML ids via a late AddSlugIds transform, promotes explicit (name)= targets to the primary id via PrioritiseExplicitIds, and stops warning on #-links that actually resolve. Closes 8 issues.
What's strong
- Single source of truth + language-agnostic corpus.
slugs.pybeing pure/re-only withtests/fixtures/slugs.json(guarded bytest_slugs_module_has_no_third_party_deps) is a genuinely good design for cross-implementation parity, and the renderer/CLI cross-check (test_anchor_slugs_match_renderer) prevents the exact drift this replaces. - Transform-ordering discipline. Doing id emission at priority 700 (after all id assignment) rather than at parse time is the right call — the
test_slug_id_cannot_steal_later_idsandtest_slug_id_stays_secondary_under_sortidsregressions capture the two subtle failure modes precisely. I verified the id-stealing guard (slug not in self.document.ids) behaves correctly against both a later section's docutils id and a user's explicit target. - Resolution precedence.
test_project_resolution_beats_local_implicitlocks down the important invariant that the new local fallback is genuinely last-resort under Sphinx (recorded asreflocalid, used only after project-wide + intersphinx fail). The docs resolution-order list now matches the code.
Correctness — verified, no defects found
- Slug algorithms match references: github duplicate suffixing, and gitlab (
a---b→a-b,2.0→anchor-20,---→-, digit-only prefixing) all check out againstGitlab::Utils::Markdown#string_to_anchor. unique_slugloop is bounded bylen(existing);compute_unique_slug'sIterable→Containertype fix is correct (it usesin).- I specifically probed the adversarial
PrioritiseExplicitIdscase — an explicit target whose id starts withid(e.g.(idea)=), whichSortIdswould rotate to the back — and it resolves correctly (ids=[idea, my-heading]). See the one robustness note below.
One robustness note (worth addressing)
PrioritiseExplicitIdssharesdefault_priority = 261with Sphinx'sSortIds. It currently works only because docutils breaks equal-priority ties by insertion order and MyST's parser transforms are appended after the Sphinx/reader transforms, soPrioritiseExplicitIdshappens to apply last and gets the final say. That's a latent fragility: a future Sphinx change toSortIds' priority or registration point could silently flip the order and letSortIdsrotate anid-prefixed explicit id back off the front. Bumping todefault_priority = 262(strictly after bothPropagateTargets260 andSortIds261) makes the intended ordering explicit and order-independent, with no effect onAddSlugIds(700) /ResolveAnchorIds(879). Cheap insurance.
Performance
AddSlugIdswalks the whole doctree once (fine); it iterates all nodes incl. text nodes — filtering tonodes.Element/nodes.section(plus the document node) would trim it, but the impact is negligible.- The cross-doc fallback does linear scans per unresolved ref:
any(sect_id == ref_id …)over the target doc's headings, then_std_label_id_in_docover alllabels+anonlabels. Only hit on the miss path, so acceptable, but a project with many broken-ish[](./doc.md#id)links pays O(refs × labels). If it ever shows up, a per-docname label index built once would remove it.
Durability / compatibility
- The headline behavior change is #1073: explicit target → primary id, which changes
objects.invanchors and permalinks for projects that put(name)=before headings. It's the intended fix, the implicit id is retained as a secondary anchor, and it's documented with aversionchanged— well handled, but it's the one thing downstream projects deep-linking viaobjects.invcould notice. check_heading_slug_funclegacy import path still only catchesImportError—myst_heading_slug_func="os.nope"raises a rawAttributeError, and".foo"raisesValueError, both bypassing the friendlyTypeError. Pre-existing, but since the PR rewrites this block, widening toexcept (ImportError, AttributeError, ValueError)would be a tidy freebie.
Testing — strong, a couple of small gaps
- Coverage is thorough. Minor additions worth considering: the
myst_heading_anchors_html_ids=Falseescape hatch is only tested under docutils (not Sphinx); the whitespace-slug skip inAddSlugIdsis only asserted implicitly via themyst-configfixture;_std_label_id_in_doc's name-vs-id precedence is untested (very edge).
Minor / nits
slugs.pywrites the CJK range as literal ideographs[一-鿿]where the code it replaces used[一-鿿]. The escape form is more greppable and encoding-robust in review/editors — consider keeping it. (Separately, Python's Unicode\walready matches CJK, so the explicit range may be redundant — but that's a pre-existing question, not for this PR.)run()'sreflocalidfallback buildsnodes.reference()withoutinternal=True, unlikemake_refnode; therefidstill resolves same-document, but setting it would keep theinternalclass consistent with other resolved refs.default_slugifyis soft-deprecated (docstring only). Fine, but if you want tooling to flag it, aDeprecationWarningwould help. Trivial: the docstring says.. deprecated:: 5.2while the docs use5.2.0.
Security
No concerns. Slug regexes are linear (no ReDoS), unique_slug is bounded, and the only import_module path is config-driven (trusted) and now bypassed entirely by the presets.
Nice work — the adversarial/regression pass clearly paid off, and the third commit's fixes address the genuinely tricky id-lifecycle issues.
🤖 Generated with Claude Code
Generated by Claude Code
- PrioritiseExplicitIds priority 261 -> 262: strictly after sphinx SortIds, independent of transform insertion order - AddSlugIds iterates elements only - legacy heading_slug_func import paths: AttributeError/ValueError now also produce the friendly TypeError - reflocalid fallback reference marked internal - CJK range in the github regex written in escaped form - tests: escape hatch under sphinx; whitespace-slug skip asserted directly
For a heading like `# One` the slug equals the docutils id, so asserting the slug is absent from `ids` was wrong.
chrisjsewell
left a comment
There was a problem hiding this comment.
Follow-up: review notes addressed ✅
f7e91de (+ test fixup 1b920da) addresses essentially every actionable point from my earlier review. I verified each against the diff and re-ran the suite (1158 passed, 11 skipped — the two added tests, no regressions).
| Finding | Fix | |
|---|---|---|
PrioritiseExplicitIds shared priority 261 with Sphinx SortIds (main robustness note) |
default_priority = 262, strictly after both — no longer insertion-order dependent |
✅ |
AddSlugIds iterated all nodes incl. text (perf) |
findall(...)(nodes.Element), redundant isinstance dropped |
✅ |
check_heading_slug_func only caught ImportError (durability) |
except (ImportError, AttributeError, ValueError) — "os.nope" / ".foo" now give the friendly TypeError |
✅ |
reflocalid fallback ref missing internal=True |
newnode["internal"] = True |
✅ |
| Literal CJK regex chars (style) | reverted to escaped 一-鿿 |
✅ |
Deprecation 5.2 → 5.2.0 (trivial) |
fixed | ✅ |
| Escape hatch untested under Sphinx | new test_heading_anchors_html_ids_disabled_sphinx |
✅ |
| Whitespace-slug skip only implicit | new test_whitespace_slug_not_emitted_as_id |
✅ |
Left as-is, appropriately — the points I'd flagged as optional: the cross-doc _std_label_id_in_doc linear scan (miss-path only), its name/id-precedence edge test, and a runtime DeprecationWarning on default_slugify (would be noisy). The #1073 objects.inv change was an awareness flag, not an action item.
Nice touch that the escape-hatch Sphinx test got the 1b920da correction (asserting "no extra id" rather than slug absence, since for # One the slug equals the docutils id) — the test was clearly exercised rather than assumed. The one genuine latent fragility is now explicit and the two coverage gaps are closed; nothing outstanding from my side. 👍
🤖 Generated with Claude Code
Generated by Claude Code
Fixes #878, fixes #985, fixes #908, fixes #527, fixes #968, fixes #1073, fixes #885, fixes #828.
Overhauls heading-anchor slugs so
myst_heading_anchorsbehaves as documented ("GitHub-style slugs"), with one shared implementation, anchors that actually exist in published HTML, no false warnings for working links, and named slugger presets. Supersedes #1019 (see below).What changed
One slug implementation (
myst_parser/slugs.py, new): pure/stdlib-only functions (github_slugify,gitlab_slugify,unique_slug,SLUG_PRESETS) shared by the renderer and themyst-anchorsCLI — previously two parallel copies that drifted. The unit-test corpus is a language-agnostic data file (tests/fixtures/slugs.json), so alternative implementations can consume it verbatim.#878 — duplicate suffixing fixed: four identical
# Headings now produceheading, heading-1, heading-2, heading-3(the documented GitHub behaviour), notheading, heading-1, heading-1-2, heading-1-2-3. A renderer/CLI cross-check test over a 13-heading corpus guards against future drift.#908/#527/#968 — slugs become real HTML anchors: the GitHub-style slug (
ubuntu-2004,заголовок,20) is now additionally emitted as a secondary id, so the documented anchor exists in published output. Crucially this is done by a dedicated transform (AddSlugIds, priority 700) that runs after all other id assignment, so: the docutils id stays the section's primary id (tocs, permalinks,objects.inv, searchindex are byte-identical); a slug can never claim an id that docutils or the user would assign to another element; and sphinx'sSortIdscannot rotate the slug into primary position for auto-idNheadings. Escape hatch:myst_heading_anchors_html_ids = Falserestores lookup-only behaviour. Slugs from customslug_funcs containing whitespace (invalid in HTML ids) are not emitted.#1073 — explicit targets win: a new
PrioritiseExplicitIdstransform (priority 261, directly after docutils'PropagateTargets) makes an explicit(name)=target propagated onto a section its primary id, so sidebars, permalinks andobjects.invuse the stable explicit id. The implicit heading id is kept as a secondary anchor, so previously published fragments keep working.{#custom}attrs ids already behaved correctly and are unchanged.#985/#885 — no more warnings for working links:
[](#target): implicit docutils names/ids (e.g. headings beyond theheading_anchorsdepth) now resolve instead of warning — in docutils mode directly; under sphinx only as a last resort after project-wide and intersphinx resolution both fail, so links that previously resolved to another document's explicit target keep doing so (regression-tested).[](./doc.md#id): ids that demonstrably exist in the target document (section ids, std-domain label ids) resolve silently; a std label referenced by name now resolves to its actual anchor id (previously: warning + broken fragment).#828 — named presets:
myst_heading_slug_func = "github"(the default algorithm) or"gitlab"are now the canonical, declarative configuration form; dotted import paths remain as a legacy escape hatch, and unknown dotless strings get a clear error naming the presets (previously an uncaughtValueError). Themyst-anchorsCLI gains--slug-func github|gitlab. Note: the gitlab preset mirrorsGitlab::Utils::Markdown#string_to_anchor(strip, lowercase, remove non-word, squeeze hyphens,anchor-prefix for digit-only) — verified against the algorithm source statically; a sanity check against a live GitLab render before release would be prudent.Compatibility
The only behaviour changes for previously-building projects are: (1) third-and-later duplicate slugs (
x-1-2→x-2; the old output contradicted the documented contract — bugfix); (2) additional secondary anchors appearing (purely additive); (3) the explicit-target primary-id change above (old fragments keep working via the retained implicit id; called out in the docs with aversionchanged); (4) links that previously warned now resolving when the anchor exists. Everything else was verified byte-identical old-vs-new, including: all published fragments and resolved hrefs, toc/searchindex anchors (incl. non-ASCII/digit headings under sphinxSortIds),objects.invfor projects without explicit targets, and cross-document resolution precedence. epub and latex builds verified with non-ASCII/digit-leading secondary ids.The documented
#-destination resolution order indocs/syntax/cross-referencing.mdhas been corrected to match actual behaviour (the old text claimed a local-implicit step that never existed) with the new last-resort step appended.Review
The branch went through a full adversarial + behavior-regression review (old-vs-new comparison on real docutils and sphinx builds) plus a mechanical correctness/testing/performance pass; the third commit fixes everything found, each with a regression test — notably the id-stealing,
SortIdspromotion, and resolution-precedence issues that the initial implementation had.Suite: 1156 passed locally; own docs build clean.
Suggested follow-ups (out of scope)
mdit-py-plugins: drop.strip()in the anchors-plugin slugify to converge fully with the renderer.heading_slug_func, without a second callable-based config option).