search

clojure-emacs / clj-refactor.el

A CIDER extension that provides powerful commands for refactoring Clojure code.
GNU General Public License v3.0
771 stars 111 forks source link

Error with suggest_libspecs #556

Closed chpill closed 9 months ago

chpill commented 9 months ago

Hi, thanks for this amazing package!

I've refreshed my spacemacs packages and I see an error message each time I type a namespace alias that is not already in the namespace declaration (for example, I will see the error if I type str/ and the [clojure.string :as str] is not in the ns form).

Expected behavior

Adds the correct missing libspec.

Actual behavior

Error with the following stacktrace:

cljr--get-error-value: Error in nrepl-refactor: java.lang.ClassCastException: class clojure.lang.PersistentList cannot be cast to class clojure.lang.Named (clojure.lang.PersistentList and clojure.lang.Named are in unnamed module of loader ’app’)
 at clojure.core$name.invokeStatic (core.clj:1610)
    clojure.core$name.invoke (core.clj:1604)
    clojure.core$comp$fn__5876.invoke (core.clj:2586)
    clojure.core$mapv$fn__8535.invoke (core.clj:6979)
    clojure.lang.PersistentVector.reduce (PersistentVector.java:343)
    clojure.core$reduce.invokeStatic (core.clj:6885)
    clojure.core$mapv.invokeStatic (core.clj:6970)
    clojure.core$mapv.invoke (core.clj:6970)
    clojure.core$partial$fn__5908.invoke (core.clj:2641)
    clojure.core$mapv$fn__8535.invoke (core.clj:6979)
    clojure.lang.PersistentVector.reduce (PersistentVector.java:343)
    clojure.core$reduce.invokeStatic (core.clj:6885)
    clojure.core$mapv.invokeStatic (core.clj:6970)
    clojure.core$mapv.invoke (core.clj:6970)
    refactor_nrepl.ns.suggest_libspecs$parse_preferred_aliases_STAR___102053.invokeStatic (suggest_libspecs.clj:26)
    refactor_nrepl.ns.suggest_libspecs/parse_preferred_aliases_STAR_ (suggest_libspecs.clj:18)
    clojure.lang.AFn.applyToHelper (AFn.java:154)
    clojure.lang.AFn.applyTo (AFn.java:144)
    clojure.core$apply.invokeStatic (core.clj:667)
    clojure.core$memoize$fn__6946.doInvoke (core.clj:6388)
    clojure.lang.RestFn.invoke (RestFn.java:408)
    refactor_nrepl.ns.suggest_libspecs$suggest_libspecs_response.invokeStatic (suggest_libspecs.clj:270)
    refactor_nrepl.ns.suggest_libspecs$suggest_libspecs_response.invoke (suggest_libspecs.clj:218)
    clojure.lang.Var.invoke (Var.java:384)
    refactor_nrepl.middleware$suggest_libspecs_reply$fn__78564.invoke (middleware.clj:213)
    refactor_nrepl.ns.libspec_allowlist$with_memoized_libspec_allowlist_STAR_.invokeStatic (libspec_allowlist.clj:41)
    refactor_nrepl.ns.libspec_allowlist$with_memoized_libspec_allowlist_STAR_.invoke (libspec_allowlist.clj:39)
    refactor_nrepl.middleware$suggest_libspecs_reply.invokeStatic (middleware.clj:211)
    refactor_nrepl.middleware$suggest_libspecs_reply.invoke (middleware.clj:210)
    refactor_nrepl.middleware$wrap_refactor$fn__78575.invoke (middleware.clj:244)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_resource$fn__81710.invoke (nrepl.clj:613)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_log$fn__81643.invoke (nrepl.clj:362)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_refresh$fn__81702.invoke (nrepl.clj:587)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_test$fn__81734.invoke (nrepl.clj:700)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_apropos$fn__81498.invoke (nrepl.clj:165)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_complete$fn__81514.invoke (nrepl.clj:179)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_classpath$fn__81506.invoke (nrepl.clj:173)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    nrepl.middleware.completion$wrap_completion$fn__76945.invoke (completion.clj:58)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_xref$fn__81776.invoke (nrepl.clj:774)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    nrepl.middleware.interruptible_eval$interruptible_eval$fn__76435.invoke (interruptible_eval.clj:154)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    nrepl.middleware.sideloader$wrap_sideloader$fn__77058.invoke (sideloader.clj:108)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    nrepl.middleware.load_file$wrap_load_file$fn__76977.invoke (load_file.clj:81)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_content_type$fn__81470.invoke (nrepl.clj:143)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    nrepl.middleware.session$add_stdin$fn__76553.invoke (session.clj:379)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_slurp$fn__81482.invoke (nrepl.clj:157)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_ns$fn__81678.invoke (nrepl.clj:505)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_debug$fn__81526.invoke (nrepl.clj:199)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_enlighten$fn__81548.invoke (nrepl.clj:226)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.piggieback$wrap_cljs_repl$fn__77493.invoke (piggieback_impl.clj:370)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_out$fn__81686.invoke (nrepl.clj:541)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_tracker$fn__81750.invoke (nrepl.clj:742)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_inspect$fn__81613.invoke (nrepl.clj:283)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    nrepl.middleware.caught$wrap_caught$fn__76368.invoke (caught.clj:97)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    cider.nrepl$wrap_profile$fn__81694.invoke (nrepl.clj:550)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    shadow.cljs.devtools.server.nrepl_impl$handle.invokeStatic (nrepl_impl.clj:329)
    shadow.cljs.devtools.server.nrepl_impl$handle.invoke (nrepl_impl.clj:228)
    shadow.cljs.devtools.server.nrepl$middleware$fn__77615.invoke (nrepl.clj:46)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    nrepl.middleware.print$wrap_print$fn__76335.invoke (print.clj:234)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    nrepl.middleware.session$session$fn__76538.invoke (session.clj:325)
    nrepl.middleware$wrap_conj_descriptor$fn__76099.invoke (middleware.clj:16)
    nrepl.server$default_handler$fn__77106.invoke (server.clj:141)
    nrepl.server$handle_STAR_.invokeStatic (server.clj:24)
    nrepl.server$handle_STAR_.invoke (server.clj:21)
    nrepl.server$handle$fn__77074.invoke (server.clj:41)
    clojure.core$binding_conveyor_fn$fn__5823.invoke (core.clj:2047)
    clojure.lang.AFn.call (AFn.java:18)
    java.util.concurrent.FutureTask.run (FutureTask.java:264)
    java.util.concurrent.ThreadPoolExecutor.runWorker (ThreadPoolExecutor.java:1136)
    java.util.concurrent.ThreadPoolExecutor$Worker.run (ThreadPoolExecutor.java:635)
    java.lang.Thread.run (Thread.java:833)

Steps to reproduce the problem

I see an error message each time I type a namespace alias that is not already in the namespace declaration (for example, I will see the error if I type str/ and the [clojure.string :as str] is not in the ns form).

Environment & Version information

clj-refactor.el version information

3.11.1

CIDER version information

Include here the version string displayed when CIDER's REPL is launched. Here's an example:

CIDER 1.13.0-snapshot (package: 20231127.825), nREPL 1.0.0
Clojure 1.11.1, Java 17.0.7

This was injected when jacking in

nrepl/nrepl {:mvn/version "1.0.0"}
cider/cider-nrepl {:mvn/version "0.44.0"}
refactor-nrepl/refactor-nrepl {:mvn/version "3.9.0"}

Leiningen or Boot version

Clojure CLI version 1.11.1.1413

Emacs version

GNU Emacs 28.2 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo version 1.18.0, Xaw3d scroll bars)

Operating system

Linux NixOS 23.11

chpill commented 9 months ago

I've just confirmed that the feature works as expected when I manually override the files in .emacs.d/elpa/28.2/develop/clj-refactor-20231202.445 with the files from the 3.11.0 version of this repo, and then rm *.elc files.

vemv commented 9 months ago

Thanks for the report!

What's your value of cljr-magic-require-namespaces? and cljr-slash-uses-suggest-libspec?

Does it get fixed if you remove ("io" "clojure.java.io" :only ("clj")) from the former?

chpill commented 9 months ago

cljr-magic-require-namespaces:

(("edn" . "clojure.edn")
 ("io" "clojure.java.io" :only
  ("clj"))
 ("math" . "clojure.math")
 ("set" . "clojure.set")
 ("str" . "clojure.string")
 ("walk" . "clojure.walk")
 ("zip" . "clojure.zip"))

cljr-slash-uses-suggest-libspec: t

Indeed, when I remove ("io" "clojure.java.io" :only ("clj")) from cljr-magic-require-namespaces, the issue goes away.

vemv commented 9 months ago

Indeed, when I remove ("io" "clojure.java.io" :only ("clj")) from cljr-magic-require-namespaces, the issue goes away.

Thanks!

Feel free to give it a go @dgtized

dgtized commented 9 months ago

Yea I'm seeing that now. I believe that's on the parsing side in the middleware given it's a java stacktrace though. The clj-refactor client is sending across "preferred-aliases" with prin1-to-string, which has the default value:

"((\"edn\" . \"clojure.edn\") 
  (\"io\" \"clojure.java.io\" :only (\"clj\")) 
  (\"math\" . \"clojure.math\") 
  (\"set\" . \"clojure.set\") 
  (\"str\" . \"clojure.string\") 
  (\"walk\" . \"clojure.walk\") 
  (\"zip\" . \"clojure.zip\"))"

See https://github.com/clojure-emacs/clj-refactor.el/blob/master/clj-refactor.el#L2032.

So I suspect the failure here is that the middleware is having difficulty in parse-preferred-aliases. It might be that read-string is having trouble reading back ("clj") as it's not actually a list because it's not '("clj") or (list "clj"). Alternatively, it looks like the first pass of the parser may be assuming a syntax like: ["io" "clojure.java.io" :only :clj]" and not accounting for a list of language contexts for the value of :only. However, that is the syntax we decided on in https://github.com/clojure-emacs/clj-refactor.el/issues/530#issuecomment-1221624825. Specifically, by supporting a set of contexts instead of singular context, we can future proof our syntax for something like ("io" "clojure.java.io" :only ("bb" "clj")).

Looking through the suggest_libspec tests it looks like they may all assume a single value for :only instead of a set of accepted language contexts. I'm not sure if that's just a simplification in the tests or if that is also a constraint in the implementation. If it's the latter it seems reasonable to update the middleware to accept a list, but not accept any lists with multiple values yet.

So short term, it seems reasonable to revert to the old syntax here until the middleware can parse :only statements with a list. We can't add tests to verify this here as they all mock the interface with the middleware. I think just reverting https://github.com/clojure-emacs/clj-refactor.el/pull/555/commits/8806352a64ac7207138eb990915fdd33e93218ca would be sufficient for now?

vemv commented 9 months ago

Thanks much for the accurate analysis @dgtized !

I went ahead and fixed the middleware.

I've cut a new clj-refactor.el release, to be melpa-visible within the next few hours.