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

Preserve exception #130

Merged
merged 3 commits into from
Jan 22, 2022
Merged

Preserve exception #130

merged 3 commits into from
Jan 22, 2022

Conversation

roberth
Copy link
Collaborator

@roberth roberth commented Jan 18, 2022

Currently, exceptions are caught by the Cpp.Exceptions module, where the original exception is thrown away. While this can be circumvented at a small scale by catching certain exceptions in C++ code, this is not feasible at a large scale, as it goes against what exceptions are intended for: make error handling implicit and centralize it around a few high-level call sites.

This PR fixes this problem by using std::exception_ptr to represent C++ exceptions as first-class Haskell values.

As a result, the caller can write their own exception handling function, to extract valuable information from specific exceptions. For example:

renderException :: SomeException -> IO Text
renderException e | Just (C.CppStdException ex _msg _ty) <- fromException e = renderStdException ex
-- ...

renderStdException :: C.CppExceptionPtr -> IO Text
renderStdException e =
  [C.throwBlock| char * {
    std::string r;
    std::exception_ptr *e = $fptr-ptr:(std::exception_ptr *e);
    try {
      std::rethrow_exception(*e);
    } catch (const somelib::Error &e) {
      std::stringstream s;
      // obtain detailed exception info
      somelib::showErrorInfo(s, e.info());
      r = s.str();
    } catch (const std::exception &e) {
      r = e.what();
    }
    return strdup(r.c_str());
  }|] >>= unsafePackMallocCString <&> decodeUtf8With lenientDecode

Review and implementation notes

The second commit renames a file, making the combined diff unhelpful. The first commit contains all the actual changes.

I've decided to move the code from Exceptions to Exception so that

  • Compatibility with the old interface is preserved, as far as is feasible without introducing a second CppException representation
  • It is consistent with #include <exception> and import Control.Exception; both singular

I've decided to keep the string representations of the message and exception type, in order to avoid calling C++ code in the Show instance. This can be a little inefficient, but much better than storing these in [Char] like we did.

I've decided to make the exception module(s) depend on C++11. I believe requiring a decade old standard should not pose a problem. The alternative is to revive the old code as an independent module, but until we know that such a maintenance overhead is useful to anyone, this would be a wasted effort and a liability.

@bitonic
Copy link
Collaborator

bitonic commented Jan 22, 2022

Thanks!

@bitonic bitonic merged commit 063e887 into fpco:master Jan 22, 2022
@roberth
Copy link
Collaborator Author

roberth commented Feb 14, 2022

Could you make a release?
This one can be breaking, so inline-c-cpp needs a bump 0.4.x.y -> 0.5.0.0.
inline-c is still equal to its released version, so it's only about inline-c-cpp.

@bitonic
Copy link
Collaborator

bitonic commented Feb 15, 2022

Embarassingly, my Haskell setup is currently broken somehow (looks like this https://discourse.nixos.org/t/stack-build-fails-with-startprocess-invalid-argument-bad-file-descriptor/1668 ). I'll do a release as soon as I have a second to fix it.

@roberth
Copy link
Collaborator Author

roberth commented Feb 15, 2022

This seems to work (if you excuse my unpinned <nixpkgs>; it's the latest nixos-21.11)

{ pkgs ? import <nixpkgs> {} }:

pkgs.mkShell {
  nativeBuildInputs = [
    pkgs.stack
    pkgs.ghc
    pkgs.cabal-install
    pkgs.haskellPackages.haskell-language-server
    pkgs.haskellPackages.hie-bios
  ];
}
nix-shell -I nixpkgs=channel:nixos-21.11 shell.nix

If that doesn't solve it, I've had some success clearing the ~/.stack and $(find -name .stack-work) directories by hand. Possibly because stack produces store references but no gc roots. Not sure if it solves your error, but errno can be deceptive.

@bitonic
Copy link
Collaborator

bitonic commented Feb 17, 2022

Done, removing ~/.stack did it

| CppNonStdException CppExceptionPtr (Maybe ByteString)

instance Show CppException where
showsPrec p (CppStdException _ msg typ) = showParen (p >= 11) (showString "CppStdException e " . showsPrec 11 msg . showsPrec 11 typ)
Copy link
Member

Choose a reason for hiding this comment

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

@roberth Was it intended that there's this e in the message?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, now I get it. The e is suppoed to represent the _ (first field of constructor CppStdException) which is not shown because it's a pointer.

@@ -275,16 +297,16 @@ main = Hspec.hspec $ do
tag :: C.CppException -> String
tag (C.CppStdException {}) = "CppStdException"
tag (C.CppHaskellException {}) = "CppHaskellException"
tag (C.CppOtherException {}) = "CppStdException"
tag (Legacy.CppOtherException {}) = "CppStdException"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be "CppOtherException" before and after?

Copy link
Member

Choose a reason for hiding this comment

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

This would also have been pointed out by -Wall for the test suite.

Fixed in my propsed PR #154

@roberth
Copy link
Collaborator Author

roberth commented Dec 6, 2024

Thanks @nh2 for fixing the show formatting and improving the tests!

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