Add TryFrom<{integer}> for bool

[ithinuel]

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]

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]

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

[ithinuel]

😮 I'm few hours late

[ithinuel]

I updated the issue number to #46109

[scottmcm]

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

[ithinuel]

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

[ithinuel]

Updated to use TryFrom instead.

[ithinuel]

This fixes : #46109.

[ithinuel]

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

[ithinuel]

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

[scottmcm]

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

[ithinuel]

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 #33417 as the issue number.

[ithinuel]

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

[clarfon]

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]

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.

[clarfon]

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

[ithinuel]

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

[scottmcm]

@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]

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]

@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]

@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.

[clarfon]

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]

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

r? @TimNN

[pietroalbini]

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

[TimNN]

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.

1. Based on the convention 0 = success, * = error, one could argue that 0 -> Ok(true), * -> Ok(false).
2. To be strictly in line with the bool -> int implementation from #50554, one could argue that 0 -> Ok(false), 1 -> Ok(true), * -> Err(()).
3. 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]

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"

[TimNN]

@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]

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]

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

[ithinuel]

Squashed for a clean history & rebased :)

[ithinuel]

@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]

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

[sfackler]

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]

@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]

@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]

@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]

The trait could be implemented for all primitive types

This is what I mean.

[ithinuel]

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]

@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]

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]

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

• @Kimundi
• @SimonSapin
• @alexcrichton
• @dtolnay
• @sfackler
• @withoutboats

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]

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

[rfcbot]

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

[SimonSapin]

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