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

Recursive atoms cause stackoverflow error #21

Open
ghost opened this issue Jan 14, 2021 · 5 comments
Open

Recursive atoms cause stackoverflow error #21

ghost opened this issue Jan 14, 2021 · 5 comments

Comments

@ghost
Copy link

ghost commented Jan 14, 2021

This code will cause a stackoverflow error:

(def a (atom nil)) (def b (atom a)) (reset! a b)
a

Possible solutions are:

  1. print-method multimethod can be used to fix this for clojure repl. Maybe use that if it's available (https://clojuredocs.org/clojure.core/get-method)

  2. Don't print atoms target value in defstream IRef. It makes sense because atoms are used to opt out of the value semantics of clojure, and making recursive structures is a common use case.

@vlaaad
Copy link
Owner

vlaaad commented Jan 14, 2021

Hi! I'm not sure how print-method can be used to fix this, can you provide more info? Evaluating your example code in a regular REPL also tries to print it recursively (and eventually stops).

@ghost
Copy link
Author

ghost commented Jan 14, 2021

You can customize how objects are formatted in repl output.

https://clojuredocs.org/clojure.core/print-method

@vlaaad
Copy link
Owner

vlaaad commented Jan 14, 2021

The same is true for Reveal, you can redefine the print method of you don't want to see values inside reference types. I think current default (which is the same both in Reveal and Clojure printing) makes more sense because it's useful and common to look inside atoms, even though it will fail in some cases where atoms are put inside themselves.

@ghost
Copy link
Author

ghost commented Jan 14, 2021

If the goal is to be a drop in replacement for clojure repl, then implementing point 1 would be better.

@vlaaad
Copy link
Owner

vlaaad commented Jan 14, 2021

I think I understand what you mean: tighter integration with print-method will allow Reveal to play better with existing custom preferences for printing. I'll keep this in mind! In case you need it, here is how you can make Reveal to print only ref identities:

(require '[vlaaad.reveal.ext :as rx])

(rx/defstream IRef [*ref]
  (rx/horizontal
    (rx/raw-string (str "#reveal/" (.toLowerCase (.getSimpleName (class *ref)))) {:fill :object})
    rx/separator
    (let [hash (System/identityHashCode *ref)]
      (rx/as hash (rx/raw-string (format "0x%x" hash) {:fill :scalar})))))

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

1 participant