Repository navigation
feat(rt): fill JVM interop gaps that JVM-targeted Clojure hits - #1085
Conversation
| // no term namespace; a rebound buffered *out* still needs its Flush. | ||
| func flushOut(ec *vm.ExecContext) error { | ||
| if h := resolveIOHandleVar(ec, "*out*"); h != nil { | ||
| return h.Sync() |
There was a problem hiding this comment.
On wasip1, (flush) throws when stdout is a pipe. This is the wasip1 build
from go build, run with Node's WASI and stdout piped:
-e '(do (print "hi ") (flush) :ok)'
hi error: sync /dev/stdout: Capabilities insufficient
With stdout redirected to a file it works. The non-wasm flushOut ignores
the "can't fsync this fd" errnos when *out* is file-backed, but this one
returns h.Sync() unchanged. clojure.core/flush is new on this platform,
so print followed by flush now fails under Node's WASI (and maybe other
hosts) whenever output is piped. Niva's Any__echonl does exactly that.
This version passes under Node for a pipe, a file and with-out-str:
func flushOut(ec *vm.ExecContext) error {
h := resolveIOHandleVar(ec, "*out*")
if h == nil {
return nil
}
err := h.Sync()
if h.File() != nil && (errors.Is(err, syscall.ENOTCAPABLE) ||
errors.Is(err, syscall.EBADF) ||
errors.Is(err, syscall.EINVAL) ||
errors.Is(err, syscall.ENOTSUP)) {
err = nil
}
return err
}Only ENOTCAPABLE showed up under Node. The others are there in case another
host reports something different. I didn't have wasmtime to check what its
fd_sync returns on stdio.
There was a problem hiding this comment.
Thanks, reproduced it: under Node WASI with stdout piped, (do (print "hi ") (flush) :ok) printed hi then failed with sync /dev/stdout: Capabilities insufficient. Fixed in f5f54aa with your version. I added ENOTTY so the set matches the native flushOut.
With the fix, the Node runs give hi :ok for a pipe and for a file, and "x" for with-out-str. I didn't have wasmtime either.
| func (l *hostArrayList) index(v vm.Value, size int) (int, error) { | ||
| i, ok := vm.ToInt64(v) | ||
| if !ok { | ||
| return 0, fmt.Errorf("java.util.ArrayList index must be an integer, got %s", v.Type().Name()) |
There was a problem hiding this comment.
A plain error means "not handled", so two ArrayList calls fall through to a
core fn with the same name:
(.get (ArrayList. [1 2]) :a) ;=> nil, via clojure.core/get
(.remove (ArrayList. [1 2]) 0) ;=> error "complement expected Fn", via clojure.core/removeThe first should probably throw, as it does on the JVM. Throwing the
non-integer index error instead of returning it would do that. For the
second, remove is common enough on an ArrayList that it may be worth
adding. If not, the error should at least name ArrayList rather than
complement.
There was a problem hiding this comment.
Fixed in 122c3d5, both ways you suggested:
.removeis there now. An integer argument removes at that index and returns the element; anything else removes the first equal element and returns whether it found one. The integer case is the overload Clojure's reflective call picks.- A method the shim implements, called with arguments none of its overloads take, now throws
IllegalArgumentException("No matching method get found taking 1 args (let-go.lang.Keyword) for class java.util.ArrayList"). It no longer returns a plain error, so it can't fall through to a core fn of the same name. That covers your(.get l :a), plus a few other cases that fell through the same way:(.get l 0 :x)reachedgetwith a not-found value, and the same went for.set/.addwith a non-integer index and an arity the shim doesn't have. All of these are now in the shim tests.
Method names the shim doesn't implement still go on to the generic fallbacks, so .iterator, .hashCode and similar keep working.
| for s := sq.Seq(); !vm.SeqIsEmpty(s); s = s.Next() { | ||
| if k, v, ok := vm.MapEntryKV(s.First()); ok { | ||
| acc = acc.Assoc(k, v) | ||
| h.update(func(acc vm.Associative) (vm.Associative, vm.Value) { |
There was a problem hiding this comment.
putAll now walks its source while holding mu, so a lazy source that reads
the same map hangs:
(let [hm (java.util.HashMap.)]
(.putAll hm {:a 1})
(.putAll hm (map (fn [k] [k (.get hm :a)]) [:b])) ; hangs here; 7d902c4 returned 1
(.get hm :b))Realizing the entries before taking the lock fixes it. With this change the
case above returns 1, (.putAll hm {:a 1 :b 2}) still works, and a non-map
still throws ClassCastException:
// Realize the source before taking the lock: a lazy source that
// reads this map would otherwise wait on the lock it holds.
var kvs [][2]vm.Value
for s := sq.Seq(); !vm.SeqIsEmpty(s); s = s.Next() {
if k, v, ok := vm.MapEntryKV(s.First()); ok {
kvs = append(kvs, [2]vm.Value{k, v})
}
}
h.update(func(acc vm.Associative) (vm.Associative, vm.Value) {
for _, kv := range kvs {
acc = acc.Assoc(kv[0], kv[1])
}
return acc, vm.NIL
})On the JVM a lazy seq isn't a Map, so rejecting non-map sources would be the
other way out.
nooga
left a comment
There was a problem hiding this comment.
Self-review: one blocker left before merge. It's the one dharmatech raised in the last review, and it's still present at head 222de01.
HashMap.putAll (pkg/rt/host_hashmap.go:80) walks its source seq inside h.update(...), which means while holding h.mu. A lazy source that reads the same map re-enters that non-reentrant mutex:
(let [hm (java.util.HashMap.)]
(.putAll hm {:a 1})
(.putAll hm (map (fn [k] [k (.get hm :a)]) [:b]))
(println (.get hm :b)))This kills the process with fatal error: all goroutines are asleep - deadlock!. The same code printed 1 at 7d902c4. Fix: realize the entries into a slice before calling h.update, and add this case to test/jvm_interop_shims_test.lg next to the non-map putAll check.
The other review points are addressed: the wasip1 flush errno set, and the ArrayList .get/.remove fall-through (fixed in 122c3d5). Natives are registered one way, make check-generated and check-generated-manifest pass, and the compiler paths are untouched.
mparrett
left a comment
There was a problem hiding this comment.
I recommend fixing all three findings below before merging. Self-referencing ArrayList rendering terminates the process with a fatal stack overflow; String.split and HashMap.containsKey silently return incorrect results.
Reviewed 8ac8793. make build and its smoke checks passed. The new suite passed all 14 tests / 118 assertions. All three findings were reproduced locally, with JVM behavior checked through Babashka. The stack-overflow probe used a reduced stack limit.
| func (l *hostArrayList) String() string { | ||
| parts := make([]string, len(l.items)) | ||
| for i, v := range l.items { | ||
| parts[i] = strValue(v) |
There was a problem hiding this comment.
[P2] Guard direct self-references when rendering an ArrayList
A legal self-reference makes rendering recurse indefinitely: (let [l (java.util.ArrayList.)] (.add l l) (.toString l)). strValue(l) re-enters l.String(), eventually causing a fatal Go stack overflow that terminates the interpreter or embedding process. JVM ArrayList renders this as "[(this Collection)]". Please detect v == l and render the self-reference marker before recursively rendering the element. Reproduced in an isolated Go probe with a reduced stack limit.
There was a problem hiding this comment.
Fixed in 5b27e8e. An element that is the list itself renders as (this Collection), and array-list-self-reference pins [1, (this Collection)]. HashMap's toString is opaque (#<java.util.HashMap>), so it can't recurse.
| } | ||
| var parts []Value | ||
| start := 0 | ||
| for _, m := range re.FindAllStringIndex(str, -1) { |
There was a problem hiding this comment.
[P2] Preserve Java's adjacent empty regex matches in String.split
(vec (.split "abc" "a*")) returns ["" "b" "c"], but the JVM returns ["" "" "b" "c"]. Go's FindAllStringIndex suppresses an empty match immediately adjacent to the preceding match, while Java's matcher includes it. The missing match changes the fields, including with negative limits. Please preserve these empty matches and add a regression test.
There was a problem hiding this comment.
Fixed in f1a6414. split now enumerates matches the way Matcher.find does: it restores the empty match Go drops right after a non-empty one, so (.split "abc" "a*") gives ["" "" "b" "c"]. The tests pin limits 0, -1 and 2, plus the javadoc's boo:and:foo table. One caveat: patterns that look left (^, \A, \b, \B) keep Go's enumeration, because the restore check runs on the remaining text and loses left context.
| } | ||
| case "containsKey": | ||
| if len(args) == 1 { | ||
| return vm.Boolean(h.lookup(args[0], hashMapNotFound) != hashMapNotFound), nil |
There was a problem hiding this comment.
[P2] Use an unforgeable marker for an absent HashMap key
(let [m (java.util.HashMap.)] (.put m :a :let-go.host/not-found) (.containsKey m :a)) returns false even though the key exists; the JVM returns true. hashMapNotFound is an ordinary keyword that callers can legitimately store, so comparing the lookup result against it conflates presence with absence. Please use a unique private identity marker or check membership directly.
There was a problem hiding this comment.
Fixed in 25f0f8a. The marker is now a pointer to an unexported struct, compared by identity, so user code can't store it. hash-map-absence-marker checks that a key mapped to :let-go.host/not-found is present.
mparrett
left a comment
There was a problem hiding this comment.
Follow-up re-review at f1a6414: the ArrayList self-reference and HashMap absence-marker findings are fixed. I reran both original examples: the list renders as "[(this Collection)]" and containsKey returns true for a value of :let-go.host/not-found.
The original a* split example now matches the JVM too, but the split finding is only partially addressed: patterns containing anchors or word boundaries still drop adjacent empty matches, as detailed inline. I recommend fixing that remaining P2 issue before merging.
Rebuilt this head: build and smoke checks passed, and the JVM interop suite passed 16 tests / 133 assertions. Compared the additional split examples against JVM behavior through Babashka.
| ms := re.FindAllStringIndex(str, -1) | ||
| if strings.ContainsAny(pat, "^") || strings.Contains(pat, `\A`) || | ||
| strings.Contains(pat, `\b`) || strings.Contains(pat, `\B`) { | ||
| return ms |
There was a problem hiding this comment.
[P2] Preserve adjacent empty matches for patterns with anchors or boundaries too
The new a* case is fixed, but this early return restores the original bug whenever the pattern contains ^, \A, \b, or \B. For example, (vec (.split "abc" "a*|^" -1)) returns ["" "b" "c" ""], while the JVM returns ["" "" "b" "c" ""]; a*|\b has the same discrepancy. Both were rerun at f1a6414 and compared with Babashka. The anchor/boundary alternative does not remove the empty a* match at position 1, but falling back to Go enumeration suppresses it. Please preserve Java's match sequence with left context intact rather than excluding these patterns, and add regression cases for them. This is the remaining part of the earlier split finding.
There was a problem hiding this comment.
Fixed in 988f294, which drops the anchor/boundary exclusion. Every pattern now uses the restore path. The check for an empty match at the end of a non-empty one anchors \A(?s:.)(pat) at the rune before that position, so group 1 is the match there and its assertions see the real left context. RE2's \b, \B and (?m)^ only look back one rune, and \A/^ cannot match at a positive offset.
(.split "abc" "a*|^" -1) and the a*|\b case now give ["" "" "b" "c" ""]. I diffed 20 split cases against Babashka with no differences, covering \b, \B, (?m)^, \A, $, (?i) and a multibyte rune. Seven of them are pinned in "the restored empty match sees its left context". The branch is also rebased on main.
f1a6414 to
988f294
Compare
Clojure written for the JVM, such as the runtime the Niva compiler emits, reaches for host interop that let-go was missing, so it failed to compile or threw before running. This adds the missing statics and methods and fixes the two places where existing ones diverged from the JVM. - Long/parseLong and Integer/parseInt take a radix (it was ignored); number parses throw java.lang.NumberFormatException instead of a plain error. - Boolean/toString; Character/isLetter, isLetterOrDigit, isLowerCase, isUpperCase, isWhitespace, toLowerCase, toUpperCase and digit. - System/setProperty, overlaying getProperty/getProperties. - java.util.regex.Pattern/quote. - clojure.core/flush, sharing term/flush's implementation. - instance? clojure.lang.IAtom. - String .lastIndexOf, .codePoints, .codePointCount, .strip, .stripLeading, .stripTrailing, .isBlank and .split (regex + limit, JVM semantics). - StringBuilder .appendCodePoint and .setLength. - A real mutable java.util.ArrayList (replacing the load-only stub) plus Collections/shuffle and Collections/reverse. - Method dispatch no longer swallows an exception a host method throws by falling back to a same-named core fn: (.get list 99) used to return nil. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Node's WASI returns ENOTCAPABLE for fd_sync when stdout is a pipe, so (print ...) (flush) threw under piped output. For a file-backed *out*, swallow the errnos a no-op sync can report, as the native flushOut does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
str, format's %s and print showed #<java.lang.StringBuilder>; they now show what was built, as StringBuilder.toString does on the JVM, and pr prints the contents as a string literal. Fixes #1069. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A plain error from InvokeMethod means "not handled", so (.get l :a) fell through to clojure.core/get and returned nil, and (.remove l 0) reached clojure.core/remove. A method the shim has, called with args none of its overloads take, now throws IllegalArgumentException, and .remove takes an index (an integer) or an element, as on the JVM. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A call a host shim did not handle fell back to any let-go fn of the same name: (.get hm :a :b) returned :b through clojure.core/get, .remove on an iterator returned a fn, (.first al) returned the first element. Shims now carry a vm.HostShim marker; an unhandled call on one answers only from explicit host-method registrations and Object/Iterable methods (toString, hashCode, iterator), and otherwise throws IllegalArgumentException "No matching method ...", as Clojure does. Failures inside a shim method throw the JVM's class: NoSuchElementException for pop on an empty ArrayDeque and next past an iterator's end, UnsupportedOperationException for Iterator.remove and unsupported Thread methods, ClassCastException for HashMap.putAll of a non-map, IndexOutOfBoundsException and IllegalArgumentException for StringBuilder. Fills in the common methods that used to fall through: HashMap put, remove, containsKey, getOrDefault, size, isEmpty, clear; ArrayDeque size and clear; StringBuilder charAt and reverse. The HashMap holds its map behind a mutex rather than an atomic.Value, whose same-concrete-type rule an assoc or dissoc could break. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
putAll walked its source seq inside h.update, holding h.mu. A lazy source that read the same map re-entered the non-reentrant mutex and deadlocked the process. The entries are now collected first and assoc'd under the lock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…one, as Java does Also adds regression tests for the ArrayList self-reference and HashMap sentinel fixes, and regenerates the generated manifest and sums. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The previous fix tested for Java's adjacent empty match on the remaining text, which loses the rune before it, so patterns with ^, \A, \b or \B fell back to Go's enumeration and lost those matches: "abc" split on "a*|^" gave ["" "b" "c" ""] where the JVM gives ["" "" "b" "c" ""]. The test now anchors \A(?s:.)(pat) at the rune before the position, so group 1 is the match there and its assertions see the same left context. RE2's \b, \B and (?m)^ look back one rune, and \A and ^ cannot match at the end of a non-empty match. Every pattern now takes this path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
988f294 to
2fe6d95
Compare
mparrett
left a comment
There was a problem hiding this comment.
Rechecked 2fe6d95. The previously reported anchor/word-boundary cases now match the JVM: a*|^, a*|\b, and a*|\B preserve the adjacent empty match. Build/smoke passed, and the updated interop suite passed all 16 tests / 140 assertions.
However, the new wrapper compilation introduces one P2 regression for a valid open \Q literal quote, detailed inline. I recommend fixing it before merge. I verified that the same case works at the immediately preceding revision afb427d and on the JVM through Babashka. The earlier ArrayList and HashMap findings remain addressed.
| emptyAt := func(e int) (bool, error) { | ||
| if at == nil { | ||
| var err error | ||
| if at, err = regexp.Compile(`\A(?s:.)(` + pat + `)`); err != nil { |
There was a problem hiding this comment.
[P2] Preserve open literal quotes when wrapping the pattern
A valid regex may start a \Q literal quote without an explicit \E; the quote then extends to the end of the pattern. Concatenating a closing parenthesis here makes it part of that literal quote instead of closing the wrapper group. For example, (vec (.split "abc" "\\Qa" -1)) returns ["" "bc"] at afb427d and on the JVM, but returns ["abc"] at this head. A direct vm.String.InvokeMethod probe confirms the new helper errors with missing closing ): \A(?s:.)(\Qa) even though regexp.Compile on the original pattern succeeds and matches [0,1). The existing method fallback masks that error in the public call. Please preserve the parsed pattern's semantics when constructing the context-aware matcher, including open \Q quoting, and add this regression case.
There was a problem hiding this comment.
Fixed in a2bbee7. The pattern now goes into \A(?s:.)(…) in parsed form, as syntax.Parse(pat, syntax.Perl).String(), which always closes its quotes. An open \Q can no longer take in the wrapper's ).
(.split "abc" "\\Qa" -1) gives ["" "bc"] again. I diffed 18 more split cases against Babashka with no differences, including \Qa, \Qa), a*|\Q, \Qa\E*|^, \Q(, a named group and (?i) combined with \b. The interop suite pins three of them. You were right that the fallback hid the error, so the new pkg/vm/string_split_test.go calls split directly and fails on any matcher error. It fails at 2fe6d95 with your missing closing ) and passes at this head.
A pattern may open a \Q quote and leave it open to the end, so pasting its source into \A(?s:.)(pat) quoted the closing parenthesis and the matcher failed to compile: "abc" split on "\Qa" gave ["abc"] instead of the JVM's ["" "bc"]. The pattern now goes in as syntax.Parse(...).String(), which always closes its quotes. The Go test calls split directly, because the method fallback hides the matcher's error behind a wrong answer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mparrett
left a comment
There was a problem hiding this comment.
Rechecked a2bbee7: the remaining quoted-regex regression is fixed. Parsing the original pattern and splicing its serialized parsed form into the context matcher preserves open \Q quoting without consuming the wrapper's closing parenthesis.
Reran the reported examples against JVM behavior through Babashka: a*, a*|^, a*|\b, a*|\B, \Qa, and a*|\Q all match. The self-referencing ArrayList still renders as "[(this Collection)]", and containsKey still correctly reports a key storing :let-go.host/not-found.
Validation at this head: make build and smoke checks passed; the updated JVM interop suite passed 16 tests / 143 assertions with zero failures or errors. No remaining blockers from my review. Clearing my previous request for changes and approving.
Why
Clojure written for the JVM reaches for host interop that let-go didn't have. The motivating case is the runtime the Niva compiler emits for its Clojure backend (
niva/core.clj). Underlgit failed to compile (Can't resolve Boolean/toString,Character/isLetter,System/setProperty,Pattern/quote,Collections/shuffle,flush, …) or misbehaved at runtime (method strip not found on string,Long/parseLong "ff" 16throwing,catch NumberFormatExceptionnot catching). The only workaround was#?(:lg …)branches in the user's code.With this PR, that project's unmodified
niva/core.cljloads and runs onlg, and a script exercising its helpers prints the same output underlgandbb.What changes
Fixes to existing interop
Long/parseLongandInteger/parseInttake the optional radix. It was silently ignored, so(Long/parseLong "ff" 16)threw.parseLong,parseInt,parseFloat,parseDouble) throwjava.lang.NumberFormatException, with the JVM'sFor input string: "x"message. Before, they returned a plain Go error that typedcatchnever matched.CoreMethodInvokeno longer falls back to a same-named core fn when a host method throws (*vm.ThrownError). A plain error still means "not handled". Without this,(.get list 99)fell through toclojure.core/getand returnednilinstead of throwingIndexOutOfBoundsException.New statics
Boolean/toString.Character/isLetter,isLetterOrDigit,isLowerCase,isUpperCase,isWhitespace,toLowerCase,toUpperCase,digit. These moved withisDigitintohost_character.go. Each takes a char or an int code point, and the case mappings return whichever kind they were given, as the JVM overloads do.isWhitespacefollows Java's definition (no non-breaking spaces), which is shared asvm.JavaIsWhitespace.System/setProperty, a process-wide overlay thatgetPropertyandgetPropertiessee. It returns the previous value.java.util.regex.Pattern/quote. RE2 supports\Q…\E, so the JVM quoting carries over unchanged.java.util.Collections/shuffleandreverseon anArrayList. A persistent collection throwsUnsupportedOperationException, like the JVM's immutable lists.New methods
.lastIndexOf,.codePoints(returns anIntStreamvalue with.toArrayand.count),.codePointCount,.strip,.stripLeading,.stripTrailing,.isBlank,.split..splitfollows JVM semantics: regex separator, limit, trailing empties dropped at limit 0, and no leading empty for a zero-width match at index 0. It returns an array, likeString[]..appendCodePoint,.setLength.setLengthtruncates, or pads with NUL as Java does.Other
java.util.ArrayListis now a real mutable list:add/add(i,x),addAll,get,set,size,isEmpty,clear,contains,indexOf,toArray,toString. It is seqable and counted, sovec,seqandcountwork on it. It replaces the load-only stub, which returned a marker symbol, so medley's:cljconstructor sites still compile.clojure.core/flushflushes*out*.term/flushandclojure.core/flushnow share a per-platformflushOut, and wasip1, which has notermns, gets its own.ioandtermexclude the core name, just asioalready does forslurp/spit.instance? clojure.lang.IAtommaps to the atom type, alongsideclojure.lang.Atom.Each name is registered in exactly one place, through
ns.Def/defStaticNS. None of them also use//lg:native.Compile paths
All three paths get these the same way. The shims are runtime natives and host methods resolved by name. The bytecode compiler,
*ir-compile*andgogen_ir-lowered code all reach host methods through the sameclojure.core/.(CoreMethodInvoke) and resolve statics from the same namespaces, so no compiler or IR change is needed.make check-generatedconfirms thatcore_go_lowered/still compiles and dispatches natively.Verification
test/jvm_interop_shims_test.lgcovers every item above against the JVM's results, including radix/NFE cases, Unicode digits (Arabic-Indic, mathematical double-struck), non-breaking space,splitlimit/leading/trailing cases, and ArrayList bounds errors.make test: pass.make generate, thenmake check-generatedandmake check-generated-manifest: OK. Regeneratedgenerated.manifestandgenerated.sumsare included.CGO_ENABLED=0, as the CI job runs) and linux/amd64 build.make lintcouldn't run locally, because golangci-lint 2.14.0 fails in typecheck on Go 1.27.2 export data before linting anything. CI's lint job is the check here.go vet ./pkg/rt ./pkg/vmandgofmtare clean.echo Bob | lg -e "(require 'main) (main/-main)"prints the expected greeting.Not in this PR
lg main.cljstill doesn't call-main, becauselghas no-m. That's a CLI design question, separate from interop.str,format "%s"andprinton aStringBuildernow show its contents instead of#<java.lang.StringBuilder>, andprprints them as a string literal. Fixes #1069.The host shims (ArrayList, ArrayDeque, HashMap, StringBuilder, Iterator, Thread, IntStream) now fail the way Clojure's reflective call does. Before, a call a shim did not handle fell back to any let-go fn with the same name, so
(.get hm :a :b)returned:bthroughclojure.core/getand.removeon an iterator returned a fn. Now such a call throwsIllegalArgumentException("No matching method …"), and failures inside a method throw the JVM's exception class (NoSuchElementException,ClassCastException, …). The common HashMap, ArrayDeque and StringBuilder methods that used to fall through are implemented.🤖 Generated with Claude Code