Segfault when adding a module to itself

[sebpuetz]

#[pymodule]
fn othermod(_py: Python<'_>, m: &PyModule) -> PyResult<()> {
    m.add_wrapped(pyo3::wrap_pymodule!(othermod))?;
}

>>> import othermod
[1]    15567 segmentation fault (core dumped)  ipython

Problem is not fixed through #1143 either

[sebpuetz]

Importing the module causes recursion because PyInit for the module calls the function returning the module wrapper which in turn calls PyInit for its submodules.

[davidhewitt]

Interesting find. If we wanted to fix this, I guess we could add a check for recursion in the module initialisation?

OTOH, this is a classic infinite recursion caused by malformed user code, there's probably endless ways users can write these and I don't believe any of them are unsound?

[sebpuetz]

This could be fixed by changing add_submodule to take &PyModule instead of the wrapper. The recursion only happens because we're doing the full initialization dance for each submodule with the PyInit call.

• impossible to accidentally add #[pyfunctions] with add_submodule (currently possible)
• add_submodule becomes a lot more transparent.
• If both submodule and toplevel should be instantiable, #[pymodule] can be placed on the submodule fn, otherwise it can just be a method modifying the passed module.

fn submodule(module: &PyModule) -> PyResult<()> {
    module.add("foo", "bar")
}

#[pymodule]
fn submodule_can_be_instantiated(_py: Python, module: &PyModule) -> PyResult<()> {
    module.add("foo", "bar")
}

#[pymodule]
fn module_with_functions(_py: Python<'_>, _mod: &PyModule) -> PyResult<()> {
    let submod = PyModule::new(_py, "submod")?;
    submodule(submod)?;
    _mod.add_submodule(&|py| submod.into_py(py))?;
    let submod = PyModule::new(_py, "another_module")?;
    submodule_can_be_instantiated(_py,submod)?;
    _mod.add_submodule(&|py| submod.into_py(py))?;
    Ok(())
}

Maybe we can take it a step further and provide PyModule::create to initialize a PyModule but I haven't figured out how to keep the PyModuleDef alive long enough.

[davidhewitt]

Maybe as an alternative, we could work on the #[pymodule] syntax suggested in #694 ? I think that syntax would be awesome as it makes python modules more like Rust modules. And I guess we could design that syntax / implementation to make the infinite recursion impossible :)

[sebpuetz]

Do you see any advantage of having add_module take a wrapper over taking &PyModule? That could be changed independently of the full module rework discussed in #694 and changed for #1143 since that introduces the add_module in the first place.

[davidhewitt]

I guess the downside of add_module taking a module instead of a wrapper is that we have to teach a slightly different pattern in our documentation versus what currently exists. And it might be confusing to users whether they should put #[pymodule] on everything or not.

Also, it creates more work for users to migrate from add_wrapped to add_submodule if it's more than just a trivial rename.

But I don't feel too strongly. I'm just not sure whether it's worth fixing this edge case right now when maybe we can just keep it in mind for an upcoming (bigger) redesign?

[sebpuetz]

Although it changes existing patterns, I think dumping the wrap_pymodule makes the API as a whole more transparent. Right now there is such a large amount of magic going on around #[pyfunction] wrap_pyfunction, add_wrapped, etc. that it's really hard to grasp.

I think it's pretty straight forward to say, if you want a submodule in your module:

1. Create your module: PyModule::new()
2. Add your members to the module
3. Add your module to the top level module.

Done!

It's clear where the module is created, it's straight forward what you're adding to it and it enables adding already existing modules, thus making the API more flexible. E.g. imagine this fn:

#[pyfunction(pass_module)]
fn add_some_module_to_slf(slf: &PyModule, other: &PyModule) {
    slf.add_submodule(other)
}

Of course it's also possible to go through slf.add(other.name()?, other), but that argument can be brought up against all add_X functions.

Also, it creates more work for users to migrate from add_wrapped to add_submodule if it's more than just a trivial rename.

I was hoping to get a stronger typed API, it's possible to add any PyObject to a module through add_submodule as long as it's generated from a &impl Fn(Python<'a>) -> PyObject. We could probably also change the signature of the wrapper to return &PyModule. But then I'd again miss the advantage over just directly producing the PyModule. I think the wrapper is only necessary to allow importing the module, right?

The same holds for add_function but here we don't have a definition of a Rust type to rely on. That'd entail larger changes, which I'd also be in favour of. Have add_function take &PyFunction (iirc doesn't exist) and add_module take &PyModule.

edit: I'm pushing for this now since we're now introducing these new APIs. We intentionally left in add_wrapped for compatibility reasons, so I think it's reasonable to assume that people migrating to the new APIs can also adapt to the new APIs.

[davidhewitt]

👍 ok, you've convinced me 😅

One final question: should add_submodule take a &str and return the new &PyModule? That would mean no need to call PyModule::create first.

[davidhewitt]

Maybe while what I propose is nice, it means submodules can't be imported directly? Not sure if that's an issue.

[sebpuetz]

From my tests they all behave the same, but I haven't implemented the changes to add_submodule, I just faked it by using a closure &|py| submod.into_py(py) in place of wrap_pymodule!(). I'll implement the changes later and run unit tests to verify that everything still works the same.

[davidhewitt]

Is this now to be closed with #1143 merged?

[sebpuetz]

Yup, should be done!