Tests for context item in functions

[adamretter]

These tests show that eXist-db is incorrectly setting the context item inside both static and dynamic function calls.

According to https://www.w3.org/TR/xquery-31/#id-eval-function-call when evaluating the body of a function:

The focus (context item, context position, and context size) is absent

Unfortunately these tests show otherwise :-(

The issue is predominantly with RootNode.java which always establishes a context item, i.e. it is unaware of when it is inside a UserDefinedFunction.java.

I am not sure of the best way to fix this? I can see two options at the moment:

1. we make RootNode.java aware that it is being called inside a function, and have it always throw XPDY0002 in that case.
2. we modify XQueryContext so that the "focus (context item, context position, and context size)" is stack based and gets pushed down in UserDefinedFunction before the body is eval'd, so that it is unavailable to the RootNode.

@wolfgangmm thoughts please? it would be good to discuss it. Some of your XQSuite tests for the Range Index (extensions/indexes/range/src/test/xquery/range/range.xql) are relying on this incorrect behaviour, but I think they could be rewritten to start with fn:collection(...).

In addition in UserDefinedFunction, the function body is evaluated like so:

result = body.eval(contextSequence, contextItem);

I suspect that should also be adjusted to:

result = body.eval(null, null);

[duncdrum]

Phew this is a big one. Because of the amount of potential breakage i d put this on the 6.0.0 milestone (which might now happen rather soon)

[adamretter]

@duncdrum Do you see a potential for much breakage outside of broken XQSuite tests? I think it is probably just XQTS tests that might break and in the exist codebase it seems to be just those range index ones.

[duncdrum]

I think that lots of folks might have xqsuite tests involving range indexes. Also god knows how many local functions rely on the faulty behavior.

Does not scream 5.1.2 to me.

[adamretter]

@duncdrum tests on range indexes are fine.

It's the tests in exist for range indexes which start without an explicit context item that have a problem, e.g. from extensions/indexes/range/src/test/xquery/range/range.xql:

declare
    %test:args("üss")
    %test:assertEquals("Rüsselsheim")
    %test:args("ta M")
    %test:assertEquals("Almweide")
function rt:contains-string($name as xs:string) {
    //address[range:contains(name, $name)]/city/text()
};

The above is invalid XQuery as it starts // inside a function with no context item.

[dizzzz]

tests are failing...

[adamretter]

@dizzzz Exactly my point!

[line-o]

Good find!
I would like to see a positive test as well. A function with an explicit context item.
It is again the question if the fix should be part of this PR or if this can be merged before the actual issue is fixed.
I think both belong together, especially since you might have an idea how to fix it.

[adamretter]

I am still waiting on comments from @wolfgangmm

[joewiz]

@adamretter Is this still something you're hoping to include in 5.3.0? I see that in #3163 you mention that the combined fixes yield us another 4% passing in XQTS, so it's a big improvement.

[adamretter]

@joewiz I'm still waiting for input on it

[adamretter]

In todays community call, @wolfgangmm expressed a preference to fixing it with approach 1:

we make RootNode.java aware that it is being called inside a function, and have it always throw XPDY0002 in that case.

[line-o]

As @adamretter pointed out there are lots of positive tests present already.

[line-o]

@adamretter setting both contextItem and contextSequence to null was also done for HOFs to fix #1960

[line-o]

I would like to see those tests implemented as XQSuite tests.

[line-o]

RootNode.java does not need to know where it is called. As long as every Function and UserDefinedFunction call nulls the context.

[adamretter]

RootNode.java does not need to know where it is called. As long as every Function and UserDefinedFunction call nulls the context.

@line-o Hmm! I am not sure it is that simple... Off the top of my head - wouldn't that possibly break things like $sequence[my-function(.)] and also $sequence ! my-function(.)?

[joewiz]

@adamretter I see we have Wolfgang's agreement on the approach and Dannes' approval, but there are some remaining CI failures. Can this be merged, or is additional work needed to get passing CI results? I ask because I recall that this PR needs to be resolved in order for #3163.

[adamretter]

@joewiz I believe @line-o took over the context item stuff in #3163. I don't think it is ready yet though...

[line-o]

this PR is superseded by #3605