Skip to content

Adds better exception macro #2897

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

Merged
merged 8 commits into from
Aug 24, 2021
Merged

Conversation

sobolevn
Copy link
Contributor

@sobolevn sobolevn commented Aug 16, 2021

Closes #2771

I've changed a single exception to show my progress. If this is ok, then I can change all of them.

}

extends_exception! {
PyKeyboardInterrupt,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I don't like here is that I have to repeat ident and literal names. Why?

  • I cannot use just ident names with #[pyclass(name = ..., base = ...)], because they explicitly need literal strings. I even cannot use stringify!, because it is not expanded in #[pyclass] attributes
  • I cannot use just literal, because Rust cannot unstrigify! things, unless https://docs.rs/unstringify/0.1.1/unstringify/macro.unstringify.html is installed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Friendly ping @youknowone @DimitrisJim

Copy link
Member

Choose a reason for hiding this comment

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

I don't any a nice idea for this. This problem is known to be a tranditional rust macro problem. So it looks just ok to me for now. Will it be better if pyclass takes both ident and str for its macro arguments? @coolreader18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, #[pyclass(name = :ident | :literal, base = :ident | :literal)] seems like a good solution 👍
I will implement it in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like that there's nothing I can do. It is a limitation of pub type AttributeArgs = Vec<NestedMeta>;, where NestedMeta can be:

    /// ## Path
    ///
    /// A meta path is like the `test` in `#[test]`.
    ///
    /// ## List
    ///
    /// A meta list is like the `derive(Copy)` in `#[derive(Copy)]`.
    ///
    /// ## NameValue
    ///
    /// A name-value meta is like the `path = "..."` in `#[path =
    /// "sys/windows.rs"]`.

And NameValue only allows Lit values 😞
So, I guess that the only way is to pass ident and literal as two separate params.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@sobolevn sobolevn Aug 18, 2021

Choose a reason for hiding this comment

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

Hm #[pyexception($class_name, $base_class)] seems to work, I will continue my experiments 👍

}

extends_exception! {
PyKeyboardInterrupt,
Copy link
Contributor Author

@sobolevn sobolevn Aug 16, 2021

Choose a reason for hiding this comment

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

Btw, here's another example why I need #2876
As you can see it uses 2 spaces and cargo fmt ignores it.

@@ -109,6 +109,7 @@ fn new_module_item(
inner: ContentItemInner { index, attr_name },
pyattrs: pyattrs.unwrap_or_else(Vec::new),
}),
"pyexception" => unreachable!("#[pyexception] {:?}", pyattrs.unwrap_or_else(Vec::new)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test that it is not counted in module, I can leave it or remove it.

@sobolevn
Copy link
Contributor Author

@youknowone looks like a nice solution to me, what do you think?

@sobolevn sobolevn marked this pull request as ready for review August 19, 2021 15:07
@sobolevn sobolevn changed the title WIP: adds better exception macro Adds better exception macro Aug 19, 2021
@sobolevn
Copy link
Contributor Author

Am I moving in the right direction? 🙂

@sobolevn
Copy link
Contributor Author

I am asking because I am also thinking about this alternative and easier solution from a725e2b

It just enhances extend_class! macro: https://github.com/RustPython/RustPython/pull/2897/files#diff-d19484f26ba252a3b3ff27867db9682f948572dd37e83e811d6f073bfc2f07c2R646-R657

@youknowone
Copy link
Member

@coolreader18 could you review this PR?

Copy link
Member

@coolreader18 coolreader18 left a comment

Choose a reason for hiding this comment

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

LGTM. I've been thinking for a while about what to do with our inheritance situation, and though we're not there yet with e.g. OSError having fields of its own as well as Exception's, this will make it easy to add once we get there.

@youknowone youknowone merged commit 67b3388 into RustPython:main Aug 24, 2021
@sobolevn
Copy link
Contributor Author

Oh no! My bad, I should've marked it as "WIP".
It is totally not ready to be mreged!

  1. It has two competing solutions: extends_exception! macro and extend_exception! macro
  2. It has some temporary code inside

Considering 1. which is better in your opinion?

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.

Exception.__init__.__qualname__ is incorrectly set to 'BaseException.__init__'
3 participants