clojure-emacs / refactor-nrepl

nREPL middleware to support refactorings in an editor agnostic way
Eclipse Public License 1.0
257 stars 69 forks source link

Setting *warn-on-reflection* prevents symbol renaming #347

Open andreyorst opened 3 years ago

andreyorst commented 3 years ago

Expected behavior

Symbol renamed

Actual behavior

java.lang.IllegalStateException: refactor-nrepl is unable to build an AST for myapp.core. tools.analyzer encountered the following problem: Can't set!: *warn-on-reflection* from non-binding thread

Steps to reproduce the problem

I've created a simple clojure file with (set! *warn-on-reflection* true) on top and tried renaming the symbol.

Environment & Version information

clj-refactor.el version information

clj-refactor 2.5.1, refactor-nrepl 2.5.1

CIDER version information

;; CIDER 1.1.0snapshot, nREPL 0.8.3 ;; Clojure 1.10.3, Java 11.0.10

Leiningen or Boot version

Leiningen 2.9.3 on Java 11.0.10 OpenJDK 64-Bit Server VM

Emacs version

GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.13, cairo version 1.16.0) of 2021-03-17

Operating system

Fedora 33

andreyorst commented 3 years ago

Also when simply launching CIDER repl with jack in I see this in the *Messages* buffer:

error in process filter: Some namespaces are in a bad state: error "Can’t set!: *warn-on-reflection* from non-binding thread" in myapp.core

for every namespace where I set this var. When working with GraalVM this setting is very handy and I would like to keep it.

vemv commented 3 years ago

Although popular, (set! *warn-on-reflection*) is not guaranteed to work. No top-level (set! ... ) is - personally I keep seeing projects tripping up on this nuance.

Furthermore (set! *warn-on-reflection*), while is thread-local, is not local to your ns, or to your project. So setting it can change its value for anyone consuming your ns.

tldr it's not an ideal mechanism.

What can you use instead:

We're using Eastwood as a reflection linter in most https://github.com/clojure-emacs projects as a hard check in CI and it's working nicely for us. It has advantages over a simple grep laid out in its documentation.

(I'm biased though because I authored it)

vemv commented 3 years ago

I've (unsuccessfully) tried to reproduce the issue here https://github.com/clojure-emacs/refactor-nrepl/pull/325

Feel free to hack it, you might be able to break the build with a sophisticated-enough example :)

arichiardi commented 2 years ago

I can confirm this happens here as well. It was is a legacy thing that need to be removed. I resolved by removing all the instances of *warn-on-reflection*. The problem was definitely there though and happening on "rename symbol"

vemv commented 2 years ago

I'll see if I can add a temporary binding for *warn-on-reflection*.

andreyorst commented 2 years ago

Maybe it's worth adding all Clojure's special variables that can be set with set!? There aren't many AFAIK

vemv commented 2 years ago

Cheers yes there's similar tech in eastwood https://github.com/jonase/eastwood/blob/8a6655f592004a7d6e028380144cb1210bb7a9f5/src/eastwood/analyze_ns.clj#L422-L435

Although it's not as easy as a binding: given that tools.analyzer is used, any analyzed form itself could be performing a set!. There's a workaround in Eastwood which I could replicate here.

filipesilva commented 1 year ago

Just ran into this as well. My workaround was to find and replace all (set! *warn-on-reflection* true) with #_(set! *warn-on-reflection* true). When I'm done with the refactorings, I'll undo that commit.

I'm a bit surprised this doesn't happen more, since clj-kondo by default tells you to add (set! *warn-on-reflection* true) when using java interop. (edit: I'm silly, by defaults it's off)

vemv commented 1 year ago

@filipesilva Sorry, I missed your message.

I find that clj-kondo recommendation a bit odd. You don't really need to litter clojure namespaces with that flag. You can simply lint for reflection with Eastwood, Leiningen, certain Clojure CLI tools.

Anyway I could give this a try again.