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

Upgrade to cats 1.0.0-MF #724

Merged
merged 1 commit into from Aug 16, 2017

Conversation

Projects
None yet
4 participants
@dwijnand
Contributor

dwijnand commented Aug 10, 2017

No description provided.

@rkrzewski

This comment has been minimized.

Show comment
Hide comment
@rkrzewski

rkrzewski Aug 11, 2017

Contributor

I was just about to post almost identical PR :)
I'm also seeing the same test failures. I did some investigation and it appears that partial unification is messing with derivation of partial decoders.

In this test case:

"Decoder[Int => Qux[String]]" should "decode partial JSON representations" in forAll { (i: Int, s: String, j: Int) =>
val result = Json.obj(
"a" -> Json.fromString(s),
"j" -> Json.fromInt(j)
).as[Int => Qux[String]].map(_(i))
assert(result === Right(Qux(i, s, j)))
}

Decoding ends up here:
Left(DecodingFailure("CanBuildFrom for A", c.history))

My guess is that Function1[Int, Qux[String]] is partially applied as [A]Function1[Int, A] and this type has the shape of C type parameter of SeqDecoder[A, C[_]]. I have no idea though, how does the compiler arrive here through the implicit defs in https://github.com/circe/circe/blob/master/modules/core/shared/src/main/scala/io/circe/Decoder.scala

Interestingly, only auto derived decoder is affected with this problem. semi-auto derivation of incomplete decoder is OK:

implicit val decodeIntlessQux: Decoder[Int => Qux[String]] =
deriveFor[Int => Qux[String]].incomplete

Anyway, adding .disablePlugins(PartialUnification) to genericBase and genericExtrasBase makes the test failures go away. Hopefully it'll be possible to guide the compiler to avoid inferring SeqDecoder in this situation with additional constraints - I tried fiddling with that with no luck so far.

Contributor

rkrzewski commented Aug 11, 2017

I was just about to post almost identical PR :)
I'm also seeing the same test failures. I did some investigation and it appears that partial unification is messing with derivation of partial decoders.

In this test case:

"Decoder[Int => Qux[String]]" should "decode partial JSON representations" in forAll { (i: Int, s: String, j: Int) =>
val result = Json.obj(
"a" -> Json.fromString(s),
"j" -> Json.fromInt(j)
).as[Int => Qux[String]].map(_(i))
assert(result === Right(Qux(i, s, j)))
}

Decoding ends up here:
Left(DecodingFailure("CanBuildFrom for A", c.history))

My guess is that Function1[Int, Qux[String]] is partially applied as [A]Function1[Int, A] and this type has the shape of C type parameter of SeqDecoder[A, C[_]]. I have no idea though, how does the compiler arrive here through the implicit defs in https://github.com/circe/circe/blob/master/modules/core/shared/src/main/scala/io/circe/Decoder.scala

Interestingly, only auto derived decoder is affected with this problem. semi-auto derivation of incomplete decoder is OK:

implicit val decodeIntlessQux: Decoder[Int => Qux[String]] =
deriveFor[Int => Qux[String]].incomplete

Anyway, adding .disablePlugins(PartialUnification) to genericBase and genericExtrasBase makes the test failures go away. Hopefully it'll be possible to guide the compiler to avoid inferring SeqDecoder in this situation with additional constraints - I tried fiddling with that with no luck so far.

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Aug 11, 2017

Contributor

I completely forgot what Travis had said to me on twitter:

adding an sbt plugin to avoid providing type parameters in a single place seems kind of like overkill, though.

I'll try and get rid of the plugin entirely.

On a complete tangent, @rkrzewski what's the markup for these fancy code block references?

Contributor

dwijnand commented Aug 11, 2017

I completely forgot what Travis had said to me on twitter:

adding an sbt plugin to avoid providing type parameters in a single place seems kind of like overkill, though.

I'll try and get rid of the plugin entirely.

On a complete tangent, @rkrzewski what's the markup for these fancy code block references?

@rkrzewski

This comment has been minimized.

Show comment
Hide comment
@rkrzewski

rkrzewski Aug 11, 2017

Contributor

I agree, but it may show up as a problem on the Circe user's side: when they'll use Cats 1.+, they'll probably use -Ypartial-unification to have traverse work and then it'll interfere with codec derivation. In the tests only incomplete auto-derived decoders are affected but I can imagine that if any multi-parameter type constructor can be partially applied to conform to C[_] shape, SeqDecoder may pop-up in some unexpected places.

Regarding the code blocks, just paste URL or a source file on github and use a line range eg. #L127-L134 as the fragment.

Contributor

rkrzewski commented Aug 11, 2017

I agree, but it may show up as a problem on the Circe user's side: when they'll use Cats 1.+, they'll probably use -Ypartial-unification to have traverse work and then it'll interfere with codec derivation. In the tests only incomplete auto-derived decoders are affected but I can imagine that if any multi-parameter type constructor can be partially applied to conform to C[_] shape, SeqDecoder may pop-up in some unexpected places.

Regarding the code blocks, just paste URL or a source file on github and use a line range eg. #L127-L134 as the fragment.

@rkrzewski

This comment has been minimized.

Show comment
Hide comment
@rkrzewski

rkrzewski Aug 11, 2017

Contributor

I was trying to dig into the failure in AutoDerivedSuite Decoder[Int => Qux[String]] - should decode partial JSON representations
I tried bringing decoders into the test's scope in various ways, and here's what I found:

  1. using semiauto derivation makes the test case succeed
implicit val decoder = io.circe.generic.semiauto.deriveFor[Int => Qux[String]].incomplete
  1. doing what AutoDerivation and LowPriorityDecoders do (AFAICT) by hand also makes the test case succeed
implicit val decoder = implicitly[io.circe.export.Exported[Decoder[Int => Qux[String]]]].instance
  1. summoning the decoder directly has no effect (SeqDecoder is derived and testcase fails)
implicit val decoder = implicitly[Decoder[Int => Qux[String]]]

2 and 3 should basically be the same thing, right? Maybe it's a scalac bug?

The upside is that even with -Ypartial-unification on, semiauto provides a reliable workaround when automatic derivation goes wrong.

Contributor

rkrzewski commented Aug 11, 2017

I was trying to dig into the failure in AutoDerivedSuite Decoder[Int => Qux[String]] - should decode partial JSON representations
I tried bringing decoders into the test's scope in various ways, and here's what I found:

  1. using semiauto derivation makes the test case succeed
implicit val decoder = io.circe.generic.semiauto.deriveFor[Int => Qux[String]].incomplete
  1. doing what AutoDerivation and LowPriorityDecoders do (AFAICT) by hand also makes the test case succeed
implicit val decoder = implicitly[io.circe.export.Exported[Decoder[Int => Qux[String]]]].instance
  1. summoning the decoder directly has no effect (SeqDecoder is derived and testcase fails)
implicit val decoder = implicitly[Decoder[Int => Qux[String]]]

2 and 3 should basically be the same thing, right? Maybe it's a scalac bug?

The upside is that even with -Ypartial-unification on, semiauto provides a reliable workaround when automatic derivation goes wrong.

@travisbrown

This comment has been minimized.

Show comment
Hide comment
@travisbrown

travisbrown Aug 11, 2017

Member

Thanks for the investigation, @dwijnand, @rkrzewski.

I don't have a sense of why the compiler might think that SeqDecoder is a candidate here—there definitely shouldn't be CanBuildFrom instances for I => ? types.

My feeling is that we could release 0.9.0-M1 with this as a known issue (i.e. "if you use -Ypartial-unification and auto, incomplete decoders will not work"), and then make sure it's resolved before 0.9.0.

Member

travisbrown commented Aug 11, 2017

Thanks for the investigation, @dwijnand, @rkrzewski.

I don't have a sense of why the compiler might think that SeqDecoder is a candidate here—there definitely shouldn't be CanBuildFrom instances for I => ? types.

My feeling is that we could release 0.9.0-M1 with this as a known issue (i.e. "if you use -Ypartial-unification and auto, incomplete decoders will not work"), and then make sure it's resolved before 0.9.0.

@rkrzewski

This comment has been minimized.

Show comment
Hide comment
@rkrzewski

rkrzewski Aug 11, 2017

Contributor

I don't have a sense of why the compiler might think that SeqDecoder is a candidate here—there definitely shouldn't be CanBuildFrom instances for I => ? types.

Yeah, there must be a rouge CanBuidFrom instance around, because the decoder gets derived and then fails and runtime. Tracking down it's source is a promising line of investigation.

My feeling is that we could release 0.9.0-M1 with this as a known issue (i.e. "if you use -Ypartial-unification and auto, incomplete decoders will not work"), and then make sure it's resolved before 0.9.0.

👍

Contributor

rkrzewski commented Aug 11, 2017

I don't have a sense of why the compiler might think that SeqDecoder is a candidate here—there definitely shouldn't be CanBuildFrom instances for I => ? types.

Yeah, there must be a rouge CanBuidFrom instance around, because the decoder gets derived and then fails and runtime. Tracking down it's source is a promising line of investigation.

My feeling is that we could release 0.9.0-M1 with this as a known issue (i.e. "if you use -Ypartial-unification and auto, incomplete decoders will not work"), and then make sure it's resolved before 0.9.0.

👍

@durban durban referenced this pull request Aug 12, 2017

Closed

Cats 1.0.0-MF #61

3 of 3 tasks complete
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 15, 2017

Codecov Report

Merging #724 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #724      +/-   ##
==========================================
- Coverage   86.72%   86.63%   -0.09%     
==========================================
  Files          71       71              
  Lines        2267     2267              
  Branches       91       88       -3     
==========================================
- Hits         1966     1964       -2     
- Misses        301      303       +2
Impacted Files Coverage Δ
.../src/main/scala/io/circe/AccumulatingDecoder.scala 100% <100%> (ø) ⬆️
...n/scala/io/circe/streaming/ParsingEnumeratee.scala 78.57% <100%> (ø) ⬆️
...re/shared/src/main/scala/io/circe/JsonNumber.scala 96.03% <0%> (-1%) ⬇️
.../core/shared/src/main/scala/io/circe/Decoder.scala 90.65% <0%> (-0.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9d99ba...6ce4db1. Read the comment docs.

codecov-io commented Aug 15, 2017

Codecov Report

Merging #724 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #724      +/-   ##
==========================================
- Coverage   86.72%   86.63%   -0.09%     
==========================================
  Files          71       71              
  Lines        2267     2267              
  Branches       91       88       -3     
==========================================
- Hits         1966     1964       -2     
- Misses        301      303       +2
Impacted Files Coverage Δ
.../src/main/scala/io/circe/AccumulatingDecoder.scala 100% <100%> (ø) ⬆️
...n/scala/io/circe/streaming/ParsingEnumeratee.scala 78.57% <100%> (ø) ⬆️
...re/shared/src/main/scala/io/circe/JsonNumber.scala 96.03% <0%> (-1%) ⬇️
.../core/shared/src/main/scala/io/circe/Decoder.scala 90.65% <0%> (-0.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9d99ba...6ce4db1. Read the comment docs.

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Aug 15, 2017

Contributor

@travisbrown I went with your suggestion. However I didn't know where to document the known issue, so for now I've put it next to the reference to incomplete decoders in the docs.

Contributor

dwijnand commented Aug 15, 2017

@travisbrown I went with your suggestion. However I didn't know where to document the known issue, so for now I've put it next to the reference to incomplete decoders in the docs.

@Wogan Wogan referenced this pull request Aug 16, 2017

Closed

update to cats 1.0.0-MF #727

@travisbrown travisbrown merged commit 48e53ab into circe:master Aug 16, 2017

3 checks passed

codecov/patch 100% of diff hit (target 86.72%)
Details
codecov/project Absolute coverage decreased by -0.08% but relative coverage increased by +13.27% compared to e9d99ba
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dwijnand dwijnand deleted the dwijnand:cats1 branch Aug 16, 2017

rkrzewski added a commit to rkrzewski/circe that referenced this pull request Aug 20, 2017

void confusion of Int => T with IndexedSeq[T] under -Yparital-unifica…
…tion

I did further investigation of test failures related to `-Ypartial-unification` discovered in circe#724. Here are my findings:

1) Summoning the partial `Decoder` directly yields nonsensical result while summoning an `Exported[Decoder[...]]` yields correct result because `importedDecoder` is located in `LowerPriorityDecoder`, so `decodeCanBuildFrom` implicit has higher priority.

2) `decodeCanBuildFrom` is tried because under `-Ypartial-unification` `Int => Qux[String]` is partially unapplied as `[T]Int => T` to fit into `C[_]` shape of second parameter to `def decodeCanBuildFrom[A, C[_]]`.

3) `decodeCanBuildFrom` succeeds because an instance of `CanBuildFrom[Nothing, Qux[String], Int => Qux[String]]` exists. The instance, as it usually happens, is helpfully provided by `scala.Predef`: `implicit def fallbackStringCanBuildFrom[T]: CanBuildFrom[String, T, immutable.IndexedSeq[T]]`. `CanBuildFrom` has variance of `[-,-,+]` so the types align nicely.

This means that the problem is limited to partial decoders with `Int` typed hole, but it's also means it's easy to remedy by adding a constraint on `decodeCanBuildFrom` parameter to ensure it actually a collection, not an `Int => _`. I've used `Traversable`, which in turn required adding separate case for `Array` that is not a `Traversable` but needs a `CanBuildFrom` to handle.

This patch does not turn on `-Ypartial-unification`, but I've verified that all the tests pass with and without this option on Scala 2.11 and 2.12

rkrzewski added a commit to rkrzewski/circe that referenced this pull request Aug 20, 2017

avoid confusion of Int => T with IndexedSeq[T] under -Yparital-unific…
…ation

I did further investigation of test failures related to `-Ypartial-unification` discovered in circe#724. Here are my findings:

1) Summoning the partial `Decoder` directly yields nonsensical result while summoning an `Exported[Decoder[...]]` yields correct result because `importedDecoder` is located in `LowerPriorityDecoder`, so `decodeCanBuildFrom` implicit has higher priority.

2) `decodeCanBuildFrom` is tried because under `-Ypartial-unification` `Int => Qux[String]` is partially unapplied as `[T]Int => T` to fit into `C[_]` shape of second parameter to `def decodeCanBuildFrom[A, C[_]]`.

3) `decodeCanBuildFrom` succeeds because an instance of `CanBuildFrom[Nothing, Qux[String], Int => Qux[String]]` exists. The instance, as it usually happens, is helpfully provided by `scala.Predef`: `implicit def fallbackStringCanBuildFrom[T]: CanBuildFrom[String, T, immutable.IndexedSeq[T]]`. `CanBuildFrom` has variance of `[-,-,+]` so the types align nicely.

This means that the problem is limited to partial decoders with `Int` typed hole, but it's also means it's easy to remedy by adding a constraint on `decodeCanBuildFrom` parameter to ensure it actually a collection, not an `Int => _`. I've used `Traversable`, which in turn required adding separate case for `Array` that is not a `Traversable` but needs a `CanBuildFrom` to handle.

This patch does not turn on `-Ypartial-unification`, but I've verified that all the tests pass with and without this option on Scala 2.11 and 2.12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment