scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

Abstract Type in pattern match compiles inconsistently #2917

Closed scabug closed 13 years ago

scabug commented 14 years ago

Having a trait with methods that use declared abstract types for pattern matching compiles different if the method is defined in the trait or if exactly the same code is used in a concrete subclass.

Here is the example:


trait Value {
    type T
    protected val value: T
    type Self <: {val value: T}

    def myType: String

    protected def newValue(newVal: T): Self
    override def hashCode = value.hashCode
    override def equals(other: Any): Boolean = other match {
        case that: Self =>
          println(myType)
          this.value == that.value
        case _ => false
    }
}

I created a JUnit Test using that trait with two different concrete classes, I was expecting instances of the same class not to be equal even if the contained value is equal. But I to my surprise the instances compared equally.

import org.junit.Assert._
import org.junit.Test

import org.scalatest.junit.AssertionsForJUnit

class AbstractTypeTest extends AssertionsForJUnit {
    class CU1(val value: BigDecimal) extends Value  {
        type T = BigDecimal
        type Self = CU1

        def myType = classOf[Self].getName
        protected def newValue(newVal: T) = new CU1(newVal)

    }

    class CU2(val value: BigDecimal) extends Value {
        type T = BigDecimal
        type Self = CU2

        def myType = classOf[Self].getName
        protected def newValue(newVal: T) = new CU2(newVal)
    }

    @Test def debugEquality() {
        val a = new CU1(25)
        val b = new CU2(25)
        assert(a != b, "amounts in different currencies should not be equal")
        assert(b != a, "amounts in different currencies should not be equal")

    }
}

After some investigation I found out that this will hold if I override the equality method in the concrete class using exactly the same implementation as in the trait.


import org.junit.Assert._
import org.junit.Test

import org.scalatest.junit.AssertionsForJUnit

class AbstractTypeTest extends AssertionsForJUnit {
    class CU1(val value: BigDecimal) extends Value  {
        type T = BigDecimal
        type Self = CU1

        def myType = classOf[Self].getName
        protected def newValue(newVal: T) = new CU1(newVal)

        override def equals(other: Any): Boolean = other match {
            case that: Self =>
              println(myType)
              this.value == that.value
            case _ => false
        }
    }

    class CU2(val value: BigDecimal) extends Value {
        type T = BigDecimal
        type Self = CU2

        def myType = classOf[Self].getName
        protected def newValue(newVal: T) = new CU2(newVal)
    }

    @Test def debugEquality() {
        val a = new CU1(25)
        val b = new CU2(25)
        assert(a === b, "amounts in different currencies should not be equal")
        assert(b === a, "amounts in different currencies should not be equal")

    }
}

I use JD to decompile the resulting classes and saw that both classes (CU1 and CU2) have totally different implementations for the equality method which based in my understanding of traits should not be the case, both classes should be nearly identical.


 public class CU1
    implements Value, ScalaObject
  {
    private final BigDecimal value;

    public CU1(BigDecimal value)
    {
      Value.class.$$init$$(this);
    }

    public boolean equals(Object other)
    {
      Object localObject = other; if ((!(localObject instanceof CU1)) || (((CU1)localObject).oforero$$AbstractTypeTest$$CU1$$$$$$outer() != oforero$$AbstractTypeTest$$CU1$$$$$$outer()))
        break label72;
      Predef..MODULE$$.println(myType());
      localBigDecimal = ((CU1)localObject)
        .value();
      BigDecimal tmp45_34 = value(); if (tmp45_34 != null) break label57; label57: label72: tmp45_34;
    }

    public CU1 newValue(BigDecimal newVal)
    {
      return new CU1(oforero$$AbstractTypeTest$$CU1$$$$$$outer(), newVal);
    }

    public String myType()
    {
      return CU1.class.getName();
    }

    public BigDecimal value()
    {
      return this.value; } 
    public int $$tag() throws RemoteException { return ScalaObject.class.$$tag(this); } 
    public int hashCode() { return Value.class.hashCode(this);
    }
  }
}

 public class CU2
    implements Value, ScalaObject
  {
    private final BigDecimal value;

    public CU2(BigDecimal value)
    {
      Value.class.$$init$$(this);
    }

    public CU2 newValue(BigDecimal newVal)
    {
      return new CU2(oforero$$AbstractTypeTest$$CU2$$$$$$outer(), newVal);
    }

    public String myType()
    {
      return CU2.class.getName();
    }

    public BigDecimal value()
    {
      return this.value; } 
    public int $$tag() throws RemoteException { return ScalaObject.class.$$tag(this); } 
    public boolean equals(Object x$$1) { return Value.class.equals(this, x$$1); } 
    public int hashCode() { return Value.class.hashCode(this);
    }
  }

I first found this in 2.8 (Snapshot 17.01.2010) and confirmed it in 2.7.7

scabug commented 14 years ago

Imported From: https://issues.scala-lang.org/browse/SI-2917?orig=1 Reporter: Oscar Forero (oforero) Attachments:

scabug commented 14 years ago

@paulp said: You gravely need to read about erasure. You've apparently been ignoring a message like this:

warning: there were unchecked warnings; re-run with -unchecked for details

If you did what it says you would see this:

<console>:14: warning: abstract type Value.this.Self in type pattern Value.this.Self is unchecked since it is eliminated by erasure
               case that: Self =>
                          ^

When you write "case that: Self =>" you are not testing against Self but the erasure of Self, which in this case is the erasure of its upper bound. All structural types erase to Object, so you could equivalently be writing "if (true)".

scabug commented 14 years ago

Oscar Forero (oforero) said: Thanks for the answer, you are right I have not seen that message, I am compiling with maven which obscure many messages. I will compile directly next time to see if something like that comes up.

The question I have is why then overriding the method with exactly the same code works? shouldn't the method be compiled when the abstract types are resolved?

My understanding was that when a Trait has implemented methods and is mixed in with a class, the result should be equal to inherit and interface and implement on place. From this it looks that the method gets compiled and then the compiled method is plug into the class you are mixin in, which make them (IMHO) unsafe to be used with abstract members (at least types).

I see your point about the structural type, but again the type is later set to a concrete class. I just used because the following gives a compiler error:

trait Value {
    type T
    protected val value: T
    type Self <: Value

    def myType: String

    protected def newValue(newVal: T): Self
    override def hashCode = value.hashCode
    override def equals(other: Any): Boolean = other match {
        case that: Self =>
          println(myType)
          this.value == that.value
        case _ => false
    }
}

PS: Does erasure means that structural types should not be used with pattern matching?

scabug commented 14 years ago

@paulp said: Replying to [comment:2 oforero]:

PS: Does erasure means that structural types should not be used with pattern matching?

As long as you don't mind that they will always match. IOW, right, don't use them. Beyond that I have to direct you to the mailing lists or stackoverflow -- trac is not the right place for questions.