Skip to content

Relax return types of Eio.Process.Pipe#775

Merged
talex5 merged 1 commit into
ocaml-multicore:mainfrom
Arogbonlo:fix-750
Jun 25, 2026
Merged

Relax return types of Eio.Process.Pipe#775
talex5 merged 1 commit into
ocaml-multicore:mainfrom
Arogbonlo:fix-750

Conversation

@Arogbonlo

Copy link
Copy Markdown
Contributor

This PR relaxes the return types of the pipe function in Eio.Process to make the code more flexible. The main goal was to allow the pipe function to handle a broader range of types for input and output flows, while still maintaining compatibility with the rest of the codebase.

Changes:

  • Relaxed Type Definitions: I’ve updated the return types for pipe so they can accept any subtype of Flow.source_ty and Flow.sink_ty. This will give us more flexibility when working with processes that require different types of flows.

  • Polymorphic Variants: I used polymorphic variants ([< .. ]) to allow for more flexible types, ensuring that the code still works with the broader variants expected in other parts of the system.

Testing:
I ran dune runtest and confirmed that all tests are passing with these changes. The new types work fine, and the tests ensure everything is still functioning as expected.

Relaxing the types for pipe allows us to work with a wider variety of process setups and flows, without being too restrictive about the types we return. It also makes the code easier to extend in the future.

This addresses issue #750, improving flexibility when working with various APIs that use stricter type definitions for sources and sinks.

@Arogbonlo

Copy link
Copy Markdown
Contributor Author

Hey @talex5 @patricoferris .
I'm sure this is better. Please check it out and let me know. Thank you.

@patricoferris patricoferris left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Arogbonlo !

@Arogbonlo

Copy link
Copy Markdown
Contributor Author

Thanks @Arogbonlo !

I appreciate your efforts!

@talex5 talex5 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, but it also needs the same change for Eio_unix.pipe.

@Arogbonlo

Copy link
Copy Markdown
Contributor Author

Ok. I'll get that done immediately!

@Arogbonlo

Copy link
Copy Markdown
Contributor Author

Hey @talex5 @patricoferris .
I'm sure this is better. Please check it out and let me know. Thank you.

@talex5 talex5 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to update eio_unix.mli with the more flexible types.

@Arogbonlo

Copy link
Copy Markdown
Contributor Author

Hello @talex5 ,

Each time i update eio_unix.mli with the more flexible types, the build constantly fails. Please I'd like your intervention here. Thank you

@Arogbonlo

Arogbonlo commented Oct 31, 2024

Copy link
Copy Markdown
Contributor Author

In the eio_unix.mli file, there is
val pipe : Switch.t -> source_ty r * sink_ty r , which returns a pair of flows (source_ty r and sink_ty r), where source_ty and sink_ty are defined as:

type source_ty = [`Unix_fd | Eio.Resource.close_ty | Eio.Flow.source_ty]
type sink_ty   = [`Unix_fd | Eio.Resource.close_ty | Eio.Flow.sink_ty]

This union typing ([Unix_fd | Eio.Resource.close_ty | Eio.Flow.source_ty]forsource_ty) makes pipe’s output flexible enough to match what’s required in process.mli.

In process.mli, the pipe function has a similar signature :
val pipe : sw:Switch.t -> _ mgr -> [< Flow.source_ty | Resource.close_ty] r * [< Flow.sink_ty | Resource.close_ty] r

So @talex5 , I think flexibility in eio_unix.mli ensures that the flows returned by pipe can match the input requirements of functions in process.mli.

@talex5

talex5 commented Nov 1, 2024

Copy link
Copy Markdown
Collaborator

The one in process.mli now uses <. The one in eio_unix.mli doesn't. This is what you were trying change.

utop # Eio_main.run (fun _ -> Eio.Switch.run (fun sw -> let (r : Eio.Flow.source_ty Eio.Resource.t), _ = Eio_unix.pipe sw in ()));;
Error: This expression has type
         Eio_unix.source_ty Eio_unix.source * Eio_unix.sink_ty Eio_unix.source
       but an expression was expected of type
         Eio.Flow.source_ty Eio_unix.source * 'a
       Type Eio_unix.source_ty = [ `Close | `Flow | `R | `Unix_fd ]
       is not compatible with type Eio.Flow.source_ty = [ `Flow | `R ]
       The second variant type does not allow tag(s) `Close, `Unix_fd
```

@Arogbonlo

Arogbonlo commented Nov 10, 2024

Copy link
Copy Markdown
Contributor Author

The one in process.mli now uses <. The one in eio_unix.mli doesn't. This is what you were trying change.

utop # Eio_main.run (fun _ -> Eio.Switch.run (fun sw -> let (r : Eio.Flow.source_ty Eio.Resource.t), _ = Eio_unix.pipe sw in ()));;
Error: This expression has type
         Eio_unix.source_ty Eio_unix.source * Eio_unix.sink_ty Eio_unix.source
       but an expression was expected of type
         Eio.Flow.source_ty Eio_unix.source * 'a
       Type Eio_unix.source_ty = [ `Close | `Flow | `R | `Unix_fd ]
       is not compatible with type Eio.Flow.source_ty = [ `Flow | `R ]
       The second variant type does not allow tag(s) `Close, `Unix_fd

I just resolved that. I just uploaded the pipe function signature in eio_unix.mli

But after doing that, the build constantly fails.

@talex5 talex5 force-pushed the fix-750 branch 3 times, most recently from 0e86b9c to 88f8377 Compare June 25, 2026 15:50

@talex5 talex5 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the missing casts and force-pushed to your branch. It should work now.

@talex5 talex5 merged commit e7b1e3f into ocaml-multicore:main Jun 25, 2026
4 of 5 checks passed
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

Successfully merging this pull request may close these issues.

3 participants