Fixes OnCompleted.unapply result type

[khernyo]

The Scala spec says that an extractor matches a pattern with zero
argument pattern if the result type of unapply is Boolean. The result
type of Option[T] means that the extractor has exactly one argument
pattern of type T.

The "Option[Unit]" result type does work, but it's probably a compiler
bug. Based on the result type, the pattern match "case OnCompleted() =>"
should not compile and "case OnCompleted(()) =>" should be used instead.
Using the latter one crashes the compiler... Changing the result type of
unapply to Boolean does not change any of this, so using the pattern
"case OnCompleted(()) =>" still crashes the compiler, but at least the
pattern is not suggested by the unapply result type and the pattern
match results in one less object allocation.

The included test does not check for this problem, it simply makes
sure that it still works correctly when using the correct pattern.

[cloudbees-pull-request-builder]

RxJava-pull-requests #521 SUCCESS
This pull request looks good

[samuelgruetter]

Seems correct according to http://www.scala-lang.org/files/archive/nightly/pdfs/ScalaReference.pdf section 8.1.8 "Extractor Patterns", even though it looks like the spec hasn't been updated since version 2.8...

[khernyo]

Well, it is correct, but only after adapting the argument list:

$ cat e.scala
object e {
    def f(u: Unit) = u
    f()
    f(())
}
$ scalac -Ywarn-adapted-args e.scala     
e.scala:5: warning: Adapting argument list by inserting (): this is unlikely to be what you want.
        signature: e.f(u: Unit): Unit
  given arguments: <none>
 after adaptation: e.f((): Unit)
        f()
         ^
one warning found
$

And I don't think an Option[Unit] better describes the possible pattern matches than a Boolean.

[samuelgruetter]

Oops, don't get me wrong, I wanted to say that I think what you're saying is correct and that your changes should be merged in.

I just added the link to the spec because I wanted to avoid that other people have to search for it themselves ;-)

Btw in case you haven't seen it yet, the feature of inserting () was recently discussed here: https://groups.google.com/forum/#!topic/scala-debate/zwG8o2YzCWs

[khernyo]

Uh, oh, I misunderstood a bit. :)

I don't read scala-debate, so I missed that thread. The link in the latest post (https://gist.github.com/Poita79/7803974) is a nice summary of the fun you can have with "()"

[benjchristensen]

@headinthebox Do you want to pull this into the branch you have going or shall I merge it directly?

[headinthebox]

Happy to change, but I am a bit confused. This thread https://gist.github.com/Poita79/7803974 seems totally unrelated to unapply.

Also, why does returning Option[Unit] suggest matching OnCompleted(())? We all know that () vs Unit vs is kind of fuzzy.

[khernyo]

It's not related to "what type you should use for zero argument pattern".

Why does Option[Unit] suggest OnCompleted(())? Why does Option[Int] suggest OnCompleted(someInt)? Yes, you can write OnCompleted() instead of OnCompleted(()) (in fact, you have to because of the compiler bug), but it doesn't mean you should.

[headinthebox]

I must be missing something here, I can write case OnCompleted() => (), am I using a different compiler version than you?

val onCompleted = OnCompleted()
assertEquals((), onCompleted match { case OnCompleted() => () })

[khernyo]

I'm using 2.10.3

You are correct, "case OnCompleted() =>" does work. But I don't think I said otherwise. What I was trying to saying is that the correct result type for an unapply with zero argument pattern is Boolean. Why? Because of the (outdated, maybe irrelevant) specification for Scala 2.8. Also because normally Boolean is used for such an unapply. And because it misleads people. I know it misled me.

[benjchristensen]

This is a stylistic issue and not a bug so closing out. If further discussion is wanted then please start a discussion at https://groups.google.com/forum/#!forum/rxjava

[samuelgruetter]

If #662 is accepted, such a discussion won't be necessary, because OnCompleted can become an object instead of a class (as Scala users would expect).