isi-vista / immutablecollections

A library for immutable collections, in the spirit of Guava's Immutable Collections.
MIT License
3 stars 2 forks source link

_SingletonImmutableSet crashes on __getitem__ with an index of -1 #25

Closed ConstantineLignos closed 5 years ago

ConstantineLignos commented 5 years ago

This should be an easy fix; just behave the same for -1 as 0 in the conditional if item == 0 in __getitem__.

Minimal example:

$ python -c 'from immutablecollections import ImmutableSet; x = ImmutableSet.of([1]); x[-1]'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/constantinelignos/repos/immutablecollections/immutablecollections/immutableset.py", line 507, in __getitem__
    raise IndexError(f"Index {item} out-of-bounds for size 1 ImmutableSet")
IndexError: Index -1 out-of-bounds for size 1 ImmutableSet
gabbard commented 5 years ago

Assigned to @nicomitchell. @nicomitchell : first make a failing test, then do the fix

ConstantineLignos commented 5 years ago

This patch looks to be a regression overall:

>>> x = [1]
>>> x[0]
1
>>> x[-1]
1
>>> x[1]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError: list index out of range
>>> x[-2]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError: list index out of range
>>> from immutablecollections import ImmutableSet
>>> x = ImmutableSet.of([1])
>>> x[0]
1
>>> x[-1]
1
>>> x[1]
1
>>> x[-2]
1

The cause is the logic if item == 0 or -1, which always evaluates to True because -1 is truthy. It needs to be either if item == 0 or item == 1 or if item in (0, 1). Ideally we'd benchmark both. Can we also add the above examples to the tests?

gabbard commented 5 years ago

Oh, ugh, I can't believe I missed that. @nicomitchell , can you fix ASAP? Pick either implementation @ConstantineLignos suggests and make an issue to benchmark the other one as well later.

gabbard commented 5 years ago

(or just add it to the list of things to benchmark on #14

ConstantineLignos commented 5 years ago

From some lazy command-line benchmarking, my money is on item in (0, -1) without any effort to try to precompute/store that tuple. But it really doesn't matter because they're all very similar.

$ python -m timeit '[x == 0 or x == -1 for x in range (-1, 2)]'
1000000 loops, best of 3: 0.464 usec per loop
$ python -m timeit '[x in (0, -1) for x in range (-1, 2)]'
1000000 loops, best of 3: 0.439 usec per loop
$ python -m timeit -s 'y = (0, -1)' '[x in y for x in range (-1, 2)]'
1000000 loops, best of 3: 0.473 usec per loop
gabbard commented 5 years ago

The other thing to benchmark against is having SingletonImmutableList just stored a tuple as its field and then delegating to that

gabbard commented 5 years ago

Closed by #28