twitter / scalding

A Scala API for Cascading
http://twitter.com/scalding
Apache License 2.0
3.49k stars 703 forks source link

Fix macro hygiene problems #1847

Closed travisbrown closed 6 years ago

travisbrown commented 6 years ago

I've been working with these macros lately and thought I should just go ahead and PR this fix.

Here's an example of the kind of bug lack of hygiene can cause:

import com.twitter.scalding.serialization.{OrderedSerialization, Serialization}
import com.twitter.scalding.serialization.macros.impl.BinaryOrdering.ordSer

val bytes = Serialization.toBytes[Either[Int, String]](Right("very important message"))

object Test {
  def Right(b: String): Either[Nothing, String] = util.Right(b + " ha ha")

  val result = Serialization.fromBytes[Either[Int, String]](bytes)
}

…and then:

scala> Test.result
res0: scala.util.Try[Either[Int,String]] = Success(Right(very important message ha ha))

…or a little less unpleasantly:

scala> object Test {
     |   class Int
     | 
     |   val result = Serialization.fromBytes[Either[scala.Int, String]](bytes)
     | }
<console>:17: error: could not find implicit value for evidence parameter of type com.twitter.scalding.serialization.Serialization[Either[Int,String]]
         val result = Serialization.fromBytes[Either[scala.Int, String]](bytes)
                                                                        ^

I think I caught all the unrooted names, but ideally we should have a separate hygiene test project (like circe's) with -Yno-imports and -Yno-predef turned on. I'd be happy to add that in this PR if people want to see it.

CLAassistant commented 6 years ago

CLA assistant check
All committers have signed the CLA.