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

Remove reflection warnings #75

Conversation

BrunoBonacci
Copy link
Contributor

It removes the following annoying reflection warnings while using the cider-repl:

Reflection warning, compliment/utils.clj:160:15 - reference to field findAll on java.lang.Object can't be resolved.
Reflection warning, compliment/utils.clj:168:51 - reference to field list on java.lang.Object can't be resolved.
Reflection warning, compliment/utils.clj:170:7 - reference to field close on java.lang.Object can't be resolved.
Reflection warning, compliment/utils.clj:168:51 - reference to field list on java.lang.Object can't be resolved.
Reflection warning, compliment/utils.clj:170:7 - reference to field close on java.lang.Object can't be resolved.

all test pass with JDK8 and JDK9+

@BrunoBonacci
Copy link
Contributor Author

Hi @alexander-yakushev,
when you get chance could you please review and get fix merged?
These reflection warnings are really annoying and cause weird behaviours.

As soon as a new release of compliment is available with the fixes, I'll issue a PR to Cider to update the compliment release.

@alexander-yakushev
Copy link
Owner

Hello Bruno! I understand that these warnings must be annoying, but could you please specify which sort of weird behaviors does it cause? Because basically, you've rewritten the reflection done by the Clojure compiler into a manual one, so nothing should really change in the resulting bytecode.

@BrunoBonacci
Copy link
Contributor Author

Hi @alexander-yakushev

I do have *warn-on-reflection* enabled by default on all the projects. When I start the cider-repl I used to get a screenful of warnings which now I have fixed with the following PRs:

This PR fixes the last warnings I see.

the strange behaviour I see is that my buffer would jump to an apparently random buffer called "utils.clj" from another project when the evaluation triggers the reflection warning. It is very bizarre and I didn't manage to track the issue in cider-nrepl yet.
However, since I removed the reflection from cider-nrepl, orchard and compliment, this issue completely disappeared.

@alexander-yakushev
Copy link
Owner

Since you only want to get rid of the warnings here, not remove the reflection itself, it should be enough to put (set! *warn-on-reflection* false) at the top of utils.clj. I'd add something like this:

;; Disable reflection warnings in this file because we must use reflection to
;; support both JDK8 and JDK9+.
(set! *warn-on-reflection* false)

Want to do it in this PR or should I commit it on my own?

@BrunoBonacci
Copy link
Contributor Author

adding the (set! *warn-on-reflection* false) would disable it globally thus disabling it also for the user.
You will need to:

  • capture the previous value
  • (set! *warn-on-reflection* false)
  • load/eval the ns
  • restore the initial value

What's your thought on this, do you prefer the current implementation on the one proposed here?

@alexander-yakushev
Copy link
Owner

alexander-yakushev commented Oct 6, 2020

Not really. See https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L7625. All legitimate ways to load a file (build tools, require, etc.) go through this entrypoint which always creates a new frame for certain dynamic vars for the loaded file. So, changing *warn-on-reflection* within a file only influences that file.

@BrunoBonacci BrunoBonacci force-pushed the fix-reflection-warnings branch from b3ba3ff to ea3f161 Compare October 6, 2020 13:02
@BrunoBonacci
Copy link
Contributor Author

Great! I didn't know that. Thanks for pointing it out.
I've added the suggested fix and rebased the changes.

@alexander-yakushev alexander-yakushev merged commit 8f02e13 into alexander-yakushev:master Oct 6, 2020
@BrunoBonacci BrunoBonacci deleted the fix-reflection-warnings branch October 6, 2020 13:12
@alexander-yakushev
Copy link
Owner

alexander-yakushev commented Oct 6, 2020

Thank you! Published this fix in 0.3.11.

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

Successfully merging this pull request may close these issues.

2 participants