gperinazzo / dict-derive

Derive PyO3's FromPyObject to automatically transform Python dicts into Rust structs
Apache License 2.0
105 stars 10 forks source link

Deriving FromPyObject on a struct with lifetimes results in a weird error #12

Open vorner opened 4 years ago

vorner commented 4 years ago

Hello

I'm playing with the library and I've received somewhat weird error in one case:

#[derive(Clone, Debug, IntoPyObject, FromPyObject)]
struct D2<'a> {
    a: usize,
    b: &'a str,
}
  --> src/lib.rs:24:11
   |
23 | #[derive(Clone, Debug, IntoPyObject, FromPyObject)]
   |                                      - help: consider introducing lifetime `'a` here: `'a,`
24 | struct D2<'a> {
   |           ^^ undeclared lifetime

This is the problematic code that gets generated:

impl<'source> ::pyo3::FromPyObject<'source> for D2<'a> {

Indeed, the lifetime would have to appear on the impl token, beside 'source, but then, it would probably not satisfy the lifetime bounds.

I understand that creating borrowed structures from dicts might be problematic (or maybe impossible). But I think the error might be slightly better, because it shows quite an usual error message at a place where it just can't happen. Would it make sense to refuse the situation with a custom error message, eg. „Deriving structures with lifetimes is not supported“?

gperinazzo commented 4 years ago

It should be possible, I'll have a look. To simplify it should error for structs with more than one lifetime, but if you"re using a single one, it can use the 'source lifetime.

vorner commented 4 years ago

Interesting. Using 'source there works. But I'd have guessed these things should not work because of macro hygiene and that should be a different 'source than the one introduced by the macro :confused:

Thanks for the tip (though it would be nice if it worked with whatever lifetime or if it was at least documented in a the readme).

gperinazzo commented 4 years ago

I should be able to have the derive automatically generate the line as impl<'source> FromPyObject<'source> for MyType<'source>, regardless of what you named the lifetime in your type declaration.

gperinazzo commented 4 years ago

@vorner Could you try running on top of the current master, and see if this works for your use-case? I've updated it to use the lifetime declared on the structure template parameters, and use 'source if the structure doesn't have any. It should now work with arbitrary lifetime names!

Also added an explicit error if there's more than one lifetime. Would have to look into how to map that case to the single lifetime that the trait uses. Maybe using the same lifetime for all the parameters would work, but I'll investigate later.

vorner commented 4 years ago

Thanks. I've been only exploring it so I have nothing but the snippet I've sent, but yes it works (or at least compiles).

As for the multiple lifetimes, I guess it could work by requiring that 'source outlives all the individual lifetimes (instead of conflating them to the same one, which might be problematic in case some of them are invariant or how that thing is called; I'm never sure if it should be 'a: 'b or the other way around).

impl<'a, 'b, 'source: 'a + 'b> FromPyObject<'source> for X<'a, 'b>

But I think the error message is good enough solution, considering this is more aimed at converting simple „value“ structs and multiple lifetimes probably makes sense only on some very complex things where one wouldn't want the auto-implementation anyway.