Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate CoordsIter::T to be an associated type. #593

Merged
merged 2 commits into from
Jan 4, 2021
Merged

Conversation

frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Jan 3, 2021

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Previously

With the previous design, this would compile:

impl<'a> CoordsIter<'a, f32> for Polygon<f32> {
    ...
}

impl<'a> CoordsIter<'a, f64> for Polygon<f32> {
    ...
}

Even though this is not what our implementations actually look like, to the compiler it's possible for there to be multiple implementations of CoordsIter for Polygon<f32>.

This was problematic when I started rewriting our ExtremeIndices trait to be generic over any type that implements CoordsIter:

impl<'a, T, G> ExtremeIndices for G
where
    T: CoordinateType,
    G: CoordsIter<'a, T>,
{
    fn extreme_indices(&self) -> Result<Extremes, ()> {
        ...
    }
}

This is the compilation error:

the type parameter `T` is not constrained by the impl trait, self type, or predicates

The issue is that if someone writes this with this new ExtremeIndices implementation:

let p: Polygon<f32> = polygon![...];
p.extreme_indices()

The compiler doesn't know which implementation to choose: impl<'a> CoordsIter<'a, f32> for Polygon<f32> or impl<'a> CoordsIter<'a, f64> for Polygon<f32>.

So we need a way to tell the compiler we only have one implementation of CoordsIter per type. The root issue being that there is a free generic parameter in the CoordsIter definition.

Now

To fix this, we will move the T type parameter to an associated type. Writing this would be a compiler error:

impl<'a> CoordsIter<'a> for Polygon<f32> {
    type Scalar = f32;
    ...
}

impl<'a> CoordsIter<'a> for Polygon<f32> {
    type Scalar = f64;
    ...
}
conflicting implementations of trait `algorithm::coords_iter::CoordsIter<'_>` for type `geo_types::Polygon<f32>`

This unblocks #592

@frewsxcv frewsxcv mentioned this pull request Jan 4, 2021
2 tasks
Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've read about the distinction, but still haven't really internalized an intuitive grasp on when to apply associated types vs. generics, but this is a clear and helpful example. Thank you!

@michaelkirk
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 4, 2021

Build succeeded:

@bors bors bot merged commit 0b38c9b into master Jan 4, 2021
bors bot added a commit that referenced this pull request Jan 5, 2021
594: Add `CoordsIter::exterior_coords_iter` r=michaelkirk a=frewsxcv

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

This will get incorporated into #592

Fixes #542

~Based on and blocked by #593

Co-authored-by: Corey Farwell <coreyf@rwell.org>
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