fix(#6669): atomize the auto-name before the fold checks - #6672
fix(#6669): atomize the auto-name before the fold checks#6672yangjj-iso wants to merge 1 commit into
Conversation
… checks The drop template handed "@name" - an attribute node - to nine functions that each declare their "$name" as "xs:string", leaving the conversion to the function conversion rules. Saxon is free to bind a parameter to a closure over the argument expression rather than over its converted value, and the node then arrives inside the function. There "eo:resolved-name(@base) = $name" has already been compiled to a "ValueComparison" on the strength of the declared types, so it casts straight to "AtomicValue" and the sheet dies with "DOMNodeWrapper cannot be cast to AtomicValue". That template is the only place a node can enter "$name". Every other call site passes the "$name" of the inlining template, which "eo:resolved-name" builds with "substring-after" and which therefore cannot be anything but a string. Atomizing once, in the template, leaves the conversion nothing to lose and covers all nine functions at once, rather than rewriting the comparison in each of them - the same move as the "eo:signature" call in "to-eo-tree" and the "@Local" wrap in "restore-local-names". It also explains why objectionary#6638 appeared to move the crash: "[2]" evaluates the predicate through a lazy subscript, which is precisely the context in which the deferred conversion is lost, where "count()" had materialised the sequence inside the call. The failure is optimiser-dependent and does not reproduce on demand, so there is no test to add: the runtime workflow is red on master while the same sources print canonically on other runners. Before and after this change "mvn -pl eo-runtime process-sources" reports all 170 sources formatted canonically here, transpiling to the same 179 Java files, and the 346 tests of eo-printer pass. The change removes the possibility of the crash rather than fixing a reproducible one. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a nondeterministic Saxon optimizer-dependent crash in the EO printer’s inline-cactoos.xsl by ensuring that the auto-generated @name attribute is atomized to an xs:string before it is passed into multiple xs:string-typed helper functions used in fold/inline checks.
Changes:
- Atomizes
@nameonce viastring(@name)into anxs:stringvariable before running the chain of fold-check predicates. - Updates the predicate calls to pass the atomized
$nameinstead of the@nameattribute node. - Adds explanatory documentation describing the Saxon
ValueComparison/ deferred-conversion failure mode and rationale for the fix.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
Reopened to re-run CI against current master. Nothing about the branch changed. The first
This branch is based on A |
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
|
|
These counts changed in this branch,
They are defined in .github/workflows/counts.yml |
|
@yangjj-iso conflicts here |
Closes #6669
runtimeis red on master because of this, so nothing merges until it is green — including my own #6668, which is how I came to it.Root cause
The stack trace on the issue names the caller: the template rule at
inline-cactoos.xsl:314. That template hands@name— an attribute node — to nine functions in a row:Every one of them declares its
$nameasxs:string, so the atomization was left to the function conversion rules. Saxon is free to bind a parameter to a closure over the argument expression rather than over its converted value, and then the node itself arrives inside the function. There the comparisonhas already been compiled to a
ValueComparison, on the strength of both operands being staticallyxs:string— so it casts straight toAtomicValue, and the sheet dies withDOMNodeWrapper cannot be cast to AtomicValue.The other operand cannot be the node:
eo:resolved-namebuilds its result withsubstring-after, so it is a string by construction.$nameis the only candidate, and that template is the only place a node can reach it — every other call site passes the$nameof the inlining template at line 100, which is itselfeo:resolved-name(@base).This also accounts for the "moved the crash" reading in the issue.
count()materialised the sequence inside the function's own evaluation;[2]reaches it through a lazy subscript, which is exactly the context where the deferred conversion is lost. #6638 did not introduce the type mismatch — it changed the evaluation strategy enough to expose it.The change
Atomize once, in the template, before any of the nine is asked:
Two lines. It leaves the conversion nothing to lose and covers all nine functions at once, rather than rewriting the comparison inside each of them — the same move as the
eo:signaturecall into-eo-tree, and as the@localwrap inrestore-local-namesfor #6650.string()is exact here rather than merely close: the match pattern iso[starts-with(@name, $auto) and not(eo:void(.))], so@nameis always present, and the parameters are requiredxs:string. Nothing about what the nine functions are asked changes.Verification
The failure is optimiser-dependent and does not reproduce on demand — that is the shape of the bug, and it is why I have no regression test to offer. Master is red on
runtime (23)while the same sources print canonically on the other runners.So what I can show is that the change is inert on everything that does run:
mvn -pl eo-printer testmvn -pl eo-runtime process-sourcesBoth columns are this branch's base,
d421d8d; the left one was produced by stashing the change and reinstallingeo-printer, so the comparison is like for like.What I deliberately left alone
restore-local-names.xslhas the same shape —eo:recursive(., @name)and five others, called from match patterns — and it took the same[2]change in 28e7534. I did not touch it here: its parameters arexs:string?rather thanxs:string, sostring(@name)would turn an absent@namefrom an empty sequence into''and change what those functions answer. The faithful form there is@name/string(), which deserves its own change and its own reasoning rather than riding along with this one. Glad to open it if you want it.