Unexpected cider-undef-all outcome wrt. java.lang classes #3194
Comments
|
Thanks for the issue! This is what undef does: https://github.com/clojure-emacs/cider-nrepl/blob/0310f1cdb1151e980147d75baa04520ec5e358e8/src/cider/nrepl/middleware/undef.clj#L30-L38 Not sure if Which could be either an actual bug, or a slightly unidiomatic choice (there might be a better approach than |
|
I stumbled upon the same thing: https://clojurians.slack.com/archives/C0617A8PQ/p1651566926013869?thread_ts=1651525142.818839&cid=C0617A8PQ Perhaps it's the |
|
It's the clojure.lang.Namespace constructor that automatically adds mappings for java.lang classes: https://github.com/clojure/clojure/blob/b1b88dd25373a86e41310a525a21b497799dbbf2/src/jvm/clojure/lang/Namespace.java#L34 |
|
Good find, thanks! Looks like https://github.com/clojure-emacs/cider-nrepl/blob/0310f1cdb1151e980147d75baa04520ec5e358e8/src/cider/nrepl/middleware/undef.clj#L30-L38 could perform a It does work, from what I see in the repl. Anyone willing to give that a spin is welcome to hack on cider-nrepl (see https://github.com/clojure-emacs/cider-nrepl/tree/c0efb99c3cb8fe553d68125ed61090a147042d89#using-the-makefile) and give it a QA session. If you see it working with your local CIDER, PR welcome. (I don't have that much time nowadays to do it myself) |
|
@yuhan0: I see you authored undef-all clojure-emacs/cider-nrepl#698 , maybe you could give this one a spin? |
|
Thanks! I was mucking around a little with either having Of course using It's not too much a stretch I think for a command named "undef-all" to also delete the ns, after all it should be totally empty by the time the command is done. Surely no one does things like keep reference objects in the ns metadata that would be affected by this? |
|
I'm happy to submit a PR to cider-nrepl, just trying to make sure there won't be any further unintended consequences - evidently I don't use Java interop enough to have noticed this obvious bug in all the time I've been using it locally |
Yeah it would seem a highly unidiomatic thing to do. CIDER in particular always performs a fresh (non-cached) query for things like ns/var metadata since that is extremely fast in an already-running JVM.
Yes, please go ahead 🙏 it would be greatly appreciated if you can repro now and then check that the issue is gone with the PR (and that |
I think that would be a fine solution. I use |
Users can use
That seems intrinsic to And undef-all obliterates those vars, so it breaks other namespaces no matter what. |
|
@vemv about this
I cannot give you a deep explanation for this but it's not related to Consider this example: https://github.com/jumarko/clojure-experiments/blob/master/src/clojure_experiments/aws/logs.clj#L1
|
|
In all fairness, I didn't say that at first so thanks for the scrutiny! (That would be a good thing to add to the PR + QAing, @yuhan0 ) |
|
I'm not sure I understand the issue or failure case here - @jumarko did you reload the I hope I'm not just reinventing things already found in c.t.n or elsewhere, but must admit I've never used it or any of the various "reloaded" workflows, or understood what they were doing beneath the covers. |
|
The few times I tried using Cider's
No dependency graphs or auto refreshing to worry about - if other namespaces are affected I just manually reload them too. |
|
Reimplenting tools.namespace would indeed not make a lot of sense. People interested in it can just use it. I simply quoted https://github.com/clojure/tools.namespace/blob/7a171416beae284c93cea159af5b41aa5f4250eb/src/main/clojure/clojure/tools/namespace/reload.clj#L15-L19 as a reliable source of how to completely obliterate a ns. I think the workflow you describe should keep working with that additional cleanup, while also solving #3194 |
|
If that's the case, isn't the |
|
I've been trying out this src/my/lib.clj(ns my.lib)
(def foo 0)src/my/app.clj(ns my.app
(:require [my.lib :as lib]))
;; Run this, then attempt to eval the `ns` form again
(do
(remove-ns 'my.lib)
(dosync (alter @#'clojure.core/*loaded-libs* disj 'my.lib)))
After removing the my.lib namespace, now trying to load because the newly loaded (= ((ns-aliases (the-ns 'my.app)) 'lib)
(the-ns 'my.lib))
;; => falseI'm pretty sure clojure.tools.namespace and the reloaded workflows are designed around these issues, with all their apparent complexity that I wanted to avoid. A better solution might just be for the This way it would unmap |
|
@yuhan0 that problem is exactly what I was talking about above. |
|
Without the After executing it, loading produces a different error (if the required ns is aliased): |
|
Ok, this one is what I was getting when simply calling |
Yes, you are right: the scenario you describe is avoided because Here the use case is different - one wants to clear one namespace without clearing parent or children namespaces. So indeed it seems fine to use a different technical choice.
Sounds good. The PR will be exactly as good as your QAing - just make sure to try out various scenarios, describe them in the PR, and maybe draft some CIDER docs users how the "workflow" would look under the If there are limitations (which seems perfectly acceptable), it will be extremely useful that those are understood/documented upfront , so that neither users or maintainers find themselves chasing false leads in a future. Hope it helps! |
|
I made a PR to fix this issue in cider-nrepl - could anyone in this thread help with testing the change in their own workflows? The change seems innocent but I would like to avoid breaking anything else by accident again. Here's a elisp snippet to monkey-patch the change: (cider-sync-tooling-eval "(do
(require 'cider.nrepl.middleware.undef)
(in-ns 'cider.nrepl.middleware.undef)
(defn undef
\"Undefines a symbol.
When `sym` is unqualified, it is interpreted as both an alias and var to be
unmapped from the namespace `ns`.
When qualified (eg. `foo/bar`), it is interpreted as a var to be unmapped in
the namespace `foo`, which may be an alias in `ns`.\"
[{:keys [ns sym symbol]}]
(let [ns (misc/as-sym ns)
sym (or sym symbol) ;; for backwards compatibility
[sym-ns sym-name] ((juxt (comp misc/as-sym namespace) misc/name-sym)
(misc/as-sym sym))]
(if sym-ns
;; fully qualified => var in other namespace
(let [other-ns (get (ns-aliases ns) sym-ns sym-ns)]
(ns-unmap other-ns sym-name))
;; unqualified => alias or var in current ns
(do (ns-unalias ns sym-name)
(ns-unmap ns sym-name)))
sym)))") |
|
@yuhan0 thanks for working on the fix! I did this on my example that has been failing: https://github.com/jumarko/clojure-experiments/blob/master/src/clojure_experiments/security/encryption.clj#L48 I called |
|
... I copied the wrong function, agh. Here's the correct one: (cider-sync-tooling-eval
"(do
(require 'cider.nrepl.middleware.undef)
(in-ns 'cider.nrepl.middleware.undef)
(defn undef-all
\"Undefines all symbol mappings and aliases in the namespace.\"
[{:keys [ns]}]
(let [ns (misc/as-sym ns)]
;; Do not remove the default java.lang imports, as they are not relinked on the next load
;; see https://github.com/clojure-emacs/cider/issues/3194
(doseq [[sym ref] (ns-map ns)]
(when-not (identical? ref (get clojure.lang.RT/DEFAULT_IMPORTS sym))
(ns-unmap ns sym)))
(doseq [[sym _] (ns-aliases ns)]
(ns-unalias ns sym))
ns)))")Note that the change doesn't restore any unlinked classes, only prevent them from being unlinked, so you'll have to restart the repl. Add it to your cider-connected-hook if you wish, so it gets eval'ed automatically on connect / jack-in : (add-hook 'cider-connected-hook
(lambda ()
(cider-sync-tooling-eval
"(do
(require 'cider.nrepl.middleware.undef)
(in-ns 'cider.nrepl.middleware.undef)
(defn undef-all
\"Undefines all symbol mappings and aliases in the namespace.\"
[{:keys [ns]}]
(let [ns (misc/as-sym ns)]
;; Do not remove the default java.lang imports, as they are not relinked on the next load
;; see https://github.com/clojure-emacs/cider/issues/3194
(doseq [[sym ref] (ns-map ns)]
(when-not (identical? ref (get clojure.lang.RT/DEFAULT_IMPORTS sym))
(ns-unmap ns sym)))
(doseq [[sym _] (ns-aliases ns)]
(ns-unalias ns sym))
ns)))"))) |
|
@yuhan0 Tested on the file mentioned above - seems to work fine. |
I've hit upon an issue with
cider-undef-allMinimal context:
with
src/core.cljCustom binding I established to evaluate the new
undef-allfunctionality:Expected behavior
Invoking
C-c C-kshoulddwimand I should see the message in the mini-buffer.Actual behavior
Invoking
C-c C-kgives rise to:Seems like
undef-all(the use of it I've presented in any case) breaks the following:from https://clojure.org/reference/java_interop
Environment & Version information
CIDER version information
Emacs version
GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.20, cairo version 1.16.0) of 2022-03-17Operating system
Linux mark-pc 5.13.0-39-generic #44~20.04.1-Ubuntu SMP Thu Mar 24 16:43:35 UTC 2022 x86_64 x86_64 x86_64 GNU/LinuxThe text was updated successfully, but these errors were encountered: