Skip to content

cannot overload flatMap #11232

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

Closed
schlichtanders opened this issue Oct 30, 2018 · 9 comments
Closed

cannot overload flatMap #11232

schlichtanders opened this issue Oct 30, 2018 · 9 comments

Comments

@schlichtanders
Copy link

Dear Scala developers,

I just stumbled upon the standard case that Option and Iterable might be hard to intertwine in a scala for loop. See for instance this stackoverflow question

I tried to fix this by just overloading flatMap with a version which should work, however it doesn't.

When I run the following in scala 2.11.12 REPL

implicit class OptionFlatMapIterable[A](o: Option[A]){
  def flatMap[B](f: A => Iterable[B]): Iterable[B] = if (o.isEmpty) Seq.empty else f(o.get)

  def flatMap2[B](f: A => Option[B]): Option[B] = o.flatMap(f)
  def flatMap2[B](f: A => Iterable[B]): Iterable[B] = if (o.isEmpty) Seq.empty else f(o.get)
}

Some(42).flatMap2{ (o: Int) =>
  Seq(o, o + 1, o + 3).map{ (i: Int) =>
    (i,i)
  }
}

Some(42).flatMap{ (o: Int) =>
  Seq(o, o + 1, o + 3).map{ (i: Int) =>
    (i,i)
  }
}

for{
  o <- Some(42)
  i <- Seq(o, o + 1, o + 3)
} yield (o, i)

I get the following outputs

res0: Iterable[(Int, Int)] = List((42,42), (43,43), (45,45))

<console>:14: error: type mismatch;
 found   : Seq[(Int, Int)]
 required: Option[?]
         Seq(o, o + 1, o + 3).map{ (i: Int) =>
                                 ^
<console>:15: error: type mismatch;
 found   : Seq[(Int, Int)]
 required: Option[?]
         i <- Seq(o, o + 1, o + 3)
           ^

As you can see it perfectly works when using my dummy flatMap2, but fails for flatMap itself.

If this would work, it could massively simplify the support of complex flatMap chainings and hence complex for loops with different Monads intermixing a base Iterable Monad.

@Jasper-M
Copy link

So would you say the issue here is that Option should have an overloaded flatMap method, or that possible extension methods should be considered during overload resolution?

@schlichtanders
Copy link
Author

I think both makes sense for Iterable type.

Option should have an overloaded flatMap method

  • Option already has an implicit conversion to Iterable.
  • complex nesting of Iterables are by far the most common use case for for loops among scala users
    Hence it would be nice to support this out of the box

possible extension methods should be considered during overload resolution

I expected this to already work and still don't understand, why this extension is different from other implicit extensions. Hence it made it to scala/bug.

As there are many more Monads in principle, there are also many more of possible flatMap implicit extensions to seamlessly support intermixings (Try/Either would work similar to Option I guess, I myself build a ContextManager monad, ...)

So if I have to choose, then focus on

possible extension methods should be considered during overload resolution

@SethTisue
Copy link
Member

SethTisue commented Oct 31, 2018

I'd suggest moving this to https://contributors.scala-lang.org, since this still in the figuring-out-what-is-even-going-on stage, rather than the reasonably-certain-this-is-in-fact-a-bug stage

I know other open source projects vary in how they do this, but in Scala, scala/bug is for bug reports, discussions are on Discourse

@adriaanm
Copy link
Contributor

adriaanm commented Nov 7, 2018

Looks like this is an actual bug after all :-)

@SethTisue
Copy link
Member

there is some discussion at https://contributors.scala-lang.org/t/cannot-overload-flatmap/2556/10

the relevant piece of the spec is SLS 7.3, point 3: https://twitter.com/headinthebox/status/1046521593524965376

@Jasper-M

This comment has been minimized.

@Jasper-M
Copy link

Jasper-M commented Nov 7, 2018

There seems to be ticket for this: #9523
From back when I remembered that this was a bug.

@SethTisue
Copy link
Member

nice, thanks for finding that!

@schlichtanders
Copy link
Author

thanks a lot for finding the underlying cause

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

Successfully merging a pull request may close this issue.

4 participants