Behavior of toString relying on special constructor names

[jvoigtlaender]

For example, this currently leads to a runtime error:

import Graphics.Element

type V = RBNode

v = RBNode

main = Graphics.Element.show v

because the toString implementation in Native/Show.js sees the RBNode constructor name and "thinks" that type is a Dict (because the core implementation of Dict uses that constructor name as well).

Similarly, the Set type is not currently handled well in toString (after the #260/#263 change).

[evancz]

What do you propose to ameliorate this? I think we can make the risk of overlap small enough that this is not a concern in practice, and I'm not sure it'd be worth it to do anything fancier than that.

[evancz]

What do you propose to ameliorate this? I think we can make the risk of overlap small enough that this is not a concern in practice, and I'm not sure it'd be worth it to do anything fancier than that.

[jvoigtlaender]

I agree. Name the constructors something like XXX_Set or whatever. Actually I guess the RBNode etc. constructors are not much of an issue, but specifically the constructor Set on the right-hand side of type Set t = Set (Dict.Dict t ()) is too likely to be used by somebody else. So, I'm for renaming that constructor, plus changing the toString function in Native/Show.js by adding a separate case matching on that new constructor (and removing the Set-subcase of the case for Dicts there).

[jvoigtlaender]

I agree. Name the constructors something like XXX_Set or whatever. Actually I guess the RBNode etc. constructors are not much of an issue, but specifically the constructor Set on the right-hand side of type Set t = Set (Dict.Dict t ()) is too likely to be used by somebody else. So, I'm for renaming that constructor, plus changing the toString function in Native/Show.js by adding a separate case matching on that new constructor (and removing the Set-subcase of the case for Dicts there).

[jvoigtlaender]

I addressed the Set-related part of this in https://github.com/elm-lang/core/pull/323.

The only other constructors that are treated somehow specially in the native toString function and that could potentially be occurring in a user's Elm code (and thus potentially lead to conflicts) are RBNode and RBEmpty. So, one might want to replace those by something more "exotic" throughout the whole repository. Like RBNode_Internal and RBEmpty_Internal or so.

[jvoigtlaender]

I addressed the Set-related part of this in https://github.com/elm-lang/core/pull/323.

The only other constructors that are treated somehow specially in the native toString function and that could potentially be occurring in a user's Elm code (and thus potentially lead to conflicts) are RBNode and RBEmpty. So, one might want to replace those by something more "exotic" throughout the whole repository. Like RBNode_Internal and RBEmpty_Internal or so.

[evancz]

A convention like Node_elm_builtin might do the job better. Perhaps someone will think their thing really is internal in their context, but no one should think their node is an elm builtin. Seems like a fine change either way.

[evancz]

A convention like Node_elm_builtin might do the job better. Perhaps someone will think their thing really is internal in their context, but no one should think their node is an elm builtin. Seems like a fine change either way.

[jvoigtlaender]

Okay, same for Set_Internal then?

[jvoigtlaender]

Okay, same for Set_Internal then?