immutable-js / immutable-js

Immutable persistent data collections for Javascript which increase efficiency and simplicity.
https://immutable-js.com
MIT License
32.97k stars 1.79k forks source link

Strange behavior with Seq range, reverse #1730

Closed algoshipda closed 1 year ago

algoshipda commented 5 years ago

What happened

Seq([1, 2, 3]).zip(Range())
  .reverse()
// expected: [[3, 2], [2, 1], [1, 0]]
// actual: [[3, Infinity], [2, Infinity], [1, Infinity]]

and run this code below with https://clojurescript.io/

(reverse (map vector [1 2 3] (range)))
; produce ([3 2] [2 1] [1 0])

I think those codes are equivalent and Clojure's output is correct output. Is this desired behavior?

Immutablejs Version: 4.0.0-rc.12

jdeniau commented 3 years ago

It might be a side-effect of Range function defaulting to Infinity. You can see #1801 for more informations. I think this will be fixed once we decide an implementation on #1801.

You can force the behaviour as a matter of fact:

Seq([1, 2, 3]).zip(Range(0, 3))
  .reverse()
// results as expected: : [[3, 2], [2, 1], [1, 0]]
jdeniau commented 1 year ago

OK I think I do understand the issue:

actions are deferred to the latest moment possible, to [0, 1, 2] is not generated before the reverse call. When you call reverse, then we calculate like that:

But as the "end" value is Infinity, then it will take Infinity, Infinity - 1 (resolved as Infinity) and Infinity - 2 (same here).

1967 does change the Range function and will force you to have a start and end value, so I will not fix this as you will forced to do so in your code (unless you specify Infinity yourself, but then that's up to you 😅 )

In a probably more complex example, you will need to do that:

const mySequence: Array<number> = getSequence();

Seq(mySequence).zip(Range(0, mySequence.length - 1)).reverse();

You can change the reverse position if you think that it's more readable:

const mySequence: Array<number> = getSequence();

Seq(mySequence)reverse().zip(Range(mySequence.length - 1, 0));
jdeniau commented 1 year ago

Closing as you should explicitly set start and end values in your range in 4.x version