brandonbloom / fipp

Fast Idiomatic Pretty Printer for Clojure
525 stars 44 forks source link

Implement cycle detection #11

Closed brandonbloom closed 4 years ago

brandonbloom commented 11 years ago

Should print something like #<circular>

brandonbloom commented 9 years ago

wip: https://github.com/brandonbloom/fipp/tree/control

brandonbloom commented 8 years ago

This is useful to prevent the printer from going in to an infinite loop, but cyclic structures are pretty rare for pretty-printed EDN data and recursive (and potentially cyclic) printing is pretty rare for standard JVM toString implementations. I'd entertain a patch for this if were a trivially small perf impact, but I suspect that naive usage of a set won't be fast enough due to deep equality comparisons. Maybe if we could use a visited-check that does object-identity only, it would be fast enough, but then we may exceed the complexity/effort budget for this feature.

cichli commented 8 years ago

Even just doing a visited-check would cause problems with seq head retention I think. Unless there's some really clever way of doing this that isn't obvious to me, we can't avoid keeping a reference to the entire coll currently being printed, which might be an infinite lazy sequence.

brandonbloom commented 8 years ago

Laziness is interesting here, but there's a simpler problem to consider: Cycles in strict data structures, which can only happy with mutation, eg:

(def a (atom nil))
(do (reset! a a) nil)
(fipp.edn/pprint a)

In this case, the root node is, presumably, already going to be retained unless the only thing you want to do is to print the tree out and discard it. So I think there's no real problem there.

Infinite data structures are already addressed by print-length, so I don't think I care about intra-seq cycles. or anything like that.

The interesting case is lazy seqs, but I'm not even sure I care if they are retained when cycle detection is enabled. As long as it's opt in, it should be fine.

brandonbloom commented 4 years ago

Closing as I've never actually wanted this in years of using Fipp. If somebody comes along and provides a robust, tested patch for opting-in to cycle detection that doesn't have much overhead, I'd probably accept it.