Imported From: https://issues.scala-lang.org/browse/SI-6710?orig=1
Reporter: @demobox
Affected Versions: 2.10.0-RC2
See #602
Attachments:

• 2.10.0-RC2-repl-session.png (created on Nov 25, 2012 2:36:19 AM UTC, 7386 bytes)
• print-jvm-output.txt (created on Nov 25, 2012 2:36:19 AM UTC, 1835 bytes)
• Test.jad (created on Nov 25, 2012 2:36:19 AM UTC, 1323 bytes)
• Test.scala (created on Nov 25, 2012 2:36:19 AM UTC, 111 bytes)

@demobox said:
See https://github.com/demobox/scala/commit/e586a135c6639936daca298db9a8f44e43de468b for a suggested amendment. This has not been submitted as a pull request yet since it does not contain a test case.

However, the test case is not simple to automate since the runtime behaviour of scala.Int.unbox is not determined by the code that has been changed (see attached screenshot).

The test case is:

1. Define a function whose body matches scala.Int.unbox
2. Call the function with input null
3. The function should return 0 and not throw a NullPointerException

@heathermiller said:
Yes, it appears that the the implementation of unbox is now never used. This might have happened when we switched to the new GenASM backend. I'd have to check with Miguel to be sure.

Here is where the backend chooses the unboxToInt implementation:
https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala#L1298
https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala#L2485

In that case, your suggested patch looks innocent and essentially just documentation.

@demobox said (edited on Nov 26, 2012 1:25:45 PM UTC):

your suggested patch looks innocent and essentially just documentation.

Thanks for the comment, Heather. Yes, the change is indeed intended as a "documentation" update rather than any functionality change.

Luc (Bourlier) had mentioned Array [1] as an example where it's perhaps clearer that the code itself "does nothing", but I was trying to keep the diff as minimal as possible.

Shall I go ahead and turn the commit into a pull request?

[1] https://github.com/scala/scala/blob/master/src/library/scala/Array.scala#L496

@magarciaEPFL said:
object Int's unbox(Object) is used by Erasure:

https://github.com/scala/scala/blob/master/src/reflect/scala/reflect/internal/Definitions.scala#L101

https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/transform/Erasure.scala#L657

and it's also used in other places via definitions.isUnbox

One of those usages reads:

          /** Can't always remove a Box(Unbox(x)) combination because the process of boxing x
           *  may lead to throwing an exception.
           *
           *  This is important for specialization: calls to the super constructor should not box/unbox specialized
           *  fields (see TupleX). (ID)
           */
          case Apply(boxFun, List(arg)) if isUnbox(tree.symbol) && safeToRemoveUnbox(arg.tpe.typeSymbol) =>

Therefore, changing object Int's unbox(Object) is "non trivial".

@demobox said:

Therefore, changing object Int's unbox(Object) is "non trivial".

Aha! Interesting. Thanks for explaining, Miguel.

Do you have any suggestions for creating a test that could validate that the change does not break existing behaviour?

Also, even if the method is, in fact, called, is it necessary that the behaviour differ from the actual runtime behaviour of Int unboxing?

@magarciaEPFL said:

Also, even if the method is, in fact, called, is it necessary that the behaviour differ
from the actual runtime behaviour of Int unboxing?

I guess it's not necessary to change the body of object Int's unbox() . After all, the definition itself does not tell "the whole story" of when Erasure decides to leave the invocation in place vs rewrite it. If the behavior were to be documented, then it should be for "==" , as [comment-40668 in SI-602 | https://issues.scala-lang.org/browse/SI-602?focusedCommentId=40668&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-40668] makes clear.

@magarciaEPFL said:
Regarding the question why -Xprint:jvm shows

scala.this.Predef.println(scala.Boolean.box(scala.Int.unbox(null).==(0)))

but the bytecode contains an invocation of BoxesRunTime.unboxToInt()

   4:	invokestatic	#76; //Method scala/runtime/BoxesRunTime.unboxToInt:(Ljava/lang/Object;)I

That's because -Xprint:jvm actually prints trees right after CleanUp, and the rewriting that's being chased here takes place in GenICode, whose output can be seen via -Xprint-icode (excerpt follows)

  def comp_zero(): Unit {
  locals: variable boxed2
  startBlock: 1
  blocks: [1,2,3,4]
  
  1: 
    10	LOAD_MODULE object Predef
    10	CONSTANT(null)
    10	UNBOX INT
    10	CONSTANT(0)
    10	CJUMP (INT)EQ ? 2 : 3
    

Sorry about the confusion. It's "Not a bug".

@demobox said:

Regarding the question why -Xprint:jvm shows [...] but the bytecode
contains an invocation of BoxesRunTime.unboxToInt(): It's "Not a bug"

Makes sense - thanks for explaining. It's clear now why the AST does not match the bytecode.

I guess the remaining question is whether the implementation of the two functions should have the same behaviour or whether that is also "not a bug"...

@magarciaEPFL said:
We can never rule out code out there with hardcoded calls to object Int's unbox(). As to documentation, any pull request against scala.Int would have to cope with

// DO NOT EDIT, CHANGES WILL BE LOST.

because that file is automatically generated.

@demobox said:

any pull request against scala.Int would have to cope with
// DO NOT EDIT, CHANGES WILL BE LOST.

I was curious about that too ;-) I just was not able to find where this code is actually generated from. Do you have any hints?

We can never rule out code out there with hardcoded calls to object Int's unbox()

How would you make this call? From the REPL I tried calling scala.Int.unbox(null) directly [1], and it seems the behaviour is actually that of BoxesRunTime.

[1] [^2.10.0-RC2-repl-session.png]

@retronym said:
src/compiler/scala/tools/cmd/gen/AnyVals.scala

@demobox said:

src/compiler/scala/tools/cmd/gen/AnyVals.scala

Thanks, Jason! Egads: https://github.com/scala/scala/blob/master/src/compiler/scala/tools/cmd/gen/AnyVals.scala#L232 makes this look a whole lot bigger. Hm...

@demobox said:

Status CLOSED, Resolution Not a Bug

Perhaps one final question, if you would be so kind: if this is "as intended", what then is the purpose of the @unboxImpl@ definition [1] in the AnyVal classes?

If the method is not supposed to document what actually happens when unboxing takes place, what is it for?

Thanks!

[1] https://github.com/scala/scala/blob/master/src/compiler/scala/tools/cmd/gen/AnyVals.scala#L232

@magarciaEPFL said:
Historic reasons, I guess.

@demobox said:

Historic reasons, I guess.

Fair enough ;-) So, if anything, an "improvement". Thanks for your time on this!

@lrytz said:
scala/scala#5072

@demobox said:

scala/scala#5072

Yay! Glad to see we now feel the inconsistency here is weird enough to take the risk of changing the behaviour.

Thanks, @lrytz and @adriaanm!