Skip to content

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

Draft
PaarthShah wants to merge 19 commits into
yglukhov:masterfrom
PaarthShah:catchable-python-exceptions
Draft

Make Exceptions raised in nim catchable as native python Exception subclasses#300
PaarthShah wants to merge 19 commits into
yglukhov:masterfrom
PaarthShah:catchable-python-exceptions

Conversation

@PaarthShah
Copy link
Copy Markdown

@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!

Comment thread nimpy/py_lib.nim
PyExc_IOError*: PPyObject # in Python 3 IOError *is* OSError
PyExc_ValueError*: PPyObject
PyExc_AttributeError*: PPyObject
# PyExc_BlockingIOError # no
Copy link
Copy Markdown
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

Comment thread nimpy/py_lib.nim

load Py_BuildValue, "_Py_BuildValue_SizeT"
load PyTuple_New
load PyTuple_Pack
Copy link
Copy Markdown
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.

Comment thread tests/tnimfrompy.py
Copy link
Copy Markdown
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
Copy Markdown
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 thread tests/nimfrompy.nim Outdated
Comment on lines +175 to +178
proc attributeError(): string {.exportpy} =
# FieldDefect
var obj: MyObj
obj.c
Copy link
Copy Markdown
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