fix(regression): support embedded submodule #5650
Conversation
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
henryiii
force-pushed
the
henryiii/fix/embeddedsubmod
branch
from
36effe9 to
1972a8a
Compare
| if (!this_file) { | ||
| if (this_file) { | ||
| result.attr("__file__") = this_file; | ||
| } else if (PyErr_ExceptionMatches(PyExc_SystemError) != 0) { |
There was a problem hiding this comment.
Ugh ... if we have to catch a SystemError here to accommodate submodules, I regret posting this comment on PR #5584.
I think it'll be best to get rid of this #else branch then.
There was a problem hiding this comment.
Python even throws exceptions when iterating over ranges. I would expect Python's low level C error handling to be more lightweight (if one can say that when it comes to python) than what happens if this were translated to a C++ exception.
There was a problem hiding this comment.
It should be extremely rare, this is only for embedded submodules, normally it works. I think it's probably fine to leave this way.
There was a problem hiding this comment.
Up to you, I'm OK if you want to keep it.
What still makes me feel uneasy: the risk of masking unexpected SystemErrors.
Which is of course extremely low, but not zero. (Google experience: anything that can happen, does happen.)
There was a problem hiding this comment.
The function is part of the Stable API and just accesses a dictionary. My two cents: the current implementation is safe.
There was a problem hiding this comment.
Do you have a link to the implementation handy?
What's on my mind: Why does it raise SystemError, and not AttributeError?
I'd feel more cozy ignoring an AttributeError.
But as I said, I'm fine with this.
There was a problem hiding this comment.
The function explicitly sets the system error. I could not see any system errors in the source of the dictionary.
There was a problem hiding this comment.
Thanks!
The function explicitly sets the system error.
Seems like an unfortunate choice, to have it in the same class of exceptions as e.g. out-of-memory, but 🤷
There was a problem hiding this comment.
>>> SystemError.__mro__
(<class 'SystemError'>, <class 'Exception'>, <class 'BaseException'>, <class 'object'>)
>>> MemoryError.__mro__
(<class 'MemoryError'>, <class 'Exception'>, <class 'BaseException'>, <class 'object'>)
https://docs.python.org/3/library/exceptions.html#SystemError
Raised when the interpreter finds an internal error, but the situation does not look so serious to cause it to abandon all hope.
Doesn’t seem too bad, I think it’s for things like this, exceptions that shouldn’t make it up to the user.
henryiii
changed the title
henryiii
changed the title
Description
Fix #5649.
Suggested changelog entry: