Skip to content
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

Adapted @rossabaker's patch in #2891 to make it work for 2.12 as well #2933

Merged
merged 1 commit into from
Nov 26, 2019
Merged

Adapted @rossabaker's patch in #2891 to make it work for 2.12 as well #2933

merged 1 commit into from
Nov 26, 2019

Conversation

hamnis
Copy link
Contributor

@hamnis hamnis commented Oct 30, 2019

Repeating @rossabaker's comments here:

Matching on Message[F] with this new encoding causes some obscure problem with the pattern matcher on 2.12. updated Request.unapply to first match on Message[F], and Request[F], this trick avoids the pattern matcher bug on 2.12.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm disappointed that the Request.unapply no longer works, but I think it's worth it. Thanks for finishing this, and we can look for ideas on the unapply in another ticket.

@hamnis
Copy link
Contributor Author

hamnis commented Oct 31, 2019

I did a little experiment when I got to work this morning, and I tried to minimize the problem. This seems to work:

import scala.util.hashing.MurmurHash3
import java.net.URI

sealed trait Message[F[_]] {
  def entity: fs2.Stream[F, Byte]
}

final class Request[F[_]](
    val method: String,
    val uri: URI,
    val entity: fs2.Stream[F, Byte]
) extends Message[F] with Product with Serializable {
  override def hashCode(): Int = MurmurHash3.productHash(this)

  override def equals(that: Any): Boolean =
    (this eq that.asInstanceOf[Object]) || (that match {
      case that: Request[_] =>
        (this.method == that.method) &&
          (this.uri == that.uri) //equality on entity is not really useful
      case _ => false
    })

  override def canEqual(that: Any): Boolean = that.isInstanceOf[Request[F]]

  override def productArity: Int = 3

  override def productElement(n: Int): Any = n match {
    case 0 => method
    case 1 => uri
    case 2 => entity
    case _ => throw new IndexOutOfBoundsException()
  }
}

object Request {
    def unapply[F[_]](req: Message[F]): Option[(String, URI, fs2.Stream[F, Byte])] = req match {
      case r: Request[F] => Some((r.method, r.uri, r.entity))
      case _ => None
    }
}

final case class Response[F[_]](status: Int, entity: fs2.Stream[F, Byte]) extends Message[F]


object Foo {
    def onMessage[F[_]](message: Message[F]) = message match {
        case Request(method, uri, _) => s"$method $uri"
        case Response(status, _) => s"$status"
    }
    def onRequest[F[_]](req: Request[F]) = req match {
      case Request(method, uri, _) => s"$method, $uri"
    }
}

@rossabaker
Copy link
Member

I notice three interesting differences between the one that works and the one that doesn't:

  • The one that works pattern matches on F.
  • The one that doesn't work refers to F in its return type.
  • The one that works returns Option. The one that doesn't returns Some. (This is a consequence of the first point, but I'll bet not the trigger.)

@hamnis
Copy link
Contributor Author

hamnis commented Nov 5, 2019

I notice three interesting differences between the one that works and the one that doesn't:

  • The one that works pattern matches on F.

The one that works actually matches on Message[F]

  • The one that doesn't work refers to F in its return type.

I'll expand my example to see if this actually matters. It didn`t.

  • The one that works returns Option. The one that doesn't returns Some. (This is a consequence of the first point, but I'll bet not the trigger.)

Returning Some is just a trick to making pattern matching faster, then the None case does not need to be considered.

@rossabaker
Copy link
Member

The one that works actually matches on Message[F]

I'm guessing this is the trick that will make this work. And of course the Option return type becomes necessary once the None case becomes necessary.

@hamnis
Copy link
Contributor Author

hamnis commented Nov 5, 2019

@rossabaker Rebased with master and changed to the trick in #2933 (comment)

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 👍 on green.

@hamnis
Copy link
Contributor Author

hamnis commented Nov 5, 2019

not quite sure what happened in the build. can someone restart that?

* Reimplement the case classiness of Request by hand to work around scala/bug#11457.
* Work around Seq aliasing in a spec
* Eliminate early initializer in TomcatSpec, which no longer appears to be necessary

Matching on Message[F] with this new encoding causes some obscure problem with the
pattern matcher on 2.12. updated Request.unapply to first match on Message[F], and then
Request, this trick avoids the pattern matcher bug on 2.12.
@rossabaker rossabaker merged commit 36e94b1 into http4s:master Nov 26, 2019
@hamnis hamnis deleted the eliminate-compiler-warning-2.13 branch December 2, 2019 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants