Proposal: Common abstraction layer for collection types (via `Deref` and `DerefMut`)

RobWalt commented 10 months ago

I'll try to refine and update this proposal here.

Initial Idea - Not good

The PR https://github.com/georust/geo/pull/1109 made me question:

Couldn't we potentially resolve the issue by implementing Deref and DerefMut for the type instead of re-exposing the methods manually?

Obviously, this isn't a good idea since we would also re-expose a lot of other methods from Vec which wouldn't necessarily be useful for the geometry collection types.

Refined Idea - also not working

Thinking a tiny bit more about it, it might be possible to do something similar still. What if we would just do the following:

(heavily simplified to capture the essence of the idea)

struct GeoVec<T>(Vec<T>);

impl<T> GeoVec<T> {
  // ... impl common functionality for all geo collection types (len, iter, ...)
}

struct MultiPoint(GeoVec<Point>);

impl Deref for MultiPoint {
  type Target = GeoVec<Point>;
  // ... rest
}

A lot of people (myself included) use multi_polygon.0.len() already in their code bases. In case of len it wouldn't break code if we included it in the interface for GeoVec but if there is something we don't include there, then we break existing code. So we would also need to implement

impl<T> Deref for GeoVec<T> {
  type Target = Vec<T>; 
  // ... rest
}

Current Proposal

A macro or trait which unifies the common behavior MultiPoint, MultiLineString, MultiPolygon, and GeometryCollection should expose.

michaelkirk commented 10 months ago

it might be possible to do something similar still.

Since Deref will be applied recursively, doesn't this leave us at the same place as you were trying to avoid, mentioned here:

this isn't a good idea since we would also re-expose a lot of other methods from Vec

I confess I am kind of a Deref coward. It's useful to work around the "Orphan Rule" and for translating between Rust's zoo of smart pointers, but it has somewhat far reaching consequences, so I try to avoid it.

For example, it's harder to find methods in the documentation when they are gained via Deref, and it potentially adds a lot of API surface that you're committing to.

We're talking about 4 collections, right? MultiPoint, MultiLineString, MultiPolygon, and GeometryCollection.

As a more cowardly alternative, I'd be happy to audit and implement consistent "collection" methods on these 4 types. It could be consolidated into a private macro_rule if there is concern about consistency of implementation.

RobWalt commented 10 months ago

Since Deref will be applied recursively, doesn't this leave us at the same place as you were trying to avoid, mentioned here

Woops, yes. I even tried out other behavior in a playground but forgot that it traverses further down the references if a method is missing 🙈

For example, it's harder to find methods in the documentation when they are gained via Deref, and it potentially adds a lot of API surface that you're committing to.

Makes total sense. I understand and now also share your concerns! Especially because my original idea isn't working as expected and probably won't work like that anyways

As a more cowardly alternative, I'd be happy to audit and implement consistent "collection" methods on these 4 types. It could be consolidated into a private macro_rule if there is concern about consistency of implementation.

That's definitely something I would be fine with. The only other approach we would probably need to evaluate then is something like a GeoCollection trait.

georust / geo