Feature reading not reseted between two FeatureIterator instanciation

[dmarteau]

Between two calls of Layer::features(&self) -> FeatureIterator, there is no call to OGR_L_ResetReading: the consequence is that features cannot be iterated twice from a Layer and the Layer has to be recreated.

Ex

let mut ds = Dataset::open(fixture!("roads.geojson")).unwrap();
let layer = ds.layer(0).unwrap();

assert_eq!(layer.features().count(), 21);  <- Ok
assert_eq!(layer.features().count(), 21);  <- Fail, gives 0 !

As the Layer is not consummed by the iterator and taken from an immutable layer, the internal state should not be changed and it should be possible to get a new iterator over all features.

Since iterating features change the Internal state of a GDAL layer and having two iterator at the same time on the same layer is hazardous, may be the Iterator should be extracted from a mutable layer: Layer::features(&mut self) -> FeatureIterator

see https://gdal.org/api/vector_c_api.html#_CPPv418OGR_L_ResetReading9OGRLayerH

[rmanoka]

This is a good catch; I agree we should make the iterator take and store &mut self. Currently we document in Layer::feature that it should not be called while iteration, but taking mut ref seems like a cleaner solution. Do you suggest we reset the reading when the iterator is constructed?

Related to #150

[dmarteau]

Do you suggest we reset the reading when the iterator is constructed?

Yes, that the first thing that comes to me.

I agree we should make the iterator take and store &mut self

That was my first idea but I'm struggling with Iterator returning object with lifetime from borrowed field from the mutable reference :-p

Using

pub struct FeatureIterator<'a> {
    layer: &'a mut Layer<'a>,
}

gives:

error[E0495]: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
   --> src/vector/layer.rs:261:51
    |
261 |             Some(unsafe { Feature::from_c_feature(&self.layer.defn, c_feature) })
    |                                                   ^^^^^^^^^^^^^^^^
    |
note: first, the lifetime cannot outlive the anonymous lifetime #1 defined on the method body at 256:5...
   --> src/vector/layer.rs:256:5
    |
256 |     fn next(&mut self) -> Option<Feature<'a>> {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...so that reference does not outlive borrowed content
   --> src/vector/layer.rs:261:51
    |
261 |             Some(unsafe { Feature::from_c_feature(&self.layer.defn, c_feature) })
    |                                                   ^^^^^^^^^^^^^^^^
note: but, the lifetime must be valid for the lifetime `'a` as defined on the impl at 252:6...
   --> src/vector/layer.rs:252:6
    |
252 | impl<'a> Iterator for FeatureIterator<'a> {
    |      ^^
note: ...so that the expression is assignable
   --> src/vector/layer.rs:261:13
    |
261 |             Some(unsafe { Feature::from_c_feature(&self.layer.defn, c_feature) })
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: expected `Option<Feature<'a>>`
               found `Option<Feature<'_>>

So I don't know how to make the compiler happy and infer the lifetime for the mutable référence. It seems that requires some magic I do not master atm.

The easiest solution would be to stay with a non-mutable reference and stick with the intention note but that's sound unsatisfactory.

[rmanoka]

It is a bit more complicated to get it working: we have to store the &layer.defn, and the handle layer.c_layer instead of the layer itself in the iterator. I've done this in my fork: https://github.com/rmanoka/gdal/tree/fix/mut-self ; please check it out.

Also added a test that should fail to compile if un-commented (at the end of vector_tests/mod.rs). It verifies that while iterating over the features, the layer can't be borrowed.

fn test_features_mut_lifetime_enforce() {
    with_layer("roads.geojson", |mut layer| {
        for _ in layer.features() {
            let _ = layer.defn();
        }
    });
}

Update: Also added the resetting of iteration, and test to ensure it is being properly reset on creation.

[dmarteau]

Nice ! No black magic...

If I understand well the layer is borrowed mutably by the iterator because of the inherited lifetime of the defn field ?

pub struct FeatureIterator<'a> {
    defn: &'a Defn,
    c_layer: OGRLayerH,
    size_hint: Option<usize>,
}

[rmanoka]

Yeah, it is some complicated magic by the compiler. Apparently, even if we down-grade a mut-ref to a immut-ref, it is still considered as a mut-borrow in the scope where the mut borrow happened. See for eg. this playground

[rmanoka]

As per the nomicon it is apparently a limitation of the borrow checker. This stackoverflow post explains it well.

Interestingly, this "limitation" works out in our favour. We can also make the mut borrow explicit by adding a PhantomData<&'a mut ()> if really needed.

[dmarteau]

Interesting indeed...

Incidentally I have found a working way to cheat with lifetime while keeping a rerence to a mutable layer that also make the job, but it uses some unsafe dark magic:

Just for fun:

pub struct FeatureIterator<'a> {
    layer: &'a mut Layer<'a>,
}

impl<'a> Iterator for FeatureIterator<'a> {
    type Item = Feature<'a>;

    #[inline]
    fn next<'n>(&'n mut self) -> Option<Feature<'a>> {
        let c_feature = unsafe { gdal_sys::OGR_L_GetNextFeature(self.layer.c_layer) };
        if c_feature.is_null() {
            None
        } else {
            let defn: &'a Defn  = unsafe { std::mem::transmute::<&'n Defn,&'a Defn>(&self.layer.defn) };
            Some(unsafe { Feature::from_c_feature(defn, c_feature) })
        }
    }

[rmanoka]

Nice! Note that &'a mut Layer<'a> is actually restrictive. It'll prevent you from ever creating two iterators one after the other. See playground. You'll have to use &'b mut Layer<'a>.