Support clj instance field access without hyphen

[bobisageek]

Is your feature request related to a problem? Please describe.
While working on adding math.numeric-tower lib tests to babashka (babashka/babashka#1237), a handful of tests failed because numerator could not be found on clojure.lang.Ratio. Upon further investigation, it appears that the root causes of the failure are a) the Ratio class doesn't have its methods/fields exposed in bb (orthogonal to this issue), and b) the 'dot' behavior differs slightly between Clojure/JVM and sci. Specifically related to this example, in Clojure/JVM, instance fields can be accessed without the beginning hyphen, and this isn't (as far as I can tell) doable with sci.

Describe the solution you'd like
Based on my interpretation of the Clojure reference (https://clojure.org/reference/java_interop#dot), I'd say that, at least when hosted on Clojure/JVM, a two-argument call to the . special form, where the second argument doesn't begin with a hyphen, should invoke the method named by the second argument if there's a matching 0-arg method, or return the value of a public field if there's a matching field, or throw if there's no matching 0-arg method or public field.

Describe alternatives you've considered
An alternative that addresses the proximate desire of including math.numeric-tower into babashka's lib tests would be to submit an issue/patch to the library itself to use field access syntax, e.g. to change
(quot (. n numerator) (. n denominator))
to
(quot (. n -numerator) (. n -denominator))
in all the cases of instance field access.

Additional context
A small example to illustrate the behavioral difference, using the numerator public field on clojure.lang.Ratio, with the current head revision of Sci:

Clojure/JVM:

$ clojure -M
Clojure 1.10.3
user=> (. 4/5 -numerator)
4
user=> (. 4/5 numerator)
4

Sci (running on Clojure/JVM):

$ clojure -M
Clojure 1.10.3
user=> (require '[sci.core :as sci])
nil
user=> (def opts {:classes {:allow :all}})
#'user/opts
user=> (sci/eval-string "(. 4/5 -numerator)" opts)
4
user=> (sci/eval-string "(. 4/5 numerator)" opts)
Execution error (IllegalArgumentException) at sci.impl.Reflector/invokeMatchingMethod (Reflector.java:132).
No matching method numerator found taking 0 args for class clojure.lang.Ratio

[borkdude]

Is this only a problem in the tests? Is there a reason why those tests didn't use (numerator 1/2) instead?

[bobisageek]

I'm sorry - I should've made the actual location of the accesses clearer...

The actual function implementations use the dot accessor in this way:
https://github.com/clojure/math.numeric-tower/blob/97827be66f35feebc3c89ba81c546fef4adc7947/src/main/clojure/clojure/math/numeric_tower.clj#L170

and then the tests use (for example) the floor function:
https://github.com/clojure/math.numeric-tower/blob/97827be66f35feebc3c89ba81c546fef4adc7947/src/test/clojure/clojure/math/test_numeric_tower.clj#L73

So the tests don't directly do the problematic access, but they call the functions that do the problematic access. This would mean that the library would be "incompatible" (used very loosely), because something calling (floor 4/3) from a babashka repl would encounter the same exception.

As for the reason, I can only imagine that the code in question has been stable for a long time, maybe even before the hyphen for field access was added (that's purely speculative, of course), and just hasn't been updated.

[borkdude]

@bobisageek I "recently" (not sure how long ago but not too long ago) added support for looking up fields in classes. I decided to purely do this based on syntactic grounds to dispatch on whether to look up a field or a method. But there might be a way to support both without sacrificing too much performance.

There is a path to do so here:

https://github.com/clojure/clojure/blob/35bd89f05f8dc4aec47001ca10fe9163abc02ea6/src/jvm/clojure/lang/Reflector.java#L444

I would use that path in sci.impl.interop if it weren't complicated by GraalVM reflection and type hints. But since SCI has its own version of clojure.lang.Reflector, named sci.impl.Reflector, we could maybe add the target class in there. Getting late here to dive into details, but these are some pointers.

[bobisageek]

I'll at least take a look and see how lost I get. :)

[borkdude]

The reason we use a different path than Clojure for implementing reflection is that the reflection information in a native image can be limited or even absent. So we're using type hints (or a function called :public-class) to start reflecting at the right class when they are present in the code. Which lead to the current approach.

[borkdude]

@bobisageek One thought I've been having lately is that SCI could support :instance-call to provide "intrinsics" for certain method calls. E.g. for (.length x) the user can provide :instance-call {'length (fn [x] (if (string? x) (.length ^String x) ...))}.
Since field access without hyphen is supported in Clojure, but quite unusual, we could then provide an :instance-call intrinsic for enumerate and then just call the enumerate function on the Ratio (if it's a ratio).

[bobisageek]

A thing I was playing with (and thinking about if there was a better approach) was to have sort of a special case in interop/invoke-instance-method:

sci/src/sci/impl/interop.cljc

Lines 38 to 39 in e8aa6cf

	(let [methods (Reflector/getMethods target-class (count args) method false)]
	(Reflector/invokeMatchingMethod method methods obj (object-array args)))))]))

to something like:

(let [methods (Reflector/getMethods target-class (count args) method false)]
            (if (and (empty? args) (empty? methods))
              (invoke-instance-field obj target-class method)
              (Reflector/invokeMatchingMethod method methods obj (object-array args))))

In my mind, this would be somewhat analogous to the functionality implemented in invokeNoArgInstanceMember (https://github.com/clojure/clojure/blob/35bd89f05f8dc4aec47001ca10fe9163abc02ea6/src/jvm/clojure/lang/Reflector.java#L425), but without the extra reflective call to getClass.

The plus is that dot access on instance members would "just work" like clojure. My primary concern with that bit is how to handle if target-class is nil. It looks like it can't currently come up because eval-instance-method-invocation would have already thrown if target-class was nil (all these statements are based on the clj conditional, because cljs require the hyphen), but other things could call that fn eventually.

I did go digging around into the invokeInstanceMember function, but I didn't find it being called from anywhere, and I read it as giving preference to a field over a method.

[borkdude]

That's certainly interesting! Minor thing: instead of empty? we could call the cheaper zero? based on the count of the args which we already calculate.

[borkdude]

PR welcome to explore that option.

[bobisageek]

I'm experimenting with some tests for this (wrapping my head around the :public-class setup), and hopefully I'll have a PR ready over the weekend.