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

PatternSynonyms vs Show/Read #463

Open
kazu-yamamoto opened this issue May 25, 2020 · 16 comments
Open

PatternSynonyms vs Show/Read #463

kazu-yamamoto opened this issue May 25, 2020 · 16 comments
Assignees

Comments

@kazu-yamamoto
Copy link
Collaborator

PatternSynonyms solves the problem of closed sum types. However, it introduced a possible backward compatibility issue relating to Show and Read:

> import Network.Socket
> ReuseAddr 
SockOpt {sockOptLevel = 65535, sockOptName = 4}
> show ReuseAddr 
"SockOpt {sockOptLevel = 65535, sockOptName = 4}"
> read "ReuseAddr" :: SocketOption 
<interactive>:4:1: error:
    • No instance for (Read SocketOption) arising from a use of ‘read’
    • In the expression: read "ReuseAddr" :: SocketOption
      In an equation for ‘it’: it = read "ReuseAddr" :: SocketOption

It's easy to fix this issue by adding Read and Show instance by hand. I would like to discuss it's worth doing.

Relating to #459.

Cc: @vdukhovni, @eborden

@kazu-yamamoto
Copy link
Collaborator Author

One approach is:

  • Keep the perfect backward compatibility in 3.1
  • Throw away the compatibility in 3.2

That is, in 3.1:

data SocketOption = SockOpt {
    sockOptLevel :: !CInt
  , sockOptName  :: !CInt
  } deriving (Eq)

instance Show SocketOption where
  show = showSocketOption

showSocketOption ReuseAddr = "ReuseAddr"
...

And then, in 3.2:

data SocketOption = SockOpt {
    sockOptLevel :: !CInt
  , sockOptName  :: !CInt
  } deriving (Eq, Show)

I wonder how to warn show and read to SocketOption in 3.1.

@kazu-yamamoto
Copy link
Collaborator Author

Cc: also @infinity0

@kazu-yamamoto
Copy link
Collaborator Author

@eborden Note that we are planning to introduce PatternSynonyms to Family etc.

@kazu-yamamoto kazu-yamamoto self-assigned this May 25, 2020
@vdukhovni
Copy link

The variant with showSocketOption ReuseAddr = "ReuseAddr" is certainly a lot more readable if it ever crops up in an error message, ... having put in the effort with Family and SocketType my opinion is I guess biased, but am inclined to say this is worth doing, and even the Read instance turned out to be easier than I thought it would be.

@vdukhovni
Copy link

Anyone else?

@kazu-yamamoto
Copy link
Collaborator Author

@vdukhovni Let's add custom Read instances. I think that everything is necessary for SocketType but only important values are needed for Family.

@kazu-yamamoto
Copy link
Collaborator Author

@vdukhovni Please don't use pure at this moment. Pleas use return instead.

@vdukhovni
Copy link

@vdukhovni Please don't use pure at this moment. Pleas use return instead.

Just curious, can you explain why?

@kazu-yamamoto
Copy link
Collaborator Author

For backward compatibility. Probably we can throw it away now. But we decided to use return in v3.1.

@vdukhovni
Copy link

For backward compatibility. Probably we can throw it away now. But we decided to use return in v3.1.

I thought the upcoming release requires at least GHC 8.0, and that return a == pure a in all GHC 8.x releases. Am I not remembering right?

@kazu-yamamoto
Copy link
Collaborator Author

I think it is reasonable to stay with return in v3.1 series and use pure if necessary in future major versions.

@kazu-yamamoto
Copy link
Collaborator Author

@vdukhovni Would you like to add custom Read instances in v3.1.2?

@vdukhovni
Copy link

@vdukhovni Would you like to add custom Read instances in v3.1.2?

I didn't know it was up to me. If nobody else is willing to do it, I'll see what I can do...

@kazu-yamamoto
Copy link
Collaborator Author

Sorry for the ambiguity. I guess that you are the best person to implement it.

@archaephyrryx
Copy link
Contributor

archaephyrryx commented Jul 5, 2020

@vdukhovni recruited me to work on implementing new read/show instances for the various types still missing them, and i went ahead and developed a more robust paradigm for easily implementing many without doubling up on boilerplate pattern-matches for each instance.

I also caught a bug in Network.Socket.Options.

Both my WIP changes and the bug removal are included in a pull request I just filed. #465

今後よろしくお願いいたします。

@kazu-yamamoto
Copy link
Collaborator Author

@archaephyrryx Thanks.

こちらこそ、よろしくお願いします。

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

3 participants