Add Order/Hash/Show Instances For Uri And Related Types #3969

isomarcte · 2020-11-29T20:17:59Z

This commit adds Order, Hash, and Show instances for the following types.

org.http4s.Uri.Authority
org.http4s.Query
org.http4s.Uri.Host
org.http4s.Uri.RegName
org.http4s.Uri

The impetus for all of them was to support instances for org.http4s.Uri.

In addition, two functions were added to the package object org.http4s.internal scoped to be private to http4s, compareField, reduceComparisons_, and reduceComparisons.

These functions substantially reduce the amount of code required to write Order instances for Product types.

In order to support testing the laws for these types, several new instances of Arbitrary and Cogen were added, as not all types had instances for each of these.

isomarcte · 2020-11-29T20:35:48Z

@rossabaker so this commit breaks binary compatibility in two ways.

For org.http4s.Uri#Authority, the addition of a companion object messes up the type hierarchy from the one generated by the case class.

Error:  core: Failed binary compatibility check against org.http4s:http4s-core_2.13:0.21.1! Found 1 potential problems
Error:   * the type hierarchy of object org.http4s.Uri#Authority is different in current version. Missing types {scala.runtime.AbstractFunction3}
Error:     filter with: ProblemFilters.exclude[MissingTypesProblem]("org.http4s.Uri$Authority$")

It's simple enough to add that back in, though a bit gross.

  object Authority extends scala.runtime.AbstractFunction3[Option[UserInfo], Host, Option[Int], Authority] {

    override def apply(userInfo: Option[UserInfo] = None, host: Host = RegName("localhost"), port: Option[Int] = None): Authority =
      new Authority(userInfo, host, port)

The binary change is the addition of the new Cogen and Arbitrary instances. This is an additive change, so no symbols are changed/removed.

My intuition would be to add the AbstractFunction3 extension now, then remove it on the rebase to the main branch. For the new instances for Cogen and Arbitrary, I'd just add them to the ProblemFilters for mima on this branch, since they are just additions, not breaking changes.

What are your thoughts?

rossabaker · 2020-11-30T06:28:12Z

I did exactly that with an AbstractFunction1 somewhere else. I think your idea is exactly right.

The arbitrary changes pass MiMa? Adding to traits was bad in 2.11, but I think is fine starting in 2.12 as long as there is a concrete implementation.

isomarcte · 2020-11-30T16:01:18Z

@rossabaker I've updated the PR to include the AbstractFunction3 workaround.

The arbitrary changes pass MiMa? Adding to traits was bad in 2.11, but I think is fine starting in 2.12 as long as there is a concrete implementation.

Yeah, that is my understanding as well. To be clear, MiMa does fail on them, but I don't think they are actually an issue unless http4s is still targeting 2.11.x.

Thoughts?

rossabaker · 2020-11-30T17:46:11Z

I usually end up regretting making MiMa exceptions. I think I understand, and then a week later, we get a bug.

We already have a couple in the companion object. I think making them a trait was a mistake, and most people should be importing them. I'd be fine putting them in the companion, and we can reunify them somewhere in 1.0.

isomarcte · 2020-11-30T18:36:46Z

@rossabaker sounds good. I've updated this with the new instances only in the companion object.

isomarcte · 2020-11-30T18:44:38Z

@rossabaker I had to make a few things lazy to deal with some initialization errors coming from the interplay between the static scope of the companion object and the trait, just to let you know.

isomarcte · 2020-11-30T19:57:41Z

@rossabaker as per other PRs, the failures here do not appear related to my change.

rossabaker

I agree the test failures are unrelated. I did come up with a few more ideas.

rossabaker · 2020-11-30T21:09:06Z

core/src/main/scala/org/http4s/package.scala

@@ -82,4 +84,54 @@ package object http4s {
  type Http4sSyntax = syntax.AllSyntax
  @deprecated("Moved to org.http4s.syntax.all", "0.16")
  val Http4sSyntax = syntax.all
+
+  // Helper functions for writing Order instances //


We keep a lot of helpers in org.http4s.internals, just to reduce clutter.

rossabaker · 2020-11-30T21:09:06Z

core/src/main/scala/org/http4s/package.scala

+    *       and `SemigroupK[Option].combineK` have different semantics. We
+    *       ''always'' want the `SemigroupK[Option]` instance here.
+    */
+  private[http4s] def reduceComparisons(


This runs all the comparisons, right? Could we do a lazy one that only continues for as long as we get zeroes?

rossabaker · 2020-11-30T21:09:06Z

laws/src/main/scala/org/http4s/laws/discipline/ArbitraryInstances.scala

@@ -634,11 +638,12 @@ private[http4s] trait ArbitraryInstances {
    } yield Uri.Authority(maybeUserInfo, host, maybePort)
  }

-  val genPctEncoded: Gen[String] =
+  lazy val genPctEncoded: Gen[String] =


A long, long time ago, I saw lazy arbitraries making a difference in a profile, which caused us to use eager vals. I don't know whether that's still a concern, but I think you could just by moving them above the new use.

Whether eager values are smart in a trait is a separate question.

I'm fine either way.

isomarcte · 2020-11-30T22:21:08Z

@rossabaker Updated.

I moved the helpers into the org.http4s.internal package object, made reduceComparisons lazy for every comparison other than the first, and shuffled things around in ArbitraryInstances so that it could use strict values again.

This commit adds `Order`, `Hash`, and `Show` instances for the following types. * `org.http4s.Uri.Authority` * `org.http4s.Query` * `org.http4s.Uri.Host` * `org.http4s.Uri.RegName` * `org.http4s.Uri` The impetus for all of them was to support instances for `org.http4s.Uri`. In addition, two functions were added to the package object `org.http4s.internal` scoped to be private to `http4s`, `compareField`, `reduceComparisons_`, and `reduceComparisons`. These functions substantially reduce the amount of code required to write `Order` instances for Product types. In order to support testing the laws for these types, several new instances of `Arbitrary` and `Cogen` were added, as not all types had instances for each of these.

rossabaker

👍 Thanks!

isomarcte · 2020-12-02T17:04:48Z

@rossabaker I saw this was missing an import after the merge with for the implicits <-> syntax change. I've updated the branch.

rossabaker · 2020-12-02T18:00:44Z

I gambled and merged in the browser last night without compiling. Thanks for the mop-up!

isomarcte force-pushed the uri-order branch from d590fcd to c23fcdf Compare November 29, 2020 20:18

isomarcte force-pushed the uri-order branch from c23fcdf to b9f3c7a Compare November 30, 2020 16:00

isomarcte force-pushed the uri-order branch 2 times, most recently from 4e7c6bf to 78427b4 Compare November 30, 2020 18:42

rossabaker reviewed Nov 30, 2020

View changes

isomarcte force-pushed the uri-order branch from 78427b4 to ba96d4e Compare November 30, 2020 22:19

isomarcte force-pushed the uri-order branch from ba96d4e to c64855b Compare November 30, 2020 22:22

rossabaker approved these changes Nov 30, 2020

View changes

rossabaker and others added 2 commits December 2, 2020 00:40

Merge branch 'series/0.21' into uri-order

0e93f4c

Add Missing Syntax Import

684f9ef

rossabaker merged commit 929ef0b into http4s:series/0.21 Dec 2, 2020

rossabaker mentioned this pull request Dec 2, 2020

Non-trivial merge from series/0.21 #3974

Merged

isomarcte deleted the uri-order branch December 2, 2020 21:03

isomarcte mentioned this pull request Dec 10, 2020

Show for Uri #3998

Closed

Add Order/Hash/Show Instances For Uri And Related Types #3969

Add Order/Hash/Show Instances For Uri And Related Types #3969

isomarcte commented Nov 29, 2020 •

edited

isomarcte commented Nov 29, 2020

rossabaker commented Nov 30, 2020

isomarcte commented Nov 30, 2020

rossabaker commented Nov 30, 2020

isomarcte commented Nov 30, 2020

isomarcte commented Nov 30, 2020

isomarcte commented Nov 30, 2020

rossabaker left a comment

rossabaker Nov 30, 2020

rossabaker Nov 30, 2020

rossabaker Nov 30, 2020

isomarcte commented Nov 30, 2020

rossabaker left a comment

isomarcte commented Dec 2, 2020

rossabaker commented Dec 2, 2020

Add Order/Hash/Show Instances For Uri And Related Types #3969

Add Order/Hash/Show Instances For Uri And Related Types #3969

Conversation

isomarcte commented Nov 29, 2020 • edited

isomarcte commented Nov 29, 2020

rossabaker commented Nov 30, 2020

isomarcte commented Nov 30, 2020

rossabaker commented Nov 30, 2020

isomarcte commented Nov 30, 2020

isomarcte commented Nov 30, 2020

isomarcte commented Nov 30, 2020

rossabaker left a comment

Choose a reason for hiding this comment

rossabaker Nov 30, 2020

Choose a reason for hiding this comment

rossabaker Nov 30, 2020

Choose a reason for hiding this comment

rossabaker Nov 30, 2020

Choose a reason for hiding this comment

isomarcte commented Nov 30, 2020

rossabaker left a comment

Choose a reason for hiding this comment

isomarcte commented Dec 2, 2020

rossabaker commented Dec 2, 2020

isomarcte commented Nov 29, 2020 •

edited