Skip to content

Commit 04adf84

Browse files
committed
Fix not being able to parse some separator-free formats
The reproducer was: ``` offsetHours(Padding.NONE) optional { optional { char(':') } offsetMinutesOfHour() } ``` This showed us one bug and one inefficiency, both of which are fixed here. Inefficiency: the `optional { char(':') }` should for all intents and purposes be equivalent to `alternativeParsing({ char(':') }) { }`: both the empty string and the `:` should be parsed, and, according to `optional`'s definition, if none of the fields mentioned in the block are non-zero, nothing should be output. However, such `optional` still created a fake parser element that notified all the fields that they are zero when an empty string is parsed, even though there are no fields. Bug: the fake parser element that notifies the fields that they are zero even though nothing of note was parsed interfered with the mechanism supporting compact formats. Number parsers are greedy, so `number(hh); number(mm)` gets merged into `number(hhmm)`. If there is something between them, this merge can't happen: `number(hh); string("x"); number(mm)`. For this reason, parsers that don't accept any strings are forbidden (or just very unlikely to happen)--except for the zero-width unconditional modification parser. This bug is fixed by moving such parsers to the end of the parser: `number(hh); modification(); number(mm); string("x")` will get transformed to `number(hhmm); string("x"); modification()`.
1 parent cc8121a commit 04adf84

File tree

3 files changed

+42
-15
lines changed

3 files changed

+42
-15
lines changed

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

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -160,13 +160,17 @@ internal class OptionalFormatStructure<in T>(
160160
listOf(
161161
ConstantFormatStructure<T>(onZero).parser(),
162162
ParserStructure(
163-
listOf(
164-
UnconditionalModification {
165-
for (field in fields) {
166-
field.assignDefault(it)
163+
if (fields.isEmpty()) {
164+
emptyList()
165+
} else {
166+
listOf(
167+
UnconditionalModification {
168+
for (field in fields) {
169+
field.assignDefault(it)
170+
}
167171
}
168-
}
169-
),
172+
)
173+
},
170174
emptyList()
171175
)
172176
).concat()
@@ -176,12 +180,16 @@ internal class OptionalFormatStructure<in T>(
176180
override fun formatter(): FormatterStructure<T> {
177181
val formatter = format.formatter()
178182
val predicate = conjunctionPredicate(fields.map { it.isDefaultComparisonPredicate() })
179-
return ConditionalFormatter(
180-
listOf(
181-
predicate::test to ConstantStringFormatterStructure(onZero),
182-
Truth::test to formatter
183+
return if (predicate is Truth) {
184+
ConstantStringFormatterStructure(onZero)
185+
} else {
186+
ConditionalFormatter(
187+
listOf(
188+
predicate::test to ConstantStringFormatterStructure(onZero),
189+
Truth::test to formatter
190+
)
183191
)
184-
)
192+
}
185193
}
186194

187195
private class PropertyWithDefault<in T, E> private constructor(

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

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,17 +48,21 @@ internal fun <T> List<ParserStructure<T>>.concat(): ParserStructure<T> {
4848
} else {
4949
ParserStructure(operations, followedBy.map { it.append(other) })
5050
}
51-
fun <T> ParserStructure<T>.simplify(): ParserStructure<T> {
51+
fun <T> ParserStructure<T>.simplify(unconditionalModifications: List<UnconditionalModification<T>>): ParserStructure<T> {
5252
val newOperations = mutableListOf<ParserOperation<T>>()
5353
var currentNumberSpan: MutableList<NumberConsumer<T>>? = null
54-
// joining together the number consumers in this parser before the first alternative
54+
val unconditionalModificationsForTails = unconditionalModifications.toMutableList()
55+
// joining together the number consumers in this parser before the first alternative;
56+
// collecting the unconditional modifications to push them to the end of all the parser's branches.
5557
for (op in operations) {
5658
if (op is NumberSpanParserOperation) {
5759
if (currentNumberSpan != null) {
5860
currentNumberSpan.addAll(op.consumers)
5961
} else {
6062
currentNumberSpan = op.consumers.toMutableList()
6163
}
64+
} else if (op is UnconditionalModification) {
65+
unconditionalModificationsForTails.add(op)
6266
} else {
6367
if (currentNumberSpan != null) {
6468
newOperations.add(NumberSpanParserOperation(currentNumberSpan))
@@ -68,7 +72,7 @@ internal fun <T> List<ParserStructure<T>>.concat(): ParserStructure<T> {
6872
}
6973
}
7074
val mergedTails = followedBy.flatMap {
71-
val simplified = it.simplify()
75+
val simplified = it.simplify(unconditionalModificationsForTails)
7276
// parser `ParserStructure(emptyList(), p)` is equivalent to `p`,
7377
// unless `p` is empty. For example, ((a|b)|(c|d)) is equivalent to (a|b|c|d).
7478
// As a special case, `ParserStructure(emptyList(), emptyList())` represents a parser that recognizes an empty
@@ -77,6 +81,9 @@ internal fun <T> List<ParserStructure<T>>.concat(): ParserStructure<T> {
7781
simplified.followedBy.ifEmpty { listOf(simplified) }
7882
else
7983
listOf(simplified)
84+
}.ifEmpty {
85+
// preserving the invariant that `mergedTails` contains all unconditional modifications
86+
listOf(ParserStructure(unconditionalModificationsForTails, emptyList()))
8087
}
8188
return if (currentNumberSpan == null) {
8289
// the last operation was not a number span, or it was a number span that we are allowed to interrupt
@@ -114,7 +121,7 @@ internal fun <T> List<ParserStructure<T>>.concat(): ParserStructure<T> {
114121
}
115122
}
116123
val naiveParser = foldRight(ParserStructure<T>(emptyList(), emptyList())) { parser, acc -> parser.append(acc) }
117-
return naiveParser.simplify()
124+
return naiveParser.simplify(emptyList())
118125
}
119126

120127
internal interface Copyable<Self> {

core/common/test/format/DateTimeFormatTest.kt

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,4 +127,16 @@ class DateTimeFormatTest {
127127
DateTimeComponents.Format { chars(format) }.parse(format)
128128
}
129129
}
130+
131+
@Test
132+
fun testOptionalBetweenConsecutiveNumbers() {
133+
val format = UtcOffset.Format {
134+
offsetHours(Padding.NONE)
135+
optional {
136+
optional { offsetSecondsOfMinute() }
137+
offsetMinutesOfHour()
138+
}
139+
}
140+
assertEquals(UtcOffset(-7, -30), format.parse("-730"))
141+
}
130142
}

0 commit comments

Comments
 (0)