http4s icon indicating copy to clipboard operation
http4s copied to clipboard

Make message types covariant in their effect type

Open bplommer opened this issue 4 years ago • 7 comments

Depends on #5229. Closes https://github.com/http4s/http4s/issues/3114.

It's a big diff so here's a breakdown:

  • 1672e6dbb23d40a3d1af22d060be3c513450b20f is the core change.
  • 2d140833d81ee6e362302f08e336df5478f0611a removes no-longer-needed type arguments and annotations from production code; af01e9e6e8b83f1eabf24f1f4fee7bfc32c686de does the same for tests.
  • 9ab4d4b7f22b1ce6dcdafa14ad79d9fe41aece98 introduces AnyRequest, AnyResponse, AnyMessage as aliases for respective top types and uses them where possible. (it's debatable which of these uses are actually a good idea).
  • 7ada1667433fe30b0c57c8ed2365e8d3f53e3d19 and a155013a2b1ba193a69e98a61ba6607d0197edcc make main and test, respectively, compile on Scala 2.12. (It can seemingly handle using Response.covaryF for the implicit conversion from F[Response[Nothing]] to F[Response[F]] but not to F[Response[G]])

EDIT: updated commit hashes following rebase

bplommer avatar Sep 22 '21 13:09 bplommer

Is it strictly necessary that this depends on #5229? I'd prefer to also move forward with that, so don't rip this up trying to decouple them, but I'm just contemplating why they're coupled.

rossabaker avatar Oct 23 '21 16:10 rossabaker

Good question! I remember needing #5229 to get type inference working satisfactorily, but I don't remember whether covariant messages without covariant encoders actually made things worse or just didn't gain anything. I can check.

bplommer avatar Oct 23 '21 17:10 bplommer

Yikes, this calls for IntelliJ's magic merge tool 🪄

bplommer avatar Oct 24 '21 08:10 bplommer

This triggered a compiler bug with scala 3.0.2 but seems to be ok with 3.1.0 😅

bplommer avatar Oct 24 '21 13:10 bplommer

Is the principle of this controversial or is it just the implementation that needs scrutiny? I can break it into smaller PRs if that helps.

bplommer avatar Nov 03 '21 13:11 bplommer

@bplommer What would be the state for this Pull Request? You have proposed some of the changes in this PR in separate Pull Requests:

  • #5712 Covariant Messages - Minimum
  • #5463 Entity Encoder - Variance of Type Parameters.

That being the case, perhaps the next step with this PR would be to either: a) Close it, to indicate that it the intended changes are already done; or b) Either merge main into it or rebase the changes on top of current master, to discount what has already been done.

diesalbla avatar Apr 18 '22 12:04 diesalbla

@bplommer notes in Discord that

The "make message types covariant" one can actually probably be closed as the core change was done in another pr, though there's some followup work to take full advantage of it

/cc @diesalbla who asked above :)

armanbilge avatar Jun 23 '22 16:06 armanbilge