verify py method args count #2083

aviramha · 2021-12-31T13:11:49Z

This fixes #2010. During the change I found out that __ipow__ didn't receive the correct number of arguments.
While fixing it, I encountered a CPython 3.7 bug that passes invalid 3rd parameter in 3.7, so I made the parameters version dependant (3.7>= use 2 params, 3.8>= 3 params)

aviramha · 2021-12-31T15:38:47Z

trying to fix #2010
I'm currently stuck having segfault.. help?

tests/test_arithmetics_protos.rs

aviramha · 2022-01-02T16:20:11Z

Ah! Found it https://bugs.python.org/issue36379

aviramha · 2022-01-02T16:58:00Z

I'm not sure why the Py_3 cfg doesn't work for the tests?

aviramha · 2022-01-02T20:02:16Z

Ready for review @davidhewitt

davidhewitt

Wow, thanks for fixing this up, and finally getting to the bottom of the issue with __ipow__ is awesome 👍

As an FYI my laptop keyboard is currently broken so I'm not sure how well I can check in on PRs for the next couple of weeks. Writing longer comments is hard on mobile so I may be a bit slow replying sometimes.

I think the build-dependency on pyo3-build-config for pyo3-macros-backend is unfortunately not correct, however there are alternative ways to resolve it. See other comments.

As a bit of bikeshedding, I also wonder if we should always allow users to write three-argument __ipow__ and then just pass None always on 3.7. If we document this behaviour it seems like the easiest API for users to implement, rather than forcing them to use #[cfg].

davidhewitt · 2022-01-02T23:04:41Z

pyo3-macros-backend/build.rs

+use pyo3_build_config::pyo3_build_script_impl::{errors::Result, resolve_interpreter_config};
+
+fn configure_pyo3() -> Result<()> {
+    let interpreter_config = resolve_interpreter_config()?;


If I remember correctly, using resolve_interpreter_config from a proc-macro crate won't work correctly when cross compiling, because the environment variables provided by cargo are for the host system, not the cross-compile target.

So I don't think we can add this build script and use #[cfg] in pyo3_build_config. Instead in pyo3_build_config we should use the runtime APIs, like in

pyo3/pyo3-macros-backend/src/method.rs

Lines 210 to 215 in 2503a2d

fn can_use_fastcall() -> bool {

const PY37: pyo3_build_config::PythonVersion =

pyo3_build_config::PythonVersion { major: 3, minor: 7 };

let config = pyo3_build_config::get();

config.version >= PY37 && !config.abi3

}

Sorry that I did not document this 😭

I thought of using it but I was pretty sure I can't do ifs and logic when defining globals? I am still pretty confident?

Agreed, I think you'll need to define both globals and then check which one to use based on the Python version later in the macro code.

E.g. __IPOW__ and __IPOW_PY_3_7__, and then check which one to use at runtime.

This approach might not be so easy for #[pyproto], in which case I'd be fine with leaving #[pyproto] broken. We're deprecating it anyway 😁

davidhewitt · 2022-01-02T23:08:02Z

tests/test_arithmetics.rs

+    #[cfg(Py_3_8)]
+    #[pymethods]
+    impl RichComparisonToSelf {


To avoid passing this #[cfg] pain onto users, do you think we can make it so that users always write three-argument __ipow__, and then on Python 3.7 we just always fill in None as the third argument?

I'm not sure how to implement this approach and not sure if it's correct. How will the user know he wont ever receive values in Py 3.7 (besides doc?) I think compile error and strict adjustment is better...?
The issue happens where the extract layer of PyO3 tries to convert Option when it's actually an invalid value... The interface would be then OptionInvalid where it will check for python version and do it based on that..?

Agreed that compiler errors are more explicit.

I was thinking that given this is such an edge case anyway, and Python code using =** will always pass None, it's only if users explicitly invoke __ipow__ that there's ever a non-none value. So it doesn't seem so bad if we say "on CPython 3.7, due to an interpreter bug, you will always get None".

(Actually this makes me wonder, does it work with PyPy?)

If you're willing to give me a few days I'll write a PR which attempts to implement this always-pass-none case, to see if we like it?

Okay. Would we need the globals in that case though? I think not? so I'll wait with further changes until you do it.
If you want you can give me some pointers on how to get it done. I thought maybe setting the Ty::Object to be Ty::ObjectOrInvalid?

davidhewitt · 2022-01-02T23:11:10Z

guide/src/class/protocols.md

@@ -288,7 +288,7 @@ This trait also has support the augmented arithmetic assignments (`+=`, `-=`,
  * `fn __itruediv__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
  * `fn __ifloordiv__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
  * `fn __imod__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
-  * `fn __ipow__(&'p mut self, other: impl FromPyObject) -> PyResult<()>`
+  * `fn __ipow__(&'p mut self, other: impl FromPyObject, _modulo: impl FromPyObject) -> PyResult<()>`


Suggested change

* `fn __ipow__(&'p mut self, other: impl FromPyObject, _modulo: impl FromPyObject) -> PyResult<()>`

* `fn __ipow__(&'p mut self, other: impl FromPyObject, modulo: impl FromPyObject) -> PyResult<()>`

We should also document the special case for 3.7 (whether it's to have two arguments, or always pass None).

Changed the doc.. A suggestion for formatting would be welcome (couldn't think of anything pretty)

davidhewitt · 2022-01-02T23:13:04Z

tests/ui/invalid_pymethod_proto_args.stderr

+error: Expected 1 arguments, got 0
+ --> tests/ui/invalid_pymethod_proto_args.rs:8:8
+  |
+8 |     fn __truediv__(&self) -> PyResult<()> {
+  |        ^^^^^^^^^^^


Do you think we should include the receiver in these counts? E.g. the above becomes Expected 2 arguments, got 1 ?

Hmm, I was thinking about it but it might be a bit of a burden because we can also have "py" argument and take it into account which might be confusing?

Ah I see. Can we add a test with py to confirm the behavior is correct in that situation too?

Other than that, I'm happy for us to leave the error message like this for now. We can always make adjustments later after we get user feedback 👍

`pyo3-macros-backend` is now compiled with PyO3 cfgs to enable different magic method definitions based on version. Add check for correct number of arguments on magic methods.

davidhewitt · 2022-01-04T23:03:00Z

I've pushed a commit which implements the strategy to always pass None on Python 3.7. Let me know what you think of it?

aviramha · 2022-01-05T08:22:02Z

The changes look great. I'm still a bit concerned about the user getting confused why __ipow__ doesn't work as expected.
Can we maybe shoot a compiler warning if __ipow__ is implemented and compiled for Py3.7?
Sorry I am being annoyed by this - but as it seems someone was already haunted by this issue (without finding why) and then I had hell finding the root cause so I wish to help others avoid it 😆

davidhewitt · 2022-01-05T23:25:51Z

Hmm, I take your point. It's not possible for us to emit a compiler warning (unless on nightly).

Maybe instead of always returning None, we should panic! in IPowModulo::extract on Python 3.7? That way there is a safe crash instead of a segfault, and we can accompany the crash with a clear error message. Anyone implementing __ipow__ with decent testing will then understand they can't use that argument on 3.7.

At least that way users will only need to have the #[cfg(Py_3_8)] or whatever for a few statements inside the function body, rather than on the whole function or #[pymethods] block.

If you agree with that idea, I'll pass this PR back into your hands to finish off (at your leisure) if that's ok? I'm a bit tight on time for the next few days, so I'll end up being a blocker most likely 😬

aviramha · 2022-01-06T07:44:47Z

I'm not sure how you mean to achieve that without blocking users the usage of __ipow__ in Py3.7?
I think I'll drop it for now as it might be less cost-efficient to make it perfect.

aviramha · 2022-01-06T08:00:09Z

If tests pass now I'm okay with merging as is.

davidhewitt

👍 thanks very much again for fixing this!

aviramha force-pushed the magic_methods branch 7 times, most recently from 48b3dd0 to 59c3740 Compare December 31, 2021 15:30

aviramha force-pushed the magic_methods branch from 59c3740 to 77d3491 Compare December 31, 2021 16:11

mejrs reviewed Dec 31, 2021

View reviewed changes

tests/test_arithmetics_protos.rs Show resolved Hide resolved

aviramha force-pushed the magic_methods branch from 77d3491 to 9ae339d Compare January 2, 2022 08:18

aviramha force-pushed the magic_methods branch from 9ae339d to 5f596e0 Compare January 2, 2022 16:57

aviramha force-pushed the magic_methods branch 3 times, most recently from 204e6ff to 8f271fd Compare January 2, 2022 18:06

aviramha marked this pull request as ready for review January 2, 2022 18:21

aviramha changed the title *WIP* verify py method args count verify py method args count Jan 2, 2022

aviramha force-pushed the magic_methods branch from 8f271fd to adcaafb Compare January 2, 2022 20:02

aviramha requested a review from mejrs January 2, 2022 20:02

aviramha self-assigned this Jan 2, 2022

davidhewitt requested changes Jan 2, 2022

View reviewed changes

__ipow__ now supports modulo argument on Python 3.8+.

50659b6

`pyo3-macros-backend` is now compiled with PyO3 cfgs to enable different magic method definitions based on version. Add check for correct number of arguments on magic methods.

aviramha force-pushed the magic_methods branch from adcaafb to 50659b6 Compare January 3, 2022 07:54

pymethods: __ipow__ always receive None on Python 3.7

a9b98b7

davidhewitt force-pushed the magic_methods branch from c1c25d8 to a9b98b7 Compare January 4, 2022 22:59

This was referenced Jan 5, 2022

pymethods: seq methods from mapping methods #2065

Merged

0.16 Release #2079

Closed

fix tests

9ae31f2

fix guide

cbfb5ac

aviramha requested a review from davidhewitt January 6, 2022 13:50

davidhewitt approved these changes Jan 7, 2022

View reviewed changes

davidhewitt merged commit 2cee7fe into PyO3:main Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

verify py method args count #2083

verify py method args count #2083

aviramha commented Dec 31, 2021 •

edited

Loading

aviramha commented Dec 31, 2021

aviramha commented Jan 2, 2022

aviramha commented Jan 2, 2022

aviramha commented Jan 2, 2022

davidhewitt left a comment

davidhewitt Jan 2, 2022

aviramha Jan 3, 2022

davidhewitt Jan 3, 2022

davidhewitt Jan 2, 2022

aviramha Jan 3, 2022

davidhewitt Jan 3, 2022

aviramha Jan 3, 2022

davidhewitt Jan 2, 2022

aviramha Jan 3, 2022

davidhewitt Jan 2, 2022

aviramha Jan 3, 2022

davidhewitt Jan 3, 2022

davidhewitt commented Jan 4, 2022

aviramha commented Jan 5, 2022 •

edited

Loading

davidhewitt commented Jan 5, 2022

aviramha commented Jan 6, 2022 •

edited

Loading

aviramha commented Jan 6, 2022

davidhewitt left a comment

	fn can_use_fastcall() -> bool {
	const PY37: pyo3_build_config::PythonVersion =
	pyo3_build_config::PythonVersion { major: 3, minor: 7 };
	let config = pyo3_build_config::get();
	config.version >= PY37 && !config.abi3
	}

	* `fn __ipow__(&'p mut self, other: impl FromPyObject, _modulo: impl FromPyObject) -> PyResult<()>`
	* `fn __ipow__(&'p mut self, other: impl FromPyObject, modulo: impl FromPyObject) -> PyResult<()>`

verify py method args count #2083

verify py method args count #2083

Conversation

aviramha commented Dec 31, 2021 • edited Loading

aviramha commented Dec 31, 2021

aviramha commented Jan 2, 2022

aviramha commented Jan 2, 2022

aviramha commented Jan 2, 2022

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt commented Jan 4, 2022

aviramha commented Jan 5, 2022 • edited Loading

davidhewitt commented Jan 5, 2022

aviramha commented Jan 6, 2022 • edited Loading

aviramha commented Jan 6, 2022

davidhewitt left a comment

Choose a reason for hiding this comment

aviramha commented Dec 31, 2021 •

edited

Loading

aviramha commented Jan 5, 2022 •

edited

Loading

aviramha commented Jan 6, 2022 •

edited

Loading