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

`cljr--point-in-reader-conditional-branch-p` may block and is incorrect #533

Closed dgtized closed 1 year ago

dgtized commented 2 years ago

Extracted https://github.com/clojure-emacs/clj-refactor.el/pull/532#discussion_r965202125

In certain conditions cljr--point-in-reader-conditional-branch-p appears to block infinitely. It's not clear if it's in this method, or in cljr--goto-reader-conditional but the example tests in #532 introduce some test cases involving reader conditionals with :default, :bb, :cljr or missing a path that result in an infinite loop in the test.

I believe the issue here is this API is asserting we are on a specific path and returning nil if not. A better API would return nil if not in a reader conditional, or the language context of the reader conditional if we are. The revised method should handle all reader conditional cases for both #? and #?@.

(|) => nil
#? => nil
#?() => nil
#?(:clj (|)) => clj
#?(:cljs (|) :clj (expr)) => cljs
#?(:cljs (|) :clj (expr)) => cljs
#?(:cljs (|)) => cljs
#?(:cljs (expr) :clj (|)) => clj
#?(:bb (|) :cljs (expr)) => bb
#?(:cljr (|)) => cljr
#?(:default (|)) => default/nil?
#?(:default #?(:cljs (|)) => cljs?

It might make sense to nil pun default into nil since it's effectively falling upward into the top level context. I don't think I've ever seen a reader conditional nested inside another, but I think it's technically possible inside of a default block?

Whichever new method is introduced should replace the (when (cljr--point-in-reader-conditional-p)) block of cljr--language-context-in-point, that was temporarily disabled.

dgtized commented 1 year ago

Resolved now that both #541 and #542 are merged.