Skip to content

[bugfix] fn:function-lookup: capture focus for context-dependent built-ins#6352

Open
joewiz wants to merge 3 commits into
eXist-db:developfrom
joewiz:bugfix/fn-function-lookup-focus-capture
Open

[bugfix] fn:function-lookup: capture focus for context-dependent built-ins#6352
joewiz wants to merge 3 commits into
eXist-db:developfrom
joewiz:bugfix/fn-function-lookup-focus-capture

Conversation

@joewiz
Copy link
Copy Markdown
Member

@joewiz joewiz commented May 11, 2026

Summary

Per F&O 3.1 §16.1.1, when fn:function-lookup returns a context-dependent built-in, the static and dynamic context of the call to function-lookup forms part of the returned function's closure:

If the function that is retrieved by fn:function-lookup is context-dependent, that is, if it has dependencies on the static or dynamic context of its caller, the context that applies is the static and/or dynamic context of the call to the fn:function-lookup function itself. The context thus effectively forms part of the closure of the returned function.

Previously eXist did not capture this focus, so lookups of fn:node-name#0, fn:string#0, fn:position#0, fn:lang#1, fn:has-children#0, fn:id#1, etc. threw XPDY0002 (or NullPointerException) when the body needed a context item.

What Changed

  • FunctionReference carries an optional captured focus. When set, argument expressions still evaluate against the outer focus, but the function body evaluates against the captured focus.
  • UserDefinedFunction grows a propagateContextToBody flag (default false — plain UDFs cannot see the caller's focus, matching the spec).
  • FunctionFactory.wrap (the helper that lifts a built-in Function into a FunctionCall for use as a function item) opts in to context propagation. This benefits both fn:function-lookup and named function references like fn:node-name#0.
  • FunOnFunctions.eval (fn:function-lookup branch) sets the captured focus on the FunctionReference it returns.
  • Six new XQSuite tests in xquery3/higher-order.xml exercise fn:node-name#0, fn:string#0, fn:lang#1, fn:has-children#0, the 1-arg fn:id case, and confirm that named references to user-defined functions remain unchanged.

XQTS HEAD fn-function-lookup before/after

Run Tests Pass Fail Err
Before 674 631 33 2
After 671 646 16 1

18 tests recovered. Tests fixed: 002, 006, 010, 014, 050, 110, 114, 264, 268, 272, 276, 280, 284, 288, 350, 354, 360, 362.

Out of scope

The 17 remaining failures in fn-function-lookup trace to separate, unrelated bugs and are not addressed here:

  • 018, 022, 024 — XPTY0018 raised by a path expression whose last step yields a function reference
  • 355 — fn:element-with-id#2 not registered
  • 356, 358 — additional fn:element-with-id / fn:idref issues
  • 382, 384 — fn:unparsed-text-lines returns empty in the XQTS environment
  • 524, 526, 528 — xs:IDREFS / xs:NMTOKENS / xs:ENTITIES list-type constructors
  • 610 — wrong error code on dynamic arity mismatch (XPST0017 vs XPTY0004)
  • 613 — cyclic variable declaration error code (XPDY0002 vs XQDY0054)
  • 735 — xs:language constructor rejects valid input
  • 761, 764 — fn:load-xquery-module error codes
  • 766b — fn:transform error code

Spec References

Test Plan

  • mvn test -pl exist-core — all 4,698 tests pass on the branch
  • 6 new XQSuite tests in higher-order.xml pass
  • XQTS HEAD fn-function-lookup spot-check shows +18 tests recovered

🤖 Generated with Claude Code

…t-ins

Per F&O 3.1 section 16.1.1 (fn:function-lookup), when a context-dependent
built-in is retrieved via fn:function-lookup the returned function item
must carry the static and dynamic context of the call to function-lookup
as part of its closure. Subsequent invocation of the returned item must
evaluate the body against that captured focus, even if invoked from a
different focus.

Previously eXist constructed a FunctionReference without capturing the
focus, so lookups of fn:node-name#0, fn:string#0, fn:position#0,
fn:lang#1, fn:has-children#0, fn:id#1, etc. failed with XPDY0002 when
the body looked for a context item.

Changes:

- FunctionReference now carries an optional captured focus (sequence +
  item) and, when set, evaluates argument expressions against the outer
  focus while routing body evaluation through the captured focus.
- UserDefinedFunction grows a propagateContextToBody flag (default
  false). Plain user-defined functions still cannot see the caller's
  focus, matching the spec, but wrapper functions created by
  FunctionFactory.wrap (named function references and the
  fn:function-lookup return value) opt in so the wrapped built-in can
  see the captured focus.
- FunOnFunctions.eval sets the captured focus on the FunctionReference
  it returns from fn:function-lookup.
- Adds six in-repo XQSuite tests covering fn:node-name#0, fn:string#0,
  fn:lang#1, fn:has-children#0, the 1-arg fn:id case, and that named
  references to user-defined functions remain unchanged.

XQTS HEAD fn-function-lookup test set: 33 failures + 2 errors -> 16
failures + 1 error (18 tests recovered). The 17 remaining failures
trace to separate bugs (XPTY0018 in path expressions with function
calls, missing arity-2 fn:element-with-id, xs:IDREFS/NMTOKENS/ENTITIES
constructors, fn:load-xquery-module error codes, etc.) that are out of
scope for this change.

Spec reference:
  https://www.w3.org/TR/xpath-functions-31/#func-function-lookup

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@joewiz joewiz requested a review from a team as a code owner May 11, 2026 20:48
Copy link
Copy Markdown
Member

@line-o line-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a ton for identifying this gap in spec compliance and picking it up.
I propose a slightly different approach and would like to know if this might be a viable alternative.

Instead of deciding to capture the context for some functions I am in favour of the functions itself to signal if they are context dependent or not. Obviously only allowed for built-ins and false by default.

Comment thread exist-core/src/main/java/org/exist/xquery/FunctionFactory.java Outdated
@line-o line-o requested a review from a team May 11, 2026 21:21
@joewiz
Copy link
Copy Markdown
Member Author

joewiz commented May 11, 2026

[This response was co-authored with Claude Code. -Joe]

Agreed — your design is cleaner and more self-documenting. Each context-dependent built-in (fn:position, fn:last, fn:current-dateTime, fn:current-date, fn:current-time, fn:implicit-timezone, fn:default-collation, fn:static-base-uri, fn:id, fn:idref, fn:element-with-id, fn:lang, fn:document-uri, fn:node-name, and a handful of others) declares its own context-dependency, and FunctionFactory.wrap checks the flag rather than always propagating.

Current shipped fix conservatively propagates context for every wrapped built-in, which is functionally correct but slightly over-broad (it propagates even when the body wouldn't have used the focus). The flag approach narrows it to exactly the right cases.

Two ways forward, your pick:

  1. Refactor in this PR: add Function.isContextDependent() (default false), override in ~12–15 built-ins, change line 517 to func.setPropagateContextToBody(call.isContextDependent()). Expands the PR's diff surface; same +18 XQTS lift; cleaner architecture.
  2. Ship [bugfix] fn:function-lookup: capture focus for context-dependent built-ins #6352 as-is, refactor as follow-up: lands the +18 conformance fix now (already verified), then a focused [refactor] PR migrates to the flag pattern. The follow-up touches no business logic — just adds annotations + the one-line check site — so it's reviewable independently.

I'd lean (2) because the conformance fix is independently valuable and the refactor benefits from a sweep that can also surface any built-ins we'd otherwise miss. But (1) is reasonable if you'd rather have the cleaner shape land together.

Either way, happy to do the work.

@line-o
Copy link
Copy Markdown
Member

line-o commented May 11, 2026

I am in favour of going in the right direction right away now that this is in fresh memory

Per line-o's review on PR eXist-db#6352: instead of unconditionally propagating
the caller's focus into the body of every fn:function-lookup wrapper at
FunctionFactory.wrap, each Function subclass now signals its own
context-dependency via an isContextDependent() method. The default is
false, so non-context-dependent built-ins (fn:concat, fn:string-length#1,
the FOXX cast constructors, ...) and user-defined functions no longer
opt in to focus propagation.

API change:

- New Function.isContextDependent() returning false by default, with
  Javadoc enumerating the canonical override set and pointing back to
  F&O 3.1 section 16.1.1 for the closure-of-context rule.
- InternalFunctionCall delegates isContextDependent() to the wrapped
  Function so that FunctionFactory.wrap can consult the underlying
  built-in even though it receives the call wrapped.

Overrides (26 classes, ~30 signatures):

- Always context-dependent: FunPosition, FunLast, FunCurrentDateTime
  (current-dateTime, current-date, current-time), FunImplicitTimezone,
  FunDefaultCollation, FnDefaultLanguage.
- 0-arg form context-dependent: FunNodeName, FunDocumentURI, FunNilled,
  FunStrLength, FunNormalizeSpace, FunData, FunString, FnHasChildren,
  FunNumber, FunName, FunLocalName, FunNamespaceURI, FunPath, FunRoot,
  FunGenerateId.
- 1-arg form context-dependent: FunLang, FunId, FunIdRef,
  FunElementWithId (2-arg forms take an explicit node and do not).
- FunBaseURI: 0-arg base-uri() reads the focus; the dual-signature
  static-base-uri form closes over the lookup-site static context.

Call site:

- FunctionFactory.wrap line 517: setPropagateContextToBody(true)
  becomes setPropagateContextToBody(call.isContextDependent()). The
  hardcoded unconditional propagation introduced by the bugfix commit
  is replaced; the new flag generalizes the contract to every Function
  subclass rather than special-casing the lookup site.

Tests:

- exist-core/src/test/xquery/xquery3/higher-order.xml grows nine new
  XQSuite cases that exercise functions outside the bugfix commit's
  original set (fn:name#0, fn:local-name#0, fn:base-uri#0, fn:path#0
  via focus capture; fn:default-collation, fn:static-base-uri via
  lookup-site static context) and pin the negative cases (fn:concat,
  fn:string-length#1 still work via lookup).

Verification:

- xquery.xquery3.XQuery3Tests: 1026 passed, 0 failed, 0 errors.
- XQTS HEAD fn-function-lookup test set: 674 cases, 647 passed,
  16 failed, 1 error -- identical to the unconditional-propagation
  bugfix commit. The remaining 17 trace to separate bugs (XPTY0018 in
  path expressions with function calls, missing arity-2
  fn:element-with-id, xs:IDREFS/NMTOKENS/ENTITIES constructors,
  fn:load-xquery-module error codes) that are out of scope.

Spec reference:
  https://www.w3.org/TR/xpath-functions-31/#func-function-lookup

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@joewiz
Copy link
Copy Markdown
Member Author

joewiz commented May 12, 2026

[This response was co-authored with Claude Code. -Joe]

Done in abb68059ec (force-pushed). Picked path (i) per your preference — kept the watchdog/refactor approach.

Summary: added Function.isContextDependent() with default false; 26 classes (~30 signatures) declare true. FunctionFactory.wrap now reads call.isContextDependent() instead of unconditionally propagating. InternalFunctionCall delegates to the wrapped function so the wrap site sees the underlying built-in's declaration.

Coverage of the new flag (verify against your intent):

  • Always context-dependent: FunPosition, FunLast, FunCurrentDateTime, FunImplicitTimezone, FunDefaultCollation, FnDefaultLanguage
  • 0-arg context-dependent: FunNodeName, FunDocumentURI, FunNilled, FunStrLength, FunNormalizeSpace, FunData, FunString, FnHasChildren, FunNumber, FunName, FunLocalName, FunNamespaceURI, FunPath, FunRoot, FunGenerateId
  • 1-arg context-dependent: FunLang, FunId, FunIdRef, FunElementWithId
  • Mixed: FunBaseURI (isCalledAs("static-base-uri") || getArgumentCount() == 0)

XQTS HEAD fn-function-lookup: preserved at 35 → 17 fail (+18 vs develop). All 1026 in-repo XQuery3Tests still pass. 9 new XQSuite reproducers in higher-order.xml cover context-dependent built-ins not in the original lookup-site special-cased set (fn:name#0, fn:local-name#0, fn:base-uri#0, fn:path#0, fn:default-collation, fn:static-base-uri) — proves the flag-based dispatch generalizes rather than just hardcoded sleight-of-hand.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do you feel about translating the whole spec to XQsuite? I still find XSpec very cumbersome to read.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong feelings here. I'd be happy to switch, either just the ones added in this PR, or a broader set in a follow-on PR. Just let me know what you guys would like.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest switching the whole spec. Otherwise it ll be difficult to see what test there are at the same time.

@line-o line-o added the xquery issue is related to xquery implementation label May 12, 2026
@joewiz
Copy link
Copy Markdown
Member Author

joewiz commented May 12, 2026

[This response was co-authored with Claude Code. -Joe]

Understood — converting the entire higher-order.xml from XSpec to XQSuite format will be a separate dedicated session since the file has substantial existing test surface; I'll draft a tasking for that and run it as a follow-up so the conversion + #6352's correctness fix are reviewed cleanly together. Will keep this PR open while that work lands.

Per duncdrum's review on PR eXist-db#6352: switch the entire higher-order test
file to a single annotation-based assertion format so the test inventory
is easier to scan.

The previous higher-order.xml used eXist's legacy <TestSet>/<test> XML
format; the new higher-order.xqm uses XQSuite %test:assertEquals /
%test:assertEmpty / %test:assertError annotations. All 75 scenarios are
preserved -- the 62 original ones plus the 13 fn:function-lookup
focus-capture cases added earlier in this PR. Each scenario maps to one
%test-annotated function, with the body delegated to util:eval so the
per-test compilation isolation of the original format is preserved
(every test had its own xquery version prologue and conflicting
local:/ex: module-level function declarations that cannot coexist at
xqm module scope).

setUp / tearDown annotations replace the old <setup>/<tearDown>
sections, storing the ex2:fold-left helper module at
/db/xq3-test/test1.xql for the two "imported module" cases.

Verification:

- xquery.xquery3.XQuery3Tests: Tests run: 1026, Failures: 0, Errors: 0,
  Skipped: 1 (the 1 skipped is pre-existing, unrelated to this change).
- Full module gate: Tests run: 6748, Failures: 0, Errors: 0, Skipped: 97
  BUILD SUCCESS (mvn test -pl exist-core).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@joewiz
Copy link
Copy Markdown
Member Author

joewiz commented May 12, 2026

[This response was co-authored with Claude Code. -Joe]

Done in 80ae692772. Converted the full higher-order.xml to an XQSuite module at exist-core/src/test/xquery/xquery3/higher-order.xqm. All 75 scenarios preserved — the 62 original cases plus the 13 fn:function-lookup focus-capture cases this PR added — each mapped to one %test-annotated function with %test:assertEquals / %test:assertEmpty / %test:assertError.

The per-test bodies delegate to util:eval so each scenario keeps its own xquery version prologue and its own local: / ex: module-level function declarations, which would otherwise conflict at xqm module scope. %test:setUp / %test:tearDown replace the old <setup> / <tearDown>, storing the ex2:fold-left helper module for the two "imported module" cases.

Verification:

  • xquery.xquery3.XQuery3Tests: Tests run: 1026, Failures: 0, Errors: 0, Skipped: 1 (skipped is pre-existing).
  • Full module gate mvn test -pl exist-core: Tests run: 6748, Failures: 0, Errors: 0, Skipped: 97 — BUILD SUCCESS.

@joewiz
Copy link
Copy Markdown
Member Author

joewiz commented May 12, 2026

@duncdrum Is that what you had in mind? Happy to adjust based on your preferences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

xquery issue is related to xquery implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants