search

clojure-emacs / cider

The Clojure Interactive Development Environment that Rocks for Emacs
https://cider.mx
GNU General Public License v3.0
3.54k stars 646 forks source link

Unexpected cider-undef-all outcome wrt. java.lang classes #3194

Closed codeasone closed 2 years ago

codeasone commented 2 years ago

I've hit upon an issue with cider-undef-all

Minimal context:

{:paths ["src"]
 :deps  {org.clojure/clojure {:mvn/version "1.10.3"}}}

with src/core.clj

(ns core)

(print (System/getenv "HOME"))

Custom binding I established to evaluate the new undef-all functionality:

:bind (:map cider-mode-map
            ("C-c C-k" . (lambda () (interactive)
                           (cider-undef-all)
                           (cider-load-buffer)
                           (message "Reset vars in namespace")))
            ...)

Expected behavior

Invoking C-c C-k should dwim and I should see the message in the mini-buffer.

Actual behavior

Invoking C-c C-k gives rise to:

  Show: Project-Only All
  Hide: Clojure Java REPL Tooling Duplicates  (33 frames hidden)

2. Unhandled clojure.lang.Compiler$CompilerException
   Error compiling src/core.clj at (3:8)

   No such namespace: System

                 Util.java:  221  clojure.lang.Util/runtimeException
             Compiler.java: 7388  clojure.lang.Compiler/resolveIn
             Compiler.java: 7362  clojure.lang.Compiler/resolve
             Compiler.java: 7323  clojure.lang.Compiler/analyzeSymbol
             Compiler.java: 6772  clojure.lang.Compiler/analyze
             Compiler.java: 6749  clojure.lang.Compiler/analyze
             Compiler.java: 3824  clojure.lang.Compiler$InvokeExpr/parse
             Compiler.java: 7113  clojure.lang.Compiler/analyzeSeq
             Compiler.java: 6793  clojure.lang.Compiler/analyze
             Compiler.java: 6749  clojure.lang.Compiler/analyze
             Compiler.java: 3892  clojure.lang.Compiler$InvokeExpr/parse
             Compiler.java: 7113  clojure.lang.Compiler/analyzeSeq
             Compiler.java: 6793  clojure.lang.Compiler/analyze
             Compiler.java: 6749  clojure.lang.Compiler/analyze
             Compiler.java: 6124  clojure.lang.Compiler$BodyExpr$Parser/parse
             Compiler.java: 5471  clojure.lang.Compiler$FnMethod/parse
             Compiler.java: 4033  clojure.lang.Compiler$FnExpr/parse
             Compiler.java: 7109  clojure.lang.Compiler/analyzeSeq
             Compiler.java: 6793  clojure.lang.Compiler/analyze
             Compiler.java: 7178  clojure.lang.Compiler/eval
             Compiler.java: 7640  clojure.lang.Compiler/load
                      REPL:    1  core/eval9167
                      REPL:    1  core/eval9167
             Compiler.java: 7181  clojure.lang.Compiler/eval
             Compiler.java: 7136  clojure.lang.Compiler/eval
                  core.clj: 3202  clojure.core/eval
                  core.clj: 3198  clojure.core/eval
    interruptible_eval.clj:   87  nrepl.middleware.interruptible-eval/evaluate/fn/fn
                  AFn.java:  152  clojure.lang.AFn/applyToHelper
                  AFn.java:  144  clojure.lang.AFn/applyTo
                  core.clj:  667  clojure.core/apply
                  core.clj: 1977  clojure.core/with-bindings*
                  core.clj: 1977  clojure.core/with-bindings*
               RestFn.java:  425  clojure.lang.RestFn/invoke
    interruptible_eval.clj:   87  nrepl.middleware.interruptible-eval/evaluate/fn
                  main.clj:  437  clojure.main/repl/read-eval-print/fn
                  main.clj:  437  clojure.main/repl/read-eval-print
                  main.clj:  458  clojure.main/repl/fn
                  main.clj:  458  clojure.main/repl
                  main.clj:  368  clojure.main/repl
               RestFn.java: 1523  clojure.lang.RestFn/invoke
    interruptible_eval.clj:   84  nrepl.middleware.interruptible-eval/evaluate
    interruptible_eval.clj:   56  nrepl.middleware.interruptible-eval/evaluate
    interruptible_eval.clj:  152  nrepl.middleware.interruptible-eval/interruptible-eval/fn/fn
                  AFn.java:   22  clojure.lang.AFn/run
               session.clj:  218  nrepl.middleware.session/session-exec/main-loop/fn
               session.clj:  217  nrepl.middleware.session/session-exec/main-loop
                  AFn.java:   22  clojure.lang.AFn/run
               Thread.java:  833  java.lang.Thread/run

Seems like undef-all (the use of it I've presented in any case) breaks the following:

All classes in java.lang are automatically imported to every namespace

from https://clojure.org/reference/java_interop

Environment & Version information

CIDER version information

;; CIDER 1.4.0 (Kyiv), nREPL 0.9.0
;; Clojure 1.10.3, Java 17.0.2

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-17

Operating 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/Linux

vemv commented 2 years ago

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 undef-all is the piece at fault. Couldn't it be as well (cider-load-buffer) that isn't re-importing classes as you expect?

Which could be either an actual bug, or a slightly unidiomatic choice (there might be a better approach than (cider-load-buffer)), IDK.

jumarko commented 2 years ago

I stumbled upon the same thing: https://clojurians.slack.com/archives/C0617A8PQ/p1651566926013869?thread_ts=1651525142.818839&cid=C0617A8PQ

Perhaps it's the ns macro that makes the java symbols available for the first time but doesn't re-import them after cider unmaps them?

jumarko commented 2 years ago

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

vemv commented 2 years ago

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 (remove-ns ns) so that subsequent code loadings will force triggering again the code you have linked to.

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)

vemv commented 2 years ago

@yuhan0: I see you authored undef-all https://github.com/clojure-emacs/cider-nrepl/pull/698 , maybe you could give this one a spin?

yuhan0 commented 2 years ago

Thanks! I was mucking around a little with either having undef-all filter out the classes from clojure.lang.RT/DEFAULT_IMPORTS or having load-buffer auto-import them.

Of course using remove-ns is more elegant, and semantically what the op is really about - removing all stale and hidden state from the ns without having to restart the REPL. I wondered if calling it clear-ns would have been better, but there's already the confusingly named ns-refresh and ns-reload commands.

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?

yuhan0 commented 2 years ago

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

vemv commented 2 years ago

Surely no one does things like keep reference objects in the ns metadata that would be affected by this?

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.

I'm happy to submit a PR to cider-nrepl

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 undef-all remains otherwise unbroken)

jumarko commented 2 years ago

@yuhan0

undef-all filter out the classes from clojure.lang.RT/DEFAULT_IMPORTS

I think that would be a fine solution. I guess the user don't have any option how to modify those classes anyway.

I use remove-ns every now and then but it's not without issues. The other namespaces that refer to such a removed namespace might break transitively and all of them might have to be removed and re-evalted too.

vemv commented 2 years ago

I guess the user don't have any option how to modify those classes anyway.

Users can use ns-unmap. So I'd avocate for remove-ns since it gives one a clean slate. One cannot know if users (or other tools!) used ns-unmap in a way that contradicted our expectations.

The other namespaces that refer to such a removed namespace might break transitively and all of them might have to be removed and re-evalted too.

That seems intrinsic to undef-all? Namespaces don't depend on other namespaces directly (at least off the top of my head), but on the vars of those namespaces.

And undef-all obliterates those vars, so it breaks other namespaces no matter what.

jumarko commented 2 years ago

@vemv about this

The other namespaces that refer to such a removed namespace might break transitively and all of them might have to be removed and re-evalted too.

I cannot give you a deep explanation for this but it's not related to undef-all at all. I've been using remove-ns for a few years and it sometimes works fine - such as when you use it in for a test ns that is not referred anywhere else. However, it tends to break quite often if I use it for an application namespace that is required elsewhere.

Consider this example: https://github.com/jumarko/clojure-experiments/blob/master/src/clojure_experiments/aws/logs.clj#L1

vemv commented 2 years ago

remove-ns has to be used in conjunction with (dosync (alter @#'clojure.core/*loaded-libs* disj your-ns)). Which is exactly what tools.namespace does - it performs both:

https://github.com/clojure/tools.namespace/blob/7a171416beae284c93cea159af5b41aa5f4250eb/src/main/clojure/clojure/tools/namespace/reload.clj#L15-L19

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 )

yuhan0 commented 2 years ago

I'm not sure I understand the issue or failure case here - @jumarko did you reload the clojure-experiments.stats.descriptive file after removing the ns? Or do you expect the logs ns to be able to relocate it on the classpath?

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.

yuhan0 commented 2 years ago

The few times I tried using Cider's cider-ns-refresh command it put my entire project in an unusable state and I had to restart the REPL altogether.

undef-all is just part of my primitive workflow when dealing with leftover vars or outdated defmultis / protocols hanging around in the REPL state -- just unmap everything in a ns to start it off from a clean slate, followed by a manual cider-load-file.

No dependency graphs or auto refreshing to worry about - if other namespaces are affected I just manually reload them too.

vemv commented 2 years ago

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 https://github.com/clojure-emacs/cider/issues/3194

yuhan0 commented 2 years ago

If that's the case, isn't the (doseq .. ns-unmap ns-unalias ..stuff redundant if we're removing the entire namespace? Maybe the entire op doesn't have a good reason for existing if on the frontend Cider could just send a (remove-ns (cider-current-ns)) oneliner when you do C-u cider-load-buffer, treating it as a single conceptual operation.

yuhan0 commented 2 years ago

I've been trying out this remove-ns + disj *loaded-libs* approach, but it causes even deeper problems - namespace equality appears to be reference based, so in the following example

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 my.app throws a confusing looking error 

Execution error (IllegalStateException) at my.app/eval15972$loading (REPL:1).
Alias lib already exists in namespace my.app, aliasing my.lib

because the newly loaded my.lib namespace is not referentially equal to the old aliased one. This can be verified

(= ((ns-aliases (the-ns 'my.app)) 'lib)
   (the-ns 'my.lib))
 ;; => false

I'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 undef-all op in cider-nrepl to treat the default imports specially:

(doseq [[sym ref] (ns-map ns)]
  (when-not (identical? ref (get clojure.lang.RT/DEFAULT_IMPORTS sym))
    (ns-unmap ns sym)))

This way it would unmap Math only if it refers to eg. some user-defined protocol but not if it was the the default java.lang.Math import.

jumarko commented 2 years ago

@yuhan0 that problem is exactly what I was talking about above. @vemv suggested that it has to be complemented with (dosync (alter @#'clojure.core/*loaded-libs* disj your-ns)) https://github.com/clojure-emacs/cider/issues/3194#issuecomment-1117012105 (although I didn't try that so don't know if that still poses challenges or not).

yuhan0 commented 2 years ago

Without the (dosync (alter @#'clojure.core/*loaded-libs* disj your-ns)) invocation, loading produces the error

namespace 'my.lib' not found

After executing it, loading produces a different error (if the required ns is aliased):

Alias lib already exists in namespace my.app, aliasing my.lib
jumarko commented 2 years ago

Ok, this one is what I was getting when simply calling *(remove *ns*) in the REPL and then loading the ns buffer and a buffer that required the previously removed ns: 

Alias lib already exists in namespace my.app, aliasing my.lib
vemv commented 2 years ago

I'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.

Yes, you are right: the scenario you describe is avoided because remove-ns + disj *loaded-libs* calls are performed in dependency order. Children (dependent) namespaces are removed prior to parent namespaces.

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.

A better solution might just be for the undef-all op in cider-nrepl to treat the default imports specially: [...]

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 undef-all approach.

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!

yuhan0 commented 2 years ago

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)))")
jumarko commented 2 years ago

@yuhan0 thanks for working on the fix! How exactly should I try your patch?

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 ielm and evaluated your snippet. Then I tried cider-undef-all on my ns and then tried cider-eval-buffer but it's still failing with the same problem:

1. Caused by java.lang.RuntimeException
   No such namespace: System
yuhan0 commented 2 years ago

... 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)))")))
jumarko commented 2 years ago

@yuhan0 Tested on the file mentioned above - seems to work fine.

vemv commented 2 years ago

Fix available in https://github.com/clojure-emacs/cider/blob/v1.4.1/CHANGELOG.md#141-2022-05-25