Merge pull request #12 from FritzMock/master

Hidden typecast for Float objects in JSONobject.increment(String key)

Author: stleary

JSONObjectTest.java

@@ -493,7 +493,7 @@ public void jsonInvalidNumberValues() {
                 "\"negativeHexFloat\":-0x1.fffp1,"+
                 "\"hexFloat\":0x1.0P-1074,"+
                 "\"floatIdentifier\":0.1f,"+
-                "\"doubleIdentifier\":0.1d,"+
+                "\"doubleIdentifier\":0.1d"+
             "}";
         JSONObject jsonObject = new JSONObject(str);
         Object obj;
@@ -694,6 +694,20 @@ public void jsonObjectIncrement() {
             "\"keyLong\":9999999993,"+
             "\"keyDouble\":3.1,"+
             // TODO: not sure if this will work on other platforms
+
+            // Should work the same way on any platform! @see https://docs.oracle.com/javase/specs/jls/se7/html/jls-4.html#jls-4.2.3
+            // This is the effect of a float to double conversion and is inherent to the shortcomings of the IEEE 754 format, when
+            // converting 32-bit into double-precision 64-bit.
+            // Java type-casts float to double. A 32 bit float is type-casted to 64 bit double by simply appending zero-bits to the
+            // mantissa (and extended the signed exponent by 3 bits.) and there is no way to obtain more information than it is
+            // stored in the 32-bits float.
+
+            // Like 1/3 cannot be represented as base10 number because it is periodically, 1/5 (for example) cannot be represented
+            // as base2 number since it is periodically in base2 (take a look at http://www.h-schmidt.net/FloatConverter/)
+            // The same happens to 3.1, that decimal number (base10 representation) is periodic in base2 representation, therefore
+            // appending zero-bits is inaccurate. Only repeating the periodically occuring bits (0110) would be a proper conversion.
+            // However one cannot detect from a 32 bit IEE754 representation which bits would "repeat infinitely", since the missing
+            // bits would not fit into the 32 bit float, i.e. the information needed simply is not there!
             "\"keyFloat\":3.0999999046325684,"+
         "}";
         JSONObject jsonObject = new JSONObject(str);
@@ -709,6 +723,54 @@ public void jsonObjectIncrement() {
         jsonObject.increment("keyFloat");
         JSONObject expectedJsonObject = new JSONObject(expectedStr);
         Util.compareActualVsExpectedJsonObjects(jsonObject, expectedJsonObject);
+
+		/*
+        float f = 3.1f;
+        double df = (double) f;
+        double d = 3.1d;
+        System.out.println(Integer.toBinaryString(Float.floatToRawIntBits(f)));
+        System.out.println(Long.toBinaryString(Double.doubleToRawLongBits(df)));
+        System.out.println(Long.toBinaryString(Double.doubleToRawLongBits(d)));
+
+		- Float:
+		seeeeeeeemmmmmmmmmmmmmmmmmmmmmmm
+		 1000000010001100110011001100110
+		- Double
+		seeeeeeeeeeemmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmm
+		 10000000   10001100110011001100110
+		 100000000001000110011001100110011000000000000000000000000000000
+		 100000000001000110011001100110011001100110011001100110011001101
+		*/
+        // Examples of well documented but probably unexpected behavior in java / with 32-bit float to 64-bit float conversion.
+        assertFalse("Document unexpected behaviour with explicit type-casting float as double!", (double)0.2f == 0.2d );
+        assertFalse("Document unexpected behaviour with implicit type-cast!", 0.2f == 0.2d );
+        Double d1 = new Double( 1.1f );
+        Double d2 = new Double( "1.1f" );
+        assertFalse( "Document implicit type cast from float to double before calling Double(double d) constructor", d1.equals( d2 ) );
+
+        assertTrue( "Correctly converting float to double via base10 (string) representation!", new Double( 3.1d ).equals(  new Double( new Float( 3.1f ).toString() ) ) );
+
+        // Pinpointing the not so obvious "buggy" conversion from float to double in JSONObject
+        JSONObject jo = new JSONObject();
+        jo.put( "bug", 3.1f ); // will call put( String key, double value ) with implicit and "buggy" type-cast from float to double
+        assertFalse( "The java-compiler did add some zero bits for you to the mantissa (unexpected, but well documented)", jo.get( "bug" ).equals(  new Double( 3.1d ) ) );
+
+        JSONObject inc = new JSONObject();
+        inc.put( "bug", new Float( 3.1f ) ); // This will put in instance of Float into JSONObject, i.e. call put( String key, Object value ) 
+        assertTrue( "Everything is ok here!", inc.get( "bug" ) instanceof Float );
+        inc.increment( "bug" ); // after adding 1, increment will call put( String key, double value ) with implicit and "buggy" type-cast from float to double!
+        // this.put(key, (Float) value + 1);
+        // 1.	The (Object)value will be typecasted to (Float)value since it is an instanceof Float actually nothing is done. 
+        // 2.	Float instance will be autoboxed into float because the + operator will work on primitives not Objects!
+        // 3.	A float+float operation will be performed and results into a float primitive.
+        // 4.	There is no method that matches the signature put( String key, float value), java-compiler will choose the method
+        //		put( String key, double value) and does an implicit type-cast(!) by appending zero-bits to the mantissa
+        assertTrue( "JSONObject increment unexpected behavior, Float will not stay Float!", jo.get( "bug" ) instanceof Float );
+        // correct implementation (with change of behavior) would be:
+        // this.put(key, new Float((Float) value + 1)); 
+        // Probably it would be better to deprecate the method and remove some day, while convenient processing the "payload" is not
+        // really in the the scope of a JSON-library (IMHO.)
+
     }
 
     @Test