Skip to content

No static typing #5673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

No static typing #5673

wants to merge 3 commits into from

Conversation

line-o
Copy link
Member

@line-o line-o commented Mar 16, 2025 •
edited
Loading

Description:

fixes #2445

Reference:

Type of tests:

XQSuite tests

@line-o line-o requested a review from a team as a code owner March 16, 2025 19:11
dizzzz
dizzzz requested changes Mar 16, 2025
View reviewed changes
Copy link
Member

@dizzzz dizzzz left a comment

Choose a reason for hiding this comment

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

Build is failing;
can we add tests eg from the original bug report?

@dizzzz dizzzz requested review from wolfgangmm and a team March 16, 2025 20:02
@line-o line-o force-pushed the no-static-typing branch 2 times, most recently from 06f4eea to a12a11e Compare March 16, 2025 20:56
@line-o
Copy link
Member Author

line-o commented Mar 16, 2025

I applied commits from an older PR that targeted develop-6.x.x and it took 3 attempts to get it to compile.
I will look to add one or two tests from the original issue.

@line-o
Copy link
Member Author

line-o commented Mar 16, 2025

I would expect for one to many additional XQTS test cases to now pass.

@line-o
Copy link
Member Author

line-o commented Mar 16, 2025

There seems to be zero tests that assert error XPST0005 to be raised. ;)

@dizzzz
Copy link
Member

dizzzz commented Mar 17, 2025

I am wondering.... should this code change be 'protected' to it would not be effective for 'query 3.1' (and older) based queries?

@line-o
Copy link
Member Author

line-o commented Mar 17, 2025 •
edited
Loading

The error with code XPST0005 should only be raised when a XQuery processor implements the static typing feature.

exist-db never claimed to support this optional feature, which is why I believe we do not need to guard against it.
I do agree that one can argue it is a breaking change which is why I ported my original PR #3653 to target exist 7 instead.

With this PR applied XPath queries will behave much more like in any other Xquery processor (namely baseX and Saxon).

@line-o line-o force-pushed the no-static-typing branch from a12a11e to 48082b2 Compare March 17, 2025 18:18
@line-o line-o mentioned this pull request Mar 17, 2025
Closed
@line-o line-o force-pushed the no-static-typing branch 3 times, most recently from 773a278 to 51e4984 Compare March 17, 2025 20:10
@line-o line-o requested a review from dizzzz March 17, 2025 21:42
dizzzz
dizzzz approved these changes Mar 18, 2025
View reviewed changes
Copy link
Member

@dizzzz dizzzz left a comment

Choose a reason for hiding this comment

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

as discussed in telco

@dizzzz dizzzz requested review from reinhapa and a team March 20, 2025 16:10
@dizzzz dizzzz requested a review from a team March 24, 2025 10:28
duncdrum
duncdrum approved these changes Mar 24, 2025
View reviewed changes
Copy link
Contributor

@duncdrum duncdrum left a comment

Choose a reason for hiding this comment

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

as discussed needs documentation

@line-o line-o added the needs documentation Signals issues or PRs that will require an update to the documentation repo label Apr 15, 2025
line-o and others added 3 commits July 30, 2025 23:14
@line-o
[refactor] ContextItemExpression uses staticReturnType
0e459fb 
Turns out, a property for the static return type is already defined by
the Step super class.
ContextItemExpression uses this now and its own returnType property is
removed.
@line-o
[fix] simple predicates evaluating to empty sequences
c4d4585 
Fixes eXist-db#2445

By definition the following Xpaths must evaluate to an empty sequence
- `<a/>[self::b]`
- `<a/>/@b/c`
- `<a/>/text()/b`

Instead of throwing an Exception the staticReturnType of the current step
and the contextInfo returnType are set to Type.EMPTY.
@line-o
[test] add tests to ensure no static typing
d7d13a2 
refs eXist-db#2445
These tests aim to ensure that certain XPath expressions no longer raise error XPST0005 as exist-db does not implement
the static typing feature and this feature is even dropped entirely with the next version of XQuery.
@line-o line-o force-pushed the no-static-typing branch from 51e4984 to d7d13a2 Compare July 30, 2025 21:15
@adamretter
Copy link
Contributor

adamretter commented Aug 4, 2025 •
edited
Loading

An interesting idea of how to fix the issue... But it must be recognised that this small change is very significant!

Before it is merged two things need to be checked carefully in great detail:

  1. No regression in the XQTS results
  2. No performance regression (looking at the code change, I suspect this PR will result in a regression in its current form)

adamretter
adamretter suggested changes Aug 4, 2025
View reviewed changes
Copy link
Contributor

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

I would like to see a full report of the before-and-after XQTS results, and a number of performance tests added that should show this does not lead to a performance regression.

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

@dizzzz dizzzz dizzzz approved these changes

@reinhapa reinhapa reinhapa approved these changes

@duncdrum duncdrum duncdrum approved these changes

@wolfgangmm wolfgangmm Awaiting requested review from wolfgangmm

+1 more reviewer

@adamretter adamretter adamretter requested changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
needs documentation Signals issues or PRs that will require an update to the documentation repo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

self:: axis inside predicate throws XPST0005
5 participants
@line-o @dizzzz @adamretter @reinhapa @duncdrum