fix: ifFail dropping matchers, operator case, and quoted literals - #57
Merged
Conversation
Four defects, each reproduced before the change and covered by a regression
test:
* ifFail rebuilt the expression from left, operator and right only, so
allMatch(...) ifFail FALSE returned an empty, invalid expression and silently
discarded every matcher. It copies now.
* Operators were stored under name().lowercase() but looked up raw, so an
operator whose name carried an uppercase letter was unreachable by its own
name. The lookup lowercases too, which is what the stored form always meant.
* The quoted literal check used Regex("\".*\""), and . does not match a newline,
so a literal spanning lines fell through to path resolution and threw
NoSuchElementException. A startsWith and endsWith check has neither the
newline blind spot nor the per-operand regex.
* requireNotNull(operator) { "Unknown operator: $operator" } interpolated the
null local, so the message always read null.
Kotlin engine, three runs per configuration in one session, medians: 2,225,259
to 2,598,127 ops/s, and 849 B to 375 B per evaluation. An operand no longer
pays toBigIntegerOrNull and toBigDecimalOrNull, the second of which screens with
a regex, unless it starts like a number. Expression.isValid uses all rather than
map plus firstOrNull, which short circuits and stops allocating a list a group.
The benchmark default input rooted at a data class while the wide variant rooted
at a map, so the width columns priced the root kind as if it were width. Both
root at a map now, and the wide input is built from the narrow one so the two
cannot drift apart. BENCHMARKS.md is measured again throughout.
CI ran test on Java 17 against a Java 21 toolchain, which worked only because
the runner image ships a 21. It runs check on 21 now, which also enforces the
detekt and 90% kover gates that were configured but never executed.
Also removed a debugging leftover test pinned to case index 122, a duplicated
kover BRANCH bound, and two unused version properties.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four correctness defects in expression handling, a Kotlin engine speedup that
came out of fixing one of them, and a benchmark input fix that was pricing the
input's root kind as if it were input width.
Every defect below was reproduced first and has a regression test.
Bugs
ifFaildiscarded every matcher of a group. It rebuilt the expression fromleft,operatorandrightonly, soallMatch(...) ifFail FALSEreturned afully empty expression, which then failed validation. It copies now.
A mixed case operator was unreachable by its own name. Operators are stored
under
name().lowercase()but were looked up raw, so a custom operator namedmyOperatorcould never be resolved. The lookup lowercases too.A quoted literal spanning lines threw. The check was
Regex("\".*\"")and.does not match a newline, so the literal fell through to path resolution andraised
NoSuchElementException. The replacement is astartsWithandendsWithcheck, which has neither the newline blind spot nor a regex on theper-operand path.
Unknown operator:always printednull, because the message interpolatedthe null local rather than the operator name.
Performance
Kotlin engine, three runs per configuration in one session, medians:
An operand no longer runs
toBigIntegerOrNullandtoBigDecimalOrNull, thesecond of which screens with a regex, unless it starts like a number, and paths
never do.
Expression.isValidusesallrather thanmapplusfirstOrNull,which short circuits and stops allocating a list a group.
Benchmark input
The default input rooted at a data class while the wide variant rooted at a map,
so the two width columns differed by root kind as well as width. That is not
free: a map root is a hash lookup where a typed root is a reflective property
read, and it showed up as the Kotlin engine reading faster on the wider input.
Both root at a map now, and
wideInputis built fromnarrowInput, so the twocannot drift apart. BENCHMARKS.md is measured again throughout: all four tables,
the spread figures, the GC counts and the guidance.
Where the time goesis left alone. That measurement comes from a loop that isnot in this repository and cannot be reproduced from it.
CI
tests.yamlranteston Java 17 against a Java 21 toolchain, which workedonly because the runner image happens to ship a 21. It runs
checkon 21 now,which additionally enforces the detekt and 90% kover gates that were configured
but never actually executed.
Cleanup
assertSingleCaseForDebugging, a debugging leftover pinned to case index 122that duplicated a parameterized case and would break if the case list shrank
BRANCH >= 90kover bound declared twicecredentialsVersionandjacocoVersionpropertiesVerification
./gradlew clean checkpasses, which covers the tests, detekt and the koverthresholds for every module.