[GROOVY-7585] ObjectRange strange semantics for mismatched arguments

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 2.5.0-alpha-1
• Assignee: Paul King
• Reporter: Thibault Kruse
• Created: 08/Sep/15 14:01
• Updated: 02/May/17 02:03
• Resolved: 28/Jun/16 07:08

Description

in Object Range, String is a special case considered:

if (from instanceof String || to instanceof String) {
           String start = from.toString();
           String end = to.toString();
           if (start.length() > end.length()) {
                throw new IllegalArgumentException("Incompatible Strings for Range: starting String is longer than ending string");
            }
int length = Math.min(start.length(), end.length());
            int i;
            for (i = 0; i < length; i++) {
                if (start.charAt(i) != end.charAt(i)) break;
            }
            if (i < length - 1) {
                throw new IllegalArgumentException("Incompatible Strings for Range: String#next() will not reach the expected value");
            }
            ...

There are two semantic problems with that implementation. On the one hand this is plain buggy when the length of 'to' is larger than the length of 'from', and two strings are passed:

groovy:000> x = 'aa'..'aaa'
===> [aa]

This is because stepping through the strings begins with "ab", which is larger than 'aaa' in the comparison of DefaultGroovyMethods.numberAwareCompareTo().
I assume from the rest of the code that the general assumption was that aa..aaa == [aa, ab, ac, ... <something> ..., aaa]

Next, the check would not be meaningful even if the comparison worked as intended:

'aa'..'aaa' is an infinite sequence. Assuming '_' represents Character.MAX_VALUE and ^ represents Character.MIN_VALUE:
 aa, ab, ac, ..., a_, a_^, ..., a_a, a_b, ... a__, a__^, ... 

so 'aaa' is never reached. So this could only possibly work it a longer 'to' argument consisted of from[:-1] + Character.MAX_VALUE <n-times> + [a-z]. I don't think that is a relevant case any Groovy user relies on.

So there are two "bugs" cancelling each other out somehow, preventing infinite String creation.

There are more problems when mixing Types (via the unsafe constructor):

groovy:000> x = new ObjectRange('11', 11, true)
===> []
groovy:000> x = new ObjectRange(11, '11', false)
===> [11, 12, 13, 14, 15 ..., 1567]

There is some code almost handling this nicely

        if (from.getClass() == to.getClass()) {
            this.from = from;
            this.to = to;
        } else {
            this.from = normaliseStringType(from);
            this.to = normaliseStringType(to);
        }

But this merely turns Characters and single-char Strings into ints. I believe it would be nice here if after normalization (inside the else case!), if classes do not match, then this should throw an exception. Because when some non-Number, non-String Comparable is passed along with a single-char String ( new MyComparable()..'1') then that string is normalized into it's int value, which IMO is not sane behavior. It's not documented anywhere that single-char Strings will be turned into ints for a range, this can be a convenience transformation when combined with numbers for scripting, but other than that it should just fail, instead of leading to unpredictable behavior anytime later in a system.

It seems this has been like it is since the method was written/copied in 2005. So I suggest

• changing the entry condition to AND: ( if (from instanceof String && to instanceof String) {)
• to throw an exception if the length of the strings is different
• to throw an exception if after normalization 'from' is a Number or String, but the class of 'to' is not similar to that of 'from'