PyO3 v0.21 beta pyfunction annotation coverage

[reaperhulk]

In PyO3 v0.21.0-beta.0 we (pyca/cryptography) are seeing an issue where some #[pyo3::prelude::pyfunction] annotations on functions are marked as uncovered lines. This appears to occur only on functions where the function cannot error.

An example function:
https://github.com/pyca/cryptography/blob/e9954a0a31db22201b96d62535f51a5f0316e218/src/rust/src/backend/x25519.rs#L19-L24

I'm sure I can build a minimal reproducer if necessary, but if this is sufficient information then great 😄

[reaperhulk]

#1726 may be relevant as a past similar issue

[alex]

My specific hypothesis in this case is that the error code paths are never covered.

[davidhewitt]

My specific hypothesis in this case is that the error code paths are never covered.

I guess this is probably true, though it's tricky to know what we can do about it. #[coverage(off)] isn't available. Should we be adding #[automatically_derived]? I think in 1.77 they no longer get instrumented: rust-lang/rust#120185

We might be able to modify the generated code be smarter to make the compiler not care about the error paths, if that is indeed the case.

Would you be willing to use cargo expand to test against a manually expanded form of the problematic functions and see what lines are reported as missing coverage?

[alex]

I think adding automatically_derived for the generated functions makes sense (I assume pyo3 generates a function which calls the user defined function?). That'd be consistent with what we want at any rate.

[reaperhulk]

I used cargo expand (and some judicious copy/paste) to pull out (hopefully) the expanded form of generate_key on one of our uncovered paths:

[davidhewitt]

Aha that red block is the new code we added to support declarative modules, cc @Tpt

I think what we can probably do is refactor that code into traits so that we don't need to emit a function directly in that position. I'm happy to have a go at this but might not be until the weekend or so, so if someone is happy to try sooner please do!

[alex]

FYI, I think there's two other situations in which we aren't getting coverage -- one on pyclass and one on import_exception. I've poked @reaperhulk to see if we can get clean screenshots on those as well.

[davidhewitt]

It is very likely the same code for supporting those types. 👍

[alex]

It looks like for import_exception this is already in an impl PyAddToModule.

Is the goal of moving them to an impl that it should be easy to add the #[automatically_derived]?

[Tpt]

I think what we can probably do is refactor that code into traits

The tricky thing here is that traits can be implemented by stucts and enum but not function and modules so it won't work with #[pyfunction] and #[pymodule]

[davidhewitt]

Agreed that the functions and modules can't directly implement traits, but their helper MakeDef structs can.

I think rather than adding #[automatically_derived] my ideal would be that the trait doesn't even need a function implementation emitted.

Maybe something like this:

#[doc(hidden)]
mod generate_key 1 {
    pub (crate) struct MakeDef;
    pub const DEF: ::pyo3::impl_::pymethods::PyMethodDef = MakeDef::DEF;
｝
// change this bit to a trait implementation?
impl ::pyo3::impl::pymodule::PyAddToModule for generate_key::MakeDef {
    const DEF: ::pyo3::impl_::pymethods::PyMethodDef = :: pyo3::impl_::pymethods::PyMethodDef::noargs (...);
}

I think that code isn't exactly right, but hopefully it conveys the idea. Really all we need to do is get a constant exposed, and if the namespaces look the same, (e.g. function::DEF, module::DEF, Class::DEF) then the proc macros should be able to build something which some traits inside PyO3's impl_ module can then handle.

[reaperhulk]

Alex attempted implementing some of this in main...alex:pyo3:add-to-module-auto but pyclass/import_exception coverage did not improve in our tests. 😢

[Tpt]

I think that code isn't exactly right, but hopefully it conveys the idea. Really all we need to do is get a constant exposed, and if the namespaces look the same, (e.g. function::DEF, module::DEF, Class::DEF) then the proc macros should be able to build something which some traits inside PyO3's impl_ module can then handle.

Yes! The trick is to figure out the proper bounds for the trait implementation and, sadly, I am not sure to have figured them out, mostly because of the rule that prevents more than one blanket implementation

[alex]

Well, we also need to figure out why it is that #[automatically_derived] on impl PyAddToModule doesn't seem to be sufficient to get coverage.

[davidhewitt]

From @alex in #4009

• All invocations of pyo3::import_exception
• Exactly one pyclass
• One other bizzaro line (that's probably not related to pyo3)

Would you perhaps be willing to expand the pyclass and see what looks to be related there? I'll take a look at import_exception now, hopefully that's obvious...

[alex]

Yup, Paul's doing so now.

…

On Fri, Mar 29, 2024 at 7:56 AM David Hewitt ***@***.***> wrote: From @alex in #4009 All invocations of pyo3::import_exception Exactly one pyclass One other bizzaro line (that's probably not related to pyo3) Would you perhaps be willing to expand the pyclass and see what looks to be related there? I'll take a look at import_exception now, hopefully that's obvious... — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

[davidhewitt]

For import_exception, what I think might be going on is that we now have a ton of compatibility code in there so that the imported exception works either as a GIL Ref or as a Bound<T>.

I wonder if a solution would be to have import_exception_bound as a short-term workaround which only implements the necessary semantics for Bound.

[reaperhulk]

I expanded out one of the import exception invocations:

[alex]

The answer on pyclass was that it was a #[pyo3(get)] on an attribute that was uncovered. We've added a test there, so no action required from pyo3.

[alex]

Well, I've got good news and I've got news. The good news is that we've managed to get to 100% coverage: pyca/cryptography#10671

The news is that this was done by dropping some of the migrations to bound APIs (compare with @reaperhulk's PR pyca/cryptography#10633).

My preference at this point would be to get an 0.21.1 release out with the coverage improvements we have, and then we'll do follow up PRs based on far more specific issues as we burn down the usage of the gil-refs feature.

[davidhewitt]

I'm thinking similarly re 0.21.1.

I'd like to try to add import_exception_bound! which doesn't emit the GIL Refs implementations, as I think that'll be helpful for you. In my head I push that tonight and then we get 0.21.1 out over the rest of the Easter weekend.

[alex]

Works for me.

…

On Sat, Mar 30, 2024 at 9:42 AM David Hewitt ***@***.***> wrote: I'm thinking similarly re 0.21.1. I'd like to try to add import_exception_bound! which doesn't emit the GIL Refs implementations, as I think that'll be helpful for you. In my head I push that tonight and then we get 0.21.1 out over the rest of the Easter weekend. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

[alex]

Is there anything else to get in for 0.21.1?

[davidhewitt]

Not any more :)

https://github.com/PyO3/pyo3/releases/tag/v0.21.1

[alex]

Going to declare this resolved for now, with an expectation that follow up tickets may be filed with more specific coverage issues.

[davidhewitt]

Sure thing. Thanks for the reporting and help identifying / fixing!