Repository navigation
Compare Comparable host values; identify local roots by canonical path - #1396
Merged
Merged
Conversation
… canonical path compare fell through to a ClassCastException for every host class without its own compare arm, so Path, File, StringBuilder and TimeUnit could not be sorted even though .compareTo worked. compare now asks the value's compareTo when the class model says it is Comparable and both values share a class. File, StringBuilder and TimeUnit were missing Comparable in the graph, and TimeUnit had no Enum surface. Path/File compareTo now match the JDK and refuse each other; java.time enums compare by ordinal difference. :local/root coords were keyed by their spelling, so ../libs and ../tui/../libs were two coordinates that could not be compared and every start warned. dep-id and the cpcache local walk now use the canonical path. Fixes #1394, fixes #1395.
The corpus runner loads no stdlib autoloads, so jolt.time is only reachable from the unit gate's insttime suite.
Each time type's compareTo read the other value's fields without checking its type, so (.compareTo Month/MAY 1) was an NPE and (.compareTo some-date 1) answered 1. They go through compare now, whose arm refuses another type with a ClassCastException. The orders also return the JDK's magnitudes: the first field difference for LocalDate, Year, YearMonth and MonthDay, and seconds then nano difference for Instant and Duration.
compare answered strings, symbols, keywords and chars by sign only, where the JVM hands back String.compareTo's difference ((compare "ab" "abcd") is -2). Strings now also compare by UTF-16 unit, so a supplementary char sorts before U+FFFF as it does in Java. The scan uses a new unchecked sa-ustring-ref and is faster than the string<? it replaces (sorting 200k strings 201 -> 165 ms).
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.
Fixes #1395 and #1394.
#1395: compare only knew host classes that registered their own compare arm, so a Path (what babashka.fs/list-dir returns), File, StringBuilder or TimeUnit raised ClassCastException even though .compareTo worked. compare now falls back to the value's compareTo when the class model says it is Comparable and both values share a class (different classes still raise, matching compareTo's cast on the JVM). Along the way: File, StringBuilder and TimeUnit were missing Comparable in the class graph, TimeUnit had no Enum surface (ordinal/compareTo), Path/File compareTo returned a sign and accepted each other, and the java.time enums compared by sign rather than ordinal difference. Mismatched java.time types now raise ClassCastException instead of ExceptionInfo.
#1394: the warnings were not legitimate. ./modules/agent/../libs and ./modules/agent/../tui/../libs are the same directory, but :local/root coords were keyed by their spelling, so the expander saw two versions it could not compare. dep-id :local now uses the canonical path, as tools.deps canonicalizes local roots. The cpcache key's local-deps walk had the same problem: a ../ cycle never repeated a spelling and only stopped when the path got too long.
Corpus rows are checked against JDK 20 Clojure; deps_expand_test covers the diamond and the cycle.
Two follow-ups from review. Every java.time compareTo read the other value's fields unchecked, so (.compareTo Month/MAY 1) was an NPE and (.compareTo date 1) answered 1; they go through compare now, and the orders return the JDK's magnitudes (first field difference, or seconds then nano difference). And compare itself answered strings, symbols, keywords and chars by sign only, where the JVM returns compareTo's difference; strings also compare by UTF-16 unit now, so a supplementary char sorts before U+FFFF. That scan uses a new unchecked sa-ustring-ref and sorts strings faster than the string<? it replaced (200k strings, 201 -> 165 ms A/B/A).
The zoned types live in jolt-lang/time and get the same treatment in jolt-lang/time#21.