PreparedGeometry lifetimes

[vladaburian]

I have noticed that lifetimes were removed from PreparedGeometry too, which is not correct because PreparedGeometry is holding a reference to the original non-prepared geometry.

Test like this crashes with SIGSEGV:

#[test]
fn lifetime_prepared_geom_sigsegv() {
    let prep_geom = {
        let geom = Geometry::new_from_wkt("POLYGON((0 0, 1 0, 1 1, 0 1, 0 0))").unwrap();
        geom.to_prepared_geom().unwrap()
    };

    let pt = Geometry::new_from_wkt("POINT(2 2)").unwrap();
    _ = prep_geom.contains(&pt);
}

There are actually tests testing this exact case. They are marked as compile_fail, but they are failing to compile for wrong reason (they use lifetimes, which were removed): https://github.com/georust/geos/blob/master/src/prepared_geometry.rs#L366-L410

I'm not sure that the best solution is to introduce lifetime of PreparedGeometry again. The issue with such a lifetime is that it's impossible (AFAIK) to construct geometry and prepared geometry inside a function a return them both without using unsafe rust (which is not elegant and also is dangerous).

I see 2 solutions to this problem:

1. Introduce lifetime of PrepagedGeometry again and add new_owning constructor which returns PreparedGeometry<'static'> so it can be moved around freely, if needed. (PreparedGeometry is optionally owning the original geometry.) This is most universal but also least simple solution (lifetime + 2 different constructors). See PR fix: lifetime of geometry referenced by PreparedGeometry (version 1) #175

2. Keep PreparedGeometry without a lifetime and make it always own the original geometry. It's unnecessarily cloning the geometry right now, but that could be improved. Overally I like this solution as it is simple. See PR fix: lifetime of geometry referenced by PreparedGeometry (version 2) #176 .

Optionally I could keep the public api without any change and clone the geometry inside PreparedGeometry constructor.

[GuillaumeGomez]

Oof. It's pretty bad indeed. I think just holding the lifetime in a PhantomData will be more than enough. I'm not sure it's that common to return both a Geometry and a PreparedGeometry from the same function.

[vladaburian]

I might be wrong, but having lifetime of PreparedGeometry tied to lifetime of reference to Geometry means that I can't move those two around. And that means that I can't encapsulate functionality of PraparedGeometry in any structure or anything. Maybe it can be done and I just don't have good enough knowledge of rust. But this is something that kind of frustrated me, fighting with borrowchecker and not knowing whether I'm trying to do something that maybe actually cannot be done at all. So I'm more in favor of solutions without lifetime dependencies, where PreparedGeometry could exist on it's own, could be freely moved around and be owned by anybody.

Let's say I'm writing some server and I need a component which tests if given point is inside a geometry and based on that it does something (and I want to encapsulate this implementation in a struct). With PreparedGeometry having lifetime, I don't know how to do such a thing - and I think it is probably impossible to do. I would expect that I can do something simple like:

pub struct IsInsideTester {
    // This fails because I need to provide lifetime.
    prep: PreparedGeometry,
}

impl IsInsideTester {
    pub fn new_from_wkt_file(path: &str) -> IsInsideTester {
        let wkt = std::fs::read_to_string(path).unwrap();
        let geom = Geometry::new_from_wkt(&wkt).unwrap();
        let prep = PreparedGeometry::new(&geom).unwrap();
        IsInsideTester { prep }
    }

    pub fn is_inside(&self, g: &Geometry) -> bool {
        self.prep.contains(g).unwrap()
    }
}

But I can't do that. And I don't know how to work around this. Well, I do now, but it's something ugly and a bit dangerous too:

pub struct IsInsideTester {
    // `geom` must outlive `prep` !!!
    geom: Geometry,
    prep: PreparedGeometry<'static>,
}

impl IsInsideTester {
    pub fn new_from_wkt_file(path: &str) -> IsInsideTester {
        let wkt = std::fs::read_to_string(path).unwrap();
        let geom = Geometry::new_from_wkt(&wkt).unwrap();
        let prep: PreparedGeometry<'static> =
            unsafe { std::mem::transmute(PreparedGeometry::new(&geom).unwrap()) };

        IsInsideTester { geom, prep }
    }

    pub fn is_inside(&self, g: &Geometry) -> bool {
        self.prep.contains(g).unwrap()
    }
}

With #176 I can do it easily because there are no lifetimes and PreparedGeometry is owning the related Geometry internally. The code is straightforward and easy to understand I would say:

pub struct IsInsideTester {
    prep: PreparedGeometry,
}

impl IsInsideTester {
    pub fn new_from_wkt_file(path: &str) -> IsInsideTester {
        let wkt = std::fs::read_to_string(path).unwrap();
        let geom = Geometry::new_from_wkt(&wkt).unwrap();
        let prep = PreparedGeometry::new(geom).unwrap();
        IsInsideTester { prep }
    }

    pub fn is_inside(&self, g: &Geometry) -> bool {
        self.prep.contains(g).unwrap()
    }
}

With #175 (and the new new_owned constructor) I can do it too (PreparedGeometry is optionally owning related Geometry). But it's little bit more complicated because I need to know there are 2 constructors and I need to know what static lifetime is and that it's what I need to use in this case. I would says it's not very friendly towards novice rust developers, but at least it can still be done:

pub struct IsInsideTester {
    prep: PreparedGeometry<'static>,
}

impl IsInsideTester {
    pub fn new_from_wkt_file(path: &str) -> IsInsideTester {
        let wkt = std::fs::read_to_string(path).unwrap();
        let geom = Geometry::new_from_wkt(&wkt).unwrap();
        let prep = PreparedGeometry::new_owning(geom).unwrap();

        IsInsideTester { prep }
    }

    pub fn is_inside(&self, g: &Geometry) -> bool {
        self.prep.contains(g).unwrap()
    }
}

So this last solution is the most universal because it let me choose whether I want to be the owner of Geometry and have PreparedGeometry<'a> with limited lifetime OR if I want to create PreparedGeometry<'static> which is itself owning the Geometry, have unlimited lifetime, can be moved around and be owned by anybody.

But the question is whether this complication in api is actually worth it. Someone needs to own the original Geometry anyway, so why not PreparedGeometry itself (in all cases). That would make the api easy to understand and not limiting.