Add TryFrom<{integer}> for bool #50597

ithinuel · 2018-05-10T06:06:58Z

This adds the conversion from and into boolean for integer types.

This is also discused here : https://internals.rust-lang.org/t/from-bool-for-primitive-integer-types

rust-highfive · 2018-05-10T06:07:01Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @shepmaster (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.

scottmcm · 2018-05-10T06:44:37Z

src/libcore/num/mod.rs

+impl_from! { i32, bool, #[unstable(feature = "into_bool", issue = "0")]}
+impl_from! { i64, bool, #[unstable(feature = "into_bool", issue = "0")]}
+impl_from! { i128, bool, #[unstable(feature = "into_bool", issue = "0")]}
+// Boolean -> Integer


There's a PR open for this part at #50554

😮 I'm few hours late

I updated the issue number to #46109

scottmcm · 2018-05-10T06:45:15Z

src/libcore/num/mod.rs

@@ -4580,6 +4602,29 @@ impl_from! { u32, f64, #[stable(feature = "lossless_float_conv", since = "1.6.0"
 // Float -> Float
 impl_from! { f32, f64, #[stable(feature = "lossless_float_conv", since = "1.6.0")] }

+// Integer -> Boolean


This is lossy, so feels questionable whether it should be there in From.

I use from here but I can change to TryFrom if required.

Updated to use TryFrom instead.

ithinuel · 2018-05-10T07:25:27Z

This fixes : #46109.

ithinuel · 2018-05-11T07:02:44Z

src/libcore/num/mod.rs

@@ -4259,6 +4259,25 @@ macro_rules! try_from_both_bounded {
    )*}
 }

+macro_rules! try_bool_from {
+    ($($source:ty),*) => {$(
+        #[unstable(feature = "try_bool_from", issue = "0")]


Should I create an issue for this or is the PR number ok here ?

I am not even sure this should be tagged as unstable.

impls are insta-stable, so they should be marked stable with the corresponding release (currently 1.28, I think).

Ok, Thank you.
So I changed :

the impl From<bool> for .. to stable 1.28.0 with the feature name lossless_bool_conv to be consistent with other lossless conversions.

impl TryFrom<T> for bool to the unstable feature try_from linking Tracking issue for TryFrom/TryInto traits #33417 as the issue number.

ithinuel · 2018-05-14T06:56:03Z

A practical use case of this can be found here : https://github.com/ithinuel/rusty-printer
you will need to build with the target thumbv7m-none-eabi.

cc @TimNN @clarcharr

clarfonthey · 2018-05-14T18:17:06Z

I don't think that TryFrom impls should be added; int as bool doesn't work as-is and most people will interpret this as 0 => true, other => false like C and others do.

ithinuel · 2018-05-14T22:29:43Z

int as bool doesn't work as-is and most people will interpret this as 0 => true, other => false like C and others do.

I am not sure to understand your point. At least in C, C++, JS and python True is 1 and False is 0.
Actually I don't know any language where True is 0.

int as bool does not compile indeed. rustc --explain E0054 explains that :

It is not allowed to cast to a bool. If you are trying to cast a numeric type to a bool, you can compare it with zero instead:
let x = 5;

// Not allowed, won't compile
let x_is_nonzero = x as bool;
let x = 5;

// Ok
let x_is_nonzero = x != 0;

This demonstrates 0 is False, any other value is True.

The implementation proposed here prevents loss by only accepting 1 as True.

clarfonthey · 2018-05-14T22:31:52Z

Err, you're right, I swapped true and false there.

ithinuel · 2018-05-20T16:12:48Z

@scottmcm @clarcharr @shepmaster
Do you have any additional comment ?
What is the next step ?

scottmcm · 2018-05-20T18:01:34Z

@ithinuel I'm torn. On the one hand I think bool: TryFrom<i128> is kinda weird. But at the same time, I like i128: From<bool>, and in general think that T: From<U> should always have a corresponding U: TryFrom<T>. So maybe there's nothing to do here but wait on libs to decide on it.

Well, you could remove the integer: From<bool> part of this, since that's FCP'd to go in as #50554. But if libs were to decide they don't want the TryFrom path that'd be wasted effort, so waiting is fine too.

ithinuel · 2018-05-20T18:24:11Z

Could you elaborate why you find it weird ? here or on internals.

I could indeed remove the From part. I implemented it that way because I wasn't sure true is always 1 and false 0 but it is actually stated in the documentation : https://doc.rust-lang.org/std/primitive.bool.html

EDIT: I just noticed that the other PR is lacking tests.

scottmcm · 2018-05-20T19:27:29Z

@ithinuel Because there's two interpretations. The "bool is i1" interpretation says it should only accept 1 for true, but it's very common in languages to accept anything non-zero as true. That's well-known in C, but also things like .Net's Convert.ToBoolean do it.

ithinuel · 2018-05-20T21:15:44Z

@scottmcm We could argue something equivalent to the conversion from u32 to u8 where the truncation is also well known in a lot of languages.

TBH I don't really mind either way, what really matters is to have TryFrom or even From implemented for booleans too. I opted for this implementation to make it consistent with the opposite operation that always gives 0 or 1 and not 0 or any value with at least 1 bit set.

clarfonthey · 2018-05-20T21:19:48Z

TBQH I think that bool as int would have been better not implemented, as the lack of symmetry tends to trip up API decisions like this. But, because this is part of the language now, it may be better to just have From but not TryFrom here.

shepmaster · 2018-05-21T02:05:42Z

I'm going to go ahead and reassign this to @TimNN who has better context due to #50554.

r? @TimNN

pietroalbini · 2018-05-28T08:52:24Z

Ping from triage @TimNN! This PR needs your review.

TimNN · 2018-05-28T15:32:32Z

We have #50554 for the bool -> int direction, so let's focus this PR on the int -> bool direction, i.e. the proposed implementation of impl TryFrom<int> for bool.

The largest concern I see in the discussion above is that different people could have different expectations for the int -> bool conversion.

Based on the convention 0 = success, * = error, one could argue that 0 -> Ok(true), * -> Ok(false).
To be strictly in line with the bool -> int implementation from Add From<bool> for int types #50554, one could argue that 0 -> Ok(false), 1 -> Ok(true), * -> Err(()).
Following the C compiler, one could argue that 0 -> Ok(false), * -> Ok(true).

Note that (1) and (3) could be implemented as From instead of TryFrom.

The main benefit of this PR is for generic & macro code, as I understand it.

To me it seems like the core question is whether bool should behave as a "tiny int" (i.e. an i1, or rather a u1) or as a completely distinct type.

Since this will need an FCP anyway, due to insta-stable implementations, I believe the best way forward is to delegate this to @rust-lang/libs, either to discuss how this should be implemented (1) - (3) or to propose a merge / close FCP.

SimonSapin · 2018-05-28T16:09:04Z

different people could have different expectations

I think this is a reason to not have these conversions at all. There is precedent with as casts: (true as u32, false as u32) is (1, 0), but 0 as bool causes "error[E0054]: cannot cast as bool" / "unsupported cast"

SimonSapin · 2018-06-01T07:36:06Z

I think @sfackler may have been suggesting to use a private trait. Something like:

trait TryConvert<T> {
    fn try_convert(self) -> Result<T, ()>;
}

impl<T, U> TryConvert<T> for U where U: TryInto<T> {
    fn try_convert(self) -> Result<T, ()> { self.try_into().map_err(|_| ()) }
}

impl TryConvert<bool> for u32 {
    // ...
}

TimNN · 2018-06-01T08:25:01Z

@SimonSapin I don't think impl<T, U> TryConvert<T> for U where U: TryInto<T> {} and impl TryConvert<bool> for u32 can co-exist in the current coherence rules.

ithinuel · 2018-06-01T08:42:30Z

This was my first attempt but unfortunately it is not possible as

= note: upstream crates may add new impl of trait `std::convert::From<u32>` for type `bool` in future versions

That's why the solution is to actually implement it in the upstream crate 😄

bors · 2018-06-02T10:02:05Z

☔ The latest upstream changes (presumably #50554) made this pull request unmergeable. Please resolve the merge conflicts.

ithinuel · 2018-06-02T17:22:11Z

Squashed for a clean history & rebased :)

ithinuel · 2018-06-03T16:43:23Z

@sfackler I updated the example. It will work with the next nightly that will include #50554.
However, having a special case for bool is really "meh".

ithinuel · 2018-06-10T11:00:05Z

@rust-lang/libs any update on this PR ?

sfackler · 2018-06-10T16:59:19Z

I don't believe our feelings on these impls have changed.

If you want specific behavior for your application, you can make a private trait that does that. It just can't have a blanket impl.

ithinuel · 2018-06-10T20:15:53Z

@sfackler Can you give an example of what you mean ?

Using a trait to extend the implementation does not solve the issue as demonstrated here.
The trait could be implemented for all primitive types but it would then require the users to implement From<_> as well as the new trait, which cannot be private, for their own types.

This example show a working example on my specific application.
It stays quite simple thanks to the :vis matcher.

TimNN · 2018-06-10T20:29:35Z

@ithinuel: So, if you're main use case is macros, I've just thought about another solution: Add your own trait MyTryInto { try_into(...) -> ...; } and implement for types without an std::TryInto implementation. Then ensure that both, std::TryInto and MyTryInto are in scope. IIUC, the compiler should pick the correct trait as long as the implementations don't overlap.

ithinuel · 2018-06-10T22:07:42Z

@TimNN unfortunatly, if both traits are in scope

   = note: candidate #1 is defined in an impl of the trait `core::convert::TryInto` for the type `_`
   = note: candidate #2 is defined in an impl of the trait `silica::register::MyTryInto` for the type `u32`

tested on nightly.

sfackler · 2018-06-10T23:09:23Z

The trait could be implemented for all primitive types

This is what I mean.

ithinuel · 2018-06-11T09:37:04Z

The trait could be implemented for all primitive types

This is what I mean.

but it would then require the users to implement From<_> as well as the new trait, which cannot be private, for their own types.

This means code duplication or redundant boiler plate. 😐 This is not something we want to inflict to users.

TimNN · 2018-06-12T17:48:40Z

@rust-lang/libs, it looks like the consensus here is to close the PR. Could one of you please do so if that is correct or initiate an @rfcbot fcp close

SimonSapin · 2018-06-27T13:47:01Z

there's not necessarily an inherent truthiness value nor as conversion to back it up

I think this is a strong reason.

@rfcbot fcp close

rfcbot · 2018-06-27T13:47:03Z

Team member @SimonSapin has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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 · 2018-06-27T15:46:08Z

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

rfcbot · 2018-07-07T15:54:59Z

The final comment period, with a disposition to close, as per the review above, is now complete.

SimonSapin · 2018-07-07T16:11:45Z

As discussed we’re not taking this impl, but thanks for you work on this anyway @ithinuel.

rust-highfive assigned shepmaster May 10, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2018

scottmcm reviewed May 10, 2018

View reviewed changes

ithinuel changed the title ~~Add from and into bool for integer types~~ Add From<bool> for integers and TryFrom<integers> for bool May 10, 2018

ithinuel commented May 11, 2018

View reviewed changes

ithinuel mentioned this pull request May 14, 2018

From<bool> for integers not implemented? #46109

Closed

ithinuel mentioned this pull request May 20, 2018

Add From<bool> for int types #50554

Merged

rust-highfive assigned TimNN and unassigned shepmaster May 21, 2018

clarfonthey pushed a commit to clarfonthey/rust that referenced this pull request Jun 1, 2018

Add @ithinuel's tests from rust-lang#50597

b1797d5

add conversion between integer types and bool through the TryFrom trait.

25a622a

ithinuel force-pushed the add-from-and-into-bool-for-integer-types branch from 3f20ecd to 25a622a Compare June 2, 2018 17:20

ithinuel changed the title ~~Add From<bool> for integers and TryFrom<integers> for bool~~ Add TryFrom<{integer}> for bool Jun 2, 2018

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Jun 27, 2018

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

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 7, 2018

SimonSapin closed this Jul 7, 2018

Add TryFrom<{integer}> for bool #50597

Add TryFrom<{integer}> for bool #50597

Conversation

ithinuel commented May 10, 2018

rust-highfive commented May 10, 2018

Choose a reason for hiding this comment

ithinuel May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ithinuel commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ithinuel commented May 14, 2018

clarfonthey commented May 14, 2018

ithinuel commented May 14, 2018 • edited Loading

clarfonthey commented May 14, 2018

ithinuel commented May 20, 2018

scottmcm commented May 20, 2018

ithinuel commented May 20, 2018 • edited Loading

scottmcm commented May 20, 2018

ithinuel commented May 20, 2018

clarfonthey commented May 20, 2018

shepmaster commented May 21, 2018

pietroalbini commented May 28, 2018

TimNN commented May 28, 2018

SimonSapin commented May 28, 2018

SimonSapin commented Jun 1, 2018

TimNN commented Jun 1, 2018

ithinuel commented Jun 1, 2018 • edited Loading

bors commented Jun 2, 2018

ithinuel commented Jun 2, 2018

ithinuel commented Jun 3, 2018

ithinuel commented Jun 10, 2018

sfackler commented Jun 10, 2018 • edited Loading

ithinuel commented Jun 10, 2018

TimNN commented Jun 10, 2018

ithinuel commented Jun 10, 2018

sfackler commented Jun 10, 2018

ithinuel commented Jun 11, 2018 • edited Loading

TimNN commented Jun 12, 2018

SimonSapin commented Jun 27, 2018

rfcbot commented Jun 27, 2018 • edited by dtolnay Loading

rfcbot commented Jun 27, 2018

rfcbot commented Jul 7, 2018

SimonSapin commented Jul 7, 2018

ithinuel May 10, 2018 •

edited

Loading

ithinuel commented May 14, 2018 •

edited

Loading

ithinuel commented May 20, 2018 •

edited

Loading

ithinuel commented Jun 1, 2018 •

edited

Loading

sfackler commented Jun 10, 2018 •

edited

Loading

ithinuel commented Jun 11, 2018 •

edited

Loading

rfcbot commented Jun 27, 2018 •

edited by dtolnay

Loading