8325898: ChoiceFormat returns erroneous result when formatting bad pattern

Reviewed-by: naoto

src/java.base/share/classes/java/text/ChoiceFormat.java

@@ -41,7 +41,9 @@ package java.text;
 import java.io.InvalidObjectException;
 import java.io.IOException;
 import java.io.ObjectInputStream;
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Objects;
 
 /**
  * {@code ChoiceFormat} is a concrete subclass of {@code NumberFormat} that
@@ -238,6 +240,7 @@ public class ChoiceFormat extends NumberFormat {
      * @see #ChoiceFormat(String)
      */
     public void applyPattern(String newPattern) {
+        Objects.requireNonNull(newPattern, "newPattern must not be null");
         applyPatternImpl(newPattern);
     }
 
@@ -249,86 +252,92 @@ public class ChoiceFormat extends NumberFormat {
      * further understanding of certain special characters: "#", "<", "\u2264", "|".
      */
     private void applyPatternImpl(String newPattern) {
-        StringBuilder[] segments = new StringBuilder[2];
+        // Set up components
-        for (int i = 0; i < segments.length; ++i) {
+        ArrayList<Double> limits = new ArrayList<>();
-            segments[i] = new StringBuilder();
+        ArrayList<String> formats = new ArrayList<>();
-        }
+        StringBuilder[] segments = new StringBuilder[]{new StringBuilder(),
-        double[] newChoiceLimits = new double[30];
+                new StringBuilder()};
-        String[] newChoiceFormats = new String[30];
+        int part = 0; // 0 denotes LIMIT. 1 denotes FORMAT.
-        int count = 0;
+        double limit = 0;
-        int part = 0; // 0 denotes limit, 1 denotes format
-        double startValue = 0;
-        double oldStartValue = Double.NaN;
         boolean inQuote = false;
+
+        // Parse the string, alternating the value of part
         for (int i = 0; i < newPattern.length(); ++i) {
             char ch = newPattern.charAt(i);
-            if (ch=='\'') {
+            switch (ch) {
-                // Check for "''" indicating a literal quote
+                case '\'':
-                if ((i+1)<newPattern.length() && newPattern.charAt(i+1)==ch) {
+                    // Check for "''" indicating a literal quote
+                    if ((i + 1) < newPattern.length() && newPattern.charAt(i + 1) == ch) {
+                        segments[part].append(ch);
+                        ++i;
+                    } else {
+                        inQuote = !inQuote;
+                    }
+                    break;
+                case '<', '#', '\u2264':
+                    if (inQuote || part == 1) {
+                        // Don't interpret relational symbols if parsing the format
+                        segments[part].append(ch);
+                    } else {
+                        // Build the numerical value of the limit
+                        // and switch to parsing format
+                        if (segments[0].isEmpty()) {
+                            throw new IllegalArgumentException("Each interval must" +
+                                    " contain a number before a format");
+                        }
+                        limit = stringToNum(segments[0].toString());
+                        if (ch == '<' && Double.isFinite(limit)) {
+                            limit = nextDouble(limit);
+                        }
+                        if (!limits.isEmpty() && limit <= limits.getLast()) {
+                            throw new IllegalArgumentException("Incorrect order " +
+                                    "of intervals, must be in ascending order");
+                        }
+                        segments[0].setLength(0);
+                        part = 1;
+                    }
+                    break;
+                case '|':
+                    if (inQuote) {
+                        segments[part].append(ch);
+                    } else {
+                        if (part != 1) {
+                            // Discard incorrect portion and finish building cFmt
+                            break;
+                        }
+                        // Insert an entry into the format and limit arrays
+                        // and switch to parsing limit
+                        limits.add(limit);
+                        formats.add(segments[1].toString());
+                        segments[1].setLength(0);
+                        part = 0;
+                    }
+                    break;
+                default:
                     segments[part].append(ch);
-                    ++i;
-                } else {
-                    inQuote = !inQuote;
-                }
-            } else if (inQuote) {
-                segments[part].append(ch);
-            } else if (part == 0 && (ch == '<' || ch == '#' || ch == '\u2264')) {
-                // Only consider relational symbols if parsing the limit segment (part == 0).
-                // Don't treat a relational symbol as syntactically significant
-                // when parsing Format segment (part == 1)
-                if (segments[0].length() == 0) {
-                    throw new IllegalArgumentException("Each interval must"
-                            + " contain a number before a format");
-                }
-
-                String tempBuffer = segments[0].toString();
-                if (tempBuffer.equals("\u221E")) {
-                    startValue = Double.POSITIVE_INFINITY;
-                } else if (tempBuffer.equals("-\u221E")) {
-                    startValue = Double.NEGATIVE_INFINITY;
-                } else {
-                    startValue = Double.parseDouble(tempBuffer);
-                }
-
-                if (ch == '<' && startValue != Double.POSITIVE_INFINITY &&
-                        startValue != Double.NEGATIVE_INFINITY) {
-                    startValue = nextDouble(startValue);
-                }
-                if (startValue <= oldStartValue) {
-                    throw new IllegalArgumentException("Incorrect order of"
-                            + " intervals, must be in ascending order");
-                }
-                segments[0].setLength(0);
-                part = 1;
-            } else if (ch == '|') {
-                if (count == newChoiceLimits.length) {
-                    newChoiceLimits = doubleArraySize(newChoiceLimits);
-                    newChoiceFormats = doubleArraySize(newChoiceFormats);
-                }
-                newChoiceLimits[count] = startValue;
-                newChoiceFormats[count] = segments[1].toString();
-                ++count;
-                oldStartValue = startValue;
-                segments[1].setLength(0);
-                part = 0;
-            } else {
-                segments[part].append(ch);
             }
         }
-        // clean up last one
+
+        // clean up last one (SubPattern without trailing '|')
         if (part == 1) {
-            if (count == newChoiceLimits.length) {
+            limits.add(limit);
-                newChoiceLimits = doubleArraySize(newChoiceLimits);
+            formats.add(segments[1].toString());
-                newChoiceFormats = doubleArraySize(newChoiceFormats);
-            }
-            newChoiceLimits[count] = startValue;
-            newChoiceFormats[count] = segments[1].toString();
-            ++count;
         }
-        choiceLimits = new double[count];
+        choiceLimits = limits.stream().mapToDouble(d -> d).toArray();
-        System.arraycopy(newChoiceLimits, 0, choiceLimits, 0, count);
+        choiceFormats = formats.toArray(new String[0]);
-        choiceFormats = new String[count];
+    }
-        System.arraycopy(newChoiceFormats, 0, choiceFormats, 0, count);
+
+    /**
+     * Converts a string value to its double representation; this is used
+     * to create the limit segment while applying a pattern.
+     * Handles "\u221E", as specified by the pattern syntax.
+     */
+    private static double stringToNum(String str) {
+        return switch (str) {
+            case "\u221E" -> Double.POSITIVE_INFINITY;
+            case "-\u221E" -> Double.NEGATIVE_INFINITY;
+            default -> Double.parseDouble(str);
+        };
     }
 
     /**
@@ -402,6 +411,7 @@ public class ChoiceFormat extends NumberFormat {
      * @see #applyPattern
      */
     public ChoiceFormat(String newPattern)  {
+        Objects.requireNonNull(newPattern, "newPattern must not be null");
         applyPatternImpl(newPattern);
     }
 
@@ -574,6 +584,24 @@ public class ChoiceFormat extends NumberFormat {
         return Math.nextUp(d);
     }
 
+    /**
+     * Finds the least double greater than {@code d} (if {@code positive} is
+     * {@code true}), or the greatest double less than {@code d} (if
+     * {@code positive} is {@code false}).
+     * If {@code NaN}, returns same value.
+     *
+     * @implNote This is equivalent to calling
+     * {@code positive ? Math.nextUp(d) : Math.nextDown(d)}
+     *
+     * @param d        the reference value
+     * @param positive {@code true} if the least double is desired;
+     *                 {@code false} otherwise
+     * @return the least or greater double value
+     */
+    public static double nextDouble (double d, boolean positive) {
+        return positive ? Math.nextUp(d) : Math.nextDown(d);
+    }
+
     /**
      * Finds the greatest double less than {@code d}.
      * If {@code NaN}, returns same value.
@@ -593,8 +621,7 @@ public class ChoiceFormat extends NumberFormat {
      * Overrides Cloneable
      */
     @Override
-    public Object clone()
+    public Object clone() {
-    {
         ChoiceFormat other = (ChoiceFormat) super.clone();
         // for primitives or immutables, shallow clone is enough
         other.choiceLimits = choiceLimits.clone();
@@ -685,37 +712,4 @@ public class ChoiceFormat extends NumberFormat {
      * @serial
      */
     private String[] choiceFormats;
-
-    /**
-     * Finds the least double greater than {@code d} (if {@code positive} is
-     * {@code true}), or the greatest double less than {@code d} (if
-     * {@code positive} is {@code false}).
-     * If {@code NaN}, returns same value.
-     *
-     * @implNote This is equivalent to calling
-     * {@code positive ? Math.nextUp(d) : Math.nextDown(d)}
-     *
-     * @param d        the reference value
-     * @param positive {@code true} if the least double is desired;
-     *                 {@code false} otherwise
-     * @return the least or greater double value
-     */
-    public static double nextDouble (double d, boolean positive) {
-        return positive ? Math.nextUp(d) : Math.nextDown(d);
-    }
-
-    private static double[] doubleArraySize(double[] array) {
-        int oldSize = array.length;
-        double[] newArray = new double[oldSize * 2];
-        System.arraycopy(array, 0, newArray, 0, oldSize);
-        return newArray;
-    }
-
-    private String[] doubleArraySize(String[] array) {
-        int oldSize = array.length;
-        String[] newArray = new String[oldSize * 2];
-        System.arraycopy(array, 0, newArray, 0, oldSize);
-        return newArray;
-    }
-
 }

test/jdk/java/text/Format/ChoiceFormat/PatternsTest.java

@@ -23,7 +23,7 @@
 
 /*
  * @test
- * @bug 6285888 6801704
+ * @bug 6285888 6801704 8325898
  * @summary Test the expected behavior for a wide range of patterns (both
  *          correct and incorrect). This test documents the behavior of incorrect
  *          ChoiceFormat patterns either throwing an exception, or discarding
@@ -31,14 +31,13 @@
  * @run junit PatternsTest
  */
 
-import java.text.ChoiceFormat;
-
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
 
-import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import java.text.ChoiceFormat;
+
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.params.provider.Arguments.arguments;
@@ -97,6 +96,7 @@ public class PatternsTest {
     // an exception.
     private static Arguments[] invalidPatternsThrowsTest() {
         return new Arguments[] {
+                arguments("#", ERR1), // Only relation
                 arguments("#foo", ERR1), // No Limit
                 arguments("0#foo|#|1#bar", ERR1), // Missing Relation in SubPattern
                 arguments("#|", ERR1), // Missing Limit
@@ -127,11 +127,20 @@ public class PatternsTest {
     // after discarding occurs.
     private static Arguments[] invalidPatternsDiscardedTest() {
         return new Arguments[] {
+                // Incomplete SubPattern (limit only) at end of Pattern
+                arguments("1#bar|2", "1#bar"),
                 // Incomplete SubPattern at the end of the Pattern
                 arguments("0#foo|1#bar|baz", "0#foo|1#bar"),
+                // Incomplete SubPattern with trailing | at the end of the Pattern
+                // Prior to 6801704, it created the broken "0#foo|1#bar|1#"
+                // which caused formatting 1 to return an empty string
+                arguments("0#foo|1#bar|baz|", "0#foo|1#bar"),
+                // Same as previous, with additional incomplete subPatterns
+                arguments("0#foo|1#bar|baz|quux", "0#foo|1#bar"),
 
                 // --- These throw an ArrayIndexOutOfBoundsException
-                // when attempting to format with them ---
+                // when attempting to format with them as the incomplete patterns
+                // are discarded, initializing the cFmt with empty limits and formats ---
                 // SubPattern with only a Limit (which is interpreted as a Format)
                 arguments("0", ""),
                 // SubPattern with only a Format