Avoid illegal reflective access

[vemv]

alexmiller wrote on Clojurians Slack:

yeah, this kind of code is not valid in 17 without setting additional jvm flags

https://github.com/nrepl/nrepl/blob/8223894f6c46a2afd71398517d9b8fe91cdf715d/src/clojure/nrepl/middleware/interruptible_eval.clj#L32-L40

Which relates to actual issues that users have experienced. I too think I have experienced an issue directly related to LineNumberingPushbackReader hacking, which is undone somewhere in my personal setup.

[bbatsov]

How can you undo this? Without it there would be no location metadata associated with eval, which is massive regression.

[frenchy64]

Perhaps use tools.reader's SourceLoggingPushbackReader and read instead?

[vemv]

Sounds nice to me, thanks!

Can nrepl include 3rd party dependencies? @bbatsov

[bbatsov]

Not really. We'll have to obfuscate them somehow (e.g. with MrAnderson) and my experience on cider-nrepl has made me hate dependency obfuscation. My strong preference would be to keep nREPL clean in this regard.

[vemv]

With a bit of luck we can inline only the relevant code (by hand, copy-paste, without an automated ns prefix) which might mean only one extra ns or such?

[bbatsov]

That'd be fine by me.

[alexander-yakushev]

I'm confused because this particular code doesn't transgress JDK reflection limitations.

$ clj -Srepro                                                                                                                                            ~
Clojure 1.11.3
user=> (System/getProperty "java.version")
"22"
user=> (let [reader (clojure.lang.LineNumberingPushbackReader. (java.io.StringReader. "(+ 1 2)"))
      field (doto (.getDeclaredField clojure.lang.LineNumberingPushbackReader "_columnNumber")
              (.setAccessible true))]
  (.set field reader (int 42))
  (.getColumnNumber reader))
42

This is because clojure.lang.LineNumberingPushbackReader lives in the "unnamed" module, same as nREPL.

[bbatsov]

@alexander-yakushev Is there any action you'd recommend? I'm concerned that once this reflection breaks the impact will be quite severe, as code navigation will stop working.

[alexander-yakushev]

I think that if JDK ever tightens the reflection access further (disabling all reflective access even within the same module), there will be much much more broken stuff, so we can relax about this for now.

If that ever happens, then, in this particular case, we can pad the input code with spaces to start on the desired column 😆.

[vemv]

In case the context was lost, the practical problem is:

Exception in thread "nREPL-worker-4" java.lang.reflect.InaccessibleObjectException: Unable to make field protected java.io.Reader java.io.FilterReader.in accessible: module java.base does not "opens java.io" to unnamed module @2eb231a6

Which presumably was specifically triggered by (.setAccessible true)

I don't experience these myself - or at least not in recent times.

Perhaps there's been some favorable evolution in JDKs, I don't know.

We also can add --add-opens (Enrich does this for other purposes).

[alexander-yakushev]

Exception in thread "nREPL-worker-4" java.lang.reflect.InaccessibleObjectException: Unable to make field protected java.io.Reader java.io.FilterReader.in accessible: module java.base does not "opens java.io" to unnamed module @2eb231a6

This exception has no relation to the code that you referenced. We're not trying to access java.io.FilterReader.in, it looks like Alex made a false connection.

[vemv]

Here is another stacktrace

https://clojurians.slack.com/archives/C0DF8R51A/p1641289714169100

Exception in thread "nREPL-worker-4" java.lang.reflect.InaccessibleObjectException: Unable to make field protected java.io.Reader java.io.FilterReader.in accessible: module java.base does not "opens java.io" to unnamed module @2eb231a6
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
	at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
	at nrepl.middleware.interruptible_eval$set_line_BANG_.invoke(interruptible_eval.clj:33)
	at nrepl.middleware.interruptible_eval$source_logging_pushback_reader.invoke(interruptible_eval.clj:50)
	at nrepl.middleware.interruptible_eval$evaluate$fn__742.invoke(interruptible_eval.clj:95)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.core$apply.invokeStatic(core.clj:667)
	at clojure.core$with_bindings_STAR_.invokeStatic(core.clj:1977)
	at clojure.core$with_bindings_STAR_.doInvoke(core.clj:1977)
	at clojure.lang.RestFn.invoke(RestFn.java:425)
	at nrepl.middleware.interruptible_eval$evaluate.invoke(interruptible_eval.clj:84)
	at nrepl.middleware.interruptible_eval$interruptible_eval$fn__787$fn__790.invoke(interruptible_eval.clj:220)
	at nrepl.middleware.interruptible_eval$run_next$fn__782.invoke(interruptible_eval.clj:188)
	at clojure.lang.AFn.run(AFn.java:22)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
	at java.base/java.lang.Thread.run(Thread.java:831)

It points to interruptible_eval.clj:33 so that seems pretty clear.

At the same time, I don't necessarily believe we should fix anything at the moment - not in the absence of a reproducible bug.

[alexander-yakushev]

It points to interruptible_eval.clj:33 so that seems pretty clear.

The code you reference in the issue is about setting the column — which works fine. The last stacktrace fails at setting the line (repl.middleware.interruptible_eval$set_line_BANG_) – which currently doesn't involve reflection at all, because LineNumberingPushbackReader.setLineNumber is public. The stacktrace seems to come from nREPL before this commit: https://github.com/nrepl/nrepl/commit/0799465734207b36a1bada9f0ea3ad1ca695e4ee

[vemv]

We're all set then - thanks!