Implement From<bool> for f32, f64

[jhpratt]

As is required for trait implementations, these are insta-stable. Given there is a release tomorrow and this needs FCP, I set 1.65 as the stable version.

@rustbot label +A-floating-point +C-feature-request +needs-fcp +relnotes +S-waiting-on-review +T-libs-api -T-libs

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @thomcc

(rust-highfive has picked a reviewer for you, use r? to override)

[scottmcm]

Cross-linking the Zulip conversation: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/.60impl.20From.3Cbool.3E.20for.20f.7B32.7C64.7D.60/near/292687425

Given there is a release tomorrow and this needs FCP, I set 1.65 as the stable version.

Reminder you can check https://github.com/rust-lang/rust/blob/master/src/version -- it's already been 1.65 for 5 days.

[Lokathor]

You might be thinking of these backward? The input is a bool, you couldn't pass 0.5 to these functions.

In this impl: true is 1.0, and false is 0.0, and there's no other possible values that could be passed.

[jhpratt]

Yeah, this goes from a boolean to a float. The other way around would absolutely be unwise to implement in this manner.

[thomcc]

Oh, right. Yes, my bad. I don't have an issue then (this is what I get for taking a look at a PR at 11:30PM...)

[m-ou-se]

@rfcbot merge

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

Concerns:

• needs use case resolved by Implement From<bool> for f32, f64 #100390 (comment)

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.

[bors]

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

[jhpratt]

Rebased.

[clarfonthey]

I'm a bit torn on this. I think that if we do merge this, then directly casting from bool to floats should be allowed as part of the language. It feels inconsistent otherwise.

Note that this isn't against this feature necessarily, I'm just not sure what would justify having one of as or From work here without the other if the conversion is injective.

[jhpratt]

While I understand what you're saying, I think the general trend is to move away from as as much as possible. So I think it would be best if we didn't implement that cast, as it would be counter to other efforts.

[clarfonthey]

I would be in favour of that if there were actually detailed plans on how we plan to do that, but at least for now, this will be merged long before any of those plans are executed.

I mean ultimately it probably doesn't matter that much, but I feel like it's worth at least providing a decision on the as implementation in this decision, whether it's implemented or not.

Is the actual conversion more complicated than other as implementations? Was there a good reason why it wasn't implemented before, or did just no one care? If someone made a PR implementing this as an as conversion, would it be merged?

Again, it's very simple to merge this change, but it's also very simple to do the conversion through u8 yourself. So what's the motivation there?

[Lokathor]

There is not a single, unified plan for moving away from as. It is still a general design guideline that many people, both on various teams and also those not on any teams, have tried to follow when possible. I myself agree that even if we can't replace as entirely right now, we should at least not expand it any farther.

The main thing keeping as alive in my mind is that it's available in const, while from is not.

[clarfonthey]

Basically the key point I'm trying to make is that right now, since as exists and isn't going away any time soon, we should take agreeing that this From implementation is okay as also an agreement that the equivalent as implementation is also okay.

It doesn't mean that anyone is obliged to write the code for such an implementation before this is merged, but rather that if a compelling argument exists to not accept an as implementation for this other than "I don't like as", it should also be used as a reason against this change. Basically: why doesn't an as implementation exist right now, and is that reason actually a good design decision that can be applied here too?

[dtolnay]

I'm marking @yaahc's box because she has stepped down from T-libs-api after the point that this feature got proposed for FCP.

[rfcbot]

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

[dtolnay]

@bors r+

[bors]

📌 Commit b134d11 has been approved by dtolnay

It is now in the queue for this repository.

[jhpratt]

@dtolnay This needs to complete FCP, no?

[jhpratt]

Closing and reopening to avoid bors merging this for the moment, as I believe FCP is still necessary. This will also cancel FCP, but that can be restarted by any libs-api team member (checking the same boxes as are currently checked).

[dtolnay]

I do not feel that blocking this on FCP is necessary. The FCP proposal has been active for 4 months which is ample time for any team member to register their concern. In general the automation around FCP does not attempt to take into account size of the feature. The amount of process it makes sense to have around adding a trait impl to an existing type, or adding const to an existing method, is vastly different compared to adding some major language feature like async or const generics. The team has discretion to adapt the process to the situation.

[bors]

⌛ Testing commit b134d11 with merge 4914381...

[bors]

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 4914381 to master...

[rust-timer]

Finished benchmarking commit (4914381): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-3.2%	[-4.2%, -2.2%]	5
Improvements ✅ (secondary)	-2.7%	[-4.2%, -0.8%]	29
All ❌✅ (primary)	-3.2%	[-4.2%, -2.2%]	5

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[0.7%, 0.7%]	1