Revisit reify "one class with all interfaces"

[borkdude]

Implementing reify has the restriction on GraalVM that we cannot generate new Java classes at runtime. So we have to do this at compile time.

Previous approach: generate classes for each subset of supported interfaces.
Problem: combinatorial explosion of combinations.

Current approach: one class that implements all interfaces which then "proxies" to the user-defined functions, or throws an exception "not implemented".

The current approach has the problem that all reified objects are "Seqable", "clojure.lang.IFn", etc. But when passing this to functions like seqable? or ifn? you will always get true, which might result in some very weird bugs.

$ clojure -M:babashka/dev -e '(ifn? (reify))'
true

Possible solutions:

• Patch all core functions that are affected by this. This would still not be enough, since there might be native functions in babashka which do the instance checks manually
• ...

[wilkerlucio]

Thinking here, patching would have an issue with third-part code, would be unsustainable for Babashka to patch those.

[borkdude]

Yes, that's what I meant with "since there might be native functions in babashka which do the instance checks manually".

As a last resort, we could make a preselection of common groups. Most reify usage only uses one class at a time, with exceptions like your smart map case. So we could make groups for individual Java classes like FileFilter or whatnot and then the collection-like interfaces in one group which you then must all implement. Then we are basically back to the previous approach, but with less groups.

[borkdude]

If we go with preselected groups, for e.g. smart maps, and pathom3 is using this, but meanwhile babashka decides to add one more interface to this group, then pathom3 will break. So the "last resort" might not be feasible either.

I'm starting to think it might be better to drop all support for reify, except for protocols, unless we have a better answer.

/cc @GreshamDanielStephens

[GreshamDanielStephens]

Been thinking but haven't got much with promise.

• Seems like there was some progress on dynamic classes in graal which I guess means this problem might go away entirelyat some point, but it hasn't been worked on in a bit, I will have a look but I think it's pretty beyond my skill set currently! Support dynamic class loading oracle/graal#2442

• I don't really understand babashka pods well, my limited understanding is that you can add binary dependencies at runtime, would it be possible to make reify look like previous with the lookup factory by classes, and those classes could be supplied via a pod? This still isn't great but it does mean if a reify is really needed, a user could compile some pod separately for their classes and pull them in. EDIT - I guess this might be more possible with a standard dependency? I'll probably re-read this tomorrow and wonder what I was talking about, my mind is melting!

• Something AOP compile time-esque 🤮 using aspectjtools perhaps to rewrite what Class.isInstance does.

😬 nothing great, I'll keep thinking.

[borkdude]

Looking at the Graal issue:

A separated configuration file META-INF/native-image/dynamicClassLoading-config.json is used to store dynamically generated class information.

Doesn't this also mean you have to decide which classes you want to create dynamically at compile time?

About pods: this won't help much, because all pods do is send data back and forth. You can't create new classes from some data. If this were true, then we would have already solution.

Maybe it's good to think about the impact of potential bugs. The current approach leads to false positives with instance checks in core or other library functions. And this could be followed by a potential "not implemented" exception at runtime.

Is this worse than not supporting reify at all?

[GreshamDanielStephens]

Yeah, I do agree that it's not worth supporting in the current state for the bug risk.
Just throwing some ideas around in case anything would allow it to live 😆 I guess supporting the 'all combinations of a select few classes' is still viable and does allow their combinations with protocols safely right? But not scalable.

(Re-reading, agree that the graal PR wouldn't help)

[borkdude]

I posted this in the native-image channel of the GraalVM slack community:

I guess the answer is no, but I can try anyway: I have the following problem. In Clojure you can (reify Interface1 Interface2 ...) at runtime. This will generate an object which implements the given interfaces (I'm leaving out the methods for brevity). So when you do (instance? Interface1 obj) this returns true (obj is the result of reify here).
My problem is: I have a Clojure interpreter (non-Truffle) that wants to support this. The workaround for not being able to define classes at runtime in a native-image I've chosen is:
Define a compile time function which will create an object that implements a fixed set of interfaces and at runtime we will check in the interpreter, which classes were actually passed to the interpreter's implementation of reify and the interpreter's version of instance? will then use that information.
The problem however is, if I pass that object so some other (natively compiled) library function, this won't use the interpreter's version of instance, so basically any reified object will return true for all the interfaces that were in the fixed list.
My question: is there a trick to "disable" an interface on an object instance after the fact, either in GraalVM only or on the JVM in general?

[borkdude]

Thought from Slack:

4:18 PM
nilern I guess you could patch clojure to replace instanceofs with something that has enough indirection but that does not feel very promising either
4:19 PM
borkdude oooh haha, I could patch the Clojure that goes into babashka's compiled artifact. Hmm, that would be a major hack.
4:19 PM
but even if I would do that, I would have to patch any other library that is bundled with babashka as well, if they do manual instance checks. not going there

[borkdude]

The direction I want to go next:

Support code that was supported before 0.2.13: clj-commons/fs uses reify with one interface.

In general I want to support just one interface at a time for now since that is the most common use case of reify and is better than having nothing.

For pathom3, I want to investigate if we can add proxy + APersistentMap like described in this blog post:

https://blog.wsscode.com/guide-to-custom-map-types/

[borkdude]

Another thought:

Note that proxy objects in Clojure can also give false positives when it comes to instance:

user=> (ifn? (proxy [clojure.lang.APersistentMap] []))
true

in the sense that they return true for instance checks while not all methods are implemented.

user=> ((proxy [clojure.lang.APersistentMap] []) 1)
Execution error (UnsupportedOperationException) at user.proxy$clojure.lang.APersistentMap$ff19274a/valAt (REPL:-1).
valAt

This is similar to our reify problem. If this can already happen in Clojure, maybe it's not too bad that we have this problem in sci? Admittedly, the false positives for a reified object in sci are way bigger than this case.

[wilkerlucio]

Thank you very much for digging into this.

Do you think we could also have a way to make a custom MapEntry? I don't see an abstract protocol for that, this is why I wonder.

[borkdude]

@wilkerlucio Actually I think it might be better to migrate to proxy for your use case and then I will deprecate the "reify all interfaces at the same time" approach.

See babashka master:proxy + APersistentMap:

$ bb -e "(proxy [clojure.lang.APersistentMap] [] (seq [] (seq {:a 1 :b 2})))"
{:a 1, :b 2}```

More methods need to be added manually in the config at `babashka.impl.proxy` to support more methods, but it seems this might be the best approach forward.