support pick(last)

[pkoppstein]

Using a version of jq with pick/1:

$ jq -nc '[[10,20],30] | pick(first) '
[[10,20]]

./jq -nc '[[10,20],30] | pick(.[length-1])'
[null,30]

$ jq -nc '[[10,20],30] | pick(last) '
jq: error (at <unknown>): Out of bounds negative array index

$ jq -nc '[[10,20],30] | pick(.[-1]) '
jq: error (at <unknown>): Out of bounds negative array index

So the failure of pick(last) could be resolved by redefining def last: .[length-1];
but is there a better solution? It's not clear to me why pick(.[-1]) should fail.

[itchyny]

Since path(last) yields [-1], null | setpath([-1]; 30) fails with the negative index error.

[pkoppstein]

Yes, but redefining last/1 as .[length-1] would resolve the issue, as suggested by:

$ jq -M -nc '[[10,20],30] | pick(.[length - 1])'
[null,30]
$ jq -M -nc '[[10,20],30] | pick(.[-1])'
jq: error (at <unknown>): Out of bounds negative array index

So I think the question is: is there a better way to resolve the issue?

[itchyny]

Oops, I misread the last sentence of your comment. Yes, that looks like a reasonable change.

[nicowilliams]

So I think the question is: is there a better way to resolve the issue?

I think making jv_set() jv_array_set() take negative indices would be the better fix.

EDIT: Well, it tries to:

jv jv_array_set(jv j, int idx, jv val) {
  assert(JVP_HAS_KIND(j, JV_KIND_ARRAY));

  if (idx < 0)   // <---- here
    idx = jvp_array_length(j) + idx;
  if (idx < 0) {
    jv_free(j);
    jv_free(val);
    return jv_invalid_with_msg(jv_string("Out of bounds negative array index"));
  }
  // copy/free of val,j coalesced
  jv* slot = jvp_array_write(&j, idx);
  jv_free(*slot);
  *slot = val;
  return j;
}

[nicowilliams]

: ; ./jq -cn '
def dbg(f): (f|debug|empty),.;
def pick(pathexps):
    . as $in
  | reduce (path(pathexps)|dbg("path: \(.)")) as $a (
        null;
        dbg("accumulating $a (\($a)) into \(.)")
      | setpath($a; $in|getpath($a))
    );

[0,1,2]|pick(last)'
["DEBUG:","path: [-1]"]
["DEBUG:","accumulating $a ([-1]) into null"]
jq: error (at <unknown>): Out of bounds negative array index

[nicowilliams]

The problem is that using .[-1] in last does work, but it won't work for pick/1 because in pick/1 we really need to know the actual absolute index.

I think what we might want here is for path(.[-1]) to produce [length - 1]. Emphasis on might.

[nicowilliams]

This patch fixes this, but maybe might break something else:

diff --git a/src/execute.c b/src/execute.c
index e1894aa..9d4be20 100644
--- a/src/execute.c
+++ b/src/execute.c
@@ -684,6 +684,14 @@ jv jq_next(jq_state *jq) {
         set_error(jq, jv_invalid_with_msg(msg));
         goto do_backtrack;
       }
+      if (jv_get_kind(k) == JV_KIND_NUMBER && jv_get_kind(t) == JV_KIND_ARRAY) {
+        int idx = jv_number_value(k);
+
+        if (idx < 0) {
+          jv_free(k);
+          k = jv_number(jv_array_length(jv_copy(t)) + idx);
+        }
+      }
       jv v = jv_get(t, jv_copy(k));
       if (jv_is_valid(v)) {
         path_append(jq, k, jv_copy(v));

With more context so it's easier to understand:

diff --git a/src/execute.c b/src/execute.c
index e1894aa..9d4be20 100644
--- a/src/execute.c
+++ b/src/execute.c
@@ -672,30 +672,38 @@ jv jq_next(jq_state *jq) {
     case INDEX:
     case INDEX_OPT: {
       jv t = stack_pop(jq);
       jv k = stack_pop(jq);
       // detect invalid path expression like path(reverse | .a)
       if (!path_intact(jq, jv_copy(t))) {
         char keybuf[15];
         char objbuf[30];
         jv msg = jv_string_fmt(
             "Invalid path expression near attempt to access element %s of %s",
             jv_dump_string_trunc(k, keybuf, sizeof(keybuf)),
             jv_dump_string_trunc(t, objbuf, sizeof(objbuf)));
         set_error(jq, jv_invalid_with_msg(msg));
         goto do_backtrack;
       }
+      if (jv_get_kind(k) == JV_KIND_NUMBER && jv_get_kind(t) == JV_KIND_ARRAY) {
+        int idx = jv_number_value(k);
+
+        if (idx < 0) {
+          jv_free(k);
+          k = jv_number(jv_array_length(jv_copy(t)) + idx);
+        }
+      }
       jv v = jv_get(t, jv_copy(k));
       if (jv_is_valid(v)) {
         path_append(jq, k, jv_copy(v));
         stack_push(jq, v);
       } else {
         jv_free(k);
         if (opcode == INDEX)
           set_error(jq, v);
         else
           jv_free(v);
         goto do_backtrack;
       }
       break;
     }

So we turn negative indices into positive ones in the implementation of .[<index>]. (Another branch to slow us down a bit.)

Now I think this patch is desirable. I can see more bugs cropping up due to path(.[<negative index>]) returning negative indices if, for example, one then accumulates those paths as pick/1 is doing then one alters . to add elements to arrays, then one acts on the accumulated paths which now no longer refer to the elements they referred to when they were observed.

I think it's unlikely that this patch breaks any existing jq code, though it is possible.

[pkoppstein]

I've incorporated @nicowilliams' change (#2716 (comment)) and reverted to the old def last:. There are no problems running all the *.test tests.

Should I scrap #2717 and create a new PR?

[pkoppstein]

Fixed by #2779