Improve handling of non-integer numeric indices (fix #2815) #2821

nicowilliams · 2023-08-04T05:08:49Z

require slice indices to be integers when writing
output null when using slices with non-integer numeric indices to read
require indices to be integers when writing

emanuele6

I don't like that .[1.5] returns null. I would prefer an error, and it just rounded down to .[1].
I prefer rounding down the most.

emanuele6 · 2023-08-04T12:00:02Z

src/jv_aux.c

+  if (dstart < 0) dstart = 0;
+  if (dstart > len) dstart = len;
+
+  int start = (int)dstart;


Like for the modulo operator, this is unspecified behaviour if dstart is an integer according to jv_is_integer(), but it is bigger/smaller than INT_MAX/INT_MIN.

I think we should add some jv_int_value() and jv_intmax_value() functions that round and clamp numbers consistently, and use those everywhere we need an integer instead of (int)jv_number_value().

nicowilliams · 2023-08-04T14:59:34Z

I don't like that .[1.5] returns null. I would prefer an error, and it just rounded down to .[1].
I prefer rounding down the most.

I'm confused. You would prefer an error, but you prefer rounding down. Which is it?

emanuele6 · 2023-08-04T15:14:34Z

I prefer rounding down.
But I think an error would still be better than returning null.

I would prefer an error, and it just rounded down to .[1].

Oops, I meant to write "I would prefer an error, or that it just rounded down to .[1]".

nicowilliams · 2023-08-04T15:27:59Z

@emanuele6 yeah, I too would prefer either an error or rounding down. I'll pick one!

itchyny · 2023-08-04T21:18:21Z

+1 for rounding down

nicowilliams · 2023-08-17T15:54:44Z

Oh, I have to do the rounding down thing.

wader · 2023-08-17T18:34:32Z

src/jv_aux.c

+  if (dstart < INT_MIN) dstart = INT_MIN;
+  if (dstart > INT_MAX) dstart = INT_MAX;
+  if (dend   < INT_MIN) dend   = INT_MIN;
+  if (dend   > INT_MAX) dend   = INT_MAX;


Is it ok to mix a double and INT_MIN/MAX? guaranteed to fit without losing "integer" precision?

Good question. If sizeof(int) is 8 then... no. I'll fix it.

I've pushed a fix for this.

wader · 2023-08-17T18:36:06Z

Think i prefer rounding down also

nicowilliams · 2023-08-17T19:46:20Z

Rounding down is what I went with.

nicowilliams · 2023-08-17T23:19:00Z

Note that for slices we were already rounding the start down and the end up (because it's exclusive) before (in 1.6), and we still are.

nicowilliams · 2023-08-23T21:05:42Z

Is this ready?

itchyny · 2023-08-23T23:25:57Z

src/jv_aux.c

+      idx += jv_array_length(jv_copy(t));
+    v = jv_array_get(t, idx);
+    if (!jv_is_valid(v)) {
+      jv_free(v);
      v = jv_null();


Previously .[nan] emits null due to the jv_is_integer checking but now we rely on undefined behavior (on my M1 macOS, [1] | .[nan] emits 1 in this branch, previously null). We should check isnan first.

itchyny · 2023-08-24T00:02:44Z

src/jv_aux.c

+  if (dstart < 0) dstart += len;
+  if (dend < 0) dend += len;
+  if (dstart < 0) dstart = 0;
+  if (dstart > len) dstart = len;


If you clamp indices into [0, len], you don't need to compare against integer min/max. I think we can simplify as follows.

if (dstart < 0) dstart += len; if (dstart < 0 || isnan(dstart)) start = 0; else if (dstart > len) start = len; else start = (int)dstart; if (dend < 0) dend += len; if (dend > len || isnan(dend)) end = len; else if (dend < start) end = start; else if ((int)dend == dend) end = (int)dend; else end = (int)dend + 1;

if (dstart < 0 || isnan(dstart)) start = 0;

won't do if we want NaN indices to yield null as in 1.6.

But for slicing,

$ jq -n '[1,2,3] | .[nan:1]' [ 1 ]

This behavior might depend on an undefined behavior though. My comment above deals nan as null i.e. .[nan:N] == .[:N] and .[N:nan] == .[N:].

I see jq 1.6 trip an assertion when the start index of an array slice is a NaN:

: ; jq-1.6 -cn '[1,2,3] | .[nan:1]' jq-1.6: src/jv_aux.c:47: parse_slice: Assertion `0 <= start && start <= end && end <= len' failed. Abort

But not for the end index:

: ; jq-1.6 -cn '[1,2,3] | .[1:nan]' []

So slicing with NaNs was UB in 1.6.

itchyny · 2023-08-24T03:01:45Z

src/jv_aux.c

-static int parse_slice(jv j, jv slice, int* pstart, int* pend) {
+enum slice_for { FOR_READ, FOR_WRITE };
+
+static jv parse_slice(jv j, jv slice, int* pstart, int* pend, enum slice_for slice_for) {


Looks like slice_for is unused.

src/jv_aux.c

as itchyny said, lower bound checking before casting is missing

nicowilliams · 2023-08-24T18:18:21Z

as itchyny said, lower bound checking before casting is missing

How about now?

itchyny · 2023-08-27T09:29:00Z

src/jv_aux.c

+
+  if (isnan(dend))   dend = len;
+  if (dend < 0)      dend += len;
+  if (dend < 0)      dend  = start;


The operator looks misaligned.

emanuele6 · 2023-08-27T10:32:18Z

Thank you!

nicowilliams · 2023-08-27T16:59:45Z

As this was the last issue/PR open with 1.7 as its milestone, I've force-pushed a new jq-1.7rc2 tag, and that should make a new release.

nicowilliams requested a review from emanuele6 August 4, 2023 05:08

nicowilliams added this to the 1.7 release milestone Aug 4, 2023

emanuele6 reviewed Aug 4, 2023

View reviewed changes

nicowilliams force-pushed the fix_2815 branch 2 times, most recently from abdda81 to 28c8a82 Compare August 17, 2023 15:52

nicowilliams force-pushed the fix_2815 branch from 28c8a82 to 243aab6 Compare August 17, 2023 16:10

wader reviewed Aug 17, 2023

View reviewed changes

nicowilliams force-pushed the fix_2815 branch from 243aab6 to 9ca74f7 Compare August 17, 2023 23:16

itchyny reviewed Aug 24, 2023

View reviewed changes

nicowilliams force-pushed the fix_2815 branch from 9ca74f7 to 3e93639 Compare August 24, 2023 02:56

itchyny reviewed Aug 24, 2023

View reviewed changes

nicowilliams force-pushed the fix_2815 branch from 3e93639 to 2307658 Compare August 24, 2023 04:30

emanuele6 previously approved these changes Aug 24, 2023

View reviewed changes

itchyny reviewed Aug 24, 2023

View reviewed changes

src/jv_aux.c Show resolved Hide resolved

Improve handling of non-integer numeric indices (fix jqlang#2815)

dd1fd7a

nicowilliams force-pushed the fix_2815 branch from 2307658 to dd1fd7a Compare August 24, 2023 14:17

itchyny reviewed Aug 27, 2023

View reviewed changes

src/jv_aux.c

if (isnan(dend)) dend = len;

if (dend < 0) dend += len;

if (dend < 0) dend = start;

Copy link

Contributor

itchyny Aug 27, 2023

Choose a reason for hiding this comment

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

The operator looks misaligned.

itchyny approved these changes Aug 27, 2023

View reviewed changes

emanuele6 approved these changes Aug 27, 2023

View reviewed changes

emanuele6 merged commit 0e067ef into jqlang:master Aug 27, 2023
28 checks passed

emanuele6 mentioned this pull request Sep 28, 2023

Actually use number correctly casted from double to int as index #2916

Merged

emanuele6 added the bug label Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of non-integer numeric indices (fix #2815) #2821

Improve handling of non-integer numeric indices (fix #2815) #2821

nicowilliams commented Aug 4, 2023

emanuele6 left a comment

emanuele6 Aug 4, 2023 •

edited

Loading

nicowilliams Aug 17, 2023

nicowilliams commented Aug 4, 2023

emanuele6 commented Aug 4, 2023 •

edited

Loading

nicowilliams commented Aug 4, 2023

itchyny commented Aug 4, 2023

nicowilliams commented Aug 17, 2023

wader Aug 17, 2023 •

edited

Loading

nicowilliams Aug 17, 2023

nicowilliams Aug 17, 2023

wader commented Aug 17, 2023

nicowilliams commented Aug 17, 2023

nicowilliams commented Aug 17, 2023

nicowilliams commented Aug 23, 2023

itchyny Aug 23, 2023

itchyny Aug 24, 2023

nicowilliams Aug 24, 2023

itchyny Aug 24, 2023

itchyny Aug 24, 2023 •

edited

Loading

nicowilliams Aug 24, 2023

itchyny Aug 24, 2023

nicowilliams commented Aug 24, 2023

itchyny Aug 27, 2023

emanuele6 commented Aug 27, 2023

nicowilliams commented Aug 27, 2023

Improve handling of non-integer numeric indices (fix #2815) #2821

Improve handling of non-integer numeric indices (fix #2815) #2821

Conversation

nicowilliams commented Aug 4, 2023

emanuele6 left a comment

Choose a reason for hiding this comment

emanuele6 Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicowilliams commented Aug 4, 2023

emanuele6 commented Aug 4, 2023 • edited Loading

nicowilliams commented Aug 4, 2023

itchyny commented Aug 4, 2023

nicowilliams commented Aug 17, 2023

wader Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wader commented Aug 17, 2023

nicowilliams commented Aug 17, 2023

nicowilliams commented Aug 17, 2023

nicowilliams commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itchyny Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicowilliams commented Aug 24, 2023

Choose a reason for hiding this comment

emanuele6 commented Aug 27, 2023

nicowilliams commented Aug 27, 2023

emanuele6 Aug 4, 2023 •

edited

Loading

emanuele6 commented Aug 4, 2023 •

edited

Loading

wader Aug 17, 2023 •

edited

Loading

itchyny Aug 24, 2023 •

edited

Loading