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

f32::from(<untyped float>) inference with f16 and f128 #123831

Open
tgross35 opened this issue Apr 11, 2024 · 11 comments
Open

f32::from(<untyped float>) inference with f16 and f128 #123831

tgross35 opened this issue Apr 11, 2024 · 11 comments
Labels
F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` T-lang Relevant to the language 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

@tgross35
Copy link
Contributor

tgross35 commented Apr 11, 2024

Adding a From<f16> for f32 impl breaks some inference that currently works. From #123824:

fn main() {
    let x = f32::from(3.14);
}

The new float types are a long way from being stable, but it may be good to decide how to handle things if we would like this implementation one day. Per @fmease at #123824 (comment), there are three options:

  1. Accept this regression since technically, no stable API is broken
  2. Say that we do not want From<f16> for f32
  3. Make this implementation available starting with the new edition

I'll mark this with the edition label so it can at least come into consideration, but this is the lowest priority thing on any list.

Known failure points from adding this directly:

@rustbot label +T-lang +T-libs +A-edition-2024 +F-f16_and_f128

@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-edition-2024 Area: The 2024 edition F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` T-lang Relevant to the language 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. labels Apr 11, 2024
@tgross35
Copy link
Contributor Author

@rustbot label -needs-triage

@rustbot rustbot removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 11, 2024
@compiler-errors
Copy link
Member

compiler-errors commented Apr 12, 2024

There's no way we can provide an impl conditional on edition, since those are always global and can't really be conditional based on the way that the trait solver works.

The way that this worked for into_iter() and arrays is that it only affected dot-method syntax, and we just skipped over the candidate for [T; N] when we had [].into_iter() -- I expect this to have to be implemented differently for From::from and/or f32::from, since there's only one candidate being assembled here <f32 as From<?0>>::from where the choice of that ?0 type variable is the thing that's becoming ambiguous here.

We still may be able to hack around this, but I would strongly urge first doing the analysis of the fallout from breaking inference here, since any hacks that we need to introduce need to exist for approximately forever, which is a long time.

@tgross35
Copy link
Contributor Author

Too bad, but that makes enough sense. I guess it would be interesting to see crater results, though the fact that this was discovered within 24 hours of merge makes me think there could be quite a lot of codebases relying on this somewhere...

@rustbot label -A-edition-2024

@rustbot rustbot removed the A-edition-2024 Area: The 2024 edition label Apr 12, 2024
@tgross35
Copy link
Contributor Author

Crater run at #122470 (comment)

@workingjubilee
Copy link
Member

workingjubilee commented Apr 12, 2024

Some languages give literals their own special type for making from-literal conversions more tractable to steer in the type system.

@nicoburns
Copy link

nicoburns commented Apr 12, 2024

Some kind of improvement to type inference such that it can handle this case would seem ideal. Perhaps even a lang feature to allow user code to guide the inference process.

From a library author's position this change is awkward because it is not necessarily possible to fix it without making a semver-breaking update. But it also seems non-great if these API additions are blocked indefinitely.

@BD103
Copy link
Contributor

BD103 commented Apr 13, 2024

Can the link to the Taffy regression be updated to https://github.com/DioxusLabs/taffy/blob/f1e4edab96ee13f7d1fbc2536ac31633b0e620d6/src/style_helpers.rs#L58, before it was fixed in DioxusLabs/taffy@9f27870?

@BD103
Copy link
Contributor

BD103 commented Apr 13, 2024

I'm also getting the same warnings for this line, this line, this line, this line, and these lines.

Most of them are related to casting a tuple ({float}, {float}) / array [{float}; 2] to our custom WindowResolution struct. The last one is different, though, because it runs into Taffy's fn foo<T: Into<f32>>(x: T) functions.

I hope this helps :)

@tgross35
Copy link
Contributor Author

@compiler-errors
Copy link
Member

Hm -- looks like a lot of these are due to Into<f32> bounds (e.g. in arguments) and not due to method dispatch.

As I stated above, impls need to apply globally, so it's gonna be a bit harder to fix this (if possible at all, without hacking out something that I'd be afraid of having to support in perpetuity).

@tgross35
Copy link
Contributor Author

tgross35 commented Apr 18, 2024

Yeah, I would rather not have this impl than have something specific and hacky.

How feasible would it be for untyped literals to select their type based on the bound? As in, if there is a requirement for impl Into<T> and an unsuffixed literal could represent T, then choose that rather than falling back to the default f64/i32. Which I think would resolve most of the failures here, in addition to making something like this work:

fn foo(a: impl Into<u64>) {}

fn main() {
    foo(100);
}

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
F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` T-lang Relevant to the language 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

No branches or pull requests

6 participants