[bugfix] Look up a named attribute kind test in the attribute index on the self axis - #6703
Conversation
…n the self axis A named attribute kind test on the self axis, such as self::attribute(id), was always false on a persistent node. It was correct on in-memory nodes and correct in Saxon, so the same predicate gave different answers depending on whether the document had been stored. LocationStep.getSelf looked every non-wildcard test up in the element half of the structural index. self::attribute(id) therefore searched for an *element* named id, found nothing, and returned the empty sequence -- without ever consulting the node test. Instrumenting NameTest.matches(NodeProxy) showed zero calls in the failing case, which places the failure upstream of the name comparison, in the index lookup itself. The one hardcoded index half accounts for the whole of the observed behavior: the unnamed and wildcard forms take a different branch and never reach that line, the attribute axis uses getAttributes which selects the correct half, in-memory nodes take the memtree path, and element tests were asking for the element half already. Selecting the half from the test's own type is extracted into indexTypeFor rather than written inline, so getSelf's NPath complexity is unchanged. Two other hypotheses were tested and falsified, recorded here so they are not retraced: a Type.ITEM versus Type.NODE mismatch in NameTest.matches(NodeProxy), disproved by a probe widening that check to no effect; and the analyze()-time empty-sequence short-circuit for the self axis, disproved by the bound-variable shape $a[self::attribute(id)], which bypasses the context-step analysis and still failed. Not a regression: the line is byte-identical in the eXist-6.4.1 tag and current develop. SelfAxisAttributeKindRegressionTest covers the reported case plus a namespaced attribute, a non-matching name, a named context step, a bound variable, the unnamed and wildcard forms, the attribute axis, and the element-side tests that were already correct. Each is paired against the in-memory result, which was always correct and defines the expected answer. Four of the eight fail on unfixed develop; the four controls pass in both states. Closes eXist-db#6689 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📊 XQTS result comparisonComparison of this run against
Relative to 🔴 Newly failing tests (1)
Runtime: 302.8s (-121.04s vs |
| * @return {@link ElementValue#ATTRIBUTE} for an attribute test, {@link ElementValue#ELEMENT} otherwise | ||
| */ | ||
| private static byte indexTypeFor(final NodeTest test) { | ||
| return test.getType() == Type.ATTRIBUTE ? ElementValue.ATTRIBUTE : ElementValue.ELEMENT; |
There was a problem hiding this comment.
should this limit to attributes/elements only? e.g. PIs or comments?
There was a problem hiding this comment.
[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]
Good catch — PIs are affected, comments are not. I tested rather than reasoned, and it turned up a second problem as well.
Processing instructions: same defect. A PI is addressable by name, but the structural index has only an element half and an attribute half, so there is nowhere to look one up. self::processing-instruction('target') therefore searched the element half for an element named target and never matched:
| before | in-memory | expected | |
|---|---|---|---|
self::processing-instruction('target') |
0 | 1 | 1 |
self::element(a) |
1 | 1 | 1 |
Comments and text nodes are safe — they have no name, so comment() and text() are TypeTests that take the wildcard branch and never reach the index lookup.
I have pushed a fix. Note it is not the obvious one: there is a commented-out VirtualNodeSet branch for PIs sitting directly above the line you flagged, and restoring it turns the wrong answer into a hard error (Internal evaluation error: context is missing for node 1.1) — which I assume is why it was commented out rather than deleted. Matching PI tests directly against the context set needs no index at all and works. That also fixes the unnamed form, which was returning every child node rather than the two PIs.
New SelfAxisProcessingInstructionRegressionTest covers the named form, a non-matching target, the unnamed form, step position, and the element cases as controls.
Separately, and not fixed here: the same probe showed that unnamed kind tests on the self axis are always true in predicate position on persistent nodes.
| predicate, on a node with 3 children (2 PIs, 1 element) | persistent | in-memory | expected |
|---|---|---|---|
[self::comment()] |
3 | 0 | 0 |
[self::text()] |
3 | 0 | 0 |
[self::element()] |
3 | 1 | 1 |
Nothing is filtered. In step position (/self::comment()) the same tests are correct, so it is specific to the predicate path: getSelf's wildcard branch returns the context set unfiltered when a context id is in play, stamping contexts only on the nodes that matched.
That is a different branch of the same method with a much wider blast radius, so I have filed it separately as #6705 rather than widening an approved PR. It is also the symptom the reporter described in a follow-up on #6689, which I had wrongly told him did not reproduce — correcting that on the issue now.
Raised by review of the attribute fix: a processing instruction is addressable by name, so is it subject to the same defect? It is. The structural index has only an element half and an attribute half, so there is nowhere to look a processing instruction up. getSelf sent every non-wildcard test to that index, which meant self::processing-instruction(NAME) searched for an *element* of that name and never matched -- the same always-false failure as the attribute case, for the same reason. Comments and text nodes are not affected: they have no name, so their tests are TypeTests that take the wildcard branch and never reach the index lookup. Matching processing-instruction tests directly against the context set needs no index at all. Note this is not the branch that was commented out directly above: restoring that VirtualNodeSet path replaces the wrong answer with "Internal evaluation error: context is missing for node 1.1", which is presumably why it was commented out rather than deleted. This also corrects the unnamed form. self::processing-instruction() previously returned every node on the axis rather than only the processing instructions, because it fell into the wildcard branch, which returns the context set unfiltered when a context id is in play. That branch is still wrong for comment(), text() and element(), tracked separately as eXist-db#6705 -- processing instructions simply no longer reach it. The loop is extracted into selfProcessingInstructions rather than written inline to limit the complexity cost; even so, adding a branch to getSelf raises its PMD NPath from 680 to 1360. That is a real cost in a method already well over the threshold, accepted here because the alternative shape -- folding the branch into the existing dispatch chain -- fixes only the named form and leaves the unnamed one wrong. SelfAxisProcessingInstructionRegressionTest covers the named form, a non-matching target, the unnamed form, step position, and the element cases as controls. Three of its five cases fail without this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
[This PR was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]
Closes #6689
Summary
A named attribute kind test on the self axis —
self::attribute(id)— was always false on a persistent node. It was correct on in-memory nodes and correct in Saxon, so the same predicate gave different answers depending on whether the document had been stored.What Changed
exist-core/src/main/java/org/exist/xquery/LocationStep.javagetSelflooked every non-wildcard test up in the element half of the structural index:self::attribute(id)therefore searched for an element namedid, found nothing, and returned the empty sequence — without ever consulting the node test. The fix selects the index half from the test's own type.Root-Cause Verification
Confirmed empirically rather than by inspection: instrumenting
NameTest.matches(NodeProxy)showed zero calls in the failing case, which rules out the name comparison and places the failure upstream of it, in the index lookup.Two other hypotheses were tested and falsified along the way, both worth recording so a future reader does not retrace them:
Type.ITEMversusType.NODEmismatch inNameTest.matches(NodeProxy)— a probe widening that check changed nothing.analyze()-time empty-sequence short-circuit for the self axis — the bound-variable shape$a[self::attribute(id)]bypasses the context-step analysis entirely and still failed.The single hardcoded index half accounts for every observed case:
@*[self::attribute(id)]persistent@*[self::attribute()],@*[self::attribute(*)]$n/attribute::attribute(id)getAttributesselects the correct half@*[. instance of attribute(id)]self::NAMEon elementsThis is not a regression: the line is byte-identical in the
eXist-6.4.1tag and currentdevelop.Test Plan
SelfAxisAttributeKindRegressionTestfails on unfixeddevelopand passes with the fixmvn testonexist-corexquery.CoreTests— the wholesrc/test/xquerycorpus — greenlicense:checkcleanScope
One expression, on one axis, in one method. No change to the attribute axis, to element tests, or to the wildcard branches.
🤖 Generated with Claude Code