Skip to content

Enable deriving on generic types#10

Open
qm3ster wants to merge 1 commit intoPoiScript:masterfrom
qm3ster:generics
Open

Enable deriving on generic types#10
qm3ster wants to merge 1 commit intoPoiScript:masterfrom
qm3ster:generics

Conversation

@qm3ster
Copy link

@qm3ster qm3ster commented May 9, 2020

I actually made this on top of v5.0, because master is currently kill for me.
But the change is outside the impl_x functions, so it might just merge.

Does this need any tests?

@PoiScript
Copy link
Owner

Thanks for the pr! But we cannot add XmlWrite and XmlRead bounds for T. Because it's only required for child field.

@PoiScript
Copy link
Owner

Ok, it's trickier than I thought. We may need to introduce a new trait XmlReadOwned for preventing some lifetime issues, just like serde does.

@PoiScript PoiScript mentioned this pull request May 10, 2020
PoiScript added a commit that referenced this pull request May 10, 2020
PoiScript added a commit that referenced this pull request May 10, 2020
@PoiScript
Copy link
Owner

This was implemented by 2ba2a40. Check out this for a working example. Closing.

@PoiScript PoiScript closed this May 10, 2020
@qm3ster
Copy link
Author

qm3ster commented May 10, 2020

Henlo, can you tell me what was wrong with my PR, for educational purposes?
I did feel the issues with lifetimes when using my branch, and was looking how to solve it with my owned type.
I thought maybe by saying that the return is valid for 'static if the '__input got zero other lifetimes bound to it? So it would still be one trait?

@qm3ster
Copy link
Author

qm3ster commented May 10, 2020

Actually, it seems like your version doesn't do the same thing.
Since you don't add the bounds on generic parameters on the impl, they have to be added on the generic type. Unlike what is done in serde.
Can you clarify what you meant by

Because it's only required for child field.

Is this about attribute types only requiring string conversions rather than Xml conversions?

@PoiScript
Copy link
Owner

I thought maybe by saying that the return is valid for 'static if the '__input got zero other lifetimes bound to it? So it would still be one trait?

I'm afraid it doesn't work either. You can use 'static in the impl block, but that means the from_str function will only accepts &'static str:

use strong_xml::{XmlRead, XmlResult, XmlReader}; 

struct Foo;

impl XmlRead<'static> for Foo {
    fn from_reader(_: &mut XmlReader<'static>) -> XmlResult<Foo> {
        Ok(Foo)
    }
}

fn main() {
    let _ = Foo::from_str("");

    let s = String::new();

    let _ = Foo::from_str(s.as_str());
}
   |
16 |     let _ = Foo::from_str(s.as_str());
   |                           ^---------
   |                           |
   |                           borrowed value does not live long enough
   |                           argument requires that `s` is borrowed for `'static`
17 | }
   | - `s` dropped here while still borrowed

Is this about attribute types only requiring string conversions rather than Xml conversions?

Indeed, they only need to be bound by FromStr + Display.

Since you don't add the bounds on generic parameters on the impl, they have to be added on the generic type. Unlike what is done in serde.

Thanks for pointing out. Actually, I haven't tried it before and looks like serde is smart enough to determine which felids should be bound.

I'm going to reopen this issue. But it'll probably take some time to implement. Feel free to update this pr if you have any idea about the implementation.

@PoiScript PoiScript reopened this May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants