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

RangeInclusive has pub(crate) fields. This could be pub. #67371

Closed
Byter09 opened this issue Dec 17, 2019 · 2 comments · Fixed by #67490
Closed

RangeInclusive has pub(crate) fields. This could be pub. #67371

Byter09 opened this issue Dec 17, 2019 · 2 comments · Fixed by #67490
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Byter09
Copy link

Byter09 commented Dec 17, 2019

Hello.

I'm currently writing a library that generalizes all ranges and adds a "range set".
Core/Std compatibility is a must and I want to make the crate "feel" as if it would already be part of them. This, of course, requires From implementations for all range types for my GenericRange<T>.

All cases, for example Range<T> are trivial:

impl<T> From<Range<T>> for GenericRange<T>
where
    T: PartialOrd,
{
    fn from(range: Range<T>) -> Self {
        Self::closed_open(range.start, range.end)
    }
}

(note: this requires PartialOrd, because upon converting I assert, that start <= end to optimize other code that depends on this requirement)

The problem I'm facing now is that I want to keep clones at a minimum, but RangeInclusive<T> forces me to commit this atrocity:

impl<T> From<RangeInclusive<T>> for GenericRange<T>
where
    T: Clone + PartialOrd,
{
    /// This requires `Clone` because for some reason the internal fields of `RangeInclusive` are
    /// only `pub(crate)` even though they could be `pub` and so we're forced to use the results of
    /// `RangeBounds`, which only return borrowed values.
    fn from(range: RangeInclusive<T>) -> Self {
        Self::closed(range.start().clone(), range.end().clone())
    }
}

As mentioned in the doc comment and in the title of this issue, RangeInclusive<T> has pub(crate) fields, which prohibits other users, like me, to correctly take the fields without copying.

The crate is forbid(unsafe_code) and #![no_std] and I have thus no other way to implement From other than cloning the RangeBounds trait results.

If there is a reason for this I'm sorry for opening an issue - but I don't see one.

Thanks for taking the time to look at this!

@jonas-schievink jonas-schievink added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 17, 2019
@memoryruins
Copy link
Contributor

It is certainly different from other ranges. #49022 links to some reasons and their respective PRs.
Notably, it mentions the RangeInclusive::into_inner method, which may be what you want here.
https://doc.rust-lang.org/core/ops/struct.RangeInclusive.html#method.into_inner

pub fn into_inner(self) -> (Idx, Idx) {
    (self.start, self.end)
}

@Byter09
Copy link
Author

Byter09 commented Dec 18, 2019

@memoryruins Thank you so much. That is exactly what I wanted. I'm so sorry I did not see this.

If this issue is not relevant anymore, I'd close it, but I'm not sure this issue will not come up again in the future by someone else. It just looks inconsistent and a comment or an indicator as to why it is not fully public should be added.

Centril added a commit to Centril/rust that referenced this issue Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants