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

Cannot overload applyDynamic #6355

Closed
scabug opened this issue Sep 10, 2012 · 21 comments
Closed

Cannot overload applyDynamic #6355

scabug opened this issue Sep 10, 2012 · 21 comments
Assignees
Milestone

Comments

@scabug
Copy link

scabug commented Sep 10, 2012

It is not possible to overload applyDynamic because the name parameter is required to be by itself in the first parameter list:

import language.dynamics

object J {
   val x = new X(3)

   x.call(3,9)

   class X(i: Int) extends Dynamic {
      def applyDynamic(name: String)(in: Int): Int = i + in
      def applyDynamic(name: String)(a: Int, b: Int): Int = a + b
   }
}
J.scala:6: error: ambiguous reference to overloaded definition,
both method applyDynamic in class X of type (name: String)(a: Int, b: Int)Int
and  method applyDynamic in class X of type (name: String)(in: Int)Int
match argument types (String)
   x.call(3,9)
   ^
@scabug
Copy link
Author

scabug commented Sep 10, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6355?orig=1
Reporter: @harrah
Affected Versions: 2.10.0-M7

@scabug
Copy link
Author

scabug commented Sep 14, 2012

@gkossakowski said:
May I ask why overloading is desirable feature to have here? I believe it was conscious decision to split parameter into two lists and make overloading impossible. I think getting overloading right in dynamic setting might be extremely difficult.

Is this really critical bug? I'd call it just known limitation but far from critical.

@scabug
Copy link
Author

scabug commented Sep 14, 2012

@harrah said:
First, if overloading isn't allowed, that should be explicitly checked at definition time so a proper error can be generated. Structuring the parameter lists to cause an error at the usage site unnecessarily delays feedback to the applyDynamic author.

For overloading being potentially desirable, I figured that the straightforward design of having an actual ApplyDynamic interface with a fixed signature like:

trait ApplyDynamic {
 def applyDynamic(name: String, a: Any*)
}

was because it was deemed useful to be able to fix the number of arguments and use specific types for the parameters, like:

 def applyDynamic(name: String, a: Int, b: Double)

because the compiler will then check whether a call is applicable as usual, saving the applyDynamic author some of the work of checking argument count, types, and giving names to arguments. So, assuming fixing the parameter count and types is considered useful and given that Scala supports overloading, it seemed odd to have an inconsistency with the rest of Scala here. Otherwise, why not just define and require the ApplyDynamic interface above?

I didn't mark it as critical, but if there is anything critical about it, I'd guess it might be a decision on what to do, since that could require incompatible changes.

@scabug
Copy link
Author

scabug commented Sep 22, 2012

@odersky said (edited by @gkossakowski on Sep 22, 2012 9:17:38 AM UTC):
We discussed it at the last Scala meeting and came to the conclusion that we will leave it as is. True, merging the name into the first parameter list supports some valid use cases. But its generalization to multiple parameter lists is ugly.
If you have a call like this:

    x.foo(y)(z)

it seems more regular to map to

    x.applyDynamic("foo")(y)(z)

than to

    x.applyDynamic("foo", y)(z)

About overloading conflicts being detected early, it's not so simple, and we generally don't do that for any overloaded method. E.g. applyDynamic might have
variants with different return types and you could do a return type disambiguation at the call site, so the overloading might be valid after all. It's very hard to find and spec the right rules what a declaration site overloading detector would be. So we don't do it, and I believe Java does not do it either.

@scabug
Copy link
Author

scabug commented Sep 22, 2012

@odersky said:
No idea how the thumbs up ended up in there. It was a (y) when I wrote it.

@scabug
Copy link
Author

scabug commented Sep 22, 2012

@odersky said:
That's funny, so once more: It was a (y) when I wrote it.

@scabug
Copy link
Author

scabug commented Sep 22, 2012

@odersky said:
I can't believe it. It seems the three letter combination ( y ) always ends up as a thumbs-up ???

@scabug
Copy link
Author

scabug commented Sep 22, 2012

@gkossakowski said:
Martin: lol, you just discovered rather embarrassing bug in JIRA's parser. It seems like escaping with double curly braces (that JIRA uses instead of Markdown's backticks) does not always work.

I'll rewrite your comment using code tag. I discovered a few more issues with the parser trying to write this comment. Yet another reason to dump JIRA the first time we can...

@scabug
Copy link
Author

scabug commented Sep 22, 2012

@retronym said (edited on Sep 22, 2012 9:44:11 AM UTC):
For future (hopefully not too long into it) reference, '' '(' 'y' ')' gives: (y)

@scabug
Copy link
Author

scabug commented Sep 22, 2012

@harrah said:
I didn't mean general ambiguous overloading detection at the definition site, just disallowing any overloading at all. I can't come up with an example where overloading works. For example,

import language.dynamics

object J {
   class X(i: Int) extends Dynamic {
      def applyDynamic(name: String)(i: Int): Double = i + 0.5
      def applyDynamic(name: String)(i: String): String = "i: " + i.toString
   }

   val x = new X(3)
   val y1: String = x.call("adsf")
   val y2: Double = x.call(3)
}
AD.scala:6: error: ambiguous reference to overloaded definition,
both method applyDynamic in class X of type (name: String)(i: String)String
and  method applyDynamic in class X of type (name: String)(i: Int)Double
match argument types (String)
error after rewriting to J.this.x.<applyDynamic: error>("call")
possible cause: maybe a wrong Dynamic method signature?
   val y1: String = x.call("adsf")
                    ^
AD.scala:7: error: ambiguous reference to overloaded definition,
both method applyDynamic in class X of type (name: String)(i: String)String
and  method applyDynamic in class X of type (name: String)(i: Int)Double
match argument types (String)
error after rewriting to J.this.x.<applyDynamic: error>("call")
possible cause: maybe a wrong Dynamic method signature?
   val y2: Double = x.call(3)
                    ^

@scabug
Copy link
Author

scabug commented Nov 14, 2012

@paulp said:
You can only overload on the first parameter list, so given this resolution you can't ever overload applyDynamic, period.

@scabug
Copy link
Author

scabug commented Nov 14, 2012

@harrah said:
In case it got lost, I was trying to argue that there should be an error when overloading applyDynamic if it is not possible to overload it (and Paul confirms it is not possible). This wouldn't require the harder detection that would be required for the general case of detecting ambiguous implicits. The signature of the method doesn't matter except for the name.

@scabug
Copy link
Author

scabug commented Nov 14, 2012

@paulp said:
scala/scala#1629

@scabug
Copy link
Author

scabug commented Nov 14, 2012

@retronym said:
I haven't tried this, but I suspect that a macro applyDynamic could probably avoid the boxing/varargs tax of an unoverloaded version.

@scabug
Copy link
Author

scabug commented Nov 15, 2012

@harrah said:
My intention is not performance, but having the compiler do as much as possible in as usual a way as possible. Yes, I can write a macro that does what the compiler does, but that adds code to maintain, etc ... and well, the compiler already does it and does it in the way the user expects (mostly, anyway).

@scabug
Copy link
Author

scabug commented Nov 15, 2012

@paulp said:
Made overloading an error in 4444369ddf. This should not be taken to imply I agree with the present state of affairs, which I don't, only that it's better to give useful errors than to not.

@scabug
Copy link
Author

scabug commented Feb 3, 2013

@paulp said:
Going to revert the implementation restriction, because I thought of this, which does work given explicit type application:

import scala.language.dynamics

class A extends Dynamic {
  def applyDynamic[T1](method: String)(x1: T1): Any = 1
  def applyDynamic[T1, T2](method: String)(x: T1, y: T2): Any = 2
}

object Test {
  def main(args: Array[String]): Unit = {
    val x = new A
    println(x[Int](5))
    println(x[Int, String](5, "a"))
  }
}

However, I hope this causes people reconsider whether the overloading-hostile structure of applyDynamic makes sense. I think this shows that it does not. With no way to express a variable-length type parameter list, we have again, following in the footsteps of structural types, created a pseudo-general mechanism which is incapable by design of being used in common situations for which it would otherwise be useful, creating unnecessary surprises and friction. There is no way to write a dynamic method which can receive every application of a method name. You could push the above out to N type parameters, but then there's no way to avoid explicit type application.

@scabug
Copy link
Author

scabug commented Feb 3, 2013

@paulp said:
See also #7059.

@scabug
Copy link
Author

scabug commented Dec 27, 2013

@xeno-by said:
It looks like it's possible to overload applyDynamic using Paul's trick from #7059. The trick just needed a single small change!!

02:00 ~/Projects/Master/sandbox (master)$ cat Test.scala
import scala.language.dynamics

class A extends Dynamic {
  def applyDynamic(method: String): B = new B(method)
}
class B(method: String) {
  def apply(x: Int) = s"$method(x: Int)"
  def apply(x: String) = s"$method(x: String)"
}

object Test {
  def main(args: Array[String]): Unit = {
    val x = new A
    println(x.bippy(42))
    println(x.bippy("42"))
  }
}

02:00 ~/Projects/Master/sandbox (master)$ scalac Test.scala && scala Test
bippy(x: Int)
bippy(x: String)

@scabug
Copy link
Author

scabug commented Dec 27, 2013

@paulp said:
Oh, nice job. That's some solid tunnel vision on my part.

@scabug
Copy link
Author

scabug commented Dec 27, 2013

@xeno-by said:
scala/scala#3310

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