All exceptions are now modified with extend_exception! macro
#2974
Conversation
|
@youknowone quick question: these lines https://github.com/RustPython/RustPython/pull/2974/files#diff-d19484f26ba252a3b3ff27867db9682f948572dd37e83e811d6f073bfc2f07c2R547-R548 are not technically correct. We don't need to always call |
|
Here is a rough idea:
And other stuffs: |
|
@youknowone it is hard to understand what do you mean without a code-snippet 😞 |
|
I think the part is not directly related to this issue. You can keep it as current behaviour for now. We need to fix it for entier |
can i fix this issue? |
|
Ok, new concept. I was not able to fix this problem with Now, this works: define_exception! {
PyPermissionError,
PyOSError,
permission_error,
"Not enough permissions.",
os_error_custom_new,
}
fn os_error_custom_new(
cls: PyTypeRef,
args: FuncArgs,
vm: &VirtualMachine,
) -> PyResult<PyRef<PyBaseException>> {
println!("custom new");
PyBaseException::new(args.args, vm).into_ref_with_type(vm, cls)
}But, for some reason I experience |
|
I am trying to debug what is going on with |
|
Ok, I found it: https://github.com/RustPython/RustPython/pull/2974/files#diff-4231786211df4730d5d5dae029df9d477b4310fc6333e7dc95169e808d04cda1R347-R364 Probably, we have too many |
|
It should be good now 🎉 |
|
@youknowone I cannot reproduce this failure (https://github.com/RustPython/RustPython/pull/2974/checks?check_run_id=3454164130) locally. Is it related? |
| @@ -161,37 +162,45 @@ class SubError(MyError): | |||
| assert BaseException.__new__.__qualname__ == 'BaseException.__new__' | |||
| assert BaseException.__init__.__qualname__ == 'BaseException.__init__' | |||
| assert BaseException().__dict__ == {} | |||
| assert BaseException.__doc__ | |||
There was a problem hiding this comment.
why are these lines deleted?
There was a problem hiding this comment.
Because I now test all exceptions to have __doc__ here: https://github.com/RustPython/RustPython/pull/2974/files#diff-9948afa9cc8e4a175f769771bdb1930355f76b99f3dface36d968c35561b7d1dR202-R206
| define_exception! { | ||
| PyStopIteration, | ||
| PyException, | ||
| stop_iteration, | ||
| "Signal the end from iterator.__next__()." | ||
| } |
There was a problem hiding this comment.
This is a weak suggestion. If this one would be not a maro_rules but a proc-macro like now, I think we can keep it more structural way for readability and future extension.
/// Signal the end from iterator.__next__().
#[pyexception(name="stop_iteration", base = "PyException")]
struct PyStopIteration {}This code have a little bit more code but understandable like other common #[pyclass]es
There was a problem hiding this comment.
It is problematic to pass __init__ and tp_new this way. Since proc-macro only allows key=string pairs, not key=ident
Writting
#[pyexception(base = "PyException", ctx_name = "stop_iteration", init="import_error_init")]
struct PyImportError {}
fn import_error_init() { ... }seems a bit off.
Maybe something like:
#[pyexception(stop_iteration, base_exception_new, import_error_init)]
#[pyclass(base = "PyException")]
struct PyImportError {}?
But, this does not look way better than define_exception! macro right now.
There was a problem hiding this comment.
It is even more verbose, because pyclass requires name= and module= to be set:
/// Some doc
#[pyexception(stop_iteration, base_exception_new, import_error_init)]
#[pyclass(name = "ImportError", module = false, base = "PyException")]
struct PyImportError {}There was a problem hiding this comment.
Looks like even this case does not require struct fields: https://github.com/RustPython/RustPython/pull/2960/files#diff-d19484f26ba252a3b3ff27867db9682f948572dd37e83e811d6f073bfc2f07c2R644
There was a problem hiding this comment.
- new and init:
tp_newandinitcan be added inpyimpl. #[pyexception]can trigger#[pyclass]inside the macro. So they shouldn't be stacked.- I don't think string and ident matter. We already use string literal for pyclass
There was a problem hiding this comment.
new and init:tp_new and init can be added in pyimpl.
Do you mean like manually? Or do you mean with some extra macro syntax?
There was a problem hiding this comment.
I didn't expect extra macro there. We already have pyimpl, so I thought it could work just like pyclass.
There was a problem hiding this comment.
I expect finally it looks like:
#[pyexception(name = ...)]
struct ImportError {}
#[pyimpl]
impl importError {
#[pyslot]
fn new(...) {}
#[pymethod(magic)]
fn init(...) {}
}I don't think declaring a free function import_error_init and use the name in the macro have advantage to the traditional way.
|
What about an exception in, e.g., |
|
@coolreader18 looks busy. I left a few suggestions but I think this version is good enough because it is solving problems. |
Yes, I do! But, I don't have the time right now. This requires quite a lot of physical labor 😆 |
a25f47e to
63db6d7
Compare
|
Rebased! Please, squash it on merge |
|
thank you! |
So, I've decided to go with the simplest
extend_exception!I can think of.Why?
pub fn extend(ctx: &PyContext)ctxyetUpdate: it was really hard to make it work with subclasses ad custom
__new__and__init__in mind.So, I've switched to a more complex
struct-based macro.Closes #2973
Refs #2960
Refs #2897