馃悰 Preserve matched target identifier for case-sensitive cross-references - #2989
馃悰 Preserve matched target identifier for case-sensitive cross-references#2989coretl wants to merge 5 commits into
Conversation
resolveReferenceLinksTransform looks up a link target by its verbatim
identifier first, then falls back to the normalized (lowercased) form:
const target = opts.state.getTarget(identifier) ?? opts.state.getTarget(reference?.identifier);
but then unconditionally discards which one matched:
xref.identifier = reference.identifier; // always the lowercased form
So when two case-distinct targets exist (e.g. Python API objects
`sample.Match` py:class and `sample.match` py:function registered by a
plugin), a link to `#sample.Match` correctly *finds* the class target and
then silently re-points at the function. Same pattern in
resolveUnlinkedCitations.
Fix: keep the identifier of the target that actually matched, falling back
to the normalized identifier as before. Normal (already-normalized) prose
labels are unaffected: for them the matched identifier and the normalized
identifier are identical.
Sibling of the intersphinx-side case bug jupyter-book#1758.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WsPd4nPWGBXmubv4ypuWs7
馃 Changeset detectedLatest commit: bb6f7dd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
choldgraf
left a comment
There was a problem hiding this comment.
This seems good to me - thanks for adding a simple test for this as well. Given that it brings this behavior in line with another part of the ref transformer, it feels safe to merge to me. IMO this is also closer to what one would expect given that we haven't documented labels as case insensitive
|
@fwkoch can you take a look? |
| const xref = link as unknown as CrossReference; | ||
| xref.type = 'crossReference'; | ||
| xref.identifier = reference.identifier; | ||
| // Prefer the identifier of the target that actually matched: getTarget |
There was a problem hiding this comment.
Our logic is pretty yucky here. Wondering if we should update the getTarget API a bit to do case-insensitive matching + return of the found target label. That way the logic can become:
target, reference.identifier = opts.state.getTarget(identifier);or similar.
What do you think @fwkoch?
There was a problem hiding this comment.
I do agree with this as well 馃檪 is the author is up for a little clean up I'm happy to iterate via this PR.
I think that we want is case sensitive matching though, right?
There was a problem hiding this comment.
I've had a go at folding the normalization into getTarget. Changing the signature of getTarget seemed too big a change for me to make without understanding the codebase better. Is that any cleaner?
3127234 to
df43127
Compare
Move the case-sensitive-then-normalized lookup that both resolveReferenceLinksTransform and resolveUnlinkedCitations were doing manually (getTarget(a) ?? getTarget(b)) into ReferenceState.getTarget itself, per review feedback from stefanv and choldgraf on the PR. Callers now make a single getTarget call, and the two-step contract (case-sensitive match with a normalized fallback, not case-insensitive matching) is documented on the IReferenceStateResolver interface. Note this widens getTarget's contract for its other callers (site.ts, the MultiPageReferenceResolver delegate, container-number resolution, etc.), but those all pass already-normalized node identifiers, so the fallback is a no-op for them. The transforms keep the matched target's own identifier (target?.node.identifier) so a case-sensitive target isn't re-normalized away, falling back to the normalized label only for file-target-only resolution, as before. Adds direct unit tests on ReferenceState.getTarget for the verbatim match, normalized fallback, and case-distinct-sibling cases.
df43127 to
655f481
Compare
There was a problem hiding this comment.
Thanks for this @coretl! This is a subtle bug I agree we should fix. From my perspective, the point of normalization is two-fold: (1) making non-standard characters in identifier safe for reuse in other contexts and (2) allowing user-friendly coercion for minor mismatches rather than hard failures.
This PR addresses (2) in a nice way - it allows careful authors to set up robust case-sensitive xref/target pairs, but it still falls back to the "user-friendly" normailze-if-there-is-not-a-case-sensitive-match behavior.
However, it opens up (1) to more risk I think - if an author sets up matching xref/target pairs with non-standard characters, it won't be normalized at all and may cause issues in other contexts where these characters break the xrefs.
I'm wondering if your fix should only address case mismatch... That would cover your sample.Match example. (It's also worth pointing out - in other contexts - web, typst, tex - cross-references are all case-sensitive. So supporting case but not special characters could make sense.) If we are going this route, it would probably change the implementation a bit, where normalizeLabel (and probably createHtmlId) no longer includes toLowerCase() and we move the lower fallback to getTarget (so we never check against raw identifier; instead we check against normalized-but-not-lowered value then normalized-and-lowered value).
Does that track and make sense...? (Happy to discuss further if you think I'm missing the mark 馃槄)
|
Thanks for the reply @fwkoch. I'll try to answer it, but I work mainly in low level python, so a PR in TypeScript about front end concepts is stretching my knowledge somewhat! I leaned heavily on AI to try and explain the details to me, but I may have missed some important context. Were you suggesting that prose labels should also become case sensitive as @choldgraf suggested in #2988 (comment)? If so, then wouldn't that break the html anchors of every existing mystmd site that happened to use uppercase characters in the label text? Is there a use case for having case preserving anchors generated from prose references? The only reason I ask is that it doesn't actually seem required for this change, as long as the producer assigns a valid identifier, it is used without lowercasing (this PR stops the reference resolver forcing it to lowercase), which fixes the issue for any plugin which wants to make case sensitive references. More details (AI generated)Why this PR doesn't need prose labels to changeA preset The only real bug was resolution. The export side is already case-preserving. Why making prose labels case sensitive would break existing anchorsProse identifiers are lowercased at creation, by
Is there a use case for case-preserving prose anchors?There's a reasonable one: web, typst, and tex cross-references are all case sensitive, and Python's own docs serve |
|
I just pushed a few small improvements to this branch:
I feel like this is safe to merge - it seems like we don't want to change the HTML labels since that'd be a bigger user-facing shift, and since HTML is already getting its normalized labels via something other than the identifier, that behavior won't change. |
Fixes #2988.
The bug
resolveReferenceLinksTransformlooks up link targets by their verbatim identifier before falling back to the normalized (lowercased) form:...but then unconditionally overwrites the resulting cross-reference with the normalized identifier:
So when two case-distinct targets exist, a link to the exact-case one finds the right target and then silently re-points at its lowercase sibling. With a plugin directive registering
sample.Match(a class) andsample.match(a function) as targets, both[](#sample.Match)and[](#sample.match)end up as crossReferences tosample.match.resolveUnlinkedCitationshas the same pattern a few lines below.The fix
Keep the identifier of the target that actually matched, falling back to the normalized identifier as before:
Ordinary prose labels are unaffected: their stored identifier is the normalized identifier (
normalizeLabelat creation time), so the two expressions are identical for every target that exists today. Only targets registered with intentionally case-sensitive identifiers (which today can only come from plugins settingidentifierdirectly on nodes) change behaviour.Changes
packages/myst-transforms/src/enumerate.ts: use the matched target's identifier inresolveReferenceLinksTransformandresolveUnlinkedCitations.packages/myst-transforms/src/enumerate.spec.ts: regression test with two case-distinct targets; fails without the fix (both resolve tosample.match), passes with it. All myst-transforms tests pass.Independent CI reproduction
https://github.com/coretl/mystmd-repro-repo runs a full end-to-end reproducer (plugin + project + AST assertion) as a matrix: released 1.10.1 and upstream
mainasserting the bug, this branch asserting the fix: https://github.com/coretl/mystmd-repro-repo/actionsContext
This is the local-reference sibling of the intersphinx
$-anchor lowercasing bug (#1758, #2955), just fixed in continuous-foundation/intersphinx#6 (released as intersphinx 1.1.0). Together they make case-sensitive references correct end-to-end, which matters for Python API docs wherere.match(function) andre.Match(class) are different objects; see the discussion in #1259. We (Diamond Light Source / bluesky) hit both while migrating ophyd-async's API docs from Sphinx to MyST.AI Disclaimer
The code and this description were generated with AI and reviewed/tweaked by the author.