Skip to content
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.

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

1341 Drop $position callback from many functions #1735

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michaelhkay
Copy link
Contributor

Responding to the discussion in #1341, this (somewhat experimental) PR explores the possibility of dropping the optional
$position argument to the callback of many higher-order functions such as some(), every(), filter(), for-each(), fold-left(), fold-right(). Instead, it provides the option to wrap the input sequence in a call of numbered-items() which replaces each item in the input with an (item, position) pair.

I've done this only (so far) for higher-order sequence functions, but the intent is that the same could be done for arrays and (potentially) maps.

I left the position argument in place for a few functions where losing it seemed to cause genuine inconvenience:

  • partition(), where the function wraps the supplied items into arrays, and you don't want to have to remove the positions afterwards
  • subsequence-where(), where many use cases are likely to use positional information
  • for-each-pair(), where there are two input sequences and it seems clumsy to associate position information with one or the other

The main benefit is that we provide one basic mechanism which is automatically available everywhere, which means we don't have to have debates about whether or not there is a use case for adding position information to (say) fold-left or scan-right.

A further benefit is that the functions defined for sequences automatically become available for arrays and maps. I haven't yet explored the impact on maps and arrays; I will wait first to see what the reaction is to this proposal.

@michaelhkay michaelhkay added XQFO An issue related to Functions and Operators Enhancement A change or improvement to an existing feature Discussion A discussion on a general topic. Tests Needed Tests need to be written or merged labels Jan 27, 2025
@ChristianGruen
Copy link
Contributor

To be honest, I am not very thrilled to possibly see this feature rolled back.

Apart from my strong personal preference for keeping $pos, I think it is about time to give developers some basic security on the stability of our drafts. By experience, positional arguments are already adopted in first 4.0 projects. While we should certainly preserve our freedom to revert critical design mistakes before it is too late, we should by all means try to find out very soon which parts of the spec we believe will be part of a final version and which ones are too dubious to keep. Otherwise, chances are high that we scare away potential early adopters who don’t want to wait for day X. As an implementer, I have similar concerns: We should minimize time and efforts to realize features that will eventually be dropped again.

As for $pos, I can see some theoretical benefits for an fn:numbered-item function, but I am not sure if it would be convincing enough to fill that gap. Existing FLWOR expressions seem much more readable after all. An example:

for $item at $pos in $data
return $pos || '. ' || $item

(: vs. :)
for-each($data, fn($item, $pos) { $pos || '. ' || $item })

(: vs. :)
for-each(
  numbered-items($data),
  fn() { ?position || '. ' || ?item }
)

Next, the first two variants give you type safety: A mistyped ?pos lookup would simply return an empty sequence, whereas a $posxyz would be rejected at parse time. In addition, with variables, different names can be chosen that may best suit the given context.

Finally, if we really go that path, I think we should remove $pos from all functions, including fn:for-each-pair. Consistency should be king. It would be just frustrating to only have a dedicated set of functions that offers the flexibility that we can currently use everywhere (and their choice may seem arbitrary, depending on what you plan to do).

@ChristianGruen
Copy link
Contributor

If I remember correctly, most of the discussion about positional arguments was about fold-left and scan-left. While I still think it would be orthogonal to provide the position for all callback functions (or none), a compromise could be to drop it exclusively for folds, as the equivalent code turned out to be particularly complex for those functions.

@ChristianGruen ChristianGruen added the Blocked PR is blocked (has merge conflicts, doesn't format, etc.) label Mar 3, 2025
@dnovatchev
Copy link
Contributor

dnovatchev commented Mar 5, 2025

I compared the current specification of fn:fold-left and fn:fold-right to their corresponding new versions in this PR.
In this PR the "Formal Equivalent" section is spectacularly shorter: 2.5 to 3 times less number of lines, and twice less number of functions. It is much easier to understand and significantly less prone to error.

One observation is that the last example for fn:fold-left in the current PR has not been updated from the current version and still calls a version of fn:fold-left, whose $action argument is a function accepting 3 arguments, the last one $pos:

let $input := (11 to 21, 21 to 31)
let $target := 21
return fold-left($input, (),
  fn($result, $curr, $pos) {
    $result, if ($curr = $target) { $pos }
  }
)

This probably needs to be:

let $input := (11 to 21, 21 to 31)
let $target := 21
return fold-left(numbered-items($input), (),
  fn($result, $curr) {
    $result, if ($curr?item = $target) {$curr?position }
  }
)

Here are screen-dumps of the corresponding "Formal Equivalent" sections so that one can see the huge complexity of the current one and how much more elegant and concise is the one in this PR:

image

Compare to this:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked PR is blocked (has merge conflicts, doesn't format, etc.) Discussion A discussion on a general topic. Enhancement A change or improvement to an existing feature Tests Needed Tests need to be written or merged XQFO An issue related to Functions and Operators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants