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

A version of compile that returns a side value #201

Open
ChristopherKing42 opened this issue Sep 10, 2019 · 12 comments
Open

A version of compile that returns a side value #201

ChristopherKing42 opened this issue Sep 10, 2019 · 12 comments

Comments

@ChristopherKing42
Copy link

I think it would be useful if there was a version of compile that in addition to compiling the network allowed MomentIO to return a side value:

compile' :: MomentIO a -> IO (a, EventNetwork)

This allows you to, for example, extract the initial values of behaviors. Looking at the source code, it appears that there already is code to do the above, it just isn't exported in a nice way.

@ocharles
Copy link
Collaborator

ocharles commented Sep 8, 2022

I'd really like to have this, but am stuck in the age old problem of naming. I think changing compile would ultimately be the nicest, but it's just too big a breaking change to do without a deprecation cycle, which would itself the require another deprecation cycle to switch /back/ to compile. So I think it's best we just have both.

I don't like using a prime, as I associate that was strictness.

Any preferences for names? compileAndReturn maybe?

@ocharles
Copy link
Collaborator

ocharles commented Sep 8, 2022

Cc @HeinrichApfelmus and @mitchellwrosen

@mitchellwrosen
Copy link
Collaborator

mitchellwrosen commented Sep 9, 2022

Hmm... no strong feelings either way 😅.

In defense of the status quo, I've only ever called compile at type MomentIO (), so I'd never have preferred this variant which would hand me back a ((), EventNetwork). And it is possible to smuggle a return value out of course, with an MVar.

Having compile and compileAndReturn/compile'/compile2 would be ok; I don't have any good naming suggestions. I'm also not opposed to just changing the type of compile. Since reactive-banana is mostly used in applications (and not wrapped much by other libraries), and each application probably only has a single call to compile, the breakage would be pretty minimal.

Overall, it's unclear to me whether it'd be better to have:

  1. The current API, where the only way to get a non-() return value out of compile looks like:
var <- newEmptyMVar
network <- compile (theNetwork >> liftIO (putMVar var thing))
thing <- readMVar var
actuate network
  1. Only this proposed version of compile, where code that used to look like
network <- compile theNetwork
actuate network

would instead look like

((), network) <- compile theNetwork
actuate network
  1. Both versions in the same API.

Reflecting on it a bit... I think I weakly prefer (1), followed by (2), then (3)

@ocharles
Copy link
Collaborator

ocharles commented Sep 9, 2022

The reason I'm interested in this is because it simplifies the following pattern:

setup :: ... -> IO Callbacks
setup ... = do
  (aAddHandler, fireA) <- newAddHandler
  (bAddHandler, fireB) <- newAddHandler
  ...
  (zAddHandler, fireZ) <- newAddHandler

  actuate =<< compile do
    onA <- fromAddHandler
    onB <- fromAddHandler
    ...
    onZ <- fromAddHandler
    ...

  return Callbacks{ fireA, fireB, ..., fireZ }

to

setup :: ... -> IO Callbacks
setup ... = do 
  (eventNetwork, callbacks) <- compile do
    (onA, fireA) <- newEvent
    (onB, fireB) <- newEvent
    ...
    (onZ, fireZ) <- newEvent
    ...
    return Callbacks{..}

  actuate eventNetwork
  return callbacks 

Smuggling that out with an MVar is certainly an option, but why jump through that hoop?

I'm also fairly liberal with breakage (and don't really think reactive-banana has a huge usage at the moment sadly), and would take a compile type change.

@HeinrichApfelmus Maybe you want to be a tie-breaker 😄

@mitchellwrosen
Copy link
Collaborator

Good point, I like that pattern. Ok, I switch to (2), then (3), then (1).

@ChristopherKing42
Copy link
Author

I wasn't sure how other people would feel about it, but if it's on the table option 2 is my personal favorite.

@HeinrichApfelmus
Copy link
Owner

As far as the aesthetics of the interface goes, I would prefer (2) and (3) as well. However, there is catch, and this catch is the main reason why I made it difficult to get stuff out of a compile, and strongly prefer (1).

The catch is that it now also becomes possible to return Event or Behavior from the compile and use it in another network — but, the semantics of this is completely undefined. 😱 Example:

setup ::  -> IO ()
setup  = do
    (networkA, eA) <- compile' $ do
        (e, fire) <- newEvent
        
        return e

    (networkB, _) <- compile' $ do
        (eB, fireB) <- newEvent
        let eBoom = unionWith (+) eA eB -- 😱
        reactimate $ print <$> eBoom    -- 😱😱😱
        return ()

    actuate networkB
    actuate networkA

In earlier versions of reactive-banana, I actually used a type parameter to prevent this from happening, but eventually decided that this parameter made so much noise in the types that we would be better of with disallowing return values in compile.

So, unfortunately, I'd like to stick to (1). (But happy to add an explanation of this somewhere in the source code).

@ocharles
Copy link
Collaborator

Honestly, I think this is overly conservative. We could put network ids in Event and Behavior and check that at runtime, giving a good error message. My problem with this reasoning is it's not at all complete - I can still circumvent this by using an IORef or something, so life just becomes harder without actually really solving anything. Personally I think the better documentation is to say that compile should never return Events or Behaviors.

@ChristopherKing42
Copy link
Author

but eventually decided that this parameter made so much noise in the types that we would be better of with disallowing return values in compile

You could've just removed the "Frameworks t" constraint to get rid of most of the noise. The ST monad is the canonical example of using a type parameter to prevent leaks between different invocations; it doesn't use a type constraint.

@mitchellwrosen
Copy link
Collaborator

mitchellwrosen commented Oct 2, 2022

I agree with @ocharles, but I get where @HeinrichApfelmus is coming from. I think the pattern of "don't return this value here, which we do not attempt to prevent with the type system" is common (e.g. whenever acquiring a resource that's only usable before it's released/finalized).

And @ChristopherKing42, the "ST monad trick" is indeed how the old API used to work, and is precisely what "so much noise in the types" refers to - not the one Frameworks t constraint required at the call site of compile. And I'm sure this was not your intent, but I'd like to provide you with the feedback that beginning a comment with "you could've just <alternative>" in response to a justification for why a decision was made can come across as a bit rude, especially in textual form where one cannot discern your tone of voice ;)

@ChristopherKing42
Copy link
Author

Right, that makes sense

And I'm sure this was not your intent, but I'd like to provide you with the feedback that beginning a comment with "you could've just " in response to a justification for why a decision was made can come across as a bit rude

Oh, sorry about that! Yeah, I was a bit rude in retrospect, sorry.

@mitchellwrosen
Copy link
Collaborator

Should this issue be closed? If I understand correctly, I'd summarize the discussion as:

  • Ollie proposes allowing compile to return a value of any type, I (for what I'm worth) agree.
  • @HeinrichApfelmus would like to keep the current API, since its type prevents the honest user from returning an Event or Behavior and passing along to another network.

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

4 participants