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

Relax the return types of Eio.Process.pipe #750

Open
rizo opened this issue Aug 26, 2024 · 5 comments
Open

Relax the return types of Eio.Process.pipe #750

rizo opened this issue Aug 26, 2024 · 5 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@rizo
Copy link

rizo commented Aug 26, 2024

Motivation

Currently the Eio.Process.pipe function has the following type:

val pipe : sw:Switch.t -> _ mgr ->
  [Flow.source_ty | Resource.close_ty] Std.r *
  [Flow.sink_ty | Resource.close_ty] Std.r

This means that the created sources and sinks are required to be "closable". This restriction currently prevents compatibility with types that are monomorphically just sinks or just sources (i.e., without the `Close tag).

My suggestion is to change the type of pipe to:

val pipe : sw:Switch.t -> _ mgr ->
  [< Flow.source_ty | Resource.close_ty] Std.r *
  [< Flow.sink_ty | Resource.close_ty] Std.r

This would allow the resulting sources and sinks to be implicitly used as any of the allowed subtypes.

Examples

To elaborate on why I think this is useful, consider the two examples below.

This works: flexible source consumers

let src, _snk = Eio.Process.pipe ~sw proc_mgr in
Eio.Flow.read_all src

If we expand the read_all type, it accepts [> `R] flows, which of course type-checks because any additional variants, like `Close are allowed. Thus, we can see that read_all is a "good" consumer and does not limit our types.

This does not work: fixed flow types

Some APIs use monomorphic versions of source and sink types. For example, the Cohttp_eio.Body.t type is defined as:

type t = Eio.Flow.source_ty Eio.Resource.t

Which means that the following example does not work:

let src, _snk = Eio.Process.pipe ~sw proc_mgr in
Cohttp_eio.Server.respond ~status:`OK ~body:src

Failing with:

25 |   Cohttp_eio.Server.respond ~status:`OK ~body:src
                                                   ^^^
Error: This expression has type [ `Close | `Flow | `R ] Eio.Resource.t
       but an expression was expected of type
         Cohttp_eio__.Body.t = Eio.Flow.source_ty Eio.Resource.t
       Type [ `Close | `Flow | `R ] is not compatible with type
         Eio.Flow.source_ty = [ `Flow | `R ]
       The second variant type does not allow tag(s) `Close

This is fairly obvious, but unless, I missed something, there's currently no way to "restrict" a [> source_ty] Std.r to just source_ty Std.r in the Eio's API.

We could argue that Cohttp_eio.Body.t, and/or functions that work on this type, should be more permissive and use open types, but, not only I think this is a non-obvious requirement for library authors, it also complicates the public APIs by exposing all the row variables.

Conclusion

By changing the type of Process.pipe to use polymorphic types for source and sinks, we allow them to be compatible with their monomorphic versions.

@rizo
Copy link
Author

rizo commented Aug 26, 2024

Forgot to mention, that I do realize that it is possible to explicitly cast any source-like type to just a source with:

let source = (source_like :> Eio.Flow.source_ty Eio.Flow.source)

But, I still think there's value in allowing pipe to return polymorphic types.

@talex5
Copy link
Collaborator

talex5 commented Aug 28, 2024

Make sense. We already made this change for e.g. Eio_unix.Net.import_socket_stream.

(and the Git version of cohttp-eio also relaxes the type for respond)

@patricoferris patricoferris added the good first issue Good for newcomers label Oct 1, 2024
@Arogbonlo
Copy link

Hello @patricoferris,

I'm Isaac Arogbonlo, and I've been accepted into the Outreachy contribution phase. I would like to work on this issue under your guidance and would be grateful if I could be assigned to it. Thank you.

@patricoferris
Copy link
Collaborator

Glad to have you @Arogbonlo -- let me know if you need any help. Feel free to open a PR whenever you are ready/need more input and ping me!

@Arogbonlo
Copy link

Thank you @patricoferris
I'll update you on any progress and setbacks I have.
Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

6 participants
@rizo @talex5 @patricoferris @Arogbonlo and others