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

Conversation

@hamnis
Copy link
Contributor

hamnis commented Oct 30, 2019

Repeating @rossabaker's comments here:

  • 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 Request[F], this trick avoids the pattern matcher bug on 2.12.

@hamnis hamnis force-pushed the hamnis:eliminate-compiler-warning-2.13 branch from 244abdd to b1a8a43 Oct 30, 2019
Copy link
Member

rossabaker left a comment

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

rossabaker 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 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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

rossabaker commented Nov 5, 2019

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 hamnis force-pushed the hamnis:eliminate-compiler-warning-2.13 branch 2 times, most recently from d678205 to 06b948d Nov 5, 2019
@hamnis

This comment has been minimized.

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 left a comment

Thanks! 👍 on green.

@hamnis

This comment has been minimized.

Copy link
Contributor Author

hamnis commented Nov 5, 2019

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

@hamnis hamnis force-pushed the hamnis:eliminate-compiler-warning-2.13 branch from 06b948d to 5756c1a Nov 6, 2019
* 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.
@hamnis hamnis force-pushed the hamnis:eliminate-compiler-warning-2.13 branch from 5756c1a to 7aa7639 Nov 9, 2019
@rossabaker rossabaker merged commit 36e94b1 into http4s:master Nov 26, 2019
2 checks passed
2 checks passed
Summary no rules match, no planned actions
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hamnis hamnis deleted the hamnis:eliminate-compiler-warning-2.13 branch Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.