alexander-yakushev / compliment

Clojure completion library that you deserve
Eclipse Public License 1.0
355 stars 38 forks source link

utils/classes-on-classpath can return non-classes #105

Open vemv opened 1 year ago

vemv commented 1 year ago

Context

In refactor-nrepl I use utils/classes-on-classpath for building a certain feature.

I do believe the issue is practical for Compliment users as well.

Brief

classes-on-classpath can return classes that cannot be imported. They certainly look like classes, but they cannot be imported into the environment, so they seem useless as completion candidates.

Examples

("org.graalvm.compiler.hotspot.management.HotSpotGraalManagement"
 "jdk.tools.jaotc.AOTBackend"
 "sun.jvm.hotspot.BsdVtblAccess"
 "com.sun.java.swing.ui.CommonMenuBar")

Repro

(->> (into []
             (comp (keep (fn [class-name]
                           (when-not (or (-> class-name (string/replace "." "/") (str ".java") io/resource)
                                         (-> class-name (string/replace "." "/") (str ".class") io/resource))
                             class-name)))
                   (distinct))
             (reduce into [] (vals (dissoc (compliment.utils/classes-on-classpath)
                                           ""))))
       (group-by first )
       vals
       (map (partial take 5)))

I don't exactly know atm what these classes have in common, perhaps something comes to mind to you?

Cheers - V

alexander-yakushev commented 1 year ago

Yeah, doesn't seem like there is an adequate way to remove those without hardcoding a blacklist. Let the issue stay open for the future.

vemv commented 1 year ago

Isn't checking for a io/resource non-nil result enough?

It's what I'm doing for the time being https://github.com/clojure-emacs/refactor-nrepl/blob/7cc8365fa910e74e4c6d2d85f4d3d11f8c7e6618/src/refactor_nrepl/ns/class_search.clj#L12-L24 , I still get plenty of results

(Disclaimer: I don't know much about the Java module system, which perhaps has something to do here?)

alexander-yakushev commented 1 year ago

Hmm, I assumed that it would be too slow to do that for all the tenth of thousands of classes on the classpath. Apparently, that only adds ~60 ms for the 13000 classes I've just tested on. Looks acceptable, given that it is only done once.

alexander-yakushev commented 1 year ago

Want to make a PR? Or I can fix it later.

vemv commented 1 year ago

Can do.

I'm also waiting for a verdict on the current PR 🤝