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

add TryFrom for NonZeroInteger types #72712

Closed
wants to merge 2 commits into from
Closed

add TryFrom for NonZeroInteger types #72712

wants to merge 2 commits into from

Conversation

valarauca
Copy link

@valarauca valarauca commented May 28, 2020

Adds TryFrom<{integer}> for NonZero{integer} types.

Piggybacks the existing TryFrom<{integer}> (to handle ranges) as well the NonZero{integer}::new (to handle the zero check). Meaning no new logic is added in this patch. Existing logic is just chained together.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 28, 2020
@jonas-schievink jonas-schievink added relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 28, 2020
@jonas-schievink jonas-schievink added this to the 1.45 milestone May 28, 2020
@sfackler
Copy link
Member

@rfcbot fcp merge

This adds TryFrom impls to convert any integer type to any nonzero integer type.

@rfcbot
Copy link

rfcbot commented May 28, 2020

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 28, 2020
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label May 28, 2020
@rfcbot
Copy link

rfcbot commented May 28, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label May 28, 2020
@SimonSapin
Copy link
Contributor

SimonSapin commented May 29, 2020

@rfcbot concern too much or too little

This PR is conflating two kinds of conversion:

  • Between integers of different bit width or signedness
  • From primitive to non-zero

It’s two different conversions chained, conceptually and even in the implementation. As far as I know there is no precedent so far for this. For example we have u32: From<NonZeroU32> and u64: From<u32>, but not u64: From<NonZeroU32>.

I think I’d prefer for this PR to stick to TryFrom impls that can simply call a new constructor, and let users chain conversions explicitly if they wish.

Or, if uses cases (please explain them!) for chained conversion in a single impl are deemed useful enough, for consistency this PR should also add all combinations of conversions from non-zero to primitive integers. Note that some combinations should be From rather than (only) TryFrom.

@SimonSapin
Copy link
Contributor

@rfcbot concern too much or too little

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 29, 2020
@valarauca
Copy link
Author

Or, if uses cases (please explain them!) for chained conversion in a single impl are deemed useful enough

Given the existence of TryFrom<{Signed}> for {Unsigned} (and its inverse) for all bitwidths. Especially when the std::convert::TryFrom<_> documentation introduces the core concept the trait expresses via integer bit-width conversions. The extension of TryFrom to NonZero{integer} seems obvious?

The primary usecase is reduction in boilerplate and simplicity. The fact the only alternative, to convert all integer types into a NonZero{integer} (without casting), is to write something like:

use std::{convert::TryFrom, num::NonZeroI8};

pub fn into_nzi8<T>(x: T) -> Option<NonZeroI8>
where
    i8: TryFrom<T>,
{
    i8::try_from(x).ok().and_then(NonZeroI8::new)
}

This is some non-trivial generic boilerplate for integer conversions!

Hopefully this speaks for itself as a failure of usability/ergonomics. The added storage semantics of NonZero{integer} do not justify additional ceremony in its creation.

While the chained conversion does increase the complexity. The chain is intuitive based on existing documentation & type names. The declaration TryFrom<u64> to NonZeroI8 speaks for itself. It will accept all values within the (1..=127) range, all others will see an error.


[...] for consistency this PR should also add all combinations of conversions from non-zero to primitive integers. Note that some combinations should be From rather than (only) TryFrom

Could you say more? If this is request for the inverse operations I can facilitate it.

@SimonSapin
Copy link
Contributor

The fact the only alternative, to convert all integer types into a NonZero{integer} (without casting), is to write something like: […]

Right, but how often to you actually want to do that in the first place?

I’m not totally opposed to adding these "implicitly chained" conversions if the rest of @rust-lang/libs is on board. Only this wasn’t apparent in the PR description or FCP proposal, and I feel it should be a deliberate decision.

[...] for consistency this PR should also add all combinations of conversions from non-zero to primitive integers. Note that some combinations should be From rather than (only) TryFrom

Could you say more? If this is request for the inverse operations I can facilitate it.

I mean, assuming we want to go in that direction:

  • For every existing impl From<A> for B where A and B are primitive integer types, add impl From<NonZeroA> for B
  • For every (other) existing impl TryFrom<A> for B where A and B are primitive integer types, add impl TryFrom<NonZeroA> for B

@valarauca
Copy link
Author

Is there anything I can do help advance this decision making process?

AFAIT, the decision is now being left up to Libs-Team internal politics to determine what, if any, future actions are required.

@sfackler
Copy link
Member

I think it might be best to add only the same-width conversions for now and leave the others for a later time.

@valarauca valarauca closed this Jun 3, 2020
@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 3, 2020
@crlf0710 crlf0710 removed the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants