Skip to content

Implement getClass #734

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 27, 2015
Merged

Implement getClass #734

merged 2 commits into from
Jul 27, 2015

Conversation

alexander-myltsev
Copy link
Contributor

Closes #733

@alexander-myltsev
Copy link
Contributor Author

@DarkDimius @smarter, please check if a test covers expected behaviour.

@DarkDimius
Copy link
Contributor

This PR does not currently have an implementation, only a test that fails due to non-implemented #733. Did you forget to push something?

@alexander-myltsev
Copy link
Contributor Author

It does not have yet. Is the test correct?

@DarkDimius
Copy link
Contributor

All names but name of the anonymous lambdas (Test$$anonfun$1) are correct.
For lambdas: I guess you would get something like Test$$lambda$1/XXXXX, where XXXX is a number of origin that I am unaware of.

DarkDimius
DarkDimius reviewed Jul 23, 2015
View reviewed changes
tests/run/getclass.scala
println(1f.getClass)
println(1d.getClass)

println("Class types")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add an empty string to the output here, to have output structured.

@alexander-myltsev
Copy link
Contributor Author

There are compile-time warnings:

dotty/out$ ../bin/dotc ../tests/run/getclass.scala
warning: encountered F-bounded higher-kinded type parameters for type scala$collection$generic$GenMapFactory$$CC; assuming they are invariant
warning: encountered F-bounded higher-kinded type parameters for type scala$collection$generic$MapFactory$$CC; assuming they are invariant
warning: encountered F-bounded higher-kinded type parameters for type scala$collection$generic$ImmutableMapFactory$$CC; assuming they are invariant
three warnings found

And runtime errors as follows:

dotty/out$ java -cp .:/home/vagrant/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.11.5.jar:/home/vagrant/.ivy2/cache/org.scala-lang/scala-reflect/jars/scala-reflect-2.11.5.jar:/home/vagrant/dotty/target/scala-2.11/dotty_2.11-0.1-SNAPSHOT.jar Test
Exception in thread "main" java.lang.BootstrapMethodError: java.lang.NoClassDefFoundError: scala/compat/java8/JProcedure2
        at Test$.main(getclass.scala:9)
        at Test.main(getclass.scala)
Caused by: java.lang.NoClassDefFoundError: scala/compat/java8/JProcedure2
        ... 2 more
Caused by: java.lang.ClassNotFoundException: scala.compat.java8.JProcedure2
        at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:331)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
        ... 2 more

What should be fixed?

Everything else passes.

@DarkDimius
Copy link
Contributor

There are compile-time warnings:

Those are known warnings. Ignore them.

NoClassDefFoundError: scala/compat/java8/JProcedure2

That is strange. This is a class defined in dotty/src/scala/compat/java8/JFunction2.java, and it should be(and on my machine is) part of dotty_2.11-0.1-SNAPSHOT.jar

@alexander-myltsev
Copy link
Contributor Author

Fixed. My CP was wrong.

There is another problem. Seems like Array(List("1")) would have type of Object after erasure. So, Array(List("1")).getClass would be class java.lang.Object instead of class [Lscala.collection.immutable.List;.

@DarkDimius
Copy link
Contributor

Seems like Array(List("1")) would have type of Object after erasure

AFAIK it should not. Though you should not handle arrays at all: classes that extend AnyRef have a runtime .getClass that should be used.

smarter
smarter reviewed Jul 24, 2015
View reviewed changes
src/dotty/tools/dotc/transform/GetClass.scala
* FILL ME
*/
class GetClass extends MiniPhaseTransform {
import tpd.{Apply, Tree, Literal, ref, TreeOps}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just import tpd._

@smarter
Copy link
Member

smarter commented Jul 24, 2015

There is code in InterceptedMethods to deal with getClass: https://github.com/lampepfl/dotty/blob/cf17b73cc02daf514f6d5499fd7a5562fc93349e/src/dotty/tools/dotc/transform/InterceptedMethods.scala#L116-L125
It'd be good to at least avoid duplication (for example by moving primitiveGetClassMethods inside Definitions.scala and using it in both GetClass and InterceptedMethods) but it might make sense to simply move this code from InterceptedMethods to GetClass, so that all the code that deals with getClass is inside GetClass. @DarkDimius : what do you think?

@DarkDimius
Copy link
Contributor

but it might make sense to simply move this code from InterceptedMethods to GetClass.

This code is left as it tries to cover non-trivial bug. Though I've checked now and the way how this was fixed in scalac does not work in dotty: 5.asInstanceOf[Int with AnyRef].getClass is still erased to 5.getClass.

I believe that we need a different fix. The way I would fix it in dotty is to rewrite

5.asInstanceOf[Int with AnyRef].getClass

to

(5.asInstanceOf[Int with AnyRef]: Object).getClass

This should do the trick.

DarkDimius
DarkDimius reviewed Jul 25, 2015
View reviewed changes
src/dotty/tools/dotc/transform/GetClass.scala
case defn.FloatClass => TYPE(defn.BoxedFloatModule)
case defn.DoubleClass => TYPE(defn.BoxedDoubleModule)
case defn.UnitClass => TYPE(defn.BoxedVoidModule)
case _ => qual.selectWithSig(defn.Any_getClass).appliedToNone
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not simply tree?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that works.

@alexander-myltsev alexander-myltsev force-pushed the add-getclass branch 4 times, most recently from 0f4cb1a to 721e840 Compare July 25, 2015 14:50
DarkDimius
DarkDimius reviewed Jul 27, 2015
View reviewed changes
src/dotty/tools/dotc/Compiler.scala
@@ -62,6 +62,7 @@ class Compiler {
new AugmentScala2Traits,
new ResolveSuper),
List(new Erasure),
List(new GetClass), // getClass transformation should be applied to specialized methods
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try to put it in next mini-phase block?

@DarkDimius
Copy link
Contributor

Otherwise LGTM

@alexander-myltsev
Copy link
Contributor Author

@DarkDimius , well, worked out. Should I squash both commits?

@DarkDimius
Copy link
Contributor

LGTM

DarkDimius added a commit that referenced this pull request Jul 27, 2015
@DarkDimius
Merge pull request #734 from alexander-myltsev/add-getclass
056e124 
Implement getClass
@DarkDimius DarkDimius merged commit 056e124 into scala:master Jul 27, 2015
@DarkDimius
Copy link
Contributor

@alexander-myltsev, thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexander-myltsev @DarkDimius @smarter @dottybot