Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
andreyorst opened this issue Apr 3, 2021 · 9 comments
Open

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

andreyorst opened this issue Apr 3, 2021 · 9 comments

Comments

@andreyorst
Copy link

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
Copy link
Author

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
Copy link
Member

vemv commented Aug 29, 2021

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 referenced this issue Aug 29, 2021
Tries reproducing https://github.com/clojure-emacs/clj-refactor.el/issues/485

`find-symbol` seemed to lack test coverage anyway.
@vemv
Copy link
Member

vemv commented Aug 29, 2021

I've (unsuccessfully) tried to reproduce the issue here #325

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

vemv referenced this issue Aug 31, 2021
Tries reproducing https://github.com/clojure-emacs/clj-refactor.el/issues/485

`find-symbol` seemed to lack test coverage anyway.
bbatsov referenced this issue Sep 3, 2021
Tries reproducing https://github.com/clojure-emacs/clj-refactor.el/issues/485

`find-symbol` seemed to lack test coverage anyway.
@arichiardi
Copy link
Contributor

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 vemv transferred this issue from clojure-emacs/clj-refactor.el Oct 27, 2021
@vemv
Copy link
Member

vemv commented Oct 27, 2021

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

@andreyorst
Copy link
Author

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

@vemv
Copy link
Member

vemv commented Oct 28, 2021

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.

@vemv vemv pinned this issue Feb 9, 2022
@filipesilva
Copy link

filipesilva commented Aug 1, 2023

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
Copy link
Member

vemv commented Aug 18, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants