ClojureScript

nth function produces different results from clojure when using a negative index on a sequence

Details

  • Patch:
    Code and Test
  • Approval:
    Accepted

Description

(nth (range 2) -2) produces an out-of-bounds error for clojure; for clojurescript it returns -2
(nth (seq [3 4] -2)) produces an out-of-bounds error for clojure; for clojurescript it returns nil
I expected these to fail.

I think that the clojurescript check (src/main/cljs/cljs/core.cljs line ~1763 "(not (number? n))" is not sufficient,
it could also check for non-negative.

  1. CLJS-2113.patch
    21/Jun/17 5:20 PM
    2 kB
    Michael A Wright
  2. CLJS-2113-v2.patch
    23/Jun/17 8:20 AM
    2 kB
    Mike Fikes
  3. CLJS-2113-v3.patch
    23/Jun/17 8:54 AM
    2 kB
    Mike Fikes
  1. Screenshot 2017-06-21 13.27.23.png
    53 kB
    21/Jun/17 3:31 PM
  2. Screenshot 2017-06-21 13.28.30.png
    46 kB
    21/Jun/17 3:31 PM
  3. Screenshot 2017-06-21 13.29.16.png
    44 kB
    21/Jun/17 3:31 PM
  4. Screenshot 2017-06-21 13.29.58.png
    47 kB
    21/Jun/17 3:31 PM

Activity

Hide
Michael A Wright added a comment -

comparing cljs with clj online REPLs
and between figwheel and lein REPLs

Show
Michael A Wright added a comment - comparing cljs with clj online REPLs and between figwheel and lein REPLs
Hide
Michael A Wright added a comment - - edited

New test and updated argument check fix.
Tested using JavaScriptCore, Nashorn, SpiderMonkey, and V8 locally.
I also tested my patch in Chrome and Safari using a figwheel app - a negative index passed to nth now gets the expected error.
./script/test # passed
./script/build

  1. updated my project.clj dependencies
  2. rebuilt/reran my app
Show
Michael A Wright added a comment - - edited New test and updated argument check fix. Tested using JavaScriptCore, Nashorn, SpiderMonkey, and V8 locally. I also tested my patch in Chrome and Safari using a figwheel app - a negative index passed to nth now gets the expected error. ./script/test # passed ./script/build
  1. updated my project.clj dependencies
  2. rebuilt/reran my app
Hide
Mike Fikes added a comment -

I have 3 comments:

  1. Perf testing with V8 and JavaScriptCore doesn't show a slowdown with {{(simple-benchmark [coll [1 2 3]] (nth coll 1) 100000000)}}
  2. The error message could more accurately be "Index argument to nth must be a non-negative number"
  3. Clojure returns nil for (nth nil -1) while this patch does not
Show
Mike Fikes added a comment - I have 3 comments:
  1. Perf testing with V8 and JavaScriptCore doesn't show a slowdown with {{(simple-benchmark [coll [1 2 3]] (nth coll 1) 100000000)}}
  2. The error message could more accurately be "Index argument to nth must be a non-negative number"
  3. Clojure returns nil for (nth nil -1) while this patch does not
Hide
Mike Fikes added a comment -

Given that non-negativity checks are done (or delegated) for the other cond branches, I wonder if such a check could be added to the cond branch for (implements? IIndexed coll) at
https://github.com/clojure/clojurescript/blob/fb14b183e6dc2485f67ba7f7f668981dd7cdfd3c/src/main/cljs/cljs/core.cljs#L1770

If so, perhaps the same could be done for (native-satisfies? IIndexed coll).

Perhaps, as a related side issue, not-found could be returned for that (native-satisfies? IIndexed coll) branch in the 3-arity version.

(It is not clear to me whether the (native-satisfies? IIndexed coll) branches would need both lower and upper bound checks.)

Show
Mike Fikes added a comment - Given that non-negativity checks are done (or delegated) for the other cond branches, I wonder if such a check could be added to the cond branch for (implements? IIndexed coll) at https://github.com/clojure/clojurescript/blob/fb14b183e6dc2485f67ba7f7f668981dd7cdfd3c/src/main/cljs/cljs/core.cljs#L1770 If so, perhaps the same could be done for (native-satisfies? IIndexed coll). Perhaps, as a related side issue, not-found could be returned for that (native-satisfies? IIndexed coll) branch in the 3-arity version. (It is not clear to me whether the (native-satisfies? IIndexed coll) branches would need both lower and upper bound checks.)
Hide
Mike Fikes added a comment -

It appears that implementations of IIndexed do the needed checking (including the results of `(seq [3 4])`), so the problem is really isolated to Range. The attached v2 patch is an alternative that fixes this specific issue.

Show
Mike Fikes added a comment - It appears that implementations of IIndexed do the needed checking (including the results of `(seq [3 4])`), so the problem is really isolated to Range. The attached v2 patch is an alternative that fixes this specific issue.
Hide
Mike Fikes added a comment -

The -v2 patch has a botched test in it. Attaching a rectified -v3 patch.

Show
Mike Fikes added a comment - The -v2 patch has a botched test in it. Attaching a rectified -v3 patch.
Hide
Mike Fikes added a comment -

Benchmarking:

Before -v3 patch:

JVM/Node:

$ rm -rf .cljs_node_repl/
orion:(master)clojurescript$ script/noderepljs
ClojureScript Node.js REPL server listening on 54800
To quit, type: :cljs/quit
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 12105 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 12381 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 12570 msecs
nil

self-host/JSC:

$ planck -q
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 3933 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 3914 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 3936 msecs
nil

After -v3 patch:

JVM/Node:

$ script/noderepljs
ClojureScript Node.js REPL server listening on 52136
To quit, type: :cljs/quit
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 12614 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 13156 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 13101 msecs
nil

self-host/JSC:

$ planck -q
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 3951 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 3954 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 3944 msecs
nil
Show
Mike Fikes added a comment - Benchmarking: Before -v3 patch: JVM/Node:
$ rm -rf .cljs_node_repl/
orion:(master)clojurescript$ script/noderepljs
ClojureScript Node.js REPL server listening on 54800
To quit, type: :cljs/quit
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 12105 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 12381 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 12570 msecs
nil
self-host/JSC:
$ planck -q
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 3933 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 3914 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 3936 msecs
nil
After -v3 patch: JVM/Node:
$ script/noderepljs
ClojureScript Node.js REPL server listening on 52136
To quit, type: :cljs/quit
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 12614 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 13156 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 13101 msecs
nil
self-host/JSC:
$ planck -q
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 3951 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 3954 msecs
nil
cljs.user=> (simple-benchmark [coll (range 3)] (nth coll 1) 100000000)
[coll (range 3)], (nth coll 1), 100000000 runs, 3944 msecs
nil

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: