[ https://issues.apache.org/jira/browse/GROOVY-7585?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Paul King reassigned GROOVY-7585: --------------------------------- Assignee: Paul King > ObjectRange strange semantics for mismatched arguments > ------------------------------------------------------ > > Key: GROOVY-7585 > URL: https://issues.apache.org/jira/browse/GROOVY-7585 > Project: Groovy > Issue Type: Bug > Reporter: Thibault Kruse > Assignee: Paul King > > in Object Range, String is a special case considered: > {code} > 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"); > } > ... > {code} > 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: > {code} > groovy:000> x = 'aa'..'aaa' > ===> [aa] > {code} > 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: > {code} > '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__^, ... > {code} > 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): > {code} > groovy:000> x = new ObjectRange('11', 11, true) > ===> [] > groovy:000> x = new ObjectRange(11, '11', false) > ===> [11, 12, 13, 14, 15 ..., 1567] > {code} > There is some code almost handling this nicely > {code} > if (from.getClass() == to.getClass()) { > this.from = from; > this.to = to; > } else { > this.from = normaliseStringType(from); > this.to = normaliseStringType(to); > } > {code} > 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' -- This message was sent by Atlassian JIRA (v6.3.4#6332)