Skip to content

Turns the standard AST JNumber into a safe constructor #30

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

Merged
merged 9 commits into from
Jul 5, 2017
5 changes: 3 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ Implementation is in `scalajson.ast.JValue`
of a number (http://stackoverflow.com/a/13502497/1519631)
- Equals will properly detect if two numbers are equal, i.e. `scalajson.ast.JNumber("34.00") == scalajson.ast.JNumber("34")`
- Hashcode has been designed to provide consistent hash for numbers of unlimited precision.
- If you construct a JNumber with `Float.NaN`/`Float.PositiveInfinity`/`Float.NegativeInfinity`/`Double.NaN`/`Double.PositiveInfinity`/`Double.NegativeInfinity` it will return a `JNull`
- Also provides a `.fromString` function which you can use to create unlimited precision numbers. Returns an `Option[JNumber]` (will return `None` if `String` isn't a valid number)
- `scalajson.ast.JObject` is an actual `Map[String,JValue]`. This means that it doesn't handle duplicate keys for a `scalajson.ast.JObject`,
nor does it handle key ordering.
- `scalajson.ast.JArray` is an `Vector`.
Expand Down Expand Up @@ -109,8 +111,7 @@ import scalajson.jNumberRegex
"3535353".matches(jNumberRegex) // true
```

Code of Conduct
===============
## Code of Conduct
ScalaJSON uses the [Scala Code of Conduct](https://www.scala-lang.org/conduct.html)
for all communication and discussion. This includes both GitHub, Gitter chat and
other more direct lines of communication such as email.
Expand Down
60 changes: 41 additions & 19 deletions js/src/main/scala/scalajson/ast/JValue.scala
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ final case class JString(value: String) extends JValue {
}

object JNumber {
def apply(value: Int): JNumber = JNumber(value.toString)
def apply(value: Int): JNumber = new JNumber(value.toString)

def apply(value: Integer): JNumber = JNumber(value.toString)
def apply(value: Integer): JNumber = new JNumber(value.toString)

def apply(value: Long): JNumber = JNumber(value.toString)
def apply(value: Long): JNumber = new JNumber(value.toString)

def apply(value: BigInt): JNumber = JNumber(value.toString())
def apply(value: BigInt): JNumber = new JNumber(value.toString())

def apply(value: BigDecimal): JNumber = JNumber(value.toString())
def apply(value: BigDecimal): JNumber = new JNumber(value.toString())

/**
* @param value
Expand All @@ -67,7 +67,7 @@ object JNumber {
def apply(value: Double): JValue = value match {
case n if n.isNaN => JNull
case n if n.isInfinity => JNull
case _ => JNumber(value.toString)
case _ => new JNumber(value.toString)
}

/**
Expand All @@ -77,22 +77,26 @@ object JNumber {
def apply(value: Float): JValue = value match {
case n if java.lang.Float.isNaN(n) => JNull
case n if n.isInfinity => JNull
case _ => JNumber(value.toString)
case _ => new JNumber(value.toString)
}

def fromString(value: String): Option[JNumber] =
Copy link

Choose a reason for hiding this comment

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

With Scala 2.10 out of the way we should be able to simply name this method apply as well, like all the others...

Copy link
Owner Author

Choose a reason for hiding this comment

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

I am thinking of leaving in the fromString method for all versions and aliasing it to apply for Scala 2.10.x and Scala 2.11.x, that way for a codebase that needs to targets all Scala versions you can use fromString

Copy link
Owner Author

Choose a reason for hiding this comment

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

I am thinking of leaving in the fromString method for all versions and aliasing it to apply for Scala 2.10.x and Scala 2.11.x, that way for a codebase that needs to targets all Scala versions you can use fromString

if (value.matches(jNumberRegex))
Some(new JNumber(value))
else
None

def unapply(arg: JNumber): Option[String] = Some(arg.underlying)
Copy link

Choose a reason for hiding this comment

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

This could be improved via something like this, i.e. it could be done in a way that avoids the allocation here.
Not saying that this should be done, just pointing out the opportunity.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Will have a look at this tomorrow

Copy link

Choose a reason for hiding this comment

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

From an end-user standpoint I'm not a fan of having a JNumber.unapply that provides a string. Maybe we should just remove the unapply altogether? What is a user supposed to do when getting this string? Try to parse it as a double? as a BigDecimal? It would be nice if the type system could help out more here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

The unapply for JNumber is there for these reasons

  1. Consistency in the AST (everything is a case class, containing a single value with an unapply)
  2. The string can be directly converted to the number you want (i.e. toDouble, toInt) since its always going to be a valid number. This may be improved with Remove the .to[X] converters #17 (comment)

}

/** Represents a JSON number value. If you are passing in a
* NaN or Infinity as a [[scala.Double]], [[JNumber]] will
* NaN or Infinity as a [[scala.Double]] or [[scala.Float]], [[JNumber]] will
* return a [[JNull]].
*
* @author Matthew de Detrich
* @throws scala.NumberFormatException - If the value is not a valid JSON Number
*/
final case class JNumber(value: String) extends JValue {

if (!value.matches(jNumberRegex)) {
throw new NumberFormatException(value)
}
final class JNumber(private[ast] val underlying: String) extends JValue {
Copy link

Choose a reason for hiding this comment

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

Shouldn't this constructor be private?

That said, it would be preferable to provide different implementations of JNumber for each of the cases.

Copy link

Choose a reason for hiding this comment

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

+1

private should be the default.
Making it public should require respective argumentative backup.

Copy link
Owner Author

@mdedetrich mdedetrich Jul 3, 2017

Choose a reason for hiding this comment

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

It is private, just for the package ast. This is done to save performance as it allows both the standard ast and unsafe to access the private constructor to save on performance in some cases (bypassing the regex check when not needed). Its still private to anything that is outside of the scalaJSON package.

If we end up removing the unsafe AST or it gets changed, I will definitely change it back to private.

Copy link

Choose a reason for hiding this comment

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

currently the constructor is public

Copy link

Choose a reason for hiding this comment

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

I think (barring the manual case class discussion) this would be the preferred solution:

final class JNumber private[ast](val value: String) extends JValue {
  ...

Copy link

Choose a reason for hiding this comment

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

Agreed. JNumber private[ast] (...) is the way to go.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah I misunderstood you guys, fixing it now

@inline val value: String = underlying
Copy link

Choose a reason for hiding this comment

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

What's the purpose of having underlying as well as this value?

Copy link
Owner Author

@mdedetrich mdedetrich Jul 2, 2017

Choose a reason for hiding this comment

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

To access the raw string value (you can't access value because it's private and for large numbers you may want to access the String value)

Copy link

@ktoso ktoso Jul 2, 2017

Choose a reason for hiding this comment

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

if only for that reason then a def would make more sense perhaps (not to grow the memory footprint of the object)

Copy link
Owner Author

@mdedetrich mdedetrich Jul 3, 2017

Choose a reason for hiding this comment

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

Yeah that's why I used@inline, or does this not work?

Copy link

Choose a reason for hiding this comment

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

If it's a val the value has to be stored somewhere. @inline means the content of the method (in this case accessing the field) should be inlined.

It's also generally not a good idea to use @inline on a public API method, since it makes it harder to change the implementation while maintaining backwards compatibility.

Copy link

Choose a reason for hiding this comment

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

Yep, as Greg said. @inline is bad for bin compat, does not change field semantics, may only do so for the method, and here we just want the method.

Copy link

Choose a reason for hiding this comment

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

To access the raw string value (you can't access value because it's private and for large numbers you may want to access the String value).

I don't see the benefit of adding a second value member.
Why not simply call the constructor parameter value and make it a public field?


/**
* Javascript specification for numbers specify a [[scala.Double]], so this is the default export method to `Javascript`
Expand All @@ -101,22 +105,40 @@ final case class JNumber(value: String) extends JValue {
*/
def this(value: Double) = this(value.toString)

override def toUnsafe: unsafe.JValue = unsafe.JNumber(value)
override def toUnsafe: unsafe.JValue = unsafe.JNumber(underlying)

override def toJsAny: js.Any = value.toDouble match {
override def toJsAny: js.Any = underlying.toDouble match {
case n if n.isNaN => null
case n if n.isInfinity => null
case n => n
}

override def equals(obj: Any) =
override def equals(obj: Any): Boolean =
obj match {
case jNumber: JNumber => numericStringEquals(value, jNumber.value)
case jNumber: JNumber =>
numericStringEquals(underlying, jNumber.underlying)
case _ => false
}

override def hashCode: Int =
numericStringHashcode(value)
numericStringHashcode(underlying)

override def productElement(n: Int): Any =
if (n == 0)
underlying
else
throw new IndexOutOfBoundsException(n.toString)

override def productArity: Int = 1

override def canEqual(obj: Any): Boolean = {
obj match {
case _: JNumber => true
case _ => false
}
}

override def toString: String = s"JNumber($underlying)"
}

/** Represents a JSON Boolean value, which can either be a
Expand Down
9 changes: 7 additions & 2 deletions js/src/main/scala/scalajson/ast/unsafe/JValue.scala
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
package scalajson.ast.unsafe
package scalajson.ast
package unsafe

import scalajson.ast
import scalajson.ast._
Expand Down Expand Up @@ -84,7 +85,11 @@ object JNumber {
*/
// JNumber is internally represented as a string, to improve performance
final case class JNumber(value: String) extends JValue {
override def toStandard: ast.JValue = ast.JNumber(value)
override def toStandard: ast.JValue =
if (value.matches(jNumberRegex))
new ast.JNumber(value)
else
throw new NumberFormatException(value)

def this(value: Double) = {
this(value.toString)
Expand Down
30 changes: 23 additions & 7 deletions js/src/test/scala/specs/JNumber.scala
Original file line number Diff line number Diff line change
Expand Up @@ -123,28 +123,44 @@ object JNumber extends TestSuite with UTestScalaCheck {
}.checkUTest()

def hashCodeEqualsDecimal = {
scalajson.ast.JNumber("34").## == scalajson.ast.JNumber("34.0").##
scalajson.ast.JNumber.fromString("34").get.## == scalajson.ast.JNumber
.fromString("34.0")
.get
.##
}

def hashCodeEqualsDecimal2 = {
scalajson.ast.JNumber("34").## == scalajson.ast.JNumber("34.00").##
scalajson.ast.JNumber.fromString("34").get.## == scalajson.ast.JNumber
.fromString("34.00")
.get
.##
}

def hashCodeNotEqualsDecimal = {
scalajson.ast.JNumber("34").## == scalajson.ast.JNumber("34.01").##
scalajson.ast.JNumber.fromString("34").get.## == scalajson.ast.JNumber
.fromString("34.01")
.get
.##
}

def hashCodeNotEqualsDecimal2 = {
scalajson.ast.JNumber("34").## == scalajson.ast.JNumber("34.001").##
scalajson.ast.JNumber.fromString("34").get.## == scalajson.ast.JNumber
.fromString("34.001")
.get
.##
}

def hashCodeEqualsE = {
scalajson.ast.JNumber("34e34").## != scalajson.ast.JNumber("34e034").##
scalajson.ast.JNumber.fromString("34e34").get.## != scalajson.ast.JNumber
.fromString("34e034")
.get
.##
}

def hashCodeEqualsE2 = {
scalajson.ast.JNumber("34e34").## != scalajson.ast
.JNumber("34e0034")
scalajson.ast.JNumber.fromString("34e34").get.## != scalajson.ast.JNumber
.fromString("34e0034")
.get
.##
}

Expand Down
2 changes: 1 addition & 1 deletion js/src/test/scala/specs/JValue.scala
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ object JValue extends TestSuite with UTestScalaCheck {
val cloned = jValue match {
case scalajson.ast.JNull => scalajson.ast.JNull
case jNumber: scalajson.ast.JNumber =>
scalajson.ast.JNumber(jNumber.value)
scalajson.ast.JNumber.fromString(jNumber.value).get
case jString: scalajson.ast.JString =>
scalajson.ast.JString(jString.value)
case jArray: scalajson.ast.JArray =>
Expand Down
64 changes: 46 additions & 18 deletions jvm/src/main/scala/scalajson/ast/JValue.scala
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ final case class JString(value: String) extends JValue {
* return a JNull
*/
object JNumber {
def apply(value: Int): JNumber = JNumber(value.toString)
def apply(value: Int): JNumber = new JNumber(value.toString)

def apply(value: Long): JNumber = JNumber(value.toString)
def apply(value: Long): JNumber = new JNumber(value.toString)

def apply(value: BigInt): JNumber = JNumber(value.toString)
def apply(value: BigInt): JNumber = new JNumber(value.toString)

/**
* @param value
Expand All @@ -53,10 +53,10 @@ object JNumber {
def apply(value: Float): JValue = value match {
case n if java.lang.Float.isNaN(n) => JNull
case n if n.isInfinity => JNull
case _ => JNumber(value.toString)
case _ => new JNumber(value.toString)
}

def apply(value: BigDecimal): JNumber = JNumber(value.toString())
def apply(value: BigDecimal): JNumber = new JNumber(value.toString())

/**
* @param value
Expand All @@ -65,37 +65,65 @@ object JNumber {
def apply(value: Double): JValue = value match {
case n if n.isNaN => JNull
case n if n.isInfinity => JNull
case _ => JNumber(value.toString)
case _ => new JNumber(value.toString)
}

def apply(value: Integer): JNumber = JNumber(value.toString)
def apply(value: Integer): JNumber = new JNumber(value.toString)

def apply(value: Array[Char]): JNumber = JNumber(new String(value))
def apply(value: Array[Char]): Option[JNumber] = {
val s = new String(value)
if (s.matches(jNumberRegex))
Some(new JNumber(s))
else
None
}

def fromString(value: String): Option[JNumber] =
if (value.matches(jNumberRegex))
Some(new JNumber(value))
else
None

def unapply(arg: JNumber): Option[String] = Some(arg.underlying)
}

/** Represents a JSON number value. If you are passing in a
* NaN or Infinity as a [[scala.Double]], [[JNumber]] will
* NaN or Infinity as a [[scala.Double]] or [[scala.Float]], [[JNumber]] will
* return a [[JNull]].
*
* @author Matthew de Detrich
* @throws scala.NumberFormatException - If the value is not a valid JSON Number
*/
final case class JNumber(value: String) extends JValue {
final class JNumber(private[ast] val underlying: String) extends JValue {
@inline val value: String = underlying

if (!value.matches(jNumberRegex)) {
throw new NumberFormatException(value)
}

override def toUnsafe: unsafe.JValue = unsafe.JNumber(value)
override def toUnsafe: unsafe.JValue = unsafe.JNumber(underlying)

override def equals(obj: Any): Boolean =
obj match {
case jNumber: JNumber => numericStringEquals(value, jNumber.value)
case jNumber: JNumber =>
numericStringEquals(underlying, jNumber.underlying)
case _ => false
}

override def hashCode: Int =
numericStringHashcode(value)
numericStringHashcode(underlying)

override def productElement(n: Int): Any =
if (n == 0)
underlying
else
throw new IndexOutOfBoundsException(n.toString)

override def productArity: Int = 1

override def canEqual(obj: Any): Boolean = {
obj match {
case _: JNumber => true
case _ => false
}
}

override def toString: String = s"JNumber($underlying)"
}

/** Represents a JSON Boolean value, which can either be a
Expand Down
10 changes: 8 additions & 2 deletions jvm/src/main/scala/scalajson/ast/unsafe/JValue.scala
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
package scalajson.ast.unsafe
package scalajson.ast
package unsafe

import scalajson.ast
import scalajson.ast._
Expand Down Expand Up @@ -70,7 +71,12 @@ object JNumber {
*/
// JNumber is internally represented as a string, to improve performance
final case class JNumber(value: String) extends JValue {
override def toStandard: ast.JValue = ast.JNumber(value)
override def toStandard: ast.JValue = {
if (value.matches(jNumberRegex))
new ast.JNumber(value)
else
throw new NumberFormatException(value)
}
}

/** Represents a JSON Boolean value, which can either be a
Expand Down
Loading