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

feat: add a MonadError instance for ContT #5897

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Jul 14, 2023

We already have a MonadExcept instance; this promotes it to MonadError.

This also documents an issue with the existing MonadExcept instance, but doesn't attempt to fix it.


Open in Gitpod

@eric-wieser eric-wieser requested a review from kmill July 14, 2023 11:07
@@ -120,6 +120,20 @@ instance (ε) [MonadExcept ε m] : MonadExcept ε (ContT r m) where
throw e _ := throw e
tryCatch act h f := tryCatch (act f) fun e => h e f

instance (ε) [MonadExceptOf ε m] : MonadExceptOf ε (ContT r m) where
throw e _ := throw e
tryCatch act h f := tryCatch (act f) fun e => h e f
Copy link
Contributor

@kmill kmill Jul 14, 2023

Choose a reason for hiding this comment

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

Hmm, I know this was the pre-existing implementation of MonadExcept, but it's not good:

def foo : ContT Bool Lean.MetaM Bool := do
  let x ← try pure true catch _ => return false
  throwError "oh no {x}"

#eval foo.run pure
-- false

The exception from the continuation is able to throw back into the try/catch. I'll check the haskell library this module is based on to see if they have a solution.

Copy link
Contributor

@kmill kmill Jul 14, 2023

Choose a reason for hiding this comment

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

I don't think this is salvageable. The issue is that act needs to be constrained to ContT r m r so that we can delimit the continuation.

def ContT.tryCatch [Monad m] [MonadExceptOf ε m] (act : ContT r m r) (h : ε → ContT r m α) :
    ContT r m α := fun c =>
  MonadExceptOf.tryCatch (act pure) (fun e => h e c)

Edit: That code is not right. I don't have act pure being fed into the continuation c...

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, we should add https://hub.darcs.net/ross/transformers/browse/Control/Monad/Trans/Cont.hs to the module docs since mtl imports transformers. There are still some definitions this library has that we don't have. (evalContT is what I called ContT.delimit elsewhere.)

Copy link
Contributor

@kmill kmill Jul 14, 2023

Choose a reason for hiding this comment

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

Maybe this is it? Notice that ε has to be in the same universe as r, and everything has to be constrained to ContT r m r.

def ContT.tryCatch (ε : Type u) [Monad m] [MonadExceptOf ε m]
    (act : ContT r m r) (h : ε → ContT r m r) :
    ContT r m r := fun c => do
  match ← try Sum.inr <$> act pure catch e => pure (Sum.inl e) with
    | Sum.inr v => c v
    | Sum.inl e => h e c

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I know this was the pre-existing implementation of MonadExcept

I've changed it to call the other implementation, so that we only have the incorrect code in one place rather than two!

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add my nonlocal throwError example into the docstring for MonadExcept as a warning?

Copy link
Member Author

@eric-wieser eric-wieser Jul 14, 2023

Choose a reason for hiding this comment

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

I was hoping something like this might work, but I guess there's nowhere to pass through b:

  tryCatch act h f := do
    let mut b : Bool := true
    try
      act (fun a => do
        try
          f a
        catch e2 =>
          b := false -- not legal
          throw e2
      )
    catch e =>
      if b then h e f else throw e

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I was thinking about wrapping the continuation in a try/catch too and came to the same conclusion.

In a way, try/catch for ContT isn't a good idea because you can't force a finally to actually run. You can always do an early exit from anywhere. In Scheme, they've worked out all this business with dynamic-wind to detect when you're calling a continuation and leaving a dynamic scope, so you can have continuation-safe finallys. It seems the only way to do this is to delimit continuations and chain them together (which is my vague understanding of how dynamic-wind works).

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should add my nonlocal throwError example into the docstring for MonadExcept as a warning?

Done.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made a Zulip thread

@kmill
Copy link
Contributor

kmill commented Jul 14, 2023

Could you add

<https://hub.darcs.net/ross/transformers/browse/Control/Monad/Trans/Cont.hs>

to right after line 20? The mts library doesn't actually implement the ContT transformer itself, so it'd be good to link to the relevant part of the transformers library too.

@semorrison semorrison added the t-meta Tactics, attributes or user commands label Jul 17, 2023
@eric-wieser
Copy link
Member Author

Done

@bors bors bot changed the base branch from master to ScottCarnahan/BinomialRing2 September 17, 2023 03:24
@semorrison semorrison changed the base branch from ScottCarnahan/BinomialRing2 to master September 17, 2023 12:16
@YaelDillies
Copy link
Collaborator

Can you merge master to fix the build?

@eric-wieser
Copy link
Member Author

Done. Since this doesn't make the MonadExcept instance any worse, are we happy to put this in as is?

Copy link
Collaborator

@YaelDillies YaelDillies left a comment

Choose a reason for hiding this comment

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

I agree that it isn't worse.

maintainer merge

Copy link

🚀 Pull request has been placed on the maintainer queue by YaelDillies.

Comment on lines +146 to +150
instance (r m) [Monad m] [Lean.MonadRef m] : Lean.MonadRef (ContT r m) where
getRef := monadLift <| Lean.MonadRef.getRef
withRef stx h := fun f => do
let oldStx ← Lean.MonadRef.getRef
Lean.MonadRef.withRef stx (h <| Lean.MonadRef.withRef oldStx ∘ f)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the interaction between getRef and continuations? If I capture a getRef in the current continuation and call it twice, do I get the same reference twice or two different ones?

@Vierkantor
Copy link
Contributor

I agree this doesn't make the exception situation much worse, but does it actually work well with refs?

@YaelDillies YaelDillies added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review labels Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author A reviewer has asked the author a question or requested changes t-meta Tactics, attributes or user commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants