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

Compile fails with ambiguous overload when -Xexperimental is set. #238

Closed
DylanLukes opened this issue Jul 14, 2016 · 10 comments
Closed

Compile fails with ambiguous overload when -Xexperimental is set. #238

DylanLukes opened this issue Jul 14, 2016 · 10 comments

Comments

@DylanLukes
Copy link

When the -Xexperimental flag is set, Scrooge's generated sources fail to compile due to an ambiguous overload in Filter::andThen introduced (presumably) by the Single-Abstract-Method (SAM) sugar.

Expected behavior

The sources should compile.

Actual behavior

The following sections in the generated code fail to compile with the error messages below:

  class MethodIface(serviceIface: BaseServiceIface)
    extends BinaryService[Future] {
    private[this] val __fetchBlob_service =
      ThriftServiceIface.resultFilter(self.FetchBlob) andThen serviceIface.fetchBlob
    def fetchBlob(id: Long): Future[ByteBuffer] =
      __fetchBlob_service(self.FetchBlob.Args(id))
  }
    def serviceToFunction(svc: ServiceType): FunctionType = { args: Args =>
      ThriftServiceIface.resultFilter(this).andThen(svc).apply(args)
    }

Steps to reproduce the behavior

With a build.sbt containing:

resolvers += "twitter-repo" at "https://maven.twttr.com"

libraryDependencies ++= Seq(
  "org.apache.thrift" %  "libthrift"      % "0.8.0",
  "com.twitter"       %% "finagle-core"   % "6.36.0",
  "com.twitter"       %% "finagle-thrift" % "6.36.0",
  "com.twitter"       %% "scrooge-core"   % "4.8.0"
)

scalacOptions += "-Xexperimental"

For the following minimal Thrift definition:

namespace java com.einhornmediagroup.agent.thriftjava
#@namespace scala com.einhornmediagroup.agent.thriftscala

service BinaryService {
  binary fetchBlob(1: i64 id);
}

Compilation yields:

[error] /Users/dylan/EMG/Projects/SlideDeck/agent/target/scala-2.11/src_managed/main/com/einhornmediagroup/agent/thriftscala/BinaryService.scala:68: ambiguous reference to overloaded definition,
[error] both method andThen in class Filter of type (f: com.einhornmediagroup.agent.thriftscala.BinaryService.FetchBlob.Args => com.twitter.util.Future[com.einhornmediagroup.agent.thriftscala.BinaryService.FetchBlob.Result])com.einhornmediagroup.agent.thriftscala.BinaryService.FetchBlob.Args => com.twitter.util.Future[com.einhornmediagroup.agent.thriftscala.BinaryService.FetchBlob.SuccessType]
[error] and  method andThen in class Filter of type (service: com.twitter.finagle.Service[com.einhornmediagroup.agent.thriftscala.BinaryService.FetchBlob.Args,com.einhornmediagroup.agent.thriftscala.BinaryService.FetchBlob.Result])com.twitter.finagle.Service[com.einhornmediagroup.agent.thriftscala.BinaryService.FetchBlob.Args,com.einhornmediagroup.agent.thriftscala.BinaryService.FetchBlob.SuccessType]
[error] match argument types (com.twitter.finagle.Service[com.einhornmediagroup.agent.thriftscala.BinaryService.FetchBlob.Args,com.einhornmediagroup.agent.thriftscala.BinaryService.FetchBlob.Result])
[error]       ThriftServiceIface.resultFilter(self.FetchBlob) andThen serviceIface.fetchBlob
@DylanLukes
Copy link
Author

Here's a gist containing a full example of this issue:

https://gist.github.com/DylanLukes/eb775e0818c93cb1dc6e56d46f321d31

@mosesn
Copy link
Contributor

mosesn commented Jul 25, 2016

@DylanLukes hey, thanks for filing the issue! We're going to try to reproduce on our end and get back to you. Thanks for the complete reproduction, makes our lives a lot easier.

@DylanLukes
Copy link
Author

My pleasure! Hope you're able to reproduce it easily. I'm on scala 2.11.8, also. The experimental features I'm using I believe were introduced in 2.11.x.

@mosesn
Copy link
Contributor

mosesn commented Jul 25, 2016

@DylanLukes OK, so I'm getting the same behavior locally. I wonder if this is a bug in the scala compiler though, because it looks like the problem we're running into is that Filter#andThen can take either Functions or Services, but Service is already a Function. Let's see if we can scare up some scala compiler people.

@mosesn
Copy link
Contributor

mosesn commented Jul 25, 2016

For folks following along, here is the line where Service is-a Function–https://github.com/twitter/finagle/blob/develop/finagle-core/src/main/scala/com/twitter/finagle/Service.scala#L55

@mosesn
Copy link
Contributor

mosesn commented Jul 25, 2016

Filed a ticket: https://issues.scala-lang.org/browse/SI-9871 so we'll see what they say.

For folks trying to repro, the command I ran in the environment that @DylanLukes describes was sbt compile

@DylanLukes
Copy link
Author

I'm pretty sure this has to do with the SAM sugar, which allows implicit
conversion from functions to single-abstract-method classes like Runnable.
I believe this implicit conversion is what introduces the ambiguity error.

It only happens with the -Xexperimental flag on.

I solved the problem by isolating my thrift definitions in their own
subproject that did not have that flag.

On Mon, Jul 25, 2016 at 7:35 PM Moses Nakamura notifications@github.com
wrote:

Filed a ticket: https://issues.scala-lang.org/browse/SI-9871 so we'll see
what they say.

For folks trying to repro, the command I ran in the environment that
@DylanLukes https://github.com/DylanLukes describes was sbt compile


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#238 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPRAqArpRrznUvsqYqXRZb2TRGyZ0iFks5qZUg0gaJpZM4JMnbB
.

  • Dylan

@mosesn
Copy link
Contributor

mosesn commented Jul 26, 2016

Good call, it sounds like it's related to scala/scala-dev#157 –there's a bug with overloading and SAMs. Your work around sounds like a good one. I'm going to close this ticket for now, but if there's something more for us to do, please reopen it.

@mosesn mosesn closed this as completed Jul 26, 2016
@adriaanm
Copy link

adriaanm commented Oct 28, 2016

I believe this may have been fixed by scala/scala#5307 or scala/scala#5357, which will be released as part of 2.12.0 final on Monday (already in RC1 and RC2). Would be interested to hear about your experience so that we can see what (if anything) we should plan to fix in 2.12.1 here. Thanks!

@mosesn
Copy link
Contributor

mosesn commented Oct 29, 2016

@adriaanm we're blocked on 2.12.0 because scalatest is not releasing 2.12.x releases for scalatest 2.x anymore. We're trying to migrate Twitter to scalatest 3.x, but it's going to take us a long time. Sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants