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

Lifted methods implicitly convert from Option to Iterable but Function1 and PartialFunction do not #9606

Closed
scabug opened this Issue Dec 29, 2015 · 9 comments

Comments

Projects
None yet
1 participant
@scabug
Copy link

scabug commented Dec 29, 2015

Seems lifted methods are implicitly converted more readily than Function1 or partial functions. For example, this works as expected:

scala> val vector2 = Vector(Some(1), None, Some(3), Some(4))
vector2: scala.collection.immutable.Vector[Option[Int]] = Vector(Some(1), None, Some(3), Some(4))

scala> vector2.flatMap { _.map(_*2) }
res0: Vector[Int] = Vector(2, 6, 8)

Using a method that gets lifted to a Function1 also works as expected:

scala> def times2a(v: Option[Int]): Option[Int] = v.map(_*2)
times2a: (v: Option[Int])Option[Int]

scala> vector2.flatMap { times2a }
res1: scala.collection.immutable.Vector[Int] = Vector(2, 6, 8)}}

However, attempting to use a Function1 instead of a method produces an error:

scala> val times2b: Option[Int] => Option[Int] = v => v.map(_*2)
times2b: Option[Int] => Vector[Int] = $$Lambda$2080/2084932307@52da1357

scala> vector2.flatMap { times2b }
res2: scala.collection.immutable.Vector[Int] = Vector(2, 6, 8)
<console>:14: error: type mismatch;
 found   : Option[Int] => Option[Int]
 required: Option[Int] => scala.collection.GenTraversableOnce[?]
       vector2.flatMap { times2b }

The error goes away if the result is converted to an iterator:

scala> val times2c: Option[Int] => Iterator[Int] = v => v.map(_*2).toIterator
times2c: Option[Int] => Iterator[Int] = $$Lambda$2333/302275954@6bb2a28a

scala> vector2.flatMap { times2c }
res17: scala.collection.immutable.Vector[Int] = Vector(2, 6, 8)

The error also goes away if monads are not mixed:

scala> val times2d: Option[Int] => Vector[Int] = v => v.map(_*2).toVector
times2d: Option[Int] => Vector[Int] = $$Lambda$2279/8972378@230dd372

scala> vector2.flatMap { times2d }
res2: scala.collection.immutable.Vector[Int] = Vector(2, 6, 8)

Attempting to use a partial function produces the same error as was encountered for Function1:

scala> val times2e: Option[Int] => Option[Int] = { case v: Option[Int] => v.map(_*2) }
times2e: Option[Int] => Option[Int] = $$Lambda$2291/869051292@668ea404

scala> vector2.flatMap { times2e }
<console>:14: error: type mismatch;
 found   : Option[Int] => Option[Int]
 required: Option[Int] => scala.collection.GenTraversableOnce[?]
       vector2.filter(_.isDefined).flatMap { times2d }

Again, the error for the partial function can be avoided by converting to an Iterator.

scala> scala> val times2f: Option[Int] => Iterator[Int] = { case v: Option[Int] => v.map(_*2).toIterator }
times2f: Option[Int] => Vector[Int] = $$Lambda$2301/119525506@6d50b8c2

scala> vector2.flatMap { times2f }
res10: scala.collection.immutable.Vector[Int] = Vector(2, 6, 8)

Again, the error for the partial function can be avoided by not mixing monads.

scala> scala> val times2g: Option[Int] => Vector[Int] = { case v: Option[Int] => v.map(_*2).toVector }
times2g: Option[Int] => Vector[Int] = $$Lambda$2301/119525506@6d50b8c2

scala> vector2.flatMap { times2g }
res10: scala.collection.immutable.Vector[Int] = Vector(2, 6, 8)

Is this a bug?

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Dec 29, 2015

Imported From: https://issues.scala-lang.org/browse/SI-9606?orig=1
Reporter: Mike Slinn (mslinn)
Affected Versions: 2.12.0-M3

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 19, 2016

@szeiger said:
IMHO not a bug. The inline eta-expansion works because the compiler infers the correct type whereas your explicitly eta-expanded times2b has the wrong type. Compare these:

  vector2.flatMap( _.map(_*2) )
  vector2.flatMap( (_.map(_*2)): (Option[Int] => collection.GenTraversableOnce[Int]) ) // inferred type
  vector2.flatMap( (_.map(_*2)): (Option[Int] => Option[Int]) ) // wrong

An Option\[Int\] => Option\[Int\] cannot be converted to an Option[Int] => GenTraversableOnce[Int] but a (v: Option[Int])Option[Int] can be eta-expanded to that type. The expansion looks something like this (and type-checks successfully, as expected):

  vector2.flatMap( { (x: Option[Int]) => times2a(x) } )

The implicit conversion is applied to the result of the method call, converting an Option[Int] to a GenTraversableOnce[Int].

@scabug scabug closed this Jan 19, 2016

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 19, 2016

@dwijnand said (edited on Jan 19, 2016 10:17:23 PM UTC):
While I appreciate the internal technical reasons, particularly learning a bit more about what things are called, is eta-expansion something that a Scala developer that might encounter such a bug should know and understand?

I think there's a bug here IMHO.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 19, 2016

Mike Slinn (mslinn) said:
I don't care if this is officially classified as a bug or not. What concerns me is that the a lifted method behaves differently than its FunctionN equivalent. I expect that the inconsistency is unlikely to provide benefit to a user, but is likely to cause frustration.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 20, 2016

@szeiger said:
Eta-expansion is in the spec. Leaving aside hygiene and evaluation order it's really just wrapping a method call in a lambda, i.e. f\(m\) becomes f\(x => m\(x\)\). If there is an implicit conversion from the result type of m to the expected result type of the function, it is naturally applied, no special rules required.

If you wrap a function application in another lambda you get the same result:

scala> vector2.flatMap { times2b }
<console>:13: error: type mismatch;
 found   : Option[Int] => Option[Int]
 required: Option[Int] => scala.collection.GenTraversableOnce[?]
       vector2.flatMap { times2b }
                         ^

scala> vector2.flatMap { x => times2b(x) }
res1: scala.collection.immutable.Vector[Int] = Vector(2, 6, 8)
@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 20, 2016

@dwijnand said:
Is the spec something that's expected to be known? I don't mean that as a taunt, I'm genuinely wondering.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 20, 2016

@szeiger said:
@dwijnand, my take on this is that most programmers would not have read the spec and you should be able to be productive after taking a course or reading a book that teaches the language. For deciding whether something is a bug in the compiler / standard library, knowledge of the specification is required, of course. In this particular case, it works as specified and the specified behavior is the most natural one I can think of. The resulting interaction of language features may still qualify as a Scala puzzler.

In order to turn this from a bug report into an improvement request you'd have to specify what to change. The most obvious changes would be to either make both cases work by adding an implicit conversion for functions:

// Does not work with current type inference / implicit search rules:
implicit def adaptFunction1[P, R, A](f: P => R)(implicit conv: R => A): (P => A) = (x => conv(f(x)))

Or to forbid both by changing eta expansion to typecheck without a return type. You have the same issue for parameters though, so you can either make the rules inconsistent or forbid that as well, which would in turn prevent you from ever eta-expanding an overloaded method.

I doubt either of those changes would get very far.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 20, 2016

Mike Slinn (mslinn) said:
Once again we are left with the message that "Scalac occasionally does weird shit that only the high priests understand".

Over a year ago @odersky said "we want to eliminate some traps that people have found hard in Scala -- so-called puzzlers". Perhaps scalac should recognize specific situations such as this and respond by doing what likely makes most sense to a user, even if it is inconsistent in some more general theoretical sense.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Feb 18, 2017

@som-snytt said:

The compiler could emit better advice, or possibly a tool (clippy) could take up the slack.

scala> vs.flatMap(f _)
<console>:14: error: type mismatch;
 found   : () => Option[Int] => Option[Int]
 required: Option[Int] => scala.collection.GenTraversableOnce[?]
       vs.flatMap(f _)
                  ^

scala> vs.flatMap(o => f(o))
res3: scala.collection.immutable.Vector[Int] = Vector(2)

scala> vs.flatMap(f(_))
res4: scala.collection.immutable.Vector[Int] = Vector(2)

@scabug scabug added the quickfix label Apr 7, 2017

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