Closes #579 Implement mini-phase for classOf[T]

[alexander-myltsev]

Closes #579

[DarkDimius]

Hi, Nice to see your first contribution!

[DarkDimius]

Looking at the implementation, it could be safely squashed with previous group.

[alexander-myltsev]

      List(new RefChecks,
           new ElimRepeated,
           new NormalizeFlags,
           new ExtensionMethods,
           new ExpandSAMs,
           new TailRec,
           new ClassOf),

Right?

[alexander-myltsev]

Glad to contribute :)

Regarding the test https://github.com/lampepfl/dotty/blob/master/tests/pending/run/classof.scala. I think it does not cover the case when an object of a class that is generated in ClassOf phase has same value of toString as classof.check expects, but not a class that is expected. For example, I might return some object of a class from Dotty that has boolean output of toString but is not java.lang.Boolean.TYPE under the hood.

Another question is that piece of code (https://github.com/lampepfl/dotty/blob/0d32755206e0f477c49e86b529e66e1154d7027f/src/dotty/tools/dotc/core/Definitions.scala#L274-282):

  lazy val BoxedBooleanModule = ctx.requiredModule("java.lang.Boolean")
  lazy val BoxedByteModule    = ctx.requiredModule("java.lang.Byte")
  lazy val BoxedShortModule   = ctx.requiredModule("java.lang.Short")
  lazy val BoxedCharModule    = ctx.requiredModule("java.lang.Character")
  lazy val BoxedIntModule     = ctx.requiredModule("java.lang.Integer")
  lazy val BoxedLongModule    = ctx.requiredModule("java.lang.Long")
  lazy val BoxedFloatModule   = ctx.requiredModule("java.lang.Float")
  lazy val BoxedDoubleModule  = ctx.requiredModule("java.lang.Double")
  lazy val BoxedVoidModule    = ctx.requiredModule("java.lang.Void")

Is there a better way to do it? I tried to use lazy val BoxedBooleanModule = BoxedBooleanClass.linkedClass but things do not work.

Ooops.. All tests should pass, right? I broke tests?

[DarkDimius]

Ooops.. All tests should pass, right? I broke tests?

The broken test is the one where typer inferred Class[Int] and this type was needed for typechecking, and you've replaced it by Class[Integer]. Those classes are different before erasure, but are the same after. The solution is to add ensureConforms(tree.tpe) in all branches handling primitives.

[alexander-myltsev]

Should cases as follows be covered?:

    println(classOf[Any])  // current: class java.lang.Object
    println(classOf[AnyRef])  // current: class java.lang.Object
    println(classOf[Null])  // current: class scala.runtime.Null$
    println(classOf[Nothing])  // current: class scala.runtime.Nothing$

[smarter]

If you write Literal(Constant(TypeErasure.erasure(tp, semiEraseVCs = false))) here, you should be able to remove the special case for value classes above.

[DarkDimius]

Do we actually currently have a single place that uses semiEraseVCs = false?

If we do not, I'd prefer to leave as is, as this code is simple and easy to comprehend. Instead I'd better remove semiEraseVCs flag from erasure, as it makes it more complicated.

[alexander-myltsev]

👍

[smarter]

Yes, we do: https://github.com/lampepfl/dotty/blob/master/src/dotty/tools/dotc/transform/Erasure.scala#L274

I don't think that semiEraseVCs is more confusing than a special case.

[smarter]

(It would probably help if semiEraseVCs was documented in TypeErasure#erasure, currently it's only documented for the class TypeErasure: https://github.com/lampepfl/dotty/blob/master/src/dotty/tools/dotc/core/TypeErasure.scala#L260-L262)

[alexander-myltsev]

This doesn't work. The output is as follows:

println(classOf[Unit]) // class scala.runtime.BoxedUnit instead of void
println(classOf[Boolean]) // class java.lang.Boolean instead of boolean
println(classOf[Byte]) // class java.lang.Byte instead of byte
println(classOf[Short]) // class java.lang.Short instead of short
println(classOf[Char]) // class java.lang.Character instead of char
println(classOf[Int]) // class java.lang.Integer instead of int
println(classOf[Long]) // class java.lang.Long instead of long
println(classOf[Float]) // class java.lang.Float instead of float
println(classOf[Double]) // class java.lang.Double instead of double

[smarter]

@DarkDimius : So what you're saying is that semiEraseVCs = false should be the default, I guess we could do that yeah, it just involves adding semiEraseVCs = true in a lot of places in Erasure and TypeErasure, because most of the time when you erase a type you want to semi-erase it.

[smarter]

@alexander-myltsev : I didn't say that you should remove the special cases for primitive classes, but the special case for value classes: if (ValueClasses.isDerivedValueClass(claz)) { ...}

[DarkDimius]

@smarter I am not speaking about erasure. I am speaking about this phase. This phase should for a classOf[Meter] generate a tree that is either Literal(Constant(ClassInfo(Meter)) or Literal(Constant(TypeRef(Meter)), but not Literal(Constant(ErasedValueType(Meter)). The last one is not expected to reach backend and is not guaranteed to be handled by it in any reasonable manner.

[smarter]

@DarkDimius: I agree. But this phase uses TypeErasure.erasure where the default is semiEraseVCs = true, and what I was saying is that I agree that it makes sense for the default to be semiEraseVCs = false so that any phase can use TypeErasure.erasure without worrying about semi-erasure.

[alexander-myltsev]

@smarter thanks for meaningful remarks

All should be fixed now

[smarter]

@DarkDimius : The backend contains code to deal with classOf: https://github.com/lampepfl/dotty/blob/d2c96d02fccef3a82b88ee1ff31253b6ef17f900/src/dotty/tools/backend/jvm/DottyBackendInterface.scala#L247-L248 Is it dead code?

[DarkDimius]

For dotty it is, for scalac it isn't.

[smarter]

But this code is in DottyBackendInterface, can this interface be used by scalac too?

[DarkDimius]

That's the code that handles annotations. Annotations do contain trees, but they are special in many ways and their handling is very ad-hoc.