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

Make Exceptions raised in nim catchable as native python Exception subclasses #300

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

PaarthShah
Copy link

@PaarthShah PaarthShah commented Jan 14, 2024

Fixes #207

Old behavior

Whenever an Exception was raised nim code, the nimpy python module would raise a corresponding nimpy.<exceptionName>. This Exception object is a subclass of BaseException in python.

Why is it a problem?

  • It's not intuitive to catch as except BaseException is generally a code smell in python since it catches KeyboardInterrupt raised by ctrl-c
    • And except Exception, which is also a code smell but at least far safer, doesn't even work.
  • It's not possible to do the safe/readable thing and catch exact kinds of common errors (e.g. as per How do I catch (nimpy.KeyError) in Python? #207: a KeyError raised by nim is still a nimpy.KeyError in python)

New behavior

  • Exceptions raised in nim are mapped to specific python Exception:
  • The resulting exceptions in Python are considered to be subclasses of BOTH:
    • The specific python Exception
    • <nimpymodule>.NimPyException
  • The Exceptions raised in python have the exact same names as before (so existing code that does except BaseException and checks the e.__class__.__name__ or similar should continue to work as it did before.

TODO before moving out of draft

  • Finish up mapping the rest of the nim Exception/Defects to Python Exceptions (WIP)
  • Ensure that other exceptions raised elsewhere by nimpy (such as all of the TypeErrors) fit into the general __mro__ of every other one to ensure cleanliness
  • Add example code to this PR message
  • Update the README within this PR to document this behavior
  • Probably some real-life testing as people are able to provide ideas
  • Calling it optional for myself: seeing if it's possible to add a proper traceback object to the resulting python exceptions 👀

Addendum

  • While this PR is a WIP, I'm temporarily disallowing Allow edits by maintainers so I don't run into any git disasters, but I will enable it immediately upon request by a maintainer, or as soon as this is out of draft!

  • Please feel free to give a preliminary review, including critical feedback, as this is my very first contribution to a nim open source project! My domain expertise is in python, but I've been having a lot of fun working with nimpy on various projects, so making this sort of fix was important to me, but I'm by no means claiming that my nim code is at all perfect, so feedback is very welcome!

PyExc_IOError*: PPyObject # in Python 3 IOError *is* OSError
PyExc_ValueError*: PPyObject
PyExc_AttributeError*: PPyObject
# PyExc_BlockingIOError # no
Copy link
Author

@PaarthShah PaarthShah Jan 14, 2024

Choose a reason for hiding this comment

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

I mapped many new exceptions in from https://docs.python.org/3/c-api/exceptions.html#standard-exceptions, but most of these probably will not be required as they don't directly map to a standard library nim exception as per https://nim-lang.org/docs/exceptions.html.

Before I move this out of draft, I'll make a point of removing the commented/unmapped entries.

I didn't delete any, but I made a point of alphabetizing them

@@ -212,6 +253,7 @@ proc loadPyLibFromModule(m: LibHandle): PyLib =

load Py_BuildValue, "_Py_BuildValue_SizeT"
load PyTuple_New
load PyTuple_Pack
Copy link
Author

@PaarthShah PaarthShah Jan 14, 2024

Choose a reason for hiding this comment

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

https://docs.python.org/3.13/c-api/tuple.html
It's also part of the stable c API, and has been present since at least 3.8 (probably much longer).
Added it as a convenience function for myself.

Copy link
Author

Choose a reason for hiding this comment

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

Notably: the pattern of exception raising/catching in this file doesn't technically cover the case where the nim code failed to raise an exception at all... I might need to add some boilerplate to account for that.

Though depending on what versions of python we aim to support, would it be sensible to re-write these using an actual unit testing framework? Probably makes more sense in a different PR but 🤷🏾‍♂️

Copy link
Owner

Choose a reason for hiding this comment

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

Meh, just add doassert(false) at the end of each try block. No need for frameworks

@PaarthShah PaarthShah changed the title Make Exceptions raised in nim catchable as native Python Exceptions Make Exceptions raised in nim catchable as native python Exception subclasses Jan 14, 2024
Comment on lines 175 to 178
proc attributeError(): string {.exportpy} =
# FieldDefect
var obj: MyObj
obj.c
Copy link
Author

@PaarthShah PaarthShah Jan 14, 2024

Choose a reason for hiding this comment

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

Admittedly, I'm not 100% convinced this makes sense to map FieldDefect to an AttributeError, but I'm curious what others think

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.

How do I catch (nimpy.KeyError) in Python?
2 participants