Array slices

[glyn]

No description provided.

[gregsdennis]

I think we need to add an "optional" property to the tests (or break them out to another section). The "big num" cases can't be supported by all frameworks, and we shouldn't expect it of them. There will be other cars like this as well.

I know we have a "skip" setting, but that means modifying the CTS after I've downloaded it. That should be considered bad practice. The suite should run as-is.

[mkmik]

The spec says:

A negative index j selects an element of an array of length len if and only if 0 <= j + len < len, in which case it selects the same element as the non-negative index j + len.

then it describes the iterations as:

for (i = start; i < end; i = i + step) {
  ...
}

what should happen if you call this on an input slice of length 5?:

    let s = array_slice(j.as_array().unwrap(), -5, 0, 1);

My interpretation is that it would yield

i = -5;
i < 0 //  true
push(arr[i + len]) // arr[-5+5] === arr[0]
i = i+1 //  -4
i < 0 //  true
push(arr[i + len]) // arr[-4+5] === arr[1]
...

[glyn]

This is a good example of a reference implementation and compliance test suite turning up a hole in the spec.

I think the spec should say (only more formally/precisely) that negative start is syntactic sugar for start + len and similarly for negative end. But then start and end need to be desugared before plugging them into the relevant for loop.

So, for an array of length 5, [-5, 0, 1] corresponds to the elements indexed by the values of i in the for loop:

for (i = 0; i < 0; i = i + 1) {
  ...
}

of which there are none, so the result is empty.

[mkmik]

Desugaring before iterating makes most sense to me as well.

I was tinkering with the consequences if doing it the other way around and I don't think it would provide any advantage. An interesting consequence is that it allows the resulting slice to be longer than the input array (and contain repeated elements) which I think would be quite confusing.

[glyn]

I think the implementation is correct and we just need to fix the spec. ;-)

[mkmik]

the privileges of a spec writer! :-)

[mkmik]

I wonder if something like this would have less special casing: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=dd7180d6586e5a511b4e56b0058e5562

While I don't think it's fully correct w.r.t overflows etc, but has a few advantages:

1. no temporary array; just uses rust's own array slice iterator and the ability to reverse any iterator
2. no risk of "cpu attack", since we're not iterating user provided bounds anyway; worst case of bugs we get a panic accessing a slice out of bounds
3. less duplication of code; the two branches of step > 0 and step < 0 in the PR contain quite a lot of common code with subtle differences

[glyn]

I think we need to add an "optional" property to the tests (or break them out to another section). The "big num" cases can't be supported by all frameworks, and we shouldn't expect it of them. There will be other cars like this as well.

The spec is currently silent about supported precisions of array indices. Probably needs firming up in some way.

It's interesting because for any given language or machine architecture, there will exist large values which exceed the built-in capabilities of the language or architecture.

I know we have a "skip" setting, but that means modifying the CTS after I've downloaded it. That should be considered bad practice. The suite should run as-is.

Depending on how the spec pans out, one option would be to add some kind of "category" field to the tests so that those which may be optional in some sense can be skipped en masse. I'm reticent to do that prematurely. Why not clamp the values to a suitable range (e.g. signed 31 bit) for now? Or is that too inconvenient?

[gregsdennis]

I'm happy for this to merge for now. We can come back to it when the spec addresses it. Let's discuss in your new issue.

[glyn]

I'm happy for this to merge for now. We can come back to it when the spec addresses it. Let's discuss in your new issue.

I agree the code is good, but I'd prefer to maintain consistency with the spec. I'll merge this PR once the spec is updated.

[glyn]

Blocked on ietf-wg-jsonpath/draft-ietf-jsonpath-base#31

[glyn]

@gregsdennis I'd appreciate your review of the final commit which tests the behaviour when integer representations of array indices and slice parameters overflow. This aims to enforce the current spec.

[gregsdennis]

Shouldn't the implementation at least attempt to parse the invalid selectors? It looks like cts.rs is just skipping ones that are marked as invalid.

[glyn]

@gregsdennis wrote:

Shouldn't the implementation at least attempt to parse the invalid selectors? It looks like cts.rs is just skipping ones that are marked as invalid.

I'm not sure what gives that impression. The parse function of the implementation is executed in the code below from cts.rs regardless of whether the testcase says the selector is invalid:

                let path = jsonpath::parse(&t.selector);

                if let Ok(ref p) = path {

The if statement then tests that parsing failed if and only if the testcase says the selector is invalid.

[gregsdennis]

Shows how much I know about rust.

[glyn]

I think this is ready to merge now the spec is updated. @mkmik or @gregsdennis: please approve.