Add Resource.unsafeFromSystemIOHandle by aspiwack · Pull Request #428 · tweag/linear-base

aspiwack · 2022-08-29T08:43:56Z

Resource.unsafeFromSystemIOHandle lets you convert traditional IO
functions on Handle into resource-aware function.

This also let me clean up the internal definition of handle-based
functions. I took the opportunity to remove auxiliary function and use
the multiplicity-polymorphic flip that the comments were asking for.

Closes #424. Cc @endgame .

endgame

Looks cool. There are a couple of operations we cannot do with the new functions:

lift an IO System.Handle to a RIO Handle
lift an action like hClose to RIO and consume the safe Handle.

Basically, do we want Handle-specific versions of unsafeAcquire and unsafeRelease?

tbagrel1

I left a few questions/suggestions, but the PR looks good to me!

src/System/IO/Resource/Linear/Internal.hs

tbagrel1 · 2022-08-29T13:32:32Z

src/System/IO/Resource/Linear/Internal.hs

@@ -129,50 +140,34 @@ hClose :: Handle %1 -> RIO ()
 hClose (Handle h) = unsafeRelease h


two lines above (l. 137, but I couldn't comment there in the review pane), we use Control.pure, and a few lines before, we use Control.return. Also both openFile and openBinaryFile look very similar, maybe we could factor them as part of this handle functions overhaul?

It's a little overkill. They are both pretty small functions.

src/System/IO/Resource/Linear/Internal.hs

aspiwack · 2022-08-29T14:23:10Z

@endgame

Basically, do we want Handle-specific versions of unsafeAcquire and unsafeRelease?

The Handle-specific version of unsafeRelease is hClose. I haven't thought of something for unsafeAcquire. Maybe we should?

endgame · 2022-08-29T21:51:35Z

Basically, do we want Handle-specific versions of unsafeAcquire and unsafeRelease?
The Handle-specific version of unsafeRelease is hClose.

Yes, I can see it now - thanks.

I haven't thought of something for unsafeAcquire. Maybe we should?

If linear-base had this, the lack of openBinaryFile would not have blocked me. (Technically, I could have coerced everything to make it do what I want, but that felt so bad as a library consumer that I considered it a blocker.) For handles there are more functions like mkFileHandle and mkDuplexHandle that can create them (admittedly, they are very low-level).

I'm not sure exactly what it would look like, but perhaps it's worth trying to think more generally than just RIO Handles - a programmer using linear-base will want to wrap his or her UnsafeResource a in a newtype and provide:

One or more creation functions, coercing/lifting unsafeAcquire;
One or more destruction functions, doing the same to unsafeRelease;
Wrapped versions of primitive IO functions on the resource.

Should we provide a toolkit for these, maybe in terms of Coercible (UnsafeResource a) r =>, to make writing linear wrappers easier?

endgame · 2022-09-05T21:02:52Z

Should we provide a toolkit for these, maybe in terms of Coercible (UnsafeResource a) r =>, to make writing linear wrappers easier?

To elaborate on this point: I was hoping that we could do something like generalise unsafeRelease to something like unsafeRelease :: Coercible r (UnsafeResource a) => r %1 -> RIO (). In the module where a library programmer defines a newtype over UnsafeResource a, the Coercibleinstance would be in scope, but since the constructor wrapping UnsafeResource a is often not exported, library clients do not have access to the Coercible instance. Sadly, there are a couple of problems:

This needs -XAllowAmbiguousTypes, so all ergonomic benefit is lost; and
Even in a simple case like unsafeRelease, I cannot prove to the compiler that the uncoerced resource is fully consumed.

I can't think of a good way to lift the UnsafeResource a toolkit to whatever newtype the programmer comes up with, so maybe we don't worry about it for now?

endgame · 2022-09-05T21:06:47Z

Meta: I am also a little thrown by the way that functions that return (result, resource) tuples return the result in the left side of the tuple. Example: hGetChar :: Handle %1 -> RIO (Ur Bool, Handle). This means that I cannot fmap over the result of such a computation, which I find a bit unergonomic. Is there a reason these libraries return the resource in the RHS of tuples?

aspiwack · 2022-09-06T06:39:50Z

No, I probably just didn't pay attention. I'm ok to change it. But it's a breaking change, so it needs to be advertised.

aspiwack · 2022-09-06T06:41:06Z

To elaborate on this point: I was hoping that we could do something like generalise unsafeRelease to something like unsafeRelease :: Coercible r (UnsafeResource a) => r %1 -> RIO (). In the module where a library programmer defines a newtype over UnsafeResource a, the Coercibleinstance would be in scope, but since the constructor wrapping UnsafeResource a is often not exported, library clients do not have access to the Coercible instance. Sadly, there are a couple of problems:

It's no different than the current situation: you need to do an unsafeFoo for each of the resources that you export anyway.

endgame · 2022-09-10T08:14:50Z

Scanning over the discussions, there are a few threads here. I will summarise them all so that this can hopefully get merged:

A way to lift Handle-creating functions into RIO. I think this should happen in this PR, in case there are other ways of creating Handles that people might want to lift.
Nitpicking around using flip or not to tidy up the lifted functions. I agree with @tbagrel1 that the flip-less versions look nicer.
Decide whether or not to switch the return types to RIO (Handle, x) for the sake of the tuple's Functor instance. This discussion (and maybe PR) should happen in a separate issue, and ideally before 0.3.0 since we're shipping breaking changes anyway.

endgame · 2022-09-10T08:26:57Z

3. Decide whether or not to switch the return types to RIO (Handle, x) for the sake of the tuple's Functor instance.

I will not be making a bug for this - it would make StateT much more difficult to use, and I think that's too great a cost (in addition to all the routine breakage triggered by such a swap).

aspiwack · 2022-10-24T13:40:06Z

I will not be making a bug for this - it would make StateT much more difficult to use, and I think that's too great a cost (in addition to all the routine breakage triggered by such a swap).

Oh yes, it is the StateT order, so maybe I did do this on purpose. I can never remember which component the state goes in.

aspiwack · 2022-10-24T14:59:39Z

Thanks for the discussion, even if I took so much time to get back to it.

After all this I came to the conclusion that I've been going at this all wrong. Namely it is not true that using UnsafeResource Handle directly is unsafe. What is true is that all the API functions that access UnsafeResource a generically are all unsafe function. But nothing wrong happens if I make Handle transparent: as long as you only use safe functions, you can't do anything wrong: your resources will be properly release.

Therefore, here is my work plan:

Rename UnsafeResource to Resource
Make a type alias type UnsafeResource = Resource and mark it as duplicated
Make Handle a type alias type Handle = Resource System.Handle
Don't publish specialised unsafe functions for Handle as the generic one will work.

endgame · 2022-10-24T22:09:30Z

src/System/IO/Resource/Linear/Internal.hs

-data UnsafeResource a where
-  UnsafeResource :: Int -> a -> UnsafeResource a
+data Resource a where
+  UnsafeResource :: Int -> a -> Resource a


If the constructor is not exported, does that mean it's safe to rename?

It is safe to rename. But I like how the name emphasises that accessing this constructor is very unsafe and needs to be done with care. What would you like to see instead?

I think that makes sense. Just checking.

src/System/IO/Resource/Linear/Internal.hs

endgame · 2022-10-24T22:10:20Z

src/System/IO/Resource/Linear/Internal.hs

@@ -201,7 +202,7 @@ unsafeRelease (UnsafeResource key _) = RIO (\st -> Linear.mask_ (releaseWith key
 unsafeAcquire ::


Is the docstring calling this unsafe still correct given your new posture towards resources?

I consider it is an unsafe function in the usual sense in Haskell: it has conditions for application which can't be proved by the compiler, hence are the responsibility of the programmer.

Though I must admit that ResourceT has the same function and considers it safe. So there is certainly a question.

endgame · 2022-10-24T22:12:42Z

src/System/IO/Resource/Linear/Internal.hs

+-- | Specialised alias for 'release'
 hClose :: Handle %1 -> RIO ()
-hClose (Handle h) = unsafeRelease h
+hClose = release


Deprecate this? Not with a view to ever removing it, but programmers put hClose in their programs, then get told to use release, and then learn not to provide aliases in their own libraries?

I don't know that I want to deprecate this. I would if I thought using hClose is less good than using release. But I'm not sure that it is, just it's ok that we have return despite it being a specialisation of pure.

aspiwack

Thanks @endgame for the very prompt review. Any further thought? When we're happy, I'll merge and make a release.

aspiwack · 2022-10-25T09:44:49Z

src/System/IO/Resource/Linear/Internal.hs

+-- | Specialised alias for 'release'
 hClose :: Handle %1 -> RIO ()
-hClose (Handle h) = unsafeRelease h
+hClose = release


I don't know that I want to deprecate this. I would if I thought using hClose is less good than using release. But I'm not sure that it is, just it's ok that we have return despite it being a specialisation of pure.

aspiwack · 2022-10-25T09:48:11Z

src/System/IO/Resource/Linear/Internal.hs

@@ -201,7 +202,7 @@ unsafeRelease (UnsafeResource key _) = RIO (\st -> Linear.mask_ (releaseWith key
 unsafeAcquire ::


I consider it is an unsafe function in the usual sense in Haskell: it has conditions for application which can't be proved by the compiler, hence are the responsibility of the programmer.

Though I must admit that ResourceT has the same function and considers it safe. So there is certainly a question.

src/System/IO/Resource/Linear/Internal.hs

aspiwack · 2022-10-25T09:52:41Z

src/System/IO/Resource/Linear/Internal.hs

-data UnsafeResource a where
-  UnsafeResource :: Int -> a -> UnsafeResource a
+data Resource a where
+  UnsafeResource :: Int -> a -> Resource a


It is safe to rename. But I like how the name emphasises that accessing this constructor is very unsafe and needs to be done with care. What would you like to see instead?

src/System/IO/Resource/Linear/Internal.hs

endgame · 2022-10-25T11:00:55Z

I have nothing further to add, and I don't know when I'll next try linear stuff in a project. Probably best to get this out as-is and see if further stuff emerges later.

Having `Handle` be transparent lets you convert traditional IO functions on `Handle` into resource-aware function. It would use the unsafe API, of course. But it's better than the previous situation. My initial assumption that because every function on `UnsafeResource` (now rename `Resource`) was unsafe, then I need concrete resource types to be opaque was simply faulty. Closes #424.

aspiwack · 2022-10-25T16:58:18Z

I've squashed the history as the intermediate commits were not relevant anymore. This will automerge when CI is green.

I'll make a release in the coming days.

aspiwack requested a review from tbagrel1 August 29, 2022 08:43

endgame approved these changes Aug 29, 2022

View reviewed changes

tbagrel1 approved these changes Aug 29, 2022

View reviewed changes

endgame reviewed Oct 24, 2022

View reviewed changes

aspiwack commented Oct 25, 2022

View reviewed changes

aspiwack force-pushed the unsafeFromSystemIOHandle branch from 9c1ff3a to 1e290fc Compare October 25, 2022 16:55

aspiwack enabled auto-merge October 25, 2022 16:56

aspiwack merged commit e7f1d0d into master Oct 25, 2022

aspiwack deleted the unsafeFromSystemIOHandle branch October 25, 2022 16:59

		@@ -129,50 +140,34 @@ hClose :: Handle %1 -> RIO ()
		hClose (Handle h) = unsafeRelease h

		@@ -201,7 +202,7 @@ unsafeRelease (UnsafeResource key _) = RIO (\st -> Linear.mask_ (releaseWith key
		unsafeAcquire ::

Conversation

aspiwack commented Aug 29, 2022

Uh oh!

endgame left a comment

Choose a reason for hiding this comment

Uh oh!

tbagrel1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aspiwack commented Aug 29, 2022

Uh oh!

endgame commented Aug 29, 2022

Uh oh!

endgame commented Sep 5, 2022

Uh oh!

endgame commented Sep 5, 2022

Uh oh!

aspiwack commented Sep 6, 2022

Uh oh!

aspiwack commented Sep 6, 2022

Uh oh!

endgame commented Sep 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

endgame commented Sep 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aspiwack commented Oct 24, 2022

Uh oh!

aspiwack commented Oct 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aspiwack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

endgame commented Oct 25, 2022

Uh oh!

aspiwack commented Oct 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

endgame commented Sep 10, 2022 •

edited

Loading

endgame commented Sep 10, 2022 •

edited

Loading