Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Array Lookups: partial removal of out-of-bounds checks #1715

Open
ChristianGruen opened this issue Jan 20, 2025 · 7 comments · May be fixed by #1766
Open

Array Lookups: partial removal of out-of-bounds checks #1715

ChristianGruen opened this issue Jan 20, 2025 · 7 comments · May be fixed by #1766
Labels
Enhancement A change or improvement to an existing feature PR Pending A PR has been raised to resolve this issue XPath An issue related to XPath XQFO An issue related to Functions and Operators

Comments

@ChristianGruen
Copy link
Contributor

Various QT4 tests imply that the out-of-bounds check for arrays have been removed. An example:

<test-case name="UnaryLookup-005a">
  <description>Integer subscript into an array: array index too low</description>
  <created by="Michael Kay" on="2014-11-27"/>
  <modified by="Michael Kay" on="2024-07-22" change="returns () in 4.0"/>
  <dependency type="spec" value="XP40+ XQ40+"/>
  <test>(['a', 'b'], ['c', 'd'])[ ?0 eq 'c']</test>
  <result>
    <assert-empty/>
  </result>
</test-case>

I believe this is not reflected in the spec yet, or at least it includes examples that need to be updated:

[ "a", "b" ]?3 raises a dynamic error err:FOAY0001.

I guess that #832 would have been the PR with the relevant changes (we have already observed in another issue that some changes of this PR need to survive; see #1283 (comment)).

That leads me to the original reason for creating this issue:

  • I think it’s a good idea to drop the range check for array lookups, and it would seem consistent to me to also drop it for dynamic function calls.
  • As map/array lookups and dynamic function calls are often used interchangeably, $array?0 and $array(0) should behave identically.
  • The FOAY0001 error would (and should) still be raised by the array functions, including array:get, array:put, array:remove, or array:insert-before.
@ChristianGruen ChristianGruen added Feature A change that introduces a new feature XPath An issue related to XPath labels Jan 20, 2025
@ChristianGruen ChristianGruen mentioned this issue Jan 21, 2025
Open
@michaelhkay
Copy link
Contributor

Indeed, Saxon on the development branch is currently not doing bound checks for the array lookup operator.

I would be quite happy to drop array bound checking entirely, bringing arrays into line with sequences.

@michaelhkay
Copy link
Contributor

Related: #1363

@ChristianGruen
Copy link
Contributor Author

Even better. I would be glad to see all of them go.

@michaelhkay
Copy link
Contributor

michaelhkay commented Jan 24, 2025 •
edited
Loading

We need to be aware that there is a backwards-incompatibility. Normally we treat it as OK to abolish an error condition, but this error is one where people might conceivably be relying on try/catch. Despite this, I'm inclined to go ahead.

(A processor could issue a warning if it sees catch "FOAY0001")

@ChristianGruen
Copy link
Contributor Author

(A processor could issue a warning if it sees catch "FOAY0001")

Completely true. I wanted to add a note to my last comment, indicating that I didn’t find a single occurrence of FOAY0001 in my local project base, and I think those will be rare in other projects, but of course we cannot be 100% sure.

@line-o I stumbled across eXist-db/exist#3275. What do you think about removing out-of-bounds checks for arrays?

@benibela
Copy link

It could be backward compatible with a global setting, like:

declare array-bound-checking off;

or

declare array-bound-checking on;

@michaelhkay
Copy link
Contributor

declare array-bound-checking off;

That's certainly an option to consider. I can't say I like proliferation of context-dependent behaviour, but it's viable.

@michaelhkay michaelhkay linked a pull request Feb 5, 2025 that will close this issue
Open
@michaelhkay michaelhkay added XQFO An issue related to Functions and Operators Enhancement A change or improvement to an existing feature PR Pending A PR has been raised to resolve this issue and removed Feature A change that introduces a new feature labels Feb 5, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
Enhancement A change or improvement to an existing feature PR Pending A PR has been raised to resolve this issue XPath An issue related to XPath XQFO An issue related to Functions and Operators
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

1715 Drop array bound checking michaelhkay/qtspecs
3 participants
@ChristianGruen @benibela @michaelhkay