cljr-rename-symbol results in broken call to cljr--get-valid-filename

[mitchell-horning]

Thank you for the great product. I'll try to resolve on my own, but am an Emacs debugging novice.

Expected behavior

The behavior specified in the wiki.

Actual behavior

In a new lein project, calling cljr-rename-symbol on x in the below function:

(defn foo
  "I don't do a whole lot."
  [x]
  (println x "Hello, World!"))

evokes the below error:

cljr--get-valid-filename: Wrong type argument: hash-table-p, (#s(hash-table size 7 test equal rehash-size 1.5 rehash-threshold 0.8125 data (:line-beg 5 :line-end 5 :col-beg 4 :col-end 5 :name "x" :file "/home/../emacs-test/cljr-test/src/cljr_test/core.clj" :match "[x]")) #s(hash-table size 7 test equal rehash-size 1.5 rehash-threshold 0.8125 data (:line-beg 6 :line-end 6 :col-beg 12 :col-end 13 :name "x" :file "/home/../emacs-test/cljr-test/src/cljr_test/core.clj" :match "(println x \"Hello, World!\"))")))

C-c C-k or re-running cljr-rename-symbol a second time does not fix the issue.

Steps to reproduce the problem

init.el:

(require 'package)
(add-to-list 'package-archives (cons "melpa" (concat proto "://melpa.org/packages/")
(package-initialize)

(require 'clj-refactor)

(defun my-clojure-mode-hook ()
    (clj-refactor-mode 1)
    (yas-minor-mode 1) ; for adding require/use/import statements
    (cljr-add-keybindings-with-prefix "C-c C-m"))

(add-hook 'clojure-mode-hook #'my-clojure-mode-hook)

(custom-set-variables
 '(package-selected-packages (quote (clj-refactor cider))))
(custom-set-faces)

Then call lein new cljr_test and call cljr-rename-symbol.

Environment & Version information

OpenJDK 8, 11, or 13 all seem to produce the same error.

clj-refactor.el version information

clj-refactor 2.5.0-SNAPSHOT (package:20200221.1329), refactor-nrepl 2.5.0-SNAPSHOT

CIDER version information

CIDER 0.25.0snapshot (package: 20200220.1307), nREPL 0.6.0
Clojure 1.10.0, Java 1.8.0_242

Leiningen or Boot version

Leiningen 2.9.1 on Java 1.8.0_242 OpenJDK 64-Bit Server VM

Emacs version

26.3

Operating system

Arch linux, kernel 5.5.4-arch1-1

[iarenaza]

Just hit this same issue yesterday. After debugging it a bit, the problem seems to lie in this line: https://github.com/clojure-emacs/clj-refactor.el/blob/master/clj-refactor.el#L2500

When the symbol to be renamed appears in several places, the list of locations we get from nREPL is inserted as independent maps in the temporary buffer. E.g., this is the temporary buffer that I get in the project where I hit the problem (reformatted for clarity, it's a single line in the original buffer):

{:line-beg 5,
 :line-end 7,
 :col-beg 1,
 :col-end 20,
 :file "/home/magnet/sandbox/applications/hydrogen-modules/rbac.git/src/magnet/rbac.clj",
 :name "magnet.rbac/kw->str",
 :match "(defn- kw->str\n  [k]\n  (str (symbol k)))"}

{:line-beg 67,
 :line-end 67,
 :col-beg 66,
 :col-end 73,
 :file "/home/magnet/sandbox/applications/hydrogen-modules/rbac.git/src/magnet/rbac.clj",
 :name "magnet.rbac/kw->str",
 :match "(format \"%s = ?\" (kw->str column))"}

{:line-beg 122,
 :line-end 122,
 :col-beg 29,
 :col-end 36,
 :file "/home/magnet/sandbox/applications/hydrogen-modules/rbac.git/src/magnet/rbac.clj",
 :name "magnet.rbac/kw->str",
 :match "(update :context-type kw->str)))"}

{:line-beg 159,
 :line-end 159,
 :col-beg 23,
 :col-end 30,
 :file "/home/magnet/sandbox/applications/hydrogen-modules/rbac.git/src/magnet/rbac.clj",
 :name "magnet.rbac/kw->str",
 :match "(kw->str context-type))]"}

{:line-beg 192,
 :line-end 192,
 :col-beg 29,
 :col-end 36,
 :file "/home/magnet/sandbox/applications/hydrogen-modules/rbac.git/src/magnet/rbac.clj",
 :name "magnet.rbac/kw->str",
 :match "(update :context-type kw->str)))"}

{:line-beg 227,
 :line-end 227,
 :col-beg 23,
 :col-end 30,
 :file "/home/magnet/sandbox/applications/hydrogen-modules/rbac.git/src/magnet/rbac.clj",
 :name "magnet.rbac/kw->str",
 :match "(kw->str context-type)"}

parseedn-read reads the whole temporary buffer in a single go (I stepped through the function in edebug, and verified that the (while (not (cljr--end-of-buffer-p)) ...) loop is executed only once). And it returns a list of hash-maps, with all the EDN maps in it.

edn-read on the other hand seems to read a single EDN map at a time and loops as many times as needed to read the whole temporary buffer (haven't verified this in edebug yet, but I will).

That means that while edn-read returns a single hash-map at a time, parseedn-read returns a list of hash-maps. The surrounding code assumes a single hash-map is returned, and pushes the returned value to the occurrences list. So occurrences ends up being a list of a list of hash-maps, instead of a list of hash-maps (which is what the rest of the code expects).

One quick (and very dirty) way of confirming this is changing the code from:

    (with-temp-buffer
      (insert (cljr--call-middleware-sync request "occurrence"))
      (unless (cljr--empty-buffer-p)
        (goto-char (point-min))
        (while (not (cljr--end-of-buffer-p))
          (push (parseedn-read) occurrences))))

to

    (with-temp-buffer
      (insert (cljr--call-middleware-sync request "occurrence"))
      (unless (cljr--empty-buffer-p)
        (goto-char (point-min))
        (while (not (cljr--end-of-buffer-p))
          (dolist (edn (parseedn-read))
            (push edn occurrences)))))

This fixes the issue and makes symbol renaming work as expected.

[iarenaza]

Environment & Version information

OpenJDK 8

clj-refactor.el version information

clj-refactor 2.5.0
refactor-nrepl 2.5.0

CIDER version information

CIDER 0.25
nREPL 0.6.0
Clojure 1.10.0
OpenJDK Runtime Environment (build 1.8.0_222-b10)
OpenJDK 64-Bit Server VM (build 25.222-b10, mixed mode)

Leiningen or Boot version

Leiningen 2.9.1 on Java 1.8.0_222 OpenJDK 64-Bit Server VM

Emacs version

GNU Emacs 26.1 (build 1, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars) of 2019-09-23, modified by Debian

Operating system

Debian GNU/Linux 10.1 (buster)

[iarenaza]

Just a quick note to confirm that I have verified that edn-read reads a single EDN map at a time and loops as many times as needed to read the whole temporary buffer.

So there is clearly a change in behavior from edn-read to parseedn-read, that is causing this issue.

[anonimitoraf]

Thanks for that @iarenaza . I did notice though that only every rename attempt is successful. Otherwise it fails with:
cljr-rename-symbol: Wrong type argument: char-or-string-p, nil

In any case, much much better than it not working at all

[iarenaza]

@anonimitoraf It is a bit difficult to pinpoint the code that might be producing such error (and I don't get it myself). If you could enable "debug on error", with toggle-debug-on-error and attach the resulting stack trace, that could help a bit.

[carlduevel]

I ran into the same problem and the solution above by @iarenaza fixed it. Any change this would be accepted as a PR?

[bbatsov]

The suggested fix seemed OK to me, so I applied it. Sorry for the breakage! I didn't test everything after replacing the EDN parser.