orchard.java/class-info class initialization breaks JavaFX

[jpmonettas]

Currently you can't develop JavaFX applications with the latest versions of Cider because orchard.java/class-info tries to initialize JavaFX classes without the toolkit already initialized. More of a problem is that initializing any JavaFX class before the toolkit has been initialized breaks JavaFX state, so just catching the Exception leaves the system broken.

Changing that line into :

(Class/forName (str class) false)

so it doesn't initialize the class doesn't fix the issue, because there is code further down the road that will "touch" the class and initialize it anyway.

Here is a minimal repo with instructions that doesn't run with Cider https://github.com/jpmonettas/cider-nrepl-bug-repro/

[jpmonettas]

Maybe we could somehow provide a banlist of packages which class-info should skip?

[vemv]

Maybe we could somehow provide a banlist of packages which class-info should skip?

Yes, please refer to https://github.com/clojure-emacs/orchard/pull/235#issuecomment-2049995452 for the suggested approach

If that's too much of an ask, we can also hardcode that specific class for now - that class never works anyway so a more comprehensive fix would be superset of the smaller fix.

Thanks!

[jpmonettas]

Maybe we can hardcode skip the javafx package and go for a more general solution if there is ever another problem with this? I feel it is a super niche issue to introduce a new file. Wdyt?

[vemv]

I don't know - there are all sorts of exotic Java classes so a file would seem best to me.

That, or a System property edn-encoding a list of classes, but it would seem cumbersome.

[jpmonettas]

So list of classes wouldn't work unless they are prefixes of some kind. On javafx for example almost any class can break it (any control class at least)

[vemv]

it seems fine to accept packages and classes, separately

e.g. orchard.edn with {:class-info-banlist {:classes ,,, :packages ,,,}}

[jpmonettas]

I figured that for JavaFX adding dev/user.clj with :

(ns user
  (:import [javafx.application Platform]))

(Platform/startup (fn [] (println "JavaFX toolkit initialized")))

solves this issue. Which is a better solution for me than banning it since I get the class info for all javafx packages and classes.

[vemv]

Awesome!

Let's leave this issue open for a while since users can encounter it independently and it won't be obvious to them what's going on.

Suggestions welcome.

Are all cider+flowstorm users affected?

[bbatsov]

Perhaps we should also document the JavaFX workaround in the README (e.g. under "Caveats")?

[jpmonettas]

Are all cider+flowstorm users affected?

This wasn't affecting FlowStorm users, just developing FlowStorm with the latest cider was crashing.

[vemv]

Perhaps we should also document the JavaFX workaround in the README (e.g. under "Caveats")?

Yes. Although it's also true that cider-nrepl breaks altogether, which we should prevent from happening in the first place.

If we don't come up with something better (e.g. dyamically, non-obstrusively detecting whether Javafx has been initialized), I'd prevent analysis of that package unless the user has said it has initialized it (e.g. pass a System prop, as a hint that initialization is there).

Maybe we can try slurping user.clj but it would be pretty dubious 🤠

[bbatsov]

I don't know if we should invest too much efforts into this given how niche JavaFX is these days. Might be easiest to just hardcode a workaround for this or something along those lines.

[alexander-yakushev]

If I understand correctly, the problem happens because cider-nrepl tries to initialize classes that the user didn't ask it to. Regardless of whether JavaFX is to blame here, I can imagine other instances where the user wouldn't want classes being initialized out of order, and passing false to Class/forName is not a reliable safety net (as demonstrated by this issue).

So, how important warming those caches is? If it only costs +0.5 seconds for the user on first access versus potentially breaking something, I'd say that this UX optimization is not worth it. Or, at least, limit the warmup to JDK base and Clojure classes. Or is there something else that will stop working if the warmup doesn't happen?

[vemv]

Hi, sorry, I'd really appreciate the discussion not going off rails. I've already spent a lot of time leaving this in a usable and performant manner for a wide variety of use cases.

Now all that's being discussed is the flavor of small hardcoding to be applied.

The long-term solution is https://github.com/clojure-emacs/orchard/issues/211 and I have set time in my calendar to specifically work on it May and June.

Thanks - V

[alexander-yakushev]

I've already spent a lot of time leaving this in a usable and performant manner for a wide variety of use cases.

Just wondering, does it require enrich-classpath? Because I'm not sure the work that it is supposed to be doing works for me, I have plain-old "Not documented." strings for methods and classes. It's supposed to have javadocs, right?

[vemv]

Yes, without Enrich, Orchard won't have access to javadocs that can be parsed

(btw, Enrich had some bumpy beginnings but I'd encourage you to try it out - I hear good user reports from a variety of people and much fewer bug reports. The inspector also becomes better with those javadocs)