Improve error descriptiveness in the parsing API (#360)

Fixes #359 Fixes #361 Additionally, remove delayed initialization of parsing to ensure that creating ambigous formats fails and document the thrown exception.
Kotlin · Mar 18, 2024 · 02e4e4d · 02e4e4d
1 parent cc8121a
commit 02e4e4d
Show file tree

Hide file tree

Showing 15 changed files with 101 additions and 44 deletions.
diff --git a/core/common/src/LocalDate.kt b/core/common/src/LocalDate.kt
@@ -65,6 +65,8 @@ public expect class LocalDate : Comparable<LocalDate> {
          * (for example, [dayOfMonth] is 31 for February), consider using [DateTimeComponents.Format] instead.
          *
          * There is a collection of predefined formats in [LocalDate.Formats].
+         *
+         * @throws IllegalArgumentException if parsing using this format is ambiguous.
          */
         @Suppress("FunctionName")
         public fun Format(block: DateTimeFormatBuilder.WithDate.() -> Unit): DateTimeFormat<LocalDate>

diff --git a/core/common/src/LocalDateTime.kt b/core/common/src/LocalDateTime.kt
@@ -62,6 +62,8 @@ public expect class LocalDateTime : Comparable<LocalDateTime> {
          * (for example, [dayOfMonth] is 31 for February), consider using [DateTimeComponents.Format] instead.
          *
          * There is a collection of predefined formats in [LocalDateTime.Formats].
+         *
+         * @throws IllegalArgumentException if parsing using this format is ambiguous.
          */
         @Suppress("FunctionName")
         public fun Format(builder: DateTimeFormatBuilder.WithDateTime.() -> Unit): DateTimeFormat<LocalDateTime>

diff --git a/core/common/src/LocalTime.kt b/core/common/src/LocalTime.kt
@@ -97,6 +97,8 @@ public expect class LocalTime : Comparable<LocalTime> {
          * (for example, [second] is 60), consider using [DateTimeComponents.Format] instead.
          *
          * There is a collection of predefined formats in [LocalTime.Formats].
+         *
+         * @throws IllegalArgumentException if parsing using this format is ambiguous.
          */
         @Suppress("FunctionName")
         public fun Format(builder: DateTimeFormatBuilder.WithTime.() -> Unit): DateTimeFormat<LocalTime>

diff --git a/core/common/src/UtcOffset.kt b/core/common/src/UtcOffset.kt
@@ -67,6 +67,8 @@ public expect class UtcOffset {
          * [DateTimeFormatBuilder.WithUtcOffset.offset] in a format builder for a larger data structure.
          *
          * There is a collection of predefined formats in [UtcOffset.Formats].
+         *
+         * @throws IllegalArgumentException if parsing using this format is ambiguous.
          */
         @Suppress("FunctionName")
         public fun Format(block: DateTimeFormatBuilder.WithUtcOffset.() -> Unit): DateTimeFormat<UtcOffset>

diff --git a/core/common/src/format/DateTimeComponents.kt b/core/common/src/format/DateTimeComponents.kt
@@ -69,6 +69,8 @@ public class DateTimeComponents internal constructor(internal val contents: Date
          * Creates a [DateTimeFormat] for [DateTimeComponents] values using [DateTimeFormatBuilder.WithDateTimeComponents].
          *
          * There is a collection of predefined formats in [DateTimeComponents.Formats].
+         *
+         * @throws IllegalArgumentException if parsing using this format is ambiguous.
          */
         @Suppress("FunctionName")
         public fun Format(block: DateTimeFormatBuilder.WithDateTimeComponents.() -> Unit): DateTimeFormat<DateTimeComponents> {

diff --git a/core/common/src/format/DateTimeFormat.kt b/core/common/src/format/DateTimeFormat.kt
@@ -115,7 +115,10 @@ internal sealed class AbstractDateTimeFormat<T, U : Copyable<U>> : DateTimeForma
         try {
             return valueFromIntermediate(matched)
         } catch (e: IllegalArgumentException) {
-            throw DateTimeFormatException(e.message!!)
+            throw DateTimeFormatException(when (val message = e.message) {
+                null -> "The value parsed from '$input' is invalid"
+                else -> "$message (when parsing '$input')"
+            }, e)
         }
     }
 

diff --git a/core/common/src/internal/format/FieldFormatDirective.kt b/core/common/src/internal/format/FieldFormatDirective.kt
@@ -108,7 +108,7 @@ internal abstract class NamedUnsignedIntFieldFormatDirective<in Target>(
     override fun parser(): ParserStructure<Target> =
         ParserStructure(
             listOf(
-                StringSetParserOperation(values, AssignableString(), "One of $values for $name")
+                StringSetParserOperation(values, AssignableString(), "one of $values for $name")
             ), emptyList()
         )
 }
@@ -142,7 +142,7 @@ internal abstract class NamedEnumIntFieldFormatDirective<in Target, Type>(
     override fun parser(): ParserStructure<Target> =
         ParserStructure(
             listOf(
-                StringSetParserOperation(mapping.values, AssignableString(), "One of ${mapping.values} for $name")
+                StringSetParserOperation(mapping.values, AssignableString(), "one of ${mapping.values} for $name")
             ), emptyList()
         )
 }

diff --git a/core/common/src/internal/format/FormatStructure.kt b/core/common/src/internal/format/FormatStructure.kt
@@ -231,8 +231,8 @@ internal open class ConcatenatedFormatStructure<in T>(
 
 internal class CachedFormatStructure<in T>(formats: List<NonConcatenatedFormatStructure<T>>) :
     ConcatenatedFormatStructure<T>(formats) {
-    private val cachedFormatter: FormatterStructure<T> by lazy { super.formatter() }
-    private val cachedParser: ParserStructure<T> by lazy { super.parser() }
+    private val cachedFormatter: FormatterStructure<T> = super.formatter()
+    private val cachedParser: ParserStructure<T> = super.parser()
 
     override fun formatter(): FormatterStructure<T> = cachedFormatter
 

diff --git a/core/common/src/internal/format/parser/ParserOperation.kt b/core/common/src/internal/format/parser/ParserOperation.kt
@@ -50,7 +50,12 @@ internal class NumberSpanParserOperation<Output>(
 
     init {
         require(consumers.all { (it.length ?: Int.MAX_VALUE) > 0 })
-        require(consumers.count { it.length == null } <= 1)
+        require(consumers.count { it.length == null } <= 1) {
+            val fieldNames = consumers.filter { it.length == null }.map { it.whatThisExpects }
+            "At most one variable-length numeric field in a row is allowed, but got several: $fieldNames. " +
+                "Parsing is undefined: for example, with variable-length month number " +
+                "and variable-length day of month, '111' can be parsed as Jan 11th or Nov 1st."
+        }
     }
 
     private val whatThisExpects: String

diff --git a/core/common/test/format/DateTimeComponentsFormatTest.kt b/core/common/test/format/DateTimeComponentsFormatTest.kt
@@ -32,13 +32,13 @@ class DateTimeComponentsFormatTest {
                 setOffset(UtcOffset.ZERO)
             },
             format.parse("Tue, 40 Jun 2008 11:05:30 GMT"))
-        assertFailsWith<DateTimeFormatException> { format.parse("Bue, 3 Jun 2008 11:05:30 GMT") }
+        format.assertCanNotParse("Bue, 3 Jun 2008 11:05:30 GMT")
     }
 
     @Test
     fun testInconsistentLocalTime() {
         val formatTime = LocalTime.Format {
-            hour(); char(':'); minute();
+            hour(); char(':'); minute()
             chars(" ("); amPmHour(); char(':'); minute(); char(' '); amPmMarker("AM", "PM"); char(')')
         }
         val format = DateTimeComponents.Format { time(formatTime) }
@@ -53,16 +53,16 @@ class DateTimeComponentsFormatTest {
             DateTimeComponents().apply { hour = 23; hourOfAmPm = 11; minute = 15; amPm = AmPmMarker.AM },
             format.parse(time2)
         )
-        assertFailsWith<IllegalArgumentException> { formatTime.parse(time2) }
+        formatTime.assertCanNotParse(time2)
         val time3 = "23:15 (10:15 PM)" // a time with an inconsistent number of hours
         assertDateTimeComponentsEqual(
             DateTimeComponents().apply { hour = 23; hourOfAmPm = 10; minute = 15; amPm = AmPmMarker.PM },
             format.parse(time3)
         )
-        assertFailsWith<IllegalArgumentException> { formatTime.parse(time3) }
+        formatTime.assertCanNotParse(time3)
         val time4 = "23:15 (11:16 PM)" // a time with an inconsistently duplicated field
-        assertFailsWith<IllegalArgumentException> { format.parse(time4) }
-        assertFailsWith<IllegalArgumentException> { formatTime.parse(time4) }
+        format.assertCanNotParse(time4)
+        formatTime.assertCanNotParse(time4)
     }
 
     @Test
@@ -95,7 +95,7 @@ class DateTimeComponentsFormatTest {
         assertEquals(dateTime, bag.toLocalDateTime())
         assertEquals(offset, bag.toUtcOffset())
         assertEquals(berlin, bag.timeZoneId)
-        assertFailsWith<DateTimeFormatException> { format.parse("2008-06-03T11:05:30.123456789+01:00[Mars/New_York]") }
+        format.assertCanNotParse("2008-06-03T11:05:30.123456789+01:00[Mars/New_York]")
         for (zone in TimeZone.availableZoneIds) {
             assertEquals(zone, format.parse("2008-06-03T11:05:30.123456789+01:00[$zone]").timeZoneId)
         }

diff --git a/core/common/test/format/DateTimeFormatTest.kt b/core/common/test/format/DateTimeFormatTest.kt
@@ -127,4 +127,25 @@ class DateTimeFormatTest {
             DateTimeComponents.Format { chars(format) }.parse(format)
         }
     }
+
+    @Test
+    fun testCreatingAmbiguousFormat() {
+        assertFailsWith<IllegalArgumentException> {
+            DateTimeComponents.Format {
+                monthNumber(Padding.NONE)
+                dayOfMonth(Padding.NONE)
+            }
+        }
+    }
+}
+
+fun <T> DateTimeFormat<T>.assertCanNotParse(input: String) {
+    val exception = assertFailsWith<DateTimeFormatException> { parse(input) }
+    try {
+        val message = exception.message ?: throw AssertionError("The parse exception didn't have a message")
+        assertContains(message, input)
+    } catch (e: AssertionError) {
+        e.addSuppressed(exception)
+        throw e
+    }
 }
diff --git a/core/common/test/format/LocalDateFormatTest.kt b/core/common/test/format/LocalDateFormatTest.kt
@@ -15,11 +15,11 @@ class LocalDateFormatTest {
 
     @Test
     fun testErrorHandling() {
-        val format = LocalDate.Formats.ISO
-        assertEquals(LocalDate(2023, 2, 28), format.parse("2023-02-28"))
-        val error = assertFailsWith<DateTimeFormatException> { format.parse("2023-02-40") }
-        assertContains(error.message!!, "40")
-        assertFailsWith<DateTimeFormatException> { format.parse("2023-02-XX") }
+        LocalDate.Formats.ISO.apply {
+            assertEquals(LocalDate(2023, 2, 28), parse("2023-02-28"))
+            assertCanNotParse("2023-02-40")
+            assertCanNotParse("2023-02-XX")
+        }
     }
 
     @Test

diff --git a/core/common/test/format/LocalDateTimeFormatTest.kt b/core/common/test/format/LocalDateTimeFormatTest.kt
@@ -15,11 +15,11 @@ class LocalDateTimeFormatTest {
 
     @Test
     fun testErrorHandling() {
-        val format = LocalDateTime.Formats.ISO
-        assertEquals(LocalDateTime(2023, 2, 28, 15, 36), format.parse("2023-02-28T15:36"))
-        val error = assertFailsWith<DateTimeFormatException> { format.parse("2023-02-40T15:36") }
-        assertContains(error.message!!, "40")
-        assertFailsWith<DateTimeFormatException> { format.parse("2023-02-XXT15:36") }
+        LocalDateTime.Formats.ISO.apply {
+            assertEquals(LocalDateTime(2023, 2, 28, 15, 36), parse("2023-02-28T15:36"))
+            assertCanNotParse("2023-02-40T15:36")
+            assertCanNotParse("2023-02-XXT15:36")
+        }
     }
 
     @Test
@@ -163,7 +163,7 @@ class LocalDateTimeFormatTest {
             put(LocalDateTime(123456, 1, 1, 13, 44, 0, 0), ("+123456- 1- 1 13:44: 0" to setOf()))
             put(LocalDateTime(-123456, 1, 1, 13, 44, 0, 0), ("-123456- 1- 1 13:44: 0" to setOf()))
         }
-        val format = LocalDateTime.Format {
+        LocalDateTime.Format {
             year(Padding.SPACE)
             char('-')
             monthNumber(Padding.SPACE)
@@ -175,17 +175,18 @@ class LocalDateTimeFormatTest {
             minute(Padding.SPACE)
             char(':')
             second(Padding.SPACE)
+        }.apply {
+            test(dateTimes, this)
+            parse(" 008- 7- 5  0: 0: 0")
+            assertCanNotParse("  008- 7- 5  0: 0: 0")
+            assertCanNotParse("  8- 7- 5  0: 0: 0")
+            assertCanNotParse(" 008-  7- 5  0: 0: 0")
+            assertCanNotParse(" 008-7- 5  0: 0: 0")
+            assertCanNotParse("+008- 7- 5  0: 0: 0")
+            assertCanNotParse("  -08- 7- 5  0: 0: 0")
+            assertCanNotParse("   -08- 7- 5  0: 0: 0")
+            assertCanNotParse("-8- 7- 5  0: 0: 0")
         }
-        test(dateTimes, format)
-        format.parse(" 008- 7- 5  0: 0: 0")
-        assertFailsWith<DateTimeFormatException> { format.parse("  008- 7- 5  0: 0: 0") }
-        assertFailsWith<DateTimeFormatException> { format.parse("  8- 7- 5  0: 0: 0") }
-        assertFailsWith<DateTimeFormatException> { format.parse(" 008-  7- 5  0: 0: 0") }
-        assertFailsWith<DateTimeFormatException> { format.parse(" 008-7- 5  0: 0: 0") }
-        assertFailsWith<DateTimeFormatException> { format.parse("+008- 7- 5  0: 0: 0") }
-        assertFailsWith<DateTimeFormatException> { format.parse("  -08- 7- 5  0: 0: 0") }
-        assertFailsWith<DateTimeFormatException> { format.parse("   -08- 7- 5  0: 0: 0") }
-        assertFailsWith<DateTimeFormatException> { format.parse("-8- 7- 5  0: 0: 0") }
     }
 
     @Test

diff --git a/core/common/test/format/LocalTimeFormatTest.kt b/core/common/test/format/LocalTimeFormatTest.kt
@@ -15,11 +15,11 @@ class LocalTimeFormatTest {
 
     @Test
     fun testErrorHandling() {
-        val format = LocalTime.Formats.ISO
-        assertEquals(LocalTime(15, 36), format.parse("15:36"))
-        val error = assertFailsWith<DateTimeFormatException> { format.parse("40:36") }
-        assertContains(error.message!!, "40")
-        assertFailsWith<DateTimeFormatException> { format.parse("XX:36") }
+        LocalTime.Formats.ISO.apply {
+            assertEquals(LocalTime(15, 36), parse("15:36"))
+            assertCanNotParse("40:36")
+            assertCanNotParse("XX:36")
+        }
     }
 
     @Test
@@ -199,6 +199,23 @@ class LocalTimeFormatTest {
         assertEquals("12:34:56.123", format.format(LocalTime(12, 34, 56, 123000000)))
     }
 
+    @Test
+    fun testParsingDisagreeingComponents() {
+        LocalTime.Format {
+            hour()
+            char(':')
+            minute()
+            char('(')
+            amPmHour()
+            char(' ')
+            amPmMarker("AM", "PM")
+            char(')')
+        }.apply {
+            assertEquals(LocalTime(23, 59), parse("23:59(11 PM)"))
+            assertCanNotParse("23:59(11 AM)")
+        }
+    }
+
     private fun test(strings: Map<LocalTime, Pair<String, Set<String>>>, format: DateTimeFormat<LocalTime>) {
         for ((date, stringsForDate) in strings) {
             val (canonicalString, otherStrings) = stringsForDate

diff --git a/core/common/test/format/UtcOffsetFormatTest.kt b/core/common/test/format/UtcOffsetFormatTest.kt
@@ -13,18 +13,18 @@ class UtcOffsetFormatTest {
 
     @Test
     fun testErrorHandling() {
-        val format = UtcOffset.Format {
+        UtcOffset.Format {
             isoOffset(
                 zOnZero = true,
                 useSeparator = true,
                 outputMinute = WhenToOutput.ALWAYS,
                 outputSecond = WhenToOutput.IF_NONZERO
             )
+        }.apply {
+            assertEquals(UtcOffset(hours = -4, minutes = -30), parse("-04:30"))
+            assertCanNotParse("-04:60")
+            assertCanNotParse("-04:XX")
         }
-        assertEquals(UtcOffset(hours = -4, minutes = -30), format.parse("-04:30"))
-        val error = assertFailsWith<DateTimeFormatException> { format.parse("-04:60") }
-        assertContains(error.message!!, "60")
-        assertFailsWith<DateTimeFormatException> { format.parse("-04:XX") }
     }
 
     @Test