New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove the variance annotations on Encoder / Decoder #26

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@stew
Copy link
Contributor

stew commented Sep 29, 2014

I realize this might be a controversial change. I don't know how widely people
are taking advantage of the variance on these types. We have found that the
variance can cause problems in implicit search. Here's a minimal example that
demonstrates some of the problems we have been having:

import scodec.{Decoder, Codec, codecs}
import scala.collection.immutable.{IndexedSeq}

object Foo {
//  implicit val int32 = codecs.int32
  implicit def indexedSeq[A:Codec]: Codec[IndexedSeq[A]] =
    codecs.variableSizeBytes(codecs.int32, codecs.repeated(Codec[A]))
  val x = implicitly[Decoder[IndexedSeq[Int]]]
}

When trying to compile this with variance in place, the error we get is:

diverging implicit expansion for type scodec.Decoder[scala.collection.immutable.IndexedSeq[Int]]

The implicit should not be found in this case because of the missing int32
implicit. With the variance annotations removed we correctly get an implicit
not found. While this is not the end of the world, we have more complex
examples where an implicit should be found, but the variance gets us diverging
implicits.

remove the variance annotations on Encoder / Decoder
I realize this might be a controversial change. I don't know how widely people
are taking advantage of the variance on these types. We have found that the
variance can cause problems in implicit search.  Here's a minimal example that
demonstrates some of the problems we have been having:

    import scodec.{Decoder, Codec, codecs}
    import scala.collection.immutable.{IndexedSeq}

    object Foo {
    //  implicit val int32 = codecs.int32
      implicit def indexedSeq[A:Codec]: Codec[IndexedSeq[A]] =
        codecs.variableSizeBytes(codecs.int32, codecs.repeated(Codec[A]))
      val x = implicitly[Decoder[IndexedSeq[Int]]]
    }

When trying to compile this with variance in place, the error we get is:

    diverging implicit expansion for type scodec.Decoder[scala.collection.immutable.IndexedSeq[Int]]

The implicit should not be found in this case because of the missing int32
implicit. With the variance annotations removed we correctly get an implicit
not found. While this is not the end of the world, we have more complex
examples where an implicit should be found, but the variance gets us diverging
implicits.
@pchiusano

This comment has been minimized.

Copy link
Contributor

pchiusano commented Sep 29, 2014

I don't understand where the diverging implicit comes from in this example. Could you explain?

@pchiusano

This comment has been minimized.

Copy link
Contributor

pchiusano commented Sep 29, 2014

Btw, these examples yield a regular implicit not found error:

import scodec.{Decoder, Codec, codecs}
import scala.collection.immutable.{IndexedSeq}

object Foo {
//  implicit val int32 = codecs.int32
  implicit def indexedSeq[A:Codec]: Decoder[IndexedSeq[A]] =
    codecs.variableSizeBytes(codecs.int32, codecs.repeated(Codec[A]))
  val x = implicitly[Decoder[IndexedSeq[Int]]]
}
import scodec.{Decoder, Codec, codecs}
import scala.collection.immutable.{IndexedSeq}

object Foo {
//  implicit val int32 = codecs.int32
  implicit def indexedSeq[A:Codec]: Codec[IndexedSeq[A]] =
    codecs.variableSizeBytes(codecs.int32, codecs.repeated(Codec[A]))
  val x = implicitly[Codec[IndexedSeq[Int]]]
}
@mpilquist

This comment has been minimized.

Copy link
Contributor

mpilquist commented Sep 29, 2014

I'm in favor of keeping the variance annotations but I don't want them to cause undue problems. Hence, I'd like to gather more information here before making a decision. I'd like to understand why the compiler is reporting divergence in this case and I'd also like to do an inventory of projects that use scodec and see if they are impacted by loss of variance.

@mpilquist

This comment has been minimized.

Copy link
Contributor

mpilquist commented Sep 29, 2014

I can also get the proper implicit not found error by introducing a new type IDecoder[A] and mixing that in to Codec, then implicitly searching for an IDecoder instead of a Decoder:

import scodec.{Decoder, Codec, codecs}
import scala.collection.immutable.{IndexedSeq}

object Foo {
  //implicit val int32 = codecs.int32
  implicit def indexedSeq[A:Codec]: Codec[IndexedSeq[A]] =
    codecs.variableSizeBytes(codecs.int32, codecs.repeated(Codec[A]))
  val x = implicitly[scodec.IDecoder[IndexedSeq[Int]]]
}
@stew

This comment has been minimized.

Copy link
Contributor

stew commented Oct 1, 2014

Here's the minimal broken case:

trait Decoder[+A]
trait Codec[A] extends Decoder[A]
trait F[+A]

object Foo {
  implicit def foo[A:Codec]: Codec[F[A]] = null
  val x = implicitly[Decoder[F[Int]]]
}

If you make either F or Decoder invariant it works, if you make Codec covariant it works (and by works I mean implicit not found instead of diverging implicits)

@mpilquist

This comment has been minimized.

Copy link
Contributor

mpilquist commented Oct 1, 2014

Awesome, thanks!

How do you feel about introducing IDecoder and IEncoder? Each would override the parent methods to return IDecoder and IEncoder. Then implicit search could always be done via the invariant variants.

trait Decoder[+A] 
trait IDecoder[A] extends Decoder[A]
trait Codec[A] extends Decoder[A]
trait F[+A]

object Foo {
  implicit def foo[A:Codec]: Codec[F[A]] = null
  val x = implicitly[IDecoder[F[Int]]]
}
@stew

This comment has been minimized.

Copy link
Contributor

stew commented Oct 2, 2014

I think the right answer for us is going to be searching for Codecs and giving up the idea that we might have Decoders which exist without Encoders which, as Paul pointed out, works just fine.

The IDecoder thing would work, but I can't decide how much it's cleverness is the good kind of clever or the confusing kind of clever.

This whole thing seems like a scala bug, right? Should I be looking to file this with scala?

@pchiusano

This comment has been minimized.

Copy link
Contributor

pchiusano commented Oct 2, 2014

It does seem like a Scala bug. I'd actually open a ticket with that minimal
example and see what they say.

On Thu, Oct 2, 2014 at 10:37 AM, stew notifications@github.com wrote:

I think the right answer for us is going to be searching for Codecs and
giving up the idea that we might have Decoders which exist without Encoders
which, as Paul pointed out, works just fine.

The IDecoder thing would work, but I can't decide how much it's cleverness
is the good kind of clever or the confusing kind of clever.

This whole thing seems like a scala bug, right? Should I be looking to
file this with scala?


Reply to this email directly or view it on GitHub
#26 (comment).

@stew

This comment has been minimized.

Copy link
Contributor

stew commented Oct 2, 2014

@pchiusano

This comment has been minimized.

Copy link
Contributor

pchiusano commented Oct 2, 2014

Cool. You might want to wrap the code snippets in {{{ }}} to get the
formatting.

On Thu, Oct 2, 2014 at 11:05 AM, stew notifications@github.com wrote:

ticket: https://issues.scala-lang.org/browse/SI-8877


Reply to this email directly or view it on GitHub
#26 (comment).

@mpilquist

This comment has been minimized.

Copy link
Contributor

mpilquist commented Nov 6, 2014

Going to close this for now -- at least until we hear more on SI-8877. Thanks!

@mpilquist mpilquist closed this Nov 6, 2014

timperrett pushed a commit to Verizon/remotely that referenced this pull request Dec 1, 2014

Stop storing Encoders/Decoders separately
When we simplify Codecs to just store a map of codecs instead of
separate maps of encoders and decoders, and instead of implicitly
searching for Decoders, search for Codecs, we avoid the divirging
implicit problem disucssed here:
scodec/scodec#26

This gets us back onto the upstream scodecs instead of using our
third-party build.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment