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

Allow Exception ability to be used by main functions #2029

Merged
merged 8 commits into from
Jul 26, 2021
Merged

Conversation

pchiusano
Copy link
Member

@pchiusano pchiusano commented May 27, 2021

Prior to this PR, the main type has to be '{IO} (), now it can be '{IO,Exception} (), allowing unhandled exceptions to bubble to top level. If they make it there they should just act similar to as if you called bug on the failure. io.test command gets a similar upgrade.

See this transcript

Suggested by @rlmark and @stew.

Implementation notes

Right now, the implementation literally just allowing the exceptions to bubble up to the top of the runtime. This used to give a nice uncaught request message similar to if you called bug, but in the current runtime it yields:

.> run main2

  resolve: looked up bad dynamic: 48

Which is no good. We could either fix the runtime to generate a nice message in the case of uncaught requests, or wrap all calls to run in something like this:

catchOrCrash : '{Exception,g} a -> '{g} a
catchOrCrash a _ = handle !a with cases
  {a} -> a
  {Exception.raise ex -> _} -> bug ex

The thing is that I'd like to customize the message a bit, rather than just getting the default bug message. So I guess I'd lean toward fixing up the runtime if that's easily doable. @dolio WDYT?

Testing

I added a transcript to exercise this (and it currently succeeds with that poor error message). I think that should be sufficient testing once the error message gets sorted out.

@pchiusano
Copy link
Member Author

@dolio Not urgent.

@pchiusano pchiusano requested a review from rlmark May 27, 2021 15:07
@dolio
Copy link
Contributor

dolio commented May 27, 2021

I think what's happening with the error message you get is that it's looking up the dynamically scoped raise implementation and not finding anything.

I think it should be possible to instead populate it with something that just throws an appropriate runtime exception that is caught with the other exceptions. If you use a different exception type than the one used for bug, you could do something different. But it might also be possible to just put the text you want in the raise implementation. It depends what exactly you want to be displayed.

Copy link
Contributor

@rlmark rlmark left a comment

Choose a reason for hiding this comment

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

I've tested this locally and ran some terms with the type '{IO, Exception}() and had them succeed without needing to call an Exception handler at the top level!

When raising an exception I do see the resolve statement as the error, but it sounds like Dan is on it!

@pchiusano pchiusano added this to the Beta milestone Jul 2, 2021
@pchiusano
Copy link
Member Author

pchiusano commented Jul 2, 2021

Next step on this @dolio - could you install a top-level handler for the Exception ability, so it can generate a nicer error message in this situation?

Something else I want to figure out is catching exceptions in pure code, and I wonder if they're related. Like what if bug / todo and other runtime exceptions just look for a handler of Exception, as in bug and todo are defined like -

-- a new builtin function which is just a cast
builtin Exception.unsafeRun : '{g,Exception} a -> {g} a

bug : a -> x
bug a = Exception.unsafeRun '(raise (Failure (typeLink Bug) "a bug" (Any a)))

todo : a -> x
todo a = Exception.unsafeRun '(raise (Failure (typeLink Todo) "a todo" (Any a)))

@dolio
Copy link
Contributor

dolio commented Jul 6, 2021

I'm not clear what direction these are for.

If "catch exceptions in pure code" means that unsafeRun just becomes a bomb like bug and todo currently do, then I think that's fine.

If "catch exceptions in pure code" means that there needs to be a way to catch bug/todo calls in pure code somehow, I don't know how to make sense of that.

(catch exceptions) in pure code vs. catch (exceptions in pure code). :)

@aryairani
Copy link
Contributor

aryairani commented Jul 6, 2021

I think we should go in the opposite direction: if UCM knows that your main function transitively depends on todo or bug, then the normal run command should complain instead of starting the runtime.

@pchiusano
Copy link
Member Author

I think we should go in the opposite direction: if UCM knows that your main function transitively depends on todo or bug, then the normal run command should complain instead of starting the runtime.

Hmm can you explain? Are you saying you shouldn't be able to run programs unless you can (statically) determine they don't call todo or bug? That's a very high bar...

@aryairani
Copy link
Contributor

aryairani commented Jul 6, 2021

I think we should go in the opposite direction: if UCM knows that your main function transitively depends on todo or bug, then the normal run command should complain instead of starting the runtime.

Hmm can you explain? Are you saying you shouldn't be able to run programs unless you can (statically) determine they don't call todo or bug? That's a very high bar...

I was saying you shouldn't be able to run programs with the run command if they reference todo (nevermind about bug). You could still run them with some other command.

@aryairani
Copy link
Contributor

But in the spirit of keeping the ticket minimal, let's just keep it to being able to run a program with unhandled Exception, although wouldn't it be nice to still have a command that didn't?

@pchiusano
Copy link
Member Author

pchiusano commented Jul 6, 2021

Yeah, agree, let's just keep this to installing a top-level Unison Exception handler (this is in addition to the Haskell exception handler) in the runtime. @dolio LMK if you need more info on that - I'd just add a commit or two to this existing PR.

I think this is lower priority than fixing up the new inference algorithm issues recently reported.

@dolio dolio added E4 Low-medium effort/time I2 Medium impact R3 High reach C2 Somewhat certain labels Jul 12, 2021
@pchiusano pchiusano added the P1 High priority label Jul 13, 2021
dolio added 2 commits July 20, 2021 17:02
- Might be more complicated than necessary right now, but it's more like
  a genuine handler than just calling a primop throw.
- Probably could use a better error message, but I'm not sure how to
  display `Failure` in a desirable way.
- The 'pure' case was implemented incorrectly
- Also fixed the implementation of putBytes, it was giving back a 'unit'
  value that was actually a handle.
@dolio dolio marked this pull request as ready for review July 22, 2021 20:18
@pchiusano
Copy link
Member Author

@dolio sweet! Can you merge trunk into this branch and fixup conflicts?

@dolio
Copy link
Contributor

dolio commented Jul 22, 2021

Yeah.

Do you have some idea about the error message? Right now it's just printing out the Failure value in a not particularly nice way. But I'm not sure how to make it much better.

@pchiusano
Copy link
Member Author

@dolio yeah, I'd reword a bit just to use full sentences.

The program halted with an unhandled exception:

And then I'd pass a suffixed pretty-print environment to the runtime so it uses shorter names. I think you can make this change in HandleInput.hs:

      IOTestI main -> do
        testType <- eval RuntimeTest
        parseNames0 <- (`Names3.Names` mempty) <$> basicPrettyPrintNames0A
        ppe <- prettyPrintEnv parseNames0
        -- use suffixed names for resolving the argument to display
        let
            parseNames = Names3.suffixify parseNames0

I think change to

      IOTestI main -> do
        testType <- eval RuntimeTest
        parseNames0 <- (`Names3.Names` mempty) <$> basicPrettyPrintNames0A
        let parseNames = Names3.suffixify parseNames0
        ppe <- prettyPrintEnv parseNames

@dolio
Copy link
Contributor

dolio commented Jul 26, 2021

Hopefully this merge works. I was getting local warnings about AbsN not being a complete pattern even though it's declared as one.

@dolio dolio merged commit 924ffb4 into trunk Jul 26, 2021
@dolio dolio deleted the feature/exceptions branch July 26, 2021 20:26
@pchiusano
Copy link
Member Author

@dolio if you do a stack clean locally that should fix the error you were seeing. Stack's compilation cache incorrectly fails to invalidate when you add a COMPLETE pragma I guess.

@dolio
Copy link
Contributor

dolio commented Jul 26, 2021

Nah, I'm using cabal, so it doesn't seem likely to be that. Maybe it also wasn't rebuilding the relevant file.

Is that pragma actually new? I didn't think it was.

@pchiusano
Copy link
Member Author

Maybe cabal's compilation cache has the same bug? :)

The pragma was newly added to ABT.hs in trunk kinda recently yeah.

@pchiusano pchiusano mentioned this pull request Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C2 Somewhat certain E4 Low-medium effort/time I2 Medium impact in progress P1 High priority R3 High reach
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants