Fix using optional between numbers

core/common/src/format/DateTimeFormatBuilder.kt

@@ -129,6 +129,9 @@ public sealed interface DateTimeFormatBuilder {
          *
          * [am] is used for the AM marker (0-11 hours), [pm] is used for the PM marker (12-23 hours).
          *
+         * Empty strings can not be used as markers.
+         * [IllegalArgumentException] is thrown if either [am] or [pm] is empty or if they are equal.
+         *
          * @see [amPmHour]
          */
         public fun amPmMarker(am: String, pm: String)

core/common/src/format/LocalDateFormat.kt

@@ -12,6 +12,8 @@ import kotlinx.datetime.internal.format.parser.Copyable
 
 /**
  * A description of how month names are formatted.
+ *
+ * An [IllegalArgumentException] will be thrown if some month name is empty or there are duplicate names.
  */
 public class MonthNames(
     /**
@@ -21,6 +23,14 @@ public class MonthNames(
 ) {
     init {
         require(names.size == 12) { "Month names must contain exactly 12 elements" }
+        names.indices.forEach { ix ->
+            require(names[ix].isNotEmpty()) { "A month name can not be empty" }
+            for (ix2 in 0 until ix) {
+                require(names[ix] != names[ix2]) {
+                    "Month names must be unique, but '${names[ix]}' was repeated"
+                }
+            }
+        }
     }
 
     /**
@@ -63,6 +73,8 @@ internal fun MonthNames.toKotlinCode(): String = when (this.names) {
 
 /**
  * A description of how day of week names are formatted.
+ *
+ * An [IllegalArgumentException] will be thrown if some day-of-week name is empty or there are duplicate names.
  */
 public class DayOfWeekNames(
     /**
@@ -72,6 +84,14 @@ public class DayOfWeekNames(
 ) {
     init {
         require(names.size == 7) { "Day of week names must contain exactly 7 elements" }
+        names.indices.forEach { ix ->
+            require(names[ix].isNotEmpty()) { "A day-of-week name can not be empty" }
+            for (ix2 in 0 until ix) {
+                require(names[ix] != names[ix2]) {
+                    "Day-of-week names must be unique, but '${names[ix]}' was repeated"
+                }
+            }
+        }
     }
 
     /**

core/common/src/internal/format/FormatStructure.kt

@@ -160,13 +160,17 @@ internal class OptionalFormatStructure<in T>(
             listOf(
                 ConstantFormatStructure<T>(onZero).parser(),
                 ParserStructure(
-                    listOf(
-                        UnconditionalModification {
-                            for (field in fields) {
-                                field.assignDefault(it)
+                    if (fields.isEmpty()) {
+                        emptyList()
+                    } else {
+                        listOf(
+                            UnconditionalModification {
+                                for (field in fields) {
+                                    field.assignDefault(it)
+                                }
                             }
-                        }
-                    ),
+                        )
+                    },
                     emptyList()
                 )
             ).concat()
@@ -176,12 +180,16 @@ internal class OptionalFormatStructure<in T>(
     override fun formatter(): FormatterStructure<T> {
         val formatter = format.formatter()
         val predicate = conjunctionPredicate(fields.map { it.isDefaultComparisonPredicate() })
-        return ConditionalFormatter(
-            listOf(
-                predicate::test to ConstantStringFormatterStructure(onZero),
-                Truth::test to formatter
+        return if (predicate is Truth) {
+            ConstantStringFormatterStructure(onZero)
+        } else {
+            ConditionalFormatter(
+                listOf(
+                    predicate::test to ConstantStringFormatterStructure(onZero),
+                    Truth::test to formatter
+                )
             )
-        )
+        }
     }
 
     private class PropertyWithDefault<in T, E> private constructor(

core/common/src/internal/format/parser/Parser.kt

@@ -49,17 +49,21 @@ internal fun <T> List<ParserStructure<T>>.concat(): ParserStructure<T> {
         ParserStructure(operations, followedBy.map { it.append(other) })
     }
 
-    fun <T> ParserStructure<T>.simplify(): ParserStructure<T> {
+    fun <T> ParserStructure<T>.simplify(unconditionalModifications: List<UnconditionalModification<T>>): ParserStructure<T> {
         val newOperations = mutableListOf<ParserOperation<T>>()
         var currentNumberSpan: MutableList<NumberConsumer<T>>? = null
-        // joining together the number consumers in this parser before the first alternative
+        val unconditionalModificationsForTails = unconditionalModifications.toMutableList()
+        // joining together the number consumers in this parser before the first alternative;
+        // collecting the unconditional modifications to push them to the end of all the parser's branches.
         for (op in operations) {
             if (op is NumberSpanParserOperation) {
                 if (currentNumberSpan != null) {
                     currentNumberSpan.addAll(op.consumers)
                 } else {
                     currentNumberSpan = op.consumers.toMutableList()
                 }
+            } else if (op is UnconditionalModification) {
+                unconditionalModificationsForTails.add(op)
             } else {
                 if (currentNumberSpan != null) {
                     newOperations.add(NumberSpanParserOperation(currentNumberSpan))
@@ -69,7 +73,7 @@ internal fun <T> List<ParserStructure<T>>.concat(): ParserStructure<T> {
             }
         }
         val mergedTails = followedBy.flatMap {
-            val simplified = it.simplify()
+            val simplified = it.simplify(unconditionalModificationsForTails)
             // parser `ParserStructure(emptyList(), p)` is equivalent to `p`,
             // unless `p` is empty. For example, ((a|b)|(c|d)) is equivalent to (a|b|c|d).
             // As a special case, `ParserStructure(emptyList(), emptyList())` represents a parser that recognizes an empty
@@ -78,6 +82,9 @@ internal fun <T> List<ParserStructure<T>>.concat(): ParserStructure<T> {
                 simplified.followedBy.ifEmpty { listOf(simplified) }
             else
                 listOf(simplified)
+        }.ifEmpty {
+            // preserving the invariant that `mergedTails` contains all unconditional modifications
+            listOf(ParserStructure(unconditionalModificationsForTails, emptyList()))
         }
         return if (currentNumberSpan == null) {
             // the last operation was not a number span, or it was a number span that we are allowed to interrupt
@@ -115,7 +122,7 @@ internal fun <T> List<ParserStructure<T>>.concat(): ParserStructure<T> {
         }
     }
     val naiveParser = foldRight(ParserStructure<T>(emptyList(), emptyList())) { parser, acc -> parser.append(acc) }
-    return naiveParser.simplify()
+    return naiveParser.simplify(emptyList())
 }
 
 internal interface Copyable<Self> {

core/common/src/internal/format/parser/ParserOperation.kt

@@ -157,6 +157,7 @@ internal class StringSetParserOperation<Output>(
 
     init {
         for (string in strings) {
+            require(string.isNotEmpty()) { "Found an empty string in $whatThisExpects" }
             var node = trie
             for (char in string) {
                 val searchResult = node.children.binarySearchBy(char.toString()) { it.first }
@@ -166,6 +167,7 @@ internal class StringSetParserOperation<Output>(
                     node.children[searchResult].second
                 }
             }
+            require(!node.isTerminal) { "The string '$string' was passed several times" }
             node.isTerminal = true
         }
         fun reduceTrie(trie: TrieNode) {

core/common/test/format/DateTimeFormatTest.kt

@@ -137,6 +137,18 @@ class DateTimeFormatTest {
             }
         }
     }
+
+    @Test
+    fun testOptionalBetweenConsecutiveNumbers() {
+        val format = UtcOffset.Format {
+            offsetHours(Padding.NONE)
+            optional {
+                optional { offsetSecondsOfMinute() }
+                offsetMinutesOfHour()
+            }
+        }
+        assertEquals(UtcOffset(-7, -30), format.parse("-730"))
+    }
 }
 
 fun <T> DateTimeFormat<T>.assertCanNotParse(input: String) {

core/common/test/format/LocalDateFormatTest.kt

@@ -221,6 +221,38 @@ class LocalDateFormatTest {
         assertEquals("2020 Jan 05", format.format(LocalDate(2020, 1, 5)))
     }
 
+    @Test
+    fun testEmptyMonthNames() {
+        val names = MonthNames.ENGLISH_FULL.names
+        for (i in 0 until 12) {
+            val newNames = (0 until 12).map { if (it == i) "" else names[it] }
+            assertFailsWith<IllegalArgumentException> { MonthNames(newNames) }
+        }
+    }
+
+    @Test
+    fun testEmptyDayOfWeekNames() {
+        val names = DayOfWeekNames.ENGLISH_FULL.names
+        for (i in 0 until 7) {
+            val newNames = (0 until 7).map { if (it == i) "" else names[it] }
+            assertFailsWith<IllegalArgumentException> { DayOfWeekNames(newNames) }
+        }
+    }
+
+    @Test
+    fun testIdenticalMonthNames() {
+        assertFailsWith<IllegalArgumentException> {
+            MonthNames("Jan", "Jan", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec")
+        }
+    }
+
+    @Test
+    fun testIdenticalDayOfWeekNames() {
+        assertFailsWith<IllegalArgumentException> {
+            DayOfWeekNames("Mon", "Tue", "Tue", "Thu", "Fri", "Sat", "Sun")
+        }
+    }
+
     private fun test(strings: Map<LocalDate, Pair<String, Set<String>>>, format: DateTimeFormat<LocalDate>) {
         for ((date, stringsForDate) in strings) {
             val (canonicalString, otherStrings) = stringsForDate

core/common/test/format/LocalTimeFormatTest.kt

@@ -216,6 +216,24 @@ class LocalTimeFormatTest {
         }
     }
 
+    @Test
+    fun testEmptyAmPmMarkers() {
+        assertFailsWith<IllegalArgumentException> {
+            LocalTime.Format {
+                amPmMarker("", "pm")
+            }
+        }
+    }
+
+    @Test
+    fun testIdenticalAmPmMarkers() {
+        assertFailsWith<IllegalArgumentException> {
+            LocalTime.Format {
+                amPmMarker("pm", "pm")
+            }
+        }
+    }
+
     private fun test(strings: Map<LocalTime, Pair<String, Set<String>>>, format: DateTimeFormat<LocalTime>) {
         for ((date, stringsForDate) in strings) {
             val (canonicalString, otherStrings) = stringsForDate