Jij-Inc / pyo3-stub-gen

Stub file (*.pyi) generator for PyO3
Apache License 2.0
56 stars 11 forks source link

Trait bound `PathBuf: PyStubType` is not satisfied #51

Closed michaeljones closed 2 months ago

michaeljones commented 2 months ago

Thank you for this project. It looks really interesting and solves a real problem.

I'm trying it on my code base and getting the error the trait boundPathBuf: PyStubTypeis not satisfied from an PathBuf argument in a method. Is there a way to specify PyStubType for PathBuf or am I looking at it wrong? I think the rust orphan rules (or whatever they are called) would stop me from doing that in my code base but I might be wrong.

I think I should either switch from PathBuf to String (or &str) or request that PathBuf be supported in the library as PyO3 seems to be happy with it?

I'm not very experienced in all this though so sorry if any of this is based on a simple mistake. Thanks again for the project.

michaeljones commented 2 months ago

Separately, and I can make a ticket for this if you like, we are using _ as an argument name for the &Bound<'_, PyType> argument to a classmethod and I get the following error:

error: Expected typed argument
   --> project/src/lib.rs:107:9
    |
107 |         _: &Bound<'_, PyType>,
    |         ^

error: could not compile `nrf-probe-py` (lib) due to 1 previous error

Is that expected?

Shitaro commented 2 months ago

Thank you for your question. IMO, I believe this crate needs to support std::path::PathBuf. For PyO3 users, it's natural to expect that types implementing the FromPyObject trait (see this) would also satisfy the PyStubType trait. As it stands now, with PathBuf not implementing PyStubType, users might feel a sense of inconsistency. Therefore, supporting types that implement FromPyObject would be a more comprehensive solution.

To address this issue, I think a good approach would be to add support for types provided by the standard library. Specifically, we could add a file with a name that clearly indicates it deals with standard library types, such as standard.rs, in the src/stub_type directory. Within this file, we would implement the PyStubType trait for all standard library types that implement the FromPyObject trait:

use crate::stub_type::*;

use std::path::PathBuf;

impl PyStubType for PathBuf {
    fn type_output() -> TypeInfo {
        TypeInfo {
            name: "str".to_string(),
            import: HashSet::new(),
        }
    }
}

However, we need to consider whether str is truly the correct conversion target for PathBuf. According to Mapping of Rust types to Python types in the PyO3's user guide, there are three Python types corresponding to PathBuf:

A similar discussion would be necessary for std::net::IpAddr as well.

Shitaro commented 2 months ago

Separately, and I can make a ticket for this if you like, we are using _ as an argument name for the &Bound<'_, PyType> argument to a classmethod and I get the following error:

error: Expected typed argument
   --> project/src/lib.rs:107:9
    |
107 |         _: &Bound<'_, PyType>,
    |         ^

error: could not compile `nrf-probe-py` (lib) due to 1 previous error

Is that expected?

Hmm... That does seem like it might be expected behavior, but... I'd definitely appreciate if you could create a separate ticket for this issue. Let's focus on the PathBuf matter in this current issue.

termoshtt commented 2 months ago

impl PyStubType for PathBuf has been added in #52 ,and released as 0.5.1

michaeljones commented 2 months ago

Thank you for the replies and the new support. I'm glad you think it is a suitable thing for the library to handle. I'll give it a shot today and how it goes and I'll make another ticket for the separate issue.

Thanks again!