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

Don't generate fields for BoxedUnit val and var #2400

Merged

Conversation

nicolasstucki
Copy link
Contributor

This is intended as a way to not generate fields
for phantom fields, which by this time are erased
to BoxedUnit fields.

This is intended as a way to not generate fields
for phantom fields, which by this time are erased
to BoxedUnit fields.
@DarkDimius
Copy link
Contributor

Nice idea :-)
Shouldn't we also do it for our bottom types: Null & Nothing?

@smarter
Copy link
Member

smarter commented May 8, 2017

What happens if you extend a class where val foo: Any is abstract with val foo: Unit = ... ?

@DarkDimius
Copy link
Contributor

@smarter, superclass will have abstract getter and no actual field, so it should work fine.

Getters for Null fields return trivially `null`.
Getter for Nothing will `throw null`. This is only reachable
if the field is accesed in the costructor before the field is
initialized. In this case a `NullPointerException` is thrown as before.
@nicolasstucki nicolasstucki force-pushed the implement-phantom-types-part-3 branch from 38c486c to ca8477b Compare May 8, 2017 21:48
@nicolasstucki nicolasstucki requested a review from DarkDimius May 8, 2017 21:49
@sjrd
Copy link
Member

sjrd commented May 8, 2017

If that happens before the backend, it will be virtually impossible for the Scala.js to correctly implement its interop with JS. In a Scala.js-defined JS class, such a field must exist and be visible from JavaScript with the value undefined. Same thing with Null.

@adriaanm
Copy link
Contributor

adriaanm commented May 9, 2017

You should also exclude @volatile fields. (This trick is used in ParIterableLike.scala: @volatile var result: Unit = ())

@nicolasstucki
Copy link
Contributor Author

@adriaanm I would assume that it is only necessary for @volitile Unit fields.Null fields will always return null and Nothing field will throw before the object is completely instantiated. Is there some weird usage of the latter that I might be missing?

@nicolasstucki
Copy link
Contributor Author

@sjrd I will try to put it in it's own miniphase just before backend.

@smarter
Copy link
Member

smarter commented May 9, 2017

@nicolassstucki Don't we want phantom fields to also be optimized out in other backends?

@nicolasstucki
Copy link
Contributor Author

@smarter good point. @sjrd what king of interop with JS do you expect to have on Unit fields?

@sjrd
Copy link
Member

sjrd commented May 9, 2017

If you have the following Scala.js class:

@ScalaJSDefined
class Foo extends js.Object {
  val x: Unit = ()
}

Then the following must be true to respect the Scala.js interop spec:

val foo: Foo = new Foo
foo.hasOwnProperty("x")

And also from JS, if you have a foo:

foo.hasOwnProperty("x")

@DarkDimius
Copy link
Contributor

@nicolasstucki,

Null fields will always return null and Nothing field will throw before the object is completely instantiated. Is there some weird usage of the latter that I might be missing?

It's not about the value they return, it's about memory model and reordering of operations in JVM.
Reading and writing Null field does introduce read\write barriers. Same with Nothing, you can actually write null into it(and read it!) introducing a memory barrier.

I'd propose to include a check that the field is indeed not a @volatile field for all types, including Unit.

@@ -161,6 +161,12 @@ class FirstTransform extends MiniPhaseTransform with InfoTransformer with Annota
} else ddef
}

override def transformValDef(vdef: tpd.ValDef)(implicit ctx: Context, info: TransformerInfo): tpd.Tree = {
if (vdef.symbol.hasAnnotation(defn.VolatileAnnot) && vdef.tpt.tpe.isPhantom)
ctx.error("Phantom fields can not be @volitile", vdef.pos)
Copy link
Member

Choose a reason for hiding this comment

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

typo: volitile -> volatile

class Foo {
import Boo._

var foo = {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it makes sense to allow phantom var. var implies runtime semantics that just make no sense for phantoms.

@nicolasstucki nicolasstucki force-pushed the implement-phantom-types-part-3 branch from 7f0c087 to bd7d21d Compare May 9, 2017 11:15
@nicolasstucki nicolasstucki force-pushed the implement-phantom-types-part-3 branch from bd7d21d to c30b2db Compare May 9, 2017 11:20
@felixmulder felixmulder merged commit f73483e into scala:master May 16, 2017
@nicolasstucki nicolasstucki mentioned this pull request Jul 20, 2017
@allanrenucci allanrenucci deleted the implement-phantom-types-part-3 branch December 14, 2017 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants