Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type inference failure for f32 #123824

Closed
P1n3appl3 opened this issue Apr 11, 2024 · 15 comments · Fixed by #123830
Closed

Type inference failure for f32 #123824

P1n3appl3 opened this issue Apr 11, 2024 · 15 comments · Fixed by #123830
Labels
A-inference Area: Type inference C-bug Category: This is a bug. F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@P1n3appl3
Copy link
Contributor

After this rollup we started seeing the following compile failure:

fn main() {
    let x = f32::from(3.14);
}
error[E0277]: the trait bound `f32: From<f64>` is not satisfied
  --> /tmp/repro.rs:2:13
   |
16 |     let x = f32::from(3.14);
   |             ^^^ the trait `From<f64>` is not implemented for `f32`
   |
   = help: the following other types implement trait `From<T>`:
             <f32 as From<bool>>
             <f32 as From<f16>>
             <f32 as From<i16>>
             <f32 as From<i8>>
             <f32 as From<u16>>
             <f32 as From<u8>>

error: aborting due to 1 previous error

I repro'd this locally on aa6a697 but we have CI that runs on every commit so we're fairly sure that it started with 4435924. Of the changes in that rollup #122470 is the only one that seemed to touch float stuff, but I'm not sure how adding some extra impls for f16 and f128 would break inference in this case.

@P1n3appl3 P1n3appl3 added the C-bug Category: This is a bug. label Apr 11, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 11, 2024
@P1n3appl3 P1n3appl3 changed the title Type inference failure for f32 Type inference failure for f32 Apr 11, 2024
@jieyouxu jieyouxu added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-inference Area: Type inference labels Apr 11, 2024
@jieyouxu
Copy link
Member

cc @tgross35: is it possible that adding the the From impls for f16/f128 may have caused type inference regression here?

@jieyouxu jieyouxu added S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Apr 11, 2024
@tgross35
Copy link
Contributor

It must be, but I don't know why since https://github.com/rust-lang/rust/pull/122470/files#diff-154dff23a2c2f4fb97915229fd9c27d73293940250088020bfd2074094186ce4R171 wasn't touched. Also reproduced locally

@Nemo157
Copy link
Member

Nemo157 commented Apr 11, 2024

When there is only one applicable impl it can guide inference, as soon as there are multiple it can't use those and instead goes to default-numeric-fallback which appears to be f64 for floating point.

@workingjubilee
Copy link
Member

Correct.

@tgross35
Copy link
Contributor

Just coming to the same conclusion - the literal is getting guided to f32 on stable since that's the only type that fits the from, but when there are more options it gets the default float literal type of f64

fn main() {
    let a = 3.14;
    dbg!(std::any::type_name_of_val(&a)); // prints f32 on stable
    let x = f32::from(a);
}

@tgross35
Copy link
Contributor

I'll put up a PR to remove these impls for now but what is the correct longer term fix?

cc @rust-lang/libs-api

@tgross35
Copy link
Contributor

@rustbot label -needs-triage +A-inference +F-f16_and_f128

@rustbot rustbot added the F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` label Apr 11, 2024
@jieyouxu jieyouxu added F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` labels Apr 11, 2024
@tgross35
Copy link
Contributor

Race condition 😆

image

@jieyouxu
Copy link
Member

jieyouxu commented Apr 11, 2024

I did not remove the feature label lol (it wasn't even in the list), welldone GitHub 😆

@fmease
Copy link
Member

fmease commented Apr 11, 2024

Theoretically, type inference regressions are permitted as part of Rust's 1.0 stability guarantees.

In practice, if the regressions are too bad, the changes usually get reverted and are subsequently blocked indefinitely iirc.

However, we could make use of editions if the worst comes to the worst. See arrays & IntoIterator. I think we have something similar queued up for Rust 2024. On mobile, can't link.

tgross35 added a commit to tgross35/rust that referenced this issue Apr 11, 2024
Adding additional `From` implementations that fit `f32::from(<unaffixed
float>)` broke inference. Remove these for now.

Fixes: <rust-lang#123824>
tgross35 added a commit to tgross35/rust that referenced this issue Apr 11, 2024
Adding additional `From` implementations that fit `f32::from(<unaffixed
float>)` broke inference. Remove these for now.

Fixes: <rust-lang#123824>
@tgross35
Copy link
Contributor

I put a fix up at #123830 and opened #123831 to further discuss what we want to do here.

Thanks @P1n3appl3 for identifying this so quick!

@tgross35
Copy link
Contributor

@P1n3appl3 if you get the chance, could you link some of the places that caused this failure? It would be good to have some references for how this behavior is being relied on in the wild.

@P1n3appl3
Copy link
Contributor Author

P1n3appl3 commented Apr 12, 2024

This was the one that we caught it on. It's a pretty simple case (that Coord type is just an f32 type alias) and seems reasonable to break on an edition boundary (or even just on a release as long as there's a machine applicable suggestion to fix it).

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 12, 2024
…ix, r=Nilstrieb

Remove `From` impls for unstable types that break inference

Adding additional `From` implementations that fit `f32::from(<unaffixed float>)` broke inference. Remove these for now.

I added a test to make sure this doesn't quietly change in the future, even though the behavior is not technically guaranteed rust-lang#123824 (comment)

Fixes: <rust-lang#123824>
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 12, 2024
Rollup merge of rust-lang#123830 - tgross35:f16-f128-from-inference-fix, r=Nilstrieb

Remove `From` impls for unstable types that break inference

Adding additional `From` implementations that fit `f32::from(<unaffixed float>)` broke inference. Remove these for now.

I added a test to make sure this doesn't quietly change in the future, even though the behavior is not technically guaranteed rust-lang#123824 (comment)

Fixes: <rust-lang#123824>
@BD103
Copy link
Contributor

BD103 commented Apr 12, 2024

@P1n3appl3 if you get the chance, could you link some of the places that caused this failure? It would be good to have some references for how this behavior is being relied on in the wild.

Hi! I'm from the Bevy project. We caught this bug with this run too, though the error originates from the Taffy crate (0.3.18).

@tgross35
Copy link
Contributor

tgross35 commented Apr 12, 2024

Awesome, thanks for the report! I started a list of known regressions at #123831, and there is a crater run going to figure out how best to deal with this behavior.

The fix for this was merged so should be available on the next nightly (couple of hours?).

bors added a commit to rust-lang-ci/rust that referenced this issue May 5, 2024
Re-add `From<f16> for f64`

This impl was originally added in rust-lang#122470 before being removed in rust-lang#123830 due to rust-lang#123831. However, the issue only affects `f32` (which currently only has one `From<{float}>` impl, `From<f32>`) as `f64` already has two `From<{float}>` impls (`From<f32>` and `From<f64>`) and is also the float literal fallback type anyway. Therefore it is safe to re-add `From<f16> for f64`.

This PR also updates the FIXME link to point to the open issue rust-lang#123831 rather than the closed issue rust-lang#123824.

Tracking issue: rust-lang#116909

`@rustbot` label +F-f16_and_f128 +T-libs-api
bors added a commit to rust-lang-ci/rust that referenced this issue May 16, 2024
Re-add `From<f16> for f64`

This impl was originally added in rust-lang#122470 before being removed in rust-lang#123830 due to rust-lang#123831. However, the issue only affects `f32` (which currently only has one `From<{float}>` impl, `From<f32>`) as `f64` already has two `From<{float}>` impls (`From<f32>` and `From<f64>`) and is also the float literal fallback type anyway. Therefore it is safe to re-add `From<f16> for f64`.

This PR also updates the FIXME link to point to the open issue rust-lang#123831 rather than the closed issue rust-lang#123824.

Tracking issue: rust-lang#116909

`@rustbot` label +F-f16_and_f128 +T-libs-api
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inference Area: Type inference C-bug Category: This is a bug. F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants