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

Stop toString from descending into arbitrary objects #424

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@ThomasWeiser
Contributor

ThomasWeiser commented Oct 11, 2015

The function toString has code to convert Elm-records into a string representation.
This code missed a checked whether the JS object is really an Elm-record.

Problem: If toString is called on an recursively linked JS object (not an Elm-record, e.g. from a native library function), this will lead into an infinite recursion.

The check used to be in the code up to commit 08bf809. I don't know if Evan removed it intentionally for some reason.

Stop toString from descending into arbitrary objects
 The function `toString` has code to convert Elm-records into a string representation.
 This code missed a checked whether the JS object is really an Elm-record.

 Problem: If toString is called on an recursively linked JS object (not an Elm-record, e.g. from a native library function), this will lead into an infinite recursion.

 The check used to be in the code up to commit 08bf809. I don't know if Evan removed it intentionally for some reason.
@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Oct 11, 2015

Member

Yes, it was removed because records no longer have a _ field that distinguishes them.

Can you give concrete examples of values that mess this up?

Member

evancz commented Oct 11, 2015

Yes, it was removed because records no longer have a _ field that distinguishes them.

Can you give concrete examples of values that mess this up?

@evancz evancz closed this Oct 11, 2015

@ThomasWeiser

This comment has been minimized.

Show comment
Hide comment
@ThomasWeiser

ThomasWeiser Oct 12, 2015

Contributor

Ah, didn't know about the dropped _ field.

My example is a test app for elmfire. The native code returns a reference to internal data of the Firebase library. This data happens to have a cyclic structure.

I now have a workaround, where I drop this cyclic Json data from the record before printing.

Does this mean that cyclic structures are disallowed as a Json.Encode.Value? This would be kind of a burden for native wrappers around JS libraries. But it would be a valid argument to not have to do cycle checks in functions like toString (and probably in comparing and equality checking too).

Contributor

ThomasWeiser commented Oct 12, 2015

Ah, didn't know about the dropped _ field.

My example is a test app for elmfire. The native code returns a reference to internal data of the Firebase library. This data happens to have a cyclic structure.

I now have a workaround, where I drop this cyclic Json data from the record before printing.

Does this mean that cyclic structures are disallowed as a Json.Encode.Value? This would be kind of a burden for native wrappers around JS libraries. But it would be a valid argument to not have to do cycle checks in functions like toString (and probably in comparing and equality checking too).

@ThomasWeiser ThomasWeiser deleted the ThomasWeiser:fix-toString-record branch Jan 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment