Socket type and family as CInt patterns

[vdukhovni]

The current sum-type model for Socket types and families is not extensible, and
makes it needlessly difficult to perform generic operations on sockets, see
e.g. #427

This WIP (not yet done, preview), simplifies the model by replacing the sum
types in question with newtypes around CInt + patterns. It also adds the
SO_DOMAIN and SO_PROTOCOL options (when available on the target system).

Pre-review requested to sanity check the overall approach.

Open issues:

- What to do with "Read" instances, now that the types are CInt
wrappers?  Does anyone need these?

- What are all the "Typeable" instances about?  I dropped some,
and the code continues to work.  Does anyone need these?

- Now conversions between the sum types and the CInt FFI are
total functions, do we still need to check for unsupported
values?  Or just pass them through to the API, and the let
the system calls object to unsupported values?

- The protocol family passed as the first argument to socket(2)
and returned getsockopt(SO_DOMAIN) is a CInt, but the corresponding
address family in sa_family is only an Int8 or Int16.  Perhaps some
care is required to make sure we don't have code paths that result
in trunction of the SocketType when creating ai_family/sa_family.

[kazu-yamamoto]

Of course, I will support this approach. Please go ahead.

[vdukhovni]

Of course, I will support this approach. Please go ahead.

My concern is whether there are any compatibility issues that I'm perhaps neglecting?
Also do you have any thoughts about the seemingly extraneous "Typeable" instances?
Any idea what they're for?

Any thoughts about the "Read" instances? Should they be kept, or dropped?

[infinity0]

As a user of this library, I'd support moving existing Read/Show instances to separate utility functions, and deriving Read/Show everywhere in general. Quite a lot of the instances in this library do not follow the expected contract, defined as "The result of show is a syntactically correct Haskell expression containing only constants".

[vdukhovni]

As a user of this library, I'd support moving existing Read/Show instances to separate utility functions, and deriving Read/Show everywhere in general. Quite a lot of the instances in this library do not follow the expected contract, defined as "The result of show is a syntactically correct Haskell expression containing only constants".

Depending on your point of view, unfortunately, that boat sailed a long time ago. While I sympathise with the sentiment of wanting a round-trip-capable pair of Show and Read instances which parse as Haskell expression allowing show output to be pasted verbatim into code, this makes for a poor user experience when the results of show are displayed in error messages (often via show called on an exception structure, ...)

My take is that the user seeing a friendly representation of the structure is more important... It is unfortunate that Haskell doesn't have a separate stock type class for "friendly" presentation of data, which would be used in error messages, ...

Thus we find many libraries that violate the Read/Show contract, and are sometimes right to do so.

In some DNS-related code I'm working on, I have a "present" method of a "Presentable" typeclass, used for showing DNS objects in "presentation form", which is far more useful to users than a technically correct show. In that library I could leave Show and Read as just derived, but in fact I probably want. There's little motivation to actually use the output of show as a constant expression to paste back into the code.

Which is not to say that your point is invalid, it is just a matter of tradeoffs, and I am presently not sold on the idea of technically correct, but less usable. :-(

[infinity0]

TBH, I find custom Show instances often actually less usable when they hide the internal structure of the data (as some of these do here). Also if not well-thought-out, then the attempt to make the instance "look friendly" can actually leave it ambiguous, which can be pretty infuriating when debugging an error message.

[vdukhovni]

TBH, I find custom Show instances often actually less usable when they hide the internal structure of the data (as some of these do here). Also if not well-thought-out, then the attempt to make the instance "look friendly" can actually leave it ambiguous, which can be pretty infuriating when debugging an error message.

A fair point. I'm just saying that there's no single right answer. In this PR I am changing what were previously simple Enum constructors to opaque patterns around underlying CInt values, but implementing custom show instances that preserve the original names. This means that e.g. show on Hints will produce the same output as before (... AF_INET ...) and not ... (Family 1) ..., which is what you'd get from the derived instance.

With more care, we could output ... (Family AF_INET) ... and (Type Stream) ... but that still means that that instance is no longer derived, and the constructor names may not be properly qualified when imported qualified, ...

[vdukhovni]

@simonmar Can you comment on whether the Typeable instances on various network sum types are needed, and if so what is their purpose? It would be good to know when to add or not add these going forward.

[vdukhovni]

As a user of this library, I'd support moving existing Read/Show instances to separate utility functions, and deriving Read/Show everywhere in general. Quite a lot of the instances in this library do not follow the expected contract, defined as "The result of show is a syntactically correct Haskell expression containing only constants".

I decided to do as you essentially as you asked. While the Read/Show instances are not derived, they behave as you'd expect, modulo not bothering to support the more exotic Family values in Read for now.

[vdukhovni]

I am dropping the WIP tag, please review. I think this is ready, modulo perhaps adding a bunch more AF_xxxxx values to the Family newtype's Read instance (if desired) and confirmation that it is OK to drop all mention of Typeable (perhaps out of ignorance I really see no need for the Typeable instances).

[vdukhovni]

@cartazio Would you care to review this PR?

[infinity0]

I decided to do as you essentially as you asked. While the Read/Show instances are not derived, they behave as you'd expect,

Thanks, that sounds good! BTW, I didn't mean to apply a lot of pressure and ofc the maintainers of this package have the last say, if they might prefer what you had previously.

Regarding the Typeable stuff, it can probably be dropped since it's autoderived since GHC 7.10 and the intended support for this library seems to only go back to 8.0. It basically means something that can be given a concrete type representation at runtime, including all "dumb data" types.

[kazu-yamamoto]

@vdukhovni Should the subject be "Removing Typeable"?

[vdukhovni]

@vdukhovni Should the subject be "Removing Typeable"?

There are two commits in this PR. While I was changing the definitions of Family and SocketType I also dropped Typeable from those, as there was no apparent need for it. And once I did that, I went ahead and dropped all the other instances too.

The main feedback I'm looking for now is about the new Show and Read instances. Are they OK? Do you want the Family instance for Read to list all 66 families (most of which are unused and/or obsolete)? Do we care about the efficiency of a case statement with 66 literal strings to match? Does this require a map? Or does GHC optimize this sort of thing well anyway? And perhaps we don't care about the performance of Read anyway?

[kazu-yamamoto]

First of all, I can accept this PR since there is no backward compatibility issue.

[kazu-yamamoto]

@vdukhovni Do you worry about usability when we introduce patterns?

> :set -XPatternSynonyms 
> newtype Foo = Foo Int deriving (Eq, Show, Read)
> pattern P1 = Foo 1
> 
> show P1
"Foo 1"
> read "Foo 1" :: Foo
Foo 1
> read "P1" :: Foo
Foo *** Exception: Prelude.read: no parse

If so, we already have the same issue in SocketOption. Personally, I don't think Read and Show is important for such types.

[vdukhovni]

@vdukhovni Do you worry about usability when we introduce patterns?

> :set -XPatternSynonyms 
> newtype Foo = Foo Int deriving (Eq, Show, Read)
> pattern P1 = Foo 1
> 
> show P1
"Foo 1"
> read "Foo 1" :: Foo
Foo 1
> read "P1" :: Foo
Foo *** Exception: Prelude.read: no parse

If so, we already have the same issue in SocketOption. Personally, I don't think Read and Show is important for such types.

Well, in this PR, the Show instance outputs "P1" not "Foo 1", and the Read instance parses "P1" back to P1 i.e. Foo 1. So the no parse issue goes away!

However, I hadn't yet bothered adding all the names for the exotic AF_xxxxx types. Easy to do, but first we should agree on the approach, and whether adding the additional cases is worth it (probably yes, if the overall approach is fine, and optimize the switch later if need be).

[kazu-yamamoto]

This PR itself has no problem. Let's discuss patterns vs show instances in another issue.

[vdukhovni]

Though this is merged (thanks again!), just wanted to check that you're satisfied with the "incomplete" Read instance for Family. I wasn't sure whether adding all 66 cases was justified, and what GHC does with a case switch on 66 string laterals, so I left it incomplete, but perhaps sufficient for actual needs. Actual use of read with Family names seems unlikely, especially the more unusual ones. But if someone wanted some sort of CLI that took family name strings, ... more could be added. It is easy enough to make the case list (more?) complete.

[kazu-yamamoto]

I think that defining important values such as inet, inet6 and unix is sufficient.