Upgrade to cats 1.0.0-MF #724

dwijnand · 2017-08-10T14:50:47Z

No description provided.

rkrzewski · 2017-08-11T09:38:12Z

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:

circe/modules/generic/shared/src/test/scala/io/circe/generic/AutoDerivedSuite.scala

Lines 127 to 134 in e9d99ba

    
           "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:

circe/modules/core/shared/src/main/scala/io/circe/SeqDecoder.scala

Line 28 in e9d99ba

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:

circe/modules/generic/shared/src/test/scala/io/circe/generic/SemiautoDerivedSuite.scala

Lines 27 to 28 in e9d99ba

    
           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 · 2017-08-11T09:42:37Z

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 · 2017-08-11T09:55:40Z

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 · 2017-08-11T13:15:18Z

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:

using semiauto derivation makes the test case succeed

implicit val decoder = io.circe.generic.semiauto.deriveFor[Int => Qux[String]].incomplete

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

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 · 2017-08-11T13:41:48Z

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 · 2017-08-11T13:59:54Z

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.

👍

codecov-io · 2017-08-15T10:52:33Z

Codecov Report

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

@@            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 · 2017-08-15T10:54:10Z

@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.

…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

…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

durban mentioned this pull request Aug 12, 2017

Cats 1.0.0-MF durban/seals#61

Closed

3 tasks

Upgrade to cats 1.0.0-MF

6ce4db1

dwijnand force-pushed the cats1 branch from d8ab41f to 6ce4db1 Compare August 15, 2017 10:52

wogan mentioned this pull request Aug 16, 2017

update to cats 1.0.0-MF #727

Closed

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

dwijnand deleted the cats1 branch August 16, 2017 19:21

rkrzewski mentioned this pull request Aug 20, 2017

avoid confusion of Int => T with IndexedSeq[T] under -Yparital-unification #728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to cats 1.0.0-MF #724

Upgrade to cats 1.0.0-MF #724

dwijnand commented Aug 10, 2017

rkrzewski commented Aug 11, 2017

dwijnand commented Aug 11, 2017

rkrzewski commented Aug 11, 2017

rkrzewski commented Aug 11, 2017

travisbrown commented Aug 11, 2017

rkrzewski commented Aug 11, 2017

codecov-io commented Aug 15, 2017 •

edited

Loading

dwijnand commented Aug 15, 2017

Upgrade to cats 1.0.0-MF #724

Upgrade to cats 1.0.0-MF #724

Conversation

dwijnand commented Aug 10, 2017

rkrzewski commented Aug 11, 2017

dwijnand commented Aug 11, 2017

rkrzewski commented Aug 11, 2017

rkrzewski commented Aug 11, 2017

travisbrown commented Aug 11, 2017

rkrzewski commented Aug 11, 2017

codecov-io commented Aug 15, 2017 • edited Loading

Codecov Report

dwijnand commented Aug 15, 2017

codecov-io commented Aug 15, 2017 •

edited

Loading