Fallback {float} to f32 when f32: From<{float}> and add impl From<f16> for f32

[beetrees]

View all comments

Currently, the following code compiles:

fn foo<T: Into<f32>>(_: T) {}

fn main() {
    foo(1.0);
}

This is because the only From<{float}> impl for f32 is currently From<f32>. However, once impl From<f16> for f32 is added this is no longer the case. This would cause the float literal to fallback to f64, subsequently causing a type error as f32 does not implement From<f64>. While this kind of change to type inference isn't technically a breaking change according to Rust's breaking change policy, the previous attempt to add impl From<f16> for f32 was removed #123830 due to the large number of crates affected (by my count, there were root regressions in 42 crates and 52 GitHub repos, not including duplicates). This PR solves this problem by using f32 as the fallback type for {float} when there is a trait predicate of f32: From<{float}>. This allows adding impl From<f16> for f32 without affecting the code that currently compiles (such as the example above; this PR shouldn't affect what is possible on stable).

This PR also allows adding a future-incompatibility warning for the fallback to f32 (currently implemented in the third commit) if the lang team wants one (allowing the f32 fallback to be removed in the future); alternatively this could be expanded in the future into something more general like @tgross35 suggested in #123831 (comment). I think it would be also possible to disallow the f32 fallback in a future edition.

As expected, a crater check showed no non-spurious regressions.

For reference, I've based the implementation loosely on the existing calculate_diverging_fallback. This first commit adds the f32 fallback, the second adds impl From<f16> for f32, and the third adds a FCW lint for the f32 fallback. I think this falls under the types team, so
r? types

Fixes: #123831
Tracking issue: #116909

@rustbot label +T-lang +T-types +T-libs-api +F-f16_and_f128

To decide on whether a future-incompatibility warning is desired or otherwise (see above):
@rustbot label +I-lang-nominated

cc #154024 #154005

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[compiler-errors]

@bors try

[bors]

⌛ Trying commit f4cb5a7 with merge 3ec2a77...

[bors]

☀️ Try build successful - checks-actions
Build commit: 3ec2a77 (3ec2a775459d36c7d90654e47c56ff7bb0d542ab)

[compiler-errors]

@craterbot check

[craterbot]

👌 Experiment pr-139087 created and queued.
🤖 Automatically detected try build 3ec2a77
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-139087 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚨 Report generation of pr-139087 failed: timed out waiting for connection
🛠️ If the error is fixed use the retry-report command.

🆘 Can someone from the infra team check in on this? @rust-lang/infra
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[compiler-errors]

@craterbot retry-report

[craterbot]

🛠️ Generation of the report for pr-139087 queued again.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-139087 is completed!
📊 9 regressed and 2 fixed (605729 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[folkertdev]

Based on #t-types > `From<f16> for f32` FCW lint, the types team wants to add the fallback from {float} to f32, but also keep impl From<f16> for f32 unstable, so that this fallback can be removed in the future when a better solution comes up.

For now I've kept the lint, which should make this pattern slowly disappear.

[jackh726]

Is there a test something that ensures that f32: From<f16> doesn't hold on stable?

View changes since this review

[jackh726]

When CI is green we should do a crater run just to verify that there is no breakage.

This probably resolves Niko's concerns here: there shouldn't be any change to stable behavior here, other than just an FCW (I'm always a fan to have tests be the first commit to show the change in behavior over subsequent commits.)

Procedurally, I'm not quite sure what to do here: lang,types,libs all FCPed something stronger (the stable impl with some commitment to fallback behavior) - though it didn't actually finish given missing checkboxes+concerns. I'm inclined to just let it ride once the boxes are checked and the concerns are resolved. This is now almost effectively a two-way door (other than lint name, but behavior-wise yes).

[traviscross]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 9961e55 (9961e55b3687a6d0b2bab4af0e5e81494b09be81, parent: 91775dbec9771aa0c1b9ebe268eb5bd271e79a7a)

[traviscross]

@craterbot check

[craterbot]

👌 Experiment pr-139087-1 created and queued.
🤖 Automatically detected try build 9961e55
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more