[3/4] groovy git commit: GROOVY-7585: Accept normalization if both arguments end up numbers, throw error when starting range with Number or String and ending with something else

[pa...@apache.org]

GROOVY-7585: Accept normalization if both arguments end up numbers, throw error when starting range with Number or String and ending with something else


Project: http://git-wip-us.apache.org/repos/asf/groovy/repo
Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/da40a682
Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/da40a682
Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/da40a682

Branch: refs/heads/master
Commit: da40a68232bba804ae51670597a5d38e4ea7cfd9
Parents: 320500c
Author: Thibault Kruse <th...@gmx.de>
Authored: Thu Oct 15 11:19:53 2015 +0200
Committer: paulk <pa...@asert.com.au>
Committed: Tue Jun 28 17:05:28 2016 +1000

----------------------------------------------------------------------
 src/main/groovy/lang/ObjectRange.java     | 34 ++++++++++++++++++--------
 src/test/groovy/lang/ObjectRangeTest.java |  4 +--
 2 files changed, 26 insertions(+), 12 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/groovy/blob/da40a682/src/main/groovy/lang/ObjectRange.java
----------------------------------------------------------------------
diff --git a/src/main/groovy/lang/ObjectRange.java b/src/main/groovy/lang/ObjectRange.java
index f196c93..f8c2a3a 100644
--- a/src/main/groovy/lang/ObjectRange.java
+++ b/src/main/groovy/lang/ObjectRange.java
@@ -124,29 +124,43 @@ public class ObjectRange extends AbstractList implements Range {
             larger = ((Integer) larger).longValue();
         }
 
-        // TODO: should we care about different types here?
-        if (smaller.getClass() == larger.getClass()) {
+        /*
+        areReversed() already does an implicit type compatibility check
+        based on DefaultTypeTransformation.compareToWithEqualityCheck() for mixed classes
+        but it is only invoked if reverse == null.
+        So Object Range has to perform those type checks for consistency even when not calling
+        compareToWithEqualityCheck(), and ObjectRange has
+        to use the normalized value used in a successful comparison in
+        compareToWithEqualityCheck(). Currently that means Chars and single-char Strings
+        are evaluated as the char's charValue (an integer) when compared to numbers.
+        So '7'..'9' should produce ['7', '8', '9'], whereas ['7'..9] and [7..'9'] should produce [55, 56, 57].
+        if classes match, or both numericals, no checks possible / necessary
+        */
+        if (smaller.getClass() == larger.getClass() ||
+                (smaller instanceof Number && larger instanceof Number)) {
             this.from = smaller;
             this.to = larger;
         } else {
             // Convenience hack: try convert single-char strings to ints
             Comparable tempfrom = normaliseStringType(smaller);
             Comparable tempto = normaliseStringType(larger);
-            if (tempfrom.getClass() != tempto.getClass()) {
+            // if after normalizing both are numbers, assume intended range was numbers
+            if (tempfrom instanceof Number && tempto instanceof Number) {
+                this.from = tempfrom;
+                this.to = tempto;
+            } else {
                 // if convenience hack did not make classes match,
-                // and thus "from" cannot be advanced over "to", throw exception.
-                // Note if from as an unusual Object, it could have a next() method
+                // throw exception when starting with known class, and thus "from" cannot be advanced over "to".
+                // Note if start is an unusual Object, it could have a next() method
                 // that yields a Number or String to close the range
-                if ((tempfrom instanceof Number && !(tempto instanceof Number))
-                        || (tempfrom instanceof String && !(tempto instanceof String))) {
+                Comparable start = this.reverse ? larger : smaller;
+                if (start instanceof String || start instanceof Number) {
+                    // starting with number will never reach a non-number, same for string
                     throw new IllegalArgumentException("Incompatible Argument classes for ObjectRange " + smaller.getClass() + ", " + larger.getClass());
                 }
                 // Since normalizing did not help, use original values at users risk
                 this.from = smaller;
                 this.to = larger;
-            } else {
-                this.from = tempfrom;
-                this.to = tempto;
             }
         }
         checkBoundaryCompatibility();

http://git-wip-us.apache.org/repos/asf/groovy/blob/da40a682/src/test/groovy/lang/ObjectRangeTest.java
----------------------------------------------------------------------
diff --git a/src/test/groovy/lang/ObjectRangeTest.java b/src/test/groovy/lang/ObjectRangeTest.java
index a98dd97..9463741 100644
--- a/src/test/groovy/lang/ObjectRangeTest.java
+++ b/src/test/groovy/lang/ObjectRangeTest.java
@@ -196,11 +196,11 @@ public class ObjectRangeTest extends TestCase {
 
         Range mixed = createRange('7',  59.5);
         assertEquals(5, mixed.size());
-        assertEquals(Arrays.asList('7', '8', '9', ':', ';'), mixed.step(1));
+        assertEquals(Arrays.asList(55, 56, 57, 58, 59), mixed.step(1));
 
         mixed = createRange('7', BigInteger.valueOf(59));
         assertEquals(5, mixed.size());
-        assertEquals(Arrays.asList('7', '8', '9', ':', ';'), mixed.step(1));
+        assertEquals(Arrays.asList(55, 56, 57, 58, 59), mixed.step(1));
     }