WIP Instance methods level control

[jeroenvandijk]

Please answer the following questions and leave the below in as part of your PR.

• I have read the developer documentation.

• This PR corresponds to an issue with a clear problem statement.

• This PR contains a test to prevent against future regressions

---

Sci already offers control over what functions can and can't be called, and each function can get a custom implementation. For interop there is no such feature yet. Right now, you can either call an instance method as per the original implementation or you can't (configured via :classes). Ideally, there would be a middle ground where you can offer an alternative implementation to an otherwise potentially harmful method, or in some cases deny certain methods entirely.

This is a first take at adding method level based control to Sci. It should allow to give access to functionality of configured classes while blocking other undesirable functionality, e.g. think access control of a filesystem.

The implementation in this PR allows to add a custom implementation of a method (as a function) and other methods can be denied.

E.g.

     (testing "single method override"
       (let [config {:classes {'java.lang.String
                               {:class java.lang.String
                                :instance-methods {'toString
                                                   (fn [_s]
                                                     :dude)}}}}]
         (is (= :dude (tu/eval* "(.toString \"your name\")" config)))
         (is (= 9 (tu/eval* "(.length \"your name\")" config)))))

     (testing "single method allowed"
       (let [config {:classes {'java.lang.String
                               {:class java.lang.String
                                :instance-methods {:deny true
                                                   'toString
                                                   (fn [_s]
                                                     :dude)}}}}]
         (is (= :dude (tu/eval* "(.toString \"your name\")" config)))
         (is (thrown-with-msg? Exception #"allowed"
                               (tu/eval* "(.length \"your name\")" config)))))))

I believe the overhead per method invocation of this feature to traditional paths is limited to:

(if-let [instance-methods (:instance-methods class-config)]
  <NEW-CODE>
  <OLD-CODE>)

The option :allow is checked first and if true, the rest is ignored and interop is applied directly (like before), so no overhead in that case.

The option :deny in :instance-methods could be extended to static methods, static fields and instance fields.

[jeroenvandijk]

This approach also works nicely with the new 1.12+ way of referring to an instance method:

'java.io.BufferedReader {:class java.io.BufferedReader 
                         :instance-methods 
                         {:deny true
                          'close java.io.BufferedReader/.close}}

[jeroenvandijk]

Feedback from Claude

Here's my review of PR #1027:

  PR #1027: Instance methods level control

  Concept: Adds per-method control for instance method interop — you can override individual methods with custom functions, or deny all non-listed methods. This is a useful security feature for sandboxing.

  Issues I see:

  1. Analyzer change (analyzer.cljc) is a refactor with a subtle bug

  The some-> rewrite changes behavior. Original:
  (some-> ctx :env deref :class->opts :static-methods (get ...) (get ...))
  If (-> ctx :env deref) returns nil, some-> short-circuits. The new code:
  (-> ctx :env deref :class->opts (some-> :static-methods (get ...) (get ...)))
  Now (-> ctx :env deref) is called unconditionally (which is fine — it won't be nil in practice), but deref on nil would NPE. The original some-> guarded against that. In practice :env is always present, so this is unlikely to cause issues, but
   the change is unrelated to the PR's purpose and introduces unnecessary risk. I'd drop this hunk.

  2. normal-interop closure allocated on every invocation

  In the evaluator, normal-interop is defined as a fn inside the let — this creates a new closure object on every method call. This is a hot path (every instance method invocation goes through here). The original code just had the expressions
  inline. Consider inlining the two call sites or using a macro to avoid the allocation.

  3. args are not evaluated before being passed to the custom function

  When a custom :instance-methods function is found, the PR does:
  (apply f instance-expr* args)
  But args at this point are still SCI nodes (unevaluated). The existing invoke-instance-method evaluates them internally. The custom function will receive node objects, not the actual argument values. This is a correctness bug — the tests may
  pass because .toString takes no args, but any method override with arguments would break.

  4. Method arity mismatch for overrides

  The custom function receives (instance-expr* ...args), but the PR comment/test asks "should toString also receive the class like :static-methods?" This API inconsistency should be decided before merge.

  5. :deny key lives inside the :instance-methods map alongside symbol keys

  Mixing config keys (:deny) with data keys (method symbols) in the same map is a bit awkward. A symbol can never collide with :deny, so it works, but a cleaner design might be:
  {:instance-methods {:deny-unlisted true
                      :overrides {'toString (fn [s] ...)}}}
  Or alternatively, make :instance-methods a map of {symbol fn-or-:deny} with a separate :deny-unlisted-methods key on the class config.

  6. No support for static methods, fields

  The PR description acknowledges this. It's fine for a first iteration, but the design should anticipate extending to static methods/fields.

  7. Tests use tu/eval* with when-not tu/native?

  The tests correctly guard against native mode, but per the CLAUDE.md guidelines, tests that pass custom :classes opts should use sci/eval-string directly instead of tu/eval* — this makes the intent clearer and avoids the native-mode shelling.

  Summary

  The concept is solid and useful for sandboxing. The critical issue is #3 (args not evaluated) — this will cause runtime failures for any overridden method that takes arguments. The performance concern (#2) should also be addressed given this
  is a hot path. I'd suggest the author fix the args evaluation, inline the normal-interop fn, and drop the unrelated analyzer refactor.

:black_circle_for_record: Looking at the hot path in eval-instance-method-invocation:

  1. Closure allocation on every call — The normal-interop fn is the main concern. It's created on every instance method invocation regardless of which branch is taken. On the JVM, the JIT may optimize this via escape analysis, but it's not
  guaranteed and adds GC pressure at minimum.

  2. For the :allow :all path (babashka's default config) — the overhead is minimal beyond the closure:
  - Master: (get class->opts :allow) → truthy → set target-class → call interop inline
  - PR: (get class->opts :allow) → truthy → call (normal-interop instance-class)

  Same number of lookups, just the extra closure indirection.

  3. For the per-class config path (no :allow :all) — there's one additional map lookup per call:
  - Master: (get class->opts instance-class-symbol) → truthy → call interop
  - PR: (get class->opts instance-class-symbol) → (:instance-methods class-config) → then interop

  This extra :instance-methods lookup happens even when the feature isn't used.

  Bottom line: The closure is the only real concern. The extra map lookups are negligible. You could eliminate the closure by inlining normal-interop at its 3 call sites (it's only 3 lines). For a hot-path function in an interpreter loop,
  avoiding unnecessary allocations matters — especially on GraalVM native-image where JIT escape analysis isn't available.

• I've reverted the optimization that Claude claimed could have a subtle bug. It is not needed for this PR and could be done seperately (if it is not a bug)
• I've reverted the :deny addition. Technically with this change it is possible to deny methods by overriding every method of a class. A :deny like option can be done as a seperate PR.
• I've added a test for point 4) of Claude's feedback and corrected the implementation
• The tests now use sci/eval-string instead of test.util/eval*
• I've inlined the interop calls. I can make it more dry, similar to the original change, but I think that code would be les efficient.

I'm still uncertain whether the custom functions passed via :instance-methods need to receive an extra argument like in :static-methods:

:static-methods {'forName (fn [_Class _forName] :dude)}

I don't know why this _Class argument is required for :static-methods, but maybe there is something I'm missing.

[borkdude]

I don't know why this _Class argument is required for :static-methods, but maybe there is something I'm missing.

Because a static method is called on a class. But instance methods we don't need this.

Some additional feedback:

• (symbol method-str) allocates on every call

At evaluator.cljc:162, (symbol method-str) creates a new symbol on every method invocation for the lookup. Since method-str is known at analysis time, this symbol could be pre-computed and stored alongside the other analyzed data to avoid per-call allocation in a hot path.

• Some changes affect CLJS too but there's no CLJS tests