Skip to content

Commit

Permalink
Fix not being able to parse some separator-free formats
Browse files Browse the repository at this point in the history
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()`.
  • Loading branch information
dkhalanskyjb committed Apr 29, 2024
1 parent ce30554 commit 5978487
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 15 deletions.
30 changes: 19 additions & 11 deletions core/common/src/internal/format/FormatStructure.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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(
Expand Down
15 changes: 11 additions & 4 deletions core/common/src/internal/format/parser/Parser.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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> {
Expand Down
12 changes: 12 additions & 0 deletions core/common/test/format/DateTimeFormatTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit 5978487

Please sign in to comment.