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

PartialFunction doesn't compose well #9968

Closed
scabug opened this issue Oct 18, 2016 · 12 comments
Closed

PartialFunction doesn't compose well #9968

scabug opened this issue Oct 18, 2016 · 12 comments

Comments

@scabug
Copy link

scabug commented Oct 18, 2016

I'd like to be able to say

{ case Good(whatever) => whatever }.andThen { case Even(num) => println("it's even!") }

But if I use this, although it makes a PartialFunction, it doesn't do what I want it to. In particular, it will tell me that it's defined at Good(3). This is because it inherits andThen from Function, and so it takes a Function as its argument.

I think overloading andThen to take a PartialFunction would be pretty disruptive, so I propose two better ways forward:

A. provide a compose method on the PartialFunction object (somewhat confusing because there is also a compose method on the PartialFunction instance, but its behavior is not what we want for composing two PFs)
B. change andThen to inspect the other function, and if it's a PartialFunction, upgrade its behavior. (this is a thrilling runtime change, and makes its behavior inconsistent).

What do you think?

@scabug
Copy link
Author

scabug commented Oct 18, 2016

Imported From: https://issues.scala-lang.org/browse/SI-9968?orig=1
Reporter: Moses Nakamura (mosesn)

@scabug
Copy link
Author

scabug commented Oct 18, 2016

Moses Nakamura (mosesn) said:
Some of the hoops people jump through to try to do this themselves:

http://stackoverflow.com/questions/21041626/chaining-partialfunctions-with-andthen-in-scala

For what it's worth, I typically provide

def composePartial[A, B, C](left: PartialFunction[A, B], right: PartialFunction[B, C]): PartialFunction[A, C] = {
  def isDefinedAt(a: A): Boolean = left.isDefinedAt(a) && right.isDefinedAt(left(a))
  def apply(a: A): C = left(right(a))
}

at the call site and just hate myself.

@scabug scabug added this to the Backlog milestone Apr 7, 2017
@SethTisue
Copy link
Member

SethTisue commented Mar 1, 2018

evidence people want this:

I think a PR adding this to stdlib, in some form, would be accepted (probably after some thought and bikeshedding on where it should go and what it should be called).

@SethTisue
Copy link
Member

is there really no ticket on this older than 2016? seems strange

@NthPortal
Copy link

NthPortal commented Mar 1, 2018

andThenPartial seems like a not terrible name which conveys its meaning clearly (taken from the second SO link)

@insdami
Copy link

insdami commented Mar 6, 2018

I can take this one.
I think an andThenPartial captures the intention quite well. Does it make sense to also have composePartial? So you have the equivalent of compose and andThen for PartialFunction

@jvican
Copy link
Member

jvican commented Mar 6, 2018

Are we sure here that a library change is the way to go? To me it seems the compiler should be smarter and the spec should specify the expected behaviour for partial functions, as proposed in:

B. change andThen to inspect the other function, and if it's a PartialFunction, upgrade its behavior. (this is a thrilling runtime change, and makes its behavior inconsistent).

This probably requires some (simple) change to the compiler, but I think it's the best way to move forward.

@SethTisue
Copy link
Member

"change andThen to inspect the other function": couldn't that be a library change, why would be a compiler change needed?

@NthPortal
Copy link

NthPortal commented Mar 13, 2018

Because isDefinedAt needs to compute the result of applying the first partial function in order to test if the value is defined for the second partial function, is it worthwhile to have a special implementation of PartialFunction which (conservatively) caches the result of the applying the first partial function?

e.g.

class AndThen[A <: AnyRef, B, C](left: PartialFunction[A, B],
                                 right: PartialFunction[B, C])
    extends PartialFunction[A, C] {
  private[this] var cache: (A, B) = (null.asInstanceOf[A], null.asInstanceOf[B])
  def isDefinedAt(a: A): Boolean = {
    if (left.isDefinedAt(a)) {
      val tmp = left(a)
      if (right.isDefinedAt(tmp)) {
        cache = a -> tmp
        true
      } else false
    } else false
  }
  def apply(a: A): C = {
    val tmp = cache
    val a2 = tmp._1
    if ((a2 eq a) && (a2 ne null)) right(tmp._2)
    else right(left(a))
  }
}

@Jasper-M
Copy link
Member

"change andThen to inspect the other function": couldn't that be a library change, why would be a compiler change needed?

I think a library change is partly possible. Something like

def andThen[C](k: B => C) =  k match {
  case pf: PartialFunction[B, C] => andThenPartial(pf)
  case f: Function1[B, C] => andThenTotal(k)
}

But then the example code from the issue still wouldn't work because the expected type of the argument to andThen is Function1, not PartialFunction. { case Even(num) => println("it's even!") } will simply not be compiled to a PartialFunction.
The compiler change required to change that might be simple, but it would probably require a SIP.

@SethTisue
Copy link
Member

reviewers wanted on scala/scala#7263

@SethTisue
Copy link
Member

see also scala/scala#7552

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

No branches or pull requests

9 participants