Skip to content

SI-6710 / PR 5072 follow-up: fix Unit.box / Unit.unbox #5100

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
lrytz merged 1 commit into from
Apr 20, 2016
Merged

SI-6710 / PR 5072 follow-up: fix Unit.box / Unit.unbox #5100

lrytz merged 1 commit into from
Apr 20, 2016
+133 −10

Conversation

@lrytz
Copy link
Member

@lrytz lrytz commented Apr 15, 2016 •
edited
Loading

The backend replaces .box / .unbox methods by corresponding invocations
to BoxesRunTime, but not for Unit.

This commit extends the backend to also intercept box/unbox on Unit.

@scala-jenkins scala-jenkins added this to the 2.12.0-M5 milestone Apr 15, 2016
@sjrd
Copy link
Member

sjrd commented Apr 15, 2016

Hum ... why complicate the back-end when the user-space implementation of these methods was perfectly fine?

Adding code in the compiler to avoid code in the library seems like entirely the wrong trade-off to me.

@lrytz
Copy link
Member Author

lrytz commented Apr 15, 2016 •
edited
Loading

This PR only fixes the regression of #5072.

There's a lot more that could be done around this area: if you play a bit with null.asInstanceOf[X] you'll quickly find surprises. I tried to collect the known ones into a test and make sure that at least we don't accidentally change anything.

Yet another question is whether we actually need to intercept the box/unbox methods and re-wire to the box/unbox implementations in BoxesRunTime. We could just put the implementations in the primitive classes companion objects. The only disadvantage I see is that instead of a static call we'd have a module load and an instance call.

@lrytz
Copy link
Member Author

lrytz commented Apr 15, 2016

Hum ... why complicate the back-end when the user-space implementation of these methods was perfectly fine?

yeah, it's just one way to do it, and it was more consistent. i had the same thought of course, see my other comment: i think we could just get rid of hijacking box/unbox altogether.

@lrytz
Copy link
Member Author

lrytz commented Apr 15, 2016

it seems the logic was initially added to support the .net backend: 8e56e0e

@sjrd
Copy link
Member

sjrd commented Apr 15, 2016 •
edited
Loading

Or erasure could just generate calls to BoxesRunTime directly, rather than the box/unbox methods. We implement box/unbox in user-space calling BoxesRunTime, and we deprecate those.

Another advantage of keeping the actual implem in BoxesRunTime rather than Int and co., is that BoxesRunTime is already platform-dependent, and overridden in Scala.js.

@lrytz lrytz force-pushed the unitBox branch 2 times, most recently from 115a862 to 46179eb Compare April 20, 2016 07:58
@lrytz
SI-6710 / PR 5072 follow-up: fix Unit.box / Unit.unbox
c600c15 
The backend replaces .box / .unbox methods by corresponding invocations
to BoxesRunTime, but not for Unit.

This commit restores the body of `Unit.box` and `Unit.unbox`.
@lrytz
Copy link
Member Author

lrytz commented Apr 20, 2016

I changed the patch: now it restores the implementations of Unit.box / Unit.unbox, which are not re-wired to BoxesRunTime to in the backend.

Note that the implementation of Unit.unbox is changed:

  def unbox(x: java.lang.Object): Unit = x.asInstanceOf[scala.runtime.BoxedUnit]

Initially the method was just returning (). This is however inconsistent, all other unbox methods cast to the box type. Also, the Scaladoc was always saying "it accepts any Object, but will throw an exception if the argument is not a scala.runtime.BoxedUnit".

Longer-term, I think we should remove the re-wiring to BoxesRunTime and just put the implementations in the primitive companions. However, I think we need to implement the @static support first, otherwise every box/unbox operation involves a module load.

I think we should not deprecate Int.box, as that is the recommended (and most obvious) API for users.

@sjrd
Copy link
Member

sjrd commented Apr 20, 2016

LGTM

@lrytz lrytz merged commit 2ec7e79 into scala:2.12.x Apr 20, 2016
@lrytz lrytz mentioned this pull request Apr 20, 2016
Merged
@lrytz lrytz deleted the unitBox branch May 17, 2016 13:37
retronym
retronym reviewed May 19, 2016
View reviewed changes
test/junit/scala/BoxUnboxTest.scala
def f(a: Any) = "" + a
val n3 = f(null.asInstanceOf[Unit]) // "null", should be "()". probably same cause as SI-602.
assertThrows[AssertionError](assertEquals(n3, "()")) // should not throw
}
Copy link
Member

@retronym retronym May 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry I didn't catch this before hand, but shouldn't this be a partest test, or a JUnit test which includes the code above as a String? This is a test of the compiler, not the library.

Copy link
Member Author

@lrytz lrytz May 19, 2016

Choose a reason for hiding this comment

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

right, i the entire test should be converted, i'll do that. note to slef: also do it for the inliner test (array.foreach)

@adriaanm adriaanm added 2.12 and removed 2.12 labels Oct 29, 2016
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

2.12.0-M5

Development

Successfully merging this pull request may close these issues.

5 participants

@lrytz @sjrd @retronym @adriaanm @scala-jenkins