(presumably) Bug in IndexedSeq Index typeclass

[Chris Marshall]

  scala> import scalaz._

  import scalaz._

  scala> import scalaz.syntax.index._

  import scalaz.syntax.index._

Now look at the List instance

  scala> import scalaz.std.list._

  import scalaz.std.list._

  scala> List('a, 'b, 'c) index -1

  res3: Option[Symbol] = None

So far so good. If we import the IndexedSeq instance, however...

  scala> import scalaz.std.indexedSeq._

  import scalaz.std.indexedSeq._

  scala> List('a, 'b, 'c).toIndexedSeq index -1

  java.lang.IndexOutOfBoundsException: -1

        at scala.collection.immutable.Vector.checkRangeConvert(Vector.scala:137)

        at scala.collection.immutable.Vector.apply(Vector.scala:127)

        at scalaz.std.IndexedSeqSubInstances$$anon$1.index(IndexedSeq.scala:45)

        at scalaz.std.IndexedSeqSubInstances$$anon$1.index(IndexedSeq.scala:43)

        at scalaz.syntax.IndexOps$class.index(IndexSyntax.scala:8)

        at scalaz.syntax.ToIndexOps$$anon$1.index(IndexSyntax.scala:21)

The offending code is:

  def index[A](fa: IxSq[A], i: Int) = if (fa.size > i) Some(fa(i)) else None

Which should be

  def index[A](fa: IxSq[A], i: Int) = if (i >= 0 && fa.size > i) Some(fa(i)) else None

Chris

[Jason Zaugg]

On Mon, Apr 29, 2013 at 6:05 PM, Chris Marshall <oxbow...@gmail.com> wrote:

The offending code is:

  def index[A](fa: IxSq[A], i: Int) = if (fa.size > i) Some(fa(i)) else None

Which should be

  def index[A](fa: IxSq[A], i: Int) = if (i >= 0 && fa.size > i) Some(fa(i)) else None

Or (in both cases),`fa.lift.apply(i)`

-jason

[Lars Hupel]

In issue #278 [0] I proposed to get rid of `Index` (and `Length`)
completely. The first step, the removal of `MetricSpace` and `BKTree` is
almost complete [1].

Once #347 gets merged, there are no remaining uses of `Index` and
`Length` in scalaz left.

Technically, we don't even need a separate type class for it. It could
be replaced by a `index` method on `Foldable` with a default
implementation based on `foldl`. Same for `length`.

As Kris pointed out in [1], those default implementations would have
O(n) time complexity. Instances of `Foldable` could override those
definitions, though.

Chris, what is your use case for `Index`? Would it still be covered if
`index` became a method on `Foldable`?


[0] https://github.com/scalaz/scalaz/issues/278
[1] https://github.com/scalaz/scalaz/pull/347

[Chris Marshall]

I just want to be able to use Option to indicate absence rather than the sentinel value of -1 because I prefer it. I'm not passing types around and requiring an Index typeclass, so index being a method on Foldable covers it just fine!

Chris

--
You received this message because you are subscribed to the Google Groups "scalaz" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scalaz+un...@googlegroups.com.
To post to this group, send email to sca...@googlegroups.com.
Visit this group at http://groups.google.com/group/scalaz?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[Chris Marshall]

Is it intended for this change to be released as v8.0 as it will not be source compatible, let alone binary compatible? Not that it matters much whether it's 7.1 or 8 but please do not release as 7.0.1 :-s

Chris

On Mon, Apr 29, 2013 at 5:35 PM, Lars Hupel <hu...@in.tum.de> wrote:

[Lars Hupel]

> Is it intended for this change to be released as v8.0 as it will not be
> source compatible, let alone binary compatible? Not that it matters much
> whether it's 7.1 or 8 but please do not release as 7.0.1 :-s

I was just about writing a mail detailing that :-)

The fix proposed by Jason is binary compatible, since it just changes an
implementation. The current behaviour is a bug and will be fixed in 7.0.1.

The larger task � removing `Length` and `Index` � will be addressed in
7.1.0.