Add native Function types.

[sebpuetz]

Add bindings for PyCFunction, PyFunction, PyClassMethod and
PyStaticMethod.

---

#1156

This probably requires some more work, especially I'm unsure whether we should bind PyClassMethod and PyStaticMethod. Extraction only succeeds for those if they are not bound to a class, e.g.:

>>> @staticmethod
... def foo(): pass
>>> @classmethod
... def bar(cls): pass

Defining these methods in a class leads to errors like this:

Can't convert <bound method Foo.foo of <class '__main__.Foo'>> to PyClassMethod

---

I have not included any APIs for the Rust types. For PyCFunction we can probably offer call() with the same signature as PyAny but directly go through PyCFunction_Call. Another rather obvious functions are probably name() and module(), perhaps also get_self() or something along these lines.

I'm expecting failures for pypy since I only guessed the names of the function type objects.

[davidhewitt]

Thanks! As the PyClassMethod doesn't look like it works for pypy and it's not necessary for what we need for 0.12, can I suggest we split this PR in two:

• PyFunction and PyCFunction which I'll merge now for 0.12
• PyClassMethod and PyStaticMethod which we can figure out after and merge on the way to 0.13

[sebpuetz]

Makes sense, I removed the two other method types.

I'm wondering if we can find a (nice) way to replace the wrapper: &impl Fn(...) -> &PyFunction with a function: &PyFunction in PyModule::add_function. Currently we're producing the wrapper through proc-macros, then get the identifier for the function which initializes the PyCFunction through wrap_pyfunction!() (which imo is a bit of a misnomer).

Without much thought put into this, I'd see 1-2 ways:

• Add a init_py(c)function!() macro that actually returns the &PyCFunction instead of returning the function to get the &PyCFunction
• Make the #[pyfunction] / #[pyfn] proc-macro create static PyCFunction objects (No idea about the implications on this one)

[kngwyu]

I'm wondering if we can find a (nice) way to replace the wrapper: &impl Fn(...) -> &PyFunction with a function: &PyFunction in PyModule::add_function.

I think &impl Fn(f: unsafe extern "C" fn(...), name: &str) -> &PyFunction is more realistic, as I commented.
Since PyCFunction needs a function pointer and we cannot treat normal functions as closures, we can not make PyCFuntion on the fly.

[sebpuetz]

I don't see the benefit of providing an API that takes a function to produce a function. I don't think the function-to-produce-another-function is used in any other context than inside PyModule:add_function(). It's an implementation detail which in my opinion should not be exposed in a public API.

Additionally, it's not intuitive at all: Why would add_function() take a function that is not the function that I'm trying to add to the PyModule? As a user I'd expect to pass the actual function to that method. Given that we can't - as far as i understand - expose a constructor on PyCFunction, we can still hide away the implementation details through the proc-macro & a declarative macro that calls the generated function to get the PyCFunction.

E.g.:

#[pyfunction]
fn foo() {}

#[pymodule]
fn bar(_py: Python, module: &PyModule) -> PyResult<()> {
    let module = PyModule::new("some_module")?;   
    let fun: &PyCFunction = init_pycfunction!(foo, module)?; // type annotation just for clarity
    module.add_function(fun)?;
}

impl PyModule {
    fn add_function(&self, fun: &PyCFunction) -> PyResult<()> {
        [...]
    }
}

[sebpuetz]

Adding to this: wrap_pyfunction!() really just returns the constructor for PyCFunction. Applied to some toy example that'd be like:

struct Foo(String);
impl Foo {
    fn new() {
        Foo(String::new)
    }
}

struct Bar(Vec<Foo>);
impl Bar {
    fn add_foo(&mut self, foo_constructor: impl Fn() -> Foo) {
        self.0.push(foo_constructor())
    }
}

instead of:

fn add_foo(&mut self, foo: Foo) {
        [...]
    }

[kngwyu]

Hmm... 🤔
Reading through your comment, I came up with:

        fn #function_wrapper_ident() -> pyo3::class::PyMethodDef {
            #wrapper
            pyo3::class::PyMethodDef {
                ml_name: stringify!(#python_name),
                ml_meth: pyo3::class::PyMethodType::PyCFunctionWithKeywords(__wrap),
                ml_flags: pyo3::ffi::METH_VARARGS | pyo3::ffi::METH_KEYWORDS,
                ml_doc: #doc,
            };
        }

and

m.add_function(PyCFunction::new(py, wrap_function!(function)));

.

wrap_function can return &PyCFunction if py is given, like: m.add_function(wrap_function!(py, function));, but I'm not sure which is better.

[sebpuetz]

I think we're getting there! I'd just like to add two points:

1. The constructor should probably take &PyModule over Python since we can't fill in slf for PyCFunction_NewEx otherwise.
   • we could offer another constructor for module-less functions
   • Alternatively: fn new(def: PyMethodDef, module: Option<&PyModule>) -> PyResult<&PyCFunction>
2. The not-so-nice part about the wrap_pyfunction! macro is that it operates on the identifier of the Rust function to get the constructor identifier, so we can't hide it inside PyCFunction::new.

But I don't think there's a way around this unless #[pyfunction] starts to modify the Rust function rather than writing a new function. That would probably cause too many changes, considering all #[pyfunction] annotated Rust functions are currently callable as-is in Rust.

[sebpuetz]

impl PyCFunction {
    pub fn new<'a>(name: &'static str, fun: PyCFunctionWithKeywords, doc: &'static str, m: &'a PyModule) -> PyResult<&'a PyCFunction> {
        let py = m.py();
        let def = crate::class::PyMethodDef {
            ml_name: name,
            ml_meth: crate::class::PyMethodType::PyCFunctionWithKeywords(fun),
            ml_flags: ffi::METH_VARARGS | ffi::METH_KEYWORDS,
            ml_doc: doc,
        };
        let def = def.as_method_def();
        let mod_ptr = m.as_ptr();
        let name = m.name()?;
        let name = name.into_py(py);

        unsafe {
            py.from_owned_ptr_or_err::<PyCFunction>(
                ffi::PyCFunction_NewEx(
                    Box::into_raw(Box::new(def)),
                    mod_ptr,
                    name.as_ptr()
                )
            )
        }
    }
}

What's your take on an API like this?

I'd imagine using it like:

#[pyfunction]
fn some_fun() {}

#[pymodule]
fn some_mod(_py: Python, module: &PyModule) -> PyResult<()> {
    let c_wrapper: PyCFunctionWithKeywords = wrap_pyfunction!(some_fun)?; // annotation again just for visualization
    let fun = PyCFunction::new("fun_py_name", c_wrapper, "Documentation can be defined anywhere", module)?;
    module.add_function(fun)?;
}

There are probably ways to make name and doc non-static in the constructor, I'm just mirroring the proc-macro impl for now.

[sebpuetz]

I haven't made changes to PyModule::add_function yet, I first want to get some feedback whether my solution looks decent enough.

I did not remove the old way of getting the __pyo3_get_function_ identifier since that would break backwards compatibility with the old PyModule::add_wrapped ways. Once that method goes away, we can also think about removing wrap_pyfunction!() and have #[pyfunction] only generate the raw C-FFI wrapper.

edit CI failure doesn't seem to be related, looks like some network / pypi error.

[kngwyu]

I prefer to

pub fn new(py: &Python, method_def: PyMethodDef) -> PyResult<&Self> { ... }

due to its simplicity.
By this,

• We can write m.add_function(PyCFunction::new(py, wrap_function!(function))) by just one liner.
• By adding some builder methods to PyMethodDef, this would be more flexible.

[sebpuetz]

edit Sorry for the noise, I didn't properly read the second part of your reply.

I'm not convinced, PyMethodDef still needs to be constructed. Exposing it like this in the public API makes it more likely that people end up implementing it themselves. And there are some pitfalls coming with implementing PyMethodDef manually:

• if doc is not null-terminated, segfaults happen.
  • all fields are pub, so there's no way to currently ensure that it is in fact null-terminated.
• if ml_flags doesn't match the signature of ml_meth crashes also happen.
• Users will have to manually leak the Strings to get the `'static' lifetimes required.
  • or they need to be defined at runtime

We can use the enum we implemented for #1143 to reduce the number of arguments for this function. I think the other arguments are very straight-forward. To define a PyFunction you need a name, a function, docs and an optional module. I think throwing in the obscure PyMethodDef type makes the API confusing.

---

I'd suggest to encapsulate PyMethodDef's fields independently of the chosen design.

[sebpuetz]

I thought a bit more about your proposal and it's really not clear to me how an API for PyModuleDef should look like. The straight-forward version would replicate the constructors that we have now on PyCFunction, so we'd simply copy the signatures over to PyModuleDef::new_c_func_with_kwargs and PyModuleDef::new_c_func.

What did you have in mind?

[davidhewitt]

FWIW I think the most efficient solution is for PyModule::add_function() to take PyMethodDef:

fn add_function(&self, def: PyMethodDef) -> PyResult<()>

Using the currently proposed PyCFunction::new_with_keywords constructor is not zero-cost, because the function reference is going to be put into object storage temporarily.

I want to suggest it's enough in this PR to just change add_function to take PyMethodDef and figure out the right design for the PyCFunction constructor in a separate PR.

In my eyes this is is now the only blocker to the release of 0.12. So I'd like us to agree on a minimal API we expect to be stable, merge that ASAP, and design the rest in follow up PRs once 0.12 is out the door.

[sebpuetz]

I don't think it's a "just put the PyMethodDef in there"-situation. We need to discuss a safe and sane API for PyMethodDef to do that. Right now every field is public and that comes with a bunch of pitfalls and opportunities for bugs. So whichever way we take, we'll be discussing what belongs into either PyMethodDef's API or PyCFunction's API. It seems like it's just a question of where to put the complexity.

I feel like making PyMethodDef the type that users have to interact with to get PyCFunction is a bit odd since it's just a proxy to configure the PyCFunction, eventually the same parameters and arguments need to be passed.

Using the currently proposed PyCFunction::new_with_keywords constructor is not zero-cost, because the function reference is going to be put into object storage temporarily.

This should be identical to what the proc-macro is doing (apart from the two string copies), at least I copied the expanded code? Although, I might be misunderstanding what you are referring to.

[davidhewitt]

Although, I might be misunderstanding what you are referring to.

Currently wrap_pyfunction returns a PyObject, which owns the pointer value. Returning &PyCFunction on the other hand means putting the pointer into pyo3's thread-local object storage, which has a slight cost. Returning Py<PyCFunction> would be the zero-cost equivalent.

Though I'm hoping we revisit this cost for 0.13. So maybe it's ok if we just take &PyCFunction in add_function for now...

[sebpuetz]

That cost should be negligible since it only happens once while initializing the module?

I don't know enough about the differences between &PyX vs Py<PyX> (yet) so I simply went with what I knew

[kngwyu]

I just thought most users don't want to construct PyCFunction by themselves, as I do, and then having a simple constructor with simple implementation would be better.

So I think it's better to begin from thinking of the use case of PyCFunction::new other than add_module(wrap_pyfunction!()), if we make it not-#[doc(hidden)] API.

[sebpuetz]

I have reverted the last commit adding the constructors, feel free to add an API that works for you.

The motivation to have them was offering a transparent way of constructing a PyCFunction instead of relying on proc-macro-magic and a (confusing) decl-macro.

I don't agree that the constructors are too complex either, e.g. this would have been how to construct a PyCFunction:

#[pyfunction]
fn bar<'a>(fun: &'a PyCFunction) {
    [...]
}

#[pymodule]
fn module_with_functions(_py: Python<'_>, module: &PyModule) -> PyResult<()> {
    let doc = "some docs";
    let fun = PyCFunction::new_with_keywords(pyo3::raw_pycfunction!(bar), "name", some_docs, module)?;
    module.add(fun)
}

I's clearer and allows for runtime interaction with the constructors. It was also simple to manually implement a PyCFunction without keywords, which, if I don't miss anything, is impossible without manually constructing PyMethodDef and invoking the CAPI constructor.

[kngwyu]

OK, then let's go in that direction. Please push the reverted commit again.

[davidhewitt]

Good points. I think most users will still want the "standard" use case to be as short as possible to type, so perhaps we can introduce a two-argument form of wrap_pyfunction which returns the PyCFunction? E.g.

module.add_function(wrap_pyfunction!(foo, module))

[kngwyu]

The implementation could be really complicated from the current status so I started some refactoring #1169

[kngwyu]

Hmm... 🤔

I think it still needs PyCFunction::from_method_def to implement this efficiently assuming that ml_doc: &'static str.
I don't think doubly allocating the document for all cases is the way to go.

[sebpuetz]

Fair enough, we probably don't want to keep every docstring twice, so we can leave it up to the caller to provide a &'static str but internally represent it as a CStr?

[davidhewitt]

This looks good to me now for inclusion in 0.12. @kngwyu just checking you're happy with this before I merge? I'd like to create the release PR this evening ideally :)

[kngwyu]

LGTM, thank you !
There's a room for improvements but it would require more internal changes...

[kngwyu]

Why did you change these?

[sebpuetz]

So they make use of the two-argument form of wrap_pyfunction. The other choices are using the soon-to-be-deprecated add_wrapped or calling the function returned by the single-argument form.

[kngwyu]

The other choices are using the soon-to-be-deprecated add_wrapped or calling the function returned by the single-argument form.

But why passing module to PyCFunction_NewEx is such important?
We can set module anywhen.

[sebpuetz]

What do we gain from passing Python instead of the pymodule? I'd say setting the module should be the default for PyCFunctions

[kngwyu]

OK, I got it.
I looked around related APIs but there's no API for setting module other than the m_module field itself. Maybe you're correct.

[sebpuetz]

We can expose this in the api of PyCFunction but I'd be careful with setting this in e.g. add_function. Otherwise a module could "steal" another modules function.

So imo that would need some more (careful) thought to figure out how to deep copy a function when adding it to a module instead of just taking another reference to a function already existing elsewhere.

For now I'd suggest to leave it as is and address this in 0.13

[davidhewitt]

🚀 let's merge and I'll get on with writing release PR for 0.12