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

with-redefs doesn't redef core fns #375

Closed
cldwalker opened this issue Aug 8, 2020 · 5 comments
Closed

with-redefs doesn't redef core fns #375

cldwalker opened this issue Aug 8, 2020 · 5 comments

Comments

@cldwalker
Copy link

cldwalker commented Aug 8, 2020

Hi. I'm using with-redefs for a testing library and came across the following bug:

version

Version which comes with bb 0.1.3

platform

Mac binary

problem

with-redefs is unable to redef a clojure.core fn

repro

;; Doesn't return Hi
=> (with-redefs [spit (constantly "Hi")] (spit "file" "yolo"))
nil
;; Define fn with same name as clojure.core fn
=>  (defn spit [& args] (prn args))
nil
;; Behaves correctly now that spit is defined in ns
=> (with-redefs [spit (constantly "Hi")] (spit "file" "yolo"))
"Hi"

expected behavior

I expected to be able to redef a clojure.core fn as clojure does

@borkdude
Copy link
Collaborator

borkdude commented Aug 8, 2020

I think this may be due to an optimization, but maybe that optimization isn't worth it. I'll take a look.

@borkdude
Copy link
Collaborator

borkdude commented Aug 8, 2020

Undoing the performance optimization (which is effectively the same as direct linking in Clojure):

(time (sci/eval-string "(dotimes [_ 1000000] (+ (/ 1 2 3)))"))

is about 10s with the optimization and 12s without it on the JVM. In CLJS 17 vs 19s. This is on a Macbook Air which doesn't have a super powerful processor, but it's noticeable.

In Clojure you see the same effect with direct linking:

user=> (alter-var-root #'*compiler-options* (constantly {:direct-linking true}))
{:direct-linking true}
user=> (with-redefs [assoc dissoc] (assoc {:a :b} :a :b))
{:a :b}

Maybe sci should support *compiler-options* so in babashka you can turn it off for tests and with-redefs will work the way you would expect for core vars. Although this isn't exactly the same as direct linking since it's only done for built-in vars.

@borkdude
Copy link
Collaborator

borkdude commented Aug 8, 2020

Never mind, I found another optimization which gets back the performance up to the level it was, while fixing this issue.

@borkdude
Copy link
Collaborator

borkdude commented Aug 8, 2020

Should be fixed now in babashka master.

@borkdude borkdude closed this as completed Aug 8, 2020
@cldwalker
Copy link
Author

Thanks!!

bbss pushed a commit to bbss/sci that referenced this issue Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants