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

Regression in 2.13.1 in overload resolution #11755

Closed
sjrd opened this issue Oct 1, 2019 · 8 comments · Fixed by scala/scala#8458

Comments

@sjrd
Copy link
Member

commented Oct 1, 2019

The following file compiles fine with 2.13.0 but not with 2.13.1 (nor the latest 2.13.x), without any compiler flag:

package test

trait Map[K, V]

class HashMap[K, V] extends Map[K, V] {
  override def clone(): AnyRef = super.clone()
}

class IdentityBox[+A]

class IdentityHashMap[K, V](inner: HashMap[IdentityBox[K], V])
    extends Map[K, V] {

  def this(initialMap: Map[_ <: K, _ <: V]) =
    this(new HashMap[IdentityBox[K], V]())

  override def clone(): AnyRef = {
    new IdentityHashMap(
        inner.clone().asInstanceOf[HashMap[IdentityBox[K], V]])
  }
}

With 2.13.1 the following compile error is reported:

[error] Test.scala:18:5: ambiguous reference to overloaded definition,
[error] both constructor IdentityHashMap in class IdentityHashMap of type (initialMap: test.Map[_ <: K, _ <: V])test.IdentityHashMap[K,V]
[error] and  constructor IdentityHashMap in class IdentityHashMap of type (inner: test.HashMap[test.IdentityBox[K],V])test.IdentityHashMap[K,V]
[error] match argument types (test.HashMap[test.IdentityBox[K],V]) and expected result type AnyRef
[error]     new IdentityHashMap(
[error]     ^
[error] one error found

This is a minimization of a build failure of scala-js/scala-js in the community build, initially reported at scala-js/scala-js#3792.

It seems to me that this is a bug in scalac. The first overload (the primary constructor) should be picked, IMO, as its argument has a much more specific type than the second overload.

@sjrd sjrd added the regression label Oct 1, 2019
sjrd added a commit to sjrd/scala-js that referenced this issue Oct 1, 2019
…tion.

This commit works around scala/bug#11755.
Instead of relying on an arguably sensitive overload, we use an
extra argument for explicit disambiguation.
sjrd added a commit to sjrd/scala-js that referenced this issue Oct 1, 2019
…tion.

This commit works around scala/bug#11755.
Instead of relying on an arguably sensitive overload, we use an
extra argument for explicit disambiguation.
@SethTisue

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

a possible next step here would be to use https://github.com/adriaanm/binfu/blob/master/bisect.sh to identify the PR where this regressed

@sjrd

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2019

bisect.sh blames scala/scala#8129.

@SethTisue

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

@adriaanm want to take a look?

@SethTisue SethTisue added this to the 2.13.2 milestone Oct 2, 2019
@SethTisue SethTisue added the typer label Oct 2, 2019
@adriaanm adriaanm self-assigned this Oct 2, 2019
@adriaanm

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

I have to admit I am still thinking about this, but my first observation (based on the nature of the bug fix), is that it's strange that you can reproduce the ambiguity error on older versions if you simply drop the second type param in map. That should not affect specificity in any way, right?

trait Map[K]

class HashMap[K] extends Map[K] {
  override def clone(): AnyRef = super.clone()
}

class IdentityBox[+A]

class IdentityHashMap[K](inner: HashMap[IdentityBox[K]])
  extends Map[K] {

  def this(initialMap: Map[_ <: K]) =
    this(new HashMap[IdentityBox[K]]())

  override def clone(): AnyRef = {
    new IdentityHashMap(
      inner.clone().asInstanceOf[HashMap[IdentityBox[K]]])
  }
}

Both on 2.12 and 2.13.0, this gives:

error: ambiguous reference to overloaded definition,
both constructor IdentityHashMap in class IdentityHashMap of type (initialMap: Map[_ <: K])IdentityHashMap[K]
and  constructor IdentityHashMap in class IdentityHashMap of type (inner: HashMap[IdentityBox[K]])IdentityHashMap[K]
match argument types (HashMap[IdentityBox[K]]) and expected result type AnyRef
    new IdentityHashMap(
    ^
one error found
@adriaanm

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

In other words, at least the new behavior is consistent between the versions with one and two type param. Now I need to run the specificity scoring and complete my case for why this is all progress :-)

@adriaanm

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

Then again, using a regular method instead of the constructor makes the code compile again...

trait Map[K]
class HashMap[K] extends Map[K]
class IdentityBox[+A]

class IdentityHashMap[K](inner: HashMap[IdentityBox[K]]) extends Map[K] {
  def this(initialMap: Map[_ <: K]) = this(new HashMap[IdentityBox[K]]())

  def bla[K](initialMap: Map[_ <: K]): IdentityHashMap[K] = bla(new HashMap[IdentityBox[K]]())
  def bla[K](inner: HashMap[IdentityBox[K]]): IdentityHashMap[K] = ???

//  new IdentityHashMap(??? : HashMap[IdentityBox[K]])
  bla(??? :HashMap[IdentityBox[K]])
}
@adriaanm

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

Looks like the problem lies with the constructor being for a polymorphic class, but we don't push down the type params to each constructor's signature, so you get a debug trace like:

- isApplicable false : (initialMap: Map[_ <: K])IdentityHashMap[K] to List(HashMap[IdentityBox[K]]) for ? under List()
+ isApplicable true : (initialMap: Map[_ <: K])IdentityHashMap[K] to List(HashMap[IdentityBox[K]]) for ? under List(type K)

that last isApplicable should be true if K was properly treated as a (separate) type param for each constructor

--- i/src/compiler/scala/tools/nsc/typechecker/Infer.scala
+++ w/src/compiler/scala/tools/nsc/typechecker/Infer.scala
@@ -829,7 +829,7 @@ trait Infer extends Checkable {
           case ErrorType                 => true
           case _                         => false
         }
-//      println(s"isApplicable $res : $ftpe to $argtpes0 for $pt under $undetparams")
+      println(s"isApplicable $res : $ftpe to $argtpes0 for $pt under $undetparams")
       res
     }
 
@@ -886,7 +886,7 @@ trait Infer extends Checkable {
             case _                                             => isAsSpecificValueType(ftpe1, ftpe2, Nil, Nil)
           }
       }
-      // println(s"isAsSpecific $res $ftpe1 - $ftpe2")
+       println(s"isAsSpecific $res $ftpe1 - $ftpe2")
       res
     }
@adriaanm

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.