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

Potential bug converting an Option of Java BigDecimal to an Option of Scala BigDecimal #6567

Closed
scabug opened this issue Oct 25, 2012 · 6 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Oct 25, 2012

I have been experience problems converting between an Option of a Java BigDecimal to an Option of a Scala BigDecimal.

For example, the following assertion fails:

import java.math.{BigDecimal => JBigDecimal}

object BigDecimalBug extends App {
  val nullJavaBigDecimal: JBigDecimal = null
  val x: Option[BigDecimal] = Option(nullJavaBigDecimal)
  assert(x == None)
}

The variable x is not actually None but a Some of the Scala BigDecimal object. The Scala BigDecimal object is incorrectly formed with the internal data structure set to null.

I have resolved this using the following implicit conversion, but wanted to highlight this in case this is perceived as an issue with the BigDecimal conversion logic.

object BigDecimalBug extends App {
  implicit def convertJBigDecimalOption(javaBigDecimal: JBigDecimal): Option[BigDecimal] = 
    Option(javaBigDecimal) map { x => BigDecimal(x.toString) }

  val nullJavaBigDecimal: JBigDecimal = null
  val y: Option[BigDecimal] = nullJavaBigDecimal
  assert(y == None)
}
@scabug
Copy link
Author

@scabug scabug commented Oct 25, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6567?orig=1
Reporter: Alex Luketa (aluketa)
Affected Versions: 2.9.1

@scabug
Copy link
Author

@scabug scabug commented Nov 3, 2012

@retronym said:
It seems that applying an implicit view in the argument to Option.apply would almost always be a bug.

I propose to add a warning under the compiler option -Xlint.

scala/scala#1565

@scabug
Copy link
Author

@scabug scabug commented Nov 3, 2012

Alex Luketa (aluketa) said:
Thanks Jason

@scabug
Copy link
Author

@scabug scabug commented Nov 4, 2012

@retronym said:
Merged for 2.11 scala/scala@2c6777f

@scabug
Copy link
Author

@scabug scabug commented Jan 22, 2013

@adriaanm said:
reopening for 2.10.1-RC1 backport

@scabug
Copy link
Author

@scabug scabug commented Jan 23, 2013

@retronym said:
backport: scala/scala#1954

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants