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

Handle printing of cyclic structures #1841

Closed
alex-snezhko opened this issue May 12, 2023 · 3 comments · Fixed by #1844
Closed

Handle printing of cyclic structures #1841

alex-snezhko opened this issue May 12, 2023 · 3 comments · Fixed by #1844
Assignees

Comments

@alex-snezhko
Copy link
Member

alex-snezhko commented May 12, 2023

Currently attempting to print data that contains cyclic references results in a stack overflow. A minimal example:

record Node {
  val: Number,
  mut next: Option<Node>
}

let a = { val: 1, next: None }
let aOpt = Some(a)
a.next = Some({ val: 2, next: aOpt })
// cycle between 1 and 2
print(a) // RangeError: Maximum call stack size exceeded

Perhaps an error value of "<data with cyclic reference>" (or some other more clever way of displaying this data) can be printed instead if a cycle is detected.

@spotandjake
Copy link
Member

Hmms, this could get confusing
Python i guess just resorts to not printing any more then 2 levels, when i tested it it just logged the next value as {...}, I like js's approach better where it says [circular object Object]. but this doesnt really indicate what its circular too for instance if you have like a cycle 3 levels down that might look confusing.

What if we did something instead like <Cyclic Data, Node> and said the type name, not an error but we just serialized it as a cycle so you can still print.

Notes on this: So a few important notes on this are one in grain given we want good dev x and print anything goes along with that we should try to print it at all costs imo rather then error. two i think any fix to this requires chanign the way toString works atm because i dont think that stringifying a child knows anything about the parent. secondly do we want to allow a few levels of cycles maybe like one level or do we think the first cycle is enough and three I think another question / fix for this is if we had an option on print for print depth though that would again change the way toString works and i dont think we really firmly decided on if we want the newline behaviour because it breaks passing it into stuff like forEach, that seems like a slightly seperate dicussion but might be a user experience improvment that would also help to solve this problem.

@alex-snezhko
Copy link
Member Author

@spotandjake I don't think having to specify a depth (or having an implicit depth) is great from a DX perspective.

I also just tried doing this in node and it gave me

<ref *1> { val: 1, next: { val: 2, next: [Circular *1] } }

This seems like a pretty good way of handling it imo, you get a reference for what there is a cycle to. Perhaps we could do something similar?

@spotandjake
Copy link
Member

@spotandjake I don't think having to specify a depth (or having an implicit depth) is great from a DX perspective.

I also just tried doing this in node and it gave me

<ref *1> { val: 1, next: { val: 2, next: [Circular *1] } }

This seems like a pretty good way of handling it imo, you get a reference for what there is a cycle to. Perhaps we could do something similar?

I like this approach actually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants