Fallback {float} to f32 when f32: From<{float}> and add impl From<f16> for f32
#139087
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
rustbot
added
T-lang
beetrees
force-pushed
the
impl-from-f16-for-f32
branch
from
ef59666 to
bb79c28
Compare
This comment has been minimized.
This comment has been minimized.
beetrees
force-pushed
the
impl-from-f16-for-f32
branch
from
bb79c28 to
afe5b7f
Compare
This comment has been minimized.
This comment has been minimized.
beetrees
force-pushed
the
impl-from-f16-for-f32
branch
from
afe5b7f to
f4cb5a7
Compare
|
@bors try |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 28, 2025
Fallback `{float}` to `f32` when `f32: From<{float}>` and add `impl From<f16> for f32`
Currently, the following code compiles:
```rust
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 rust-lang#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 if the lang team wants one; alternatively this could be expanded in the future into something more general like `@tgross35` suggested in rust-lang#123831 (comment). I think it would be also possible to disallow the `f32` fallback in a future edition.
This will need a crater check.
For reference, I've based the implementation loosely on the existing `calculate_diverging_fallback`. This first commit adds the `f32` fallback and the second adds `impl From<f16> for f32`. I think this falls under the types team, so
r? types
Fixes: rust-lang#123831
Tracking issue: rust-lang#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
|
☀️ Try build successful - checks-actions |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚨 Report generation of 🆘 Can someone from the infra team check in on this? @rust-lang/infra |
|
@craterbot retry-report |
|
🛠️ Generation of the report for ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
Hmm. Thanks for voicing that @BoxyUwU. I don't personally want to get too hung up on which team has "jurisdiction" or whatever. For me the bottom line is that this PR is proposing a narrow "scenario solving" sort of change (which would be subsequently reverted), and so the high order bit is that I (wearing my "Rust" hat) feel we should think a bit bigger. (And, when you start looking at those bigger problems, e.g., trying to make int literals overloadable, that seems clearly lang to me.) Rather than argue which team does what, i'd rather we argue about whether bigger changes are feasible. |
|
Another way to look at it: I want us to be talking about where we'd like to get to. To me it seems like a regression for this code to stop compiling: fn foo<T: Into<f32>>(_: T) {}
fn main() {
foo(1.0);
}Of course, it's ALSO not good to not have the |
|
(Also, to the PR author, I think this is a well thought-out proposal, no disrespect meant.) |
|
In regards to the technical feedback:
I generally agree with this. I think we should make bigger changes to the language around the 1-impl rule and literals too. I don't think we should block std being able to add this impl on that though. We can experiment with such things at any point in time, and they're likely to take a long time to get anywhere.
My understanding is that we already have problems around looking at unsolved obligations during hir typeck and the new trait solver already has ways of being compatible with such things. Though trying to find examples of that I'm coming up dry, @lcnr probably knows exactly where that ended up 🤔 edit:
|
|
I would imagine perhaps to do with closure inference (which also works pretty poorly); and some of the coercion machinery, iirc. |
|
Ah right that's it. |
|
Also, is my understanding here correct?
Do we have an estimate for how much churn there is there? |
|
I do think the intention is to FCW it so that everyone migrates off the fallback and then we remove the fallback eventually yes. I thought #139087 (comment) was the breakage from not FCWing but it seems like that's the spurrious failures with the FCW instead of an inference error... The PR description says:
Though I can't see where that count is coming from |
|
Some pretty prominent crates there. e.g., bevy. |
|
@BoxyUwU btw, on the topic of lang vs types, the way I see it is this: lang always has the power to overrule types if lang feels the problems for user-facing impact are too high. Obviously types makes calls but if the impact is high it's expected they'll clue lang in. types always has the power to overrule lang if types feels the problems for complexity, implementability, or soundness is too much. Obviously lang makes calls but if the impact is high it's expected they'll clue types in. So yeah, it's good for types to move forward with user-facing breakage. It's also ok for lang to step in sometimes and say "hmm, this one seems off". And it's fine for you to argue and say "no, it's not, let's not block a targeted change on some big years-long project". I'm still pondering that one. I think my types concerns are largely resolved now, I'm now more just pondering "is this the right thing to do" -- e.g., if bevy relies on this, are we really going to want to have them migrate to If the answer is "probably not", then the question is, are we comfortable with this change if it's permanent? If we try to do something more general, will we hit a problem due to this change? I'd like to think that through at least a bit. |
|
My thoughts here: First, procedurally, it is frustrating that this sat as lang-nominated for a while and that lang only chimed in after an pFCP was started. That being said, it's only a month, which is not that long. And I personally really do think that lang should be involved in this decision for the points Niko highlighted - unlike other decisions types decisions, this seems much more "user-facing" rather than just an implementation detail. Anyways, this is neither here nor there - if lang wants in, then I think it'd be weird for someone to say "no that's not okay". Now, technically: I don't really like adding the extra complexity of this fallback, in large part because I think removing that fallback would take a while, even with a FCW. This has a wide impact. So, I'm going to "second" nikos "niko-is-nervous" concern. |
(Minor note: the affected literals would need to be suffixed with Full list of every affected location in Crater run without fallback change
If the FCW ends up getting merged I plan to submit PRs to affected projects to help the process along. Ultimately, it's entirely possible to revert this change until |
|
FWIW, personally, regardless of how we implement it, I think this code should work in the future: fn func(_: impl Into<f32>) {}
fn main() { func(1.0); }I also think this code should work, and today it doesn't: fn func(_: impl Into<u16>) {}
fn main() { func(1); }Whether we do that via a fallback mechanism, or some more general solution for literals, I think those should work and continue working forever. If we decide to remove the fallback mechanism being proposed here, it should only be because we added something else that makes that code continue working. |
|
I guess, for me to feel comfortable about this, I would want the FCW to land first, and to see 1) that the impact of that is not too large and 2) that it causes a significant reduction in the use of this fallback, indicating a strong likelihood that we will eventually be able to remove this hack. |
Agree with @jackh726 here, and would add that ideally it'd work well enough that we don't need to add the hack at all. |
|
I think Boxy may have hinted at this, but is implementation complexity across both solvers the reason we can't do a more general solution at this time? At a surface level it seems "easy" to solve both the float and int examples by trying |
|
☔ The latest upstream changes (presumably #140682) made this pull request unmergeable. Please resolve the merge conflicts. |
traviscross
added
T-lang
beetrees
force-pushed
the
impl-from-f16-for-f32
branch
from
ab64774 to
6e6d23c
Compare
traviscross
added
S-waiting-on-team
|
Talked to @compiler-errors about the more general fallback at all hands. My understanding of the problem is that by the time the solver knows there is an unfulfilled obligation, it is already done solving and has no way to backtrack. So (aiui) it might be possible to infer the type of the literal at the location of the @Amanieu suggested there might be a way to keep track of the two possible solutions. |
|
In the spirit of brainstorming, I wonder whether pattern types could ever have a role to play here. E.g., in fn f<T: Into<u32>>(_: T) {}
fn main() {
f(1);
}maybe we could say that the literal falls back to Of course, pattern types are hard, and are I'm sure especially challenging to combine with floats. cc @oli-obk |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the following code compiles:
This is because the only
From<{float}>impl forf32is currentlyFrom<f32>. However, onceimpl From<f16> for f32is added this is no longer the case. This would cause the float literal to fallback tof64, subsequently causing a type error asf32does not implementFrom<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 addimpl From<f16> for f32was 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 usingf32as the fallback type for{float}when there is a trait predicate off32: From<{float}>. This allows addingimpl From<f16> for f32without 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 thef32fallback 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 thef32fallback 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 thef32fallback, the second addsimpl From<f16> for f32, and the third adds a FCW lint for thef32fallback. I think this falls under the types team, sor? 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