rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
93.93k stars 12.09k forks source link

regression: can't call method `abs` on ambiguous numeric type `{float}` #125198

Open BoxyUwU opened 2 weeks ago

BoxyUwU commented 2 weeks ago
[INFO] [stdout] error[E0689]: can't call method `abs` on ambiguous numeric type `{float}`
[INFO] [stdout]    --> src/activation.rs:206:39
[INFO] [stdout]     |
[INFO] [stdout] 206 |         assert!((res[0] - 0.08192506).abs() < 0.001);
[INFO] [stdout]     |    
compiler-errors commented 2 weeks ago

This was probably regressed w/ some f16/f128 stuff cc @tgross35

tgross35 commented 2 weeks ago

Hm, when did this last work? I can't get it to compile successfully even on older versions https://rust.godbolt.org/z/ExcMY36bf

edit: slightly more like the published test in https://docs.rs/crate/puffpastry/0.1.0/source/src/activation.rs, same results https://rust.godbolt.org/z/crWhqYq6h

zachs18 commented 2 weeks ago

The puffpastry crate's bound includes T: ValidNumber<T>, which includes T: ... + From<f64> + .... Adding T: Copy + From<f64> to the example makes it compile on stable https://rust.godbolt.org/z/EbKTjzM9v

tgross35 commented 2 weeks ago

Ah, then #123830 also should have removed this line https://github.com/rust-lang/rust/pull/123830/files#diff-154dff23a2c2f4fb97915229fd9c27d73293940250088020bfd2074094186ce4L173, which was added in https://github.com/rust-lang/rust/pull/122470.

Was this crate the only regression of this kind? If that is the case, maybe we would be okay leaving it based on discussion at https://github.com/rust-lang/rust/issues/123831.

apiraino commented 1 week ago

@tgross35 should the changes in #123830 / ~#123831~ #124728 be in relnotes? I'm thinking if there are changes here useful to be announced. Thanks

tgross35 commented 1 week ago

@apiraino I think the only things maybe relevant for release notes would be conversions that involve a stable type, which could have an effect on inference. These would be:

  1. From<f16> for f64 added in #122470, removed in #12380, added back in #12473 (1.80 current nightly). Already had a crater run, no regressions
  2. From<f32> for f128, added in #122470 (1.79 current beta). No known regressions
  3. From<f64> for f128, added in #122470 (1.79 current beta). Regressions

Item 3 is the subject of this issue, we'll need a decision about whether to accept the regression or remove the impl.

The remaining conversions from #122470 shouldn't affect anything without nightly so I don't think they need release notes.

apiraino commented 4 days ago

probably adding t-libs then. Opened a topic for increased visibility on Zulip

@rustbot label T-libs