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

Problem with implicit case classes #6227

Closed
scabug opened this issue Aug 13, 2012 · 13 comments
Closed

Problem with implicit case classes #6227

scabug opened this issue Aug 13, 2012 · 13 comments

Comments

@scabug
Copy link

scabug commented Aug 13, 2012

Implicit case classes are broken, because the type of the class and the type of the object are being mixed up:

scala> implicit case class IntOps( i: Int ) {
     |    def twice = i * 2
     | }
defined class IntOps

scala> 11 : IntOps
<console>:10: error: ambiguous reference to overloaded definition,
both method IntOps of type (i: Int)IntOps
and  object IntOps of type IntOps.type
match argument types (Int) and expected result type IntOps
              11 : IntOps
              ^
@scabug
Copy link
Author

scabug commented Aug 13, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6227?orig=1
Reporter: @dcsobral
Affected Versions: 2.10.0-M6

@scabug
Copy link
Author

scabug commented Aug 17, 2012

@jsuereth said:
This is far more insidious. Objects are completely ignored when determining duplicates in the namespace:

object test {

  def x(i: Int) = i
  object x { def apply(i: Int) = i }
}

scala> test.x(1)
<console>:8: error: ambiguous reference to overloaded definition,
both object x in object test of type test.x.type
and  method x in object test of type (i: Int)Int
match argument types (Int)
              test.x(1)
                   ^

@scabug
Copy link
Author

scabug commented Aug 17, 2012

@soc said:
I think this is related:

implicit case class Foo(val foo: Int)
Foo(1)

This fails:

<console>:10: error: ambiguous reference to overloaded definition,
both method Foo of type (foo: Int)Foo
and  object Foo of type Foo.type
match argument types (Int)
              Foo(1)
              ^

This is because the implicit class is desugared to an implicit method with the same name as the class, which makes the code ambiguous from the compiler POV.
In my opinion it would make sense to desugar the implicit class to a different method name, like FooImplicit.

@scabug
Copy link
Author

scabug commented Aug 17, 2012

@scabug
Copy link
Author

scabug commented Aug 29, 2012

@odersky said:
See scala/scala#1209

@scabug
Copy link
Author

scabug commented Aug 29, 2012

@odersky said:
Pullrequest pending

@scabug scabug closed this as completed Aug 29, 2012
@scabug
Copy link
Author

scabug commented Aug 30, 2012

@Blaisorblade said (edited by @jsuereth on Aug 31, 2012 2:04:06 PM UTC):
Quoting Simon Ochsenreither:

In my opinion it would make sense to desugar the implicit class to a different method name, like FooImplicit.
Wouldn't this work?

@scabug
Copy link
Author

scabug commented Aug 31, 2012

@jsuereth said:

Doing this destroys your ability to use name-binding to resolve implicit conflicts and import/remove implicits from scope. We initially wanted to do something like the above, but it turned out to be a bad idea overall.

We also talked about having the implicit method be the apply method on the companion object. That would require a fundamental change to the way implicit resolution works.

Implicits are hard enough to reason through as is. We shouldn't add any surprises here unless we know we're improving the whole of it.

@scabug
Copy link
Author

scabug commented Aug 31, 2012

@Blaisorblade said:
Thanks for your explanation, although this situation is somewhat sad. Still, you make clear that the alternatives are no better, especially at this late stage.

@scabug
Copy link
Author

scabug commented Aug 31, 2012

@soc said:
I understand that the release shouldn't be delayed over this bug, but I really hope this situation can be addressed with a fix as soon as possible. Loosing case is a pretty hefty price to pay, especially when intending to use it combined with value classes.

@scabug
Copy link
Author

scabug commented Jul 27, 2013

@Blaisorblade said (edited on Jul 27, 2013 4:37:19 PM UTC):
On the ML (https://groups.google.com/d/msg/scala-internals/8TrS3ar5LcY/Qg7JEqN7onwJ), this alternative desugaring was mentioned, but I can't find any discussion of its disadvantages ("I remember some kind of issue with the tech. details" is not enough). Note that it works with the current implicit resolution rules (https://gist.github.com/Blaisorblade/6089067).

object bip {
  class Foo(val xxx: Int)
  implicit object Foo extends (Int => Foo) { def apply(x: Int) = new Foo(x) }
}

My colleague Tillmann Rendel came up again with this encoding.

EDIT: So, why don't we use this desugaring?

@scabug
Copy link
Author

scabug commented Jul 27, 2013

@retronym said:
I don't think that can encode classes with type parameters; function types are not type-polymporhic.

@scabug
Copy link
Author

scabug commented Jul 31, 2013

@Blaisorblade said (edited on Jul 31, 2013 2:21:24 PM UTC):
Whoops, agreed. (EDIT: maybe we can avoid using function types - but objects are also not type-polymorphic, and changing that makes no sense).

And the obvious fixes require using a method instead of a companion object, at least for the implicit conversion (EDIT: which is the original encoding). I can imagine moving unapply to the method instead:

implicit def Foo = new {
  def apply(x: Int) = new Foo(x)
  def unapply(x: Foo) = ...
}

but not only this doesn't work nowadays, I'm not even sure it is a good idea, since Foo.unapply could refer to different methods. If that's not a problem, that encoding might be superior, but requires significant changes.

It seems that the only alternative is that the implicit conversion's name is derived in a documented way from the class name, either only for case classes (which preserves compatibility) or also for other classes (which ensures consistency).

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

2 participants