Skip to content

Do not make ReferenceType extend Type anymore. #2150

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 1 commit into from
Jan 12, 2016
Merged

Do not make ReferenceType extend Type anymore. #2150

merged 1 commit into from
Jan 12, 2016

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Jan 8, 2016

A ReferenceType is fundamentally not a Type. It so happens that
all concrete implementations of ReferenceType also implement Type,
but those two concepts must not be confused.

There are a few cases were it is OK-ish to treat a ReferenceType
as a Type (such as serializing/hashing), for which we introduced
casts.

This refactoring highlights a suspicious usage of ReferenceType as
Type in the OptimizerCore core. This commit leaves them as is,
with casts.

@sjrd
Copy link
Member Author

sjrd commented Jan 8, 2016

Review by @gzm0

@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2704/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3491/
Test PASSed.

@sjrd sjrd assigned nicolasstucki and gzm0 and unassigned nicolasstucki Jan 8, 2016
@gzm0
Copy link
Contributor

gzm0 commented Jan 9, 2016

It seems I am lacking fundamental knowledge here. What is the difference between a type and a reference type? (I'm assuming this is not the reference type in "value type" v.s. "reference type").

@sjrd
Copy link
Member Author

sjrd commented Jan 9, 2016

A ReferenceType has exactly the same level of precision as a JVM type. There is a one-to-one relationship between a ReferenceType and an instance of java.lang.Class. This means that:

  • All primitive types have their reference type (including scala.Byte and scala.Short), and they are different from their boxed versions.
  • Raw JS types are not erased to any
  • Array types are like on the JVM

A ReferenceType therefore uniquely identifies a classOf[T]. It is also the reference types that are used in method signatures, and which therefore dictate JVM/IR overloading.

A Type is the type of an expression (or statement) in the IR. There is a many-to-one relationship from reference types to types, because:

  • scala.Byte, scala.Short and scala.Int collapse to int
  • java.lang.Object and raw JS types all collapse to any

(in fact, there are two Types that do not have a real equivalent in reference types: StringType and UndefinedType, as they refer to the non-null variants of java.lang.String and scala.runtime.BoxedUnit, respectively)

gzm0
gzm0 reviewed Jan 11, 2016
View reviewed changes
ir/src/main/scala/org/scalajs/core/ir/Types.scala
@@ -84,13 +84,14 @@ object Types {
case object NullType extends Type

/** Reference types (allowed for classOf[], is/asInstanceOf[]). */
sealed abstract class ReferenceType extends Type
sealed trait ReferenceType
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason you do not add a toType method here that essentially returns this (with this having a self-type of Type with ReferenceType)? It seems its less fragile than the casts but essentially does the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would argue that it would be more fragile, because it would lead to people assuming that it is always safe and meaningful to convert a ReferenceType into a Type, which it is not.

The proper toType conversion would require a predicate isJSType: /* className: */ String => Boolean to know whether ClassType(className) has to erase to AnyType or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see. But why then don't you mark the cast in asInstanceOf as suspicious? IIUC we're doing the following wrong:

AsInstanceOf(???, ClassType("scala.Int")).tpe // -> ClassType("scala.Int")

But it should be IntType.

Copy link
Member Author

Choose a reason for hiding this comment

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

AsInstanceOf(???, ClassType("scala.Int")) (which would actually be AsInstanceOf(???, ClassType("I"))) is not valid IR. It would be Unbox(???, 'I') which has tpe = IntType.

Copy link
Member Author

Choose a reason for hiding this comment

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

This whole business is super hacky. We should definitely re-design this aspect of the IR for the next major version. At least with this PR we make it obvious that it's hacky ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, fair enough.

@gzm0
Copy link
Contributor

gzm0 commented Jan 11, 2016

Could you also paste what you wrote about reference types into the documentation of the Types object?

@gzm0
Copy link
Contributor

gzm0 commented Jan 11, 2016

That's all.

@sjrd
Copy link
Member Author

sjrd commented Jan 11, 2016

Could you also paste what you wrote about reference types into the documentation of the Types object?

Sure.

@gzm0
Copy link
Contributor

gzm0 commented Jan 11, 2016

Ok. So LGTM modulo the comment on Types.

@sjrd sjrd force-pushed the reftype-not-subtype-of-type branch from a2bfe19 to 0827ded Compare January 11, 2016 12:31
@sjrd
Copy link
Member Author

sjrd commented Jan 11, 2016

Updated with the documentation.

@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2722/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3513/
Test FAILed.

@sjrd sjrd force-pushed the reftype-not-subtype-of-type branch from 0827ded to b2cbb09 Compare January 11, 2016 17:16
@sjrd
Do not make ReferenceType extend Type anymore.
b2cbb09 
A ReferenceType is fundamentally *not* a Type. It so happens that
all concrete implementations of ReferenceType also implement Type,
but those two concepts must not be confused.

There are a few cases were it is OK-ish to treat a ReferenceType
as a Type (such as serializing/hashing), for which we introduced
casts.

This refactoring highlights a suspicious usage of ReferenceType as
Type in the OptimizerCore core. This commit leaves them as is,
with casts.
@sjrd
Copy link
Member Author

sjrd commented Jan 11, 2016

Rebased on top of #2148. There was a compile error with the automatic clean merge.

@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2726/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3517/
Test PASSed.

@gzm0
Copy link
Contributor

gzm0 commented Jan 12, 2016

LGTM

gzm0 added a commit that referenced this pull request Jan 12, 2016
@gzm0
Merge pull request #2150 from sjrd/reftype-not-subtype-of-type
007eb46 
Do not make ReferenceType extend Type anymore.
@gzm0 gzm0 merged commit 007eb46 into scala-js:master Jan 12, 2016
@sjrd sjrd deleted the reftype-not-subtype-of-type branch January 21, 2016 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@gzm0 gzm0

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sjrd @scala-jenkins @gzm0 @nicolasstucki