Floating point modulus has snuck back into the language

[alexcrichton]

Back in 1563ea9 we decided to remove the % operator from floats in the language, relegating the implementations of the Rem trait to library calls on the fmod and fmodf functions. (some discussion in #12278)

One of the reasons cited for its removal was the removal of support from LLVM, although I don't think that's happened yet and I'm not quite sure what the status there is.

Regardless, we can probably still remove this without affecting backwards compatibility, but we'd probably want to do so sooner rather than later. Would just be good to make sure we've got our ducks in a row!

Nominating

[alexcrichton]

Also cc #27823 where this was discovered

[huonw]

I don't think it's in the language:

#![crate_type = "lib"]
#![feature(lang_items, no_core)]
#![no_core]

fn foo() {
    1_f32 % 2_f32;
}

#[lang = "sized"] trait Sized {}

Fails to compile:

<anon>:6:5: 6:10 error: binary operation `%` cannot be applied to type `f32` [E0369]
<anon>:6     1_f32 % 2_f32;
             ^~~~~

In fact, there are explicit implementations of Rem that extern in fmod/fmodf.

#[stable(feature = "rust1", since = "1.0.0")]
impl Rem for f32 {
    type Output = f32;

    // see notes in `core::f32::Float::floor`
    #[inline]
    #[cfg(target_env = "msvc")]
    fn rem(self, other: f32) -> f32 {
        (self as f64).rem(other as f64) as f32
    }

    #[inline]
    #[cfg(not(target_env = "msvc"))]
    fn rem(self, other: f32) -> f32 {
        extern { fn fmodf(a: f32, b: f32) -> f32; }
        unsafe { fmodf(self, other) }
    }
}

#[stable(feature = "rust1", since = "1.0.0")]
impl Rem for f64 {
    type Output = f64;

    #[inline]
    fn rem(self, other: f64) -> f64 {
        extern { fn fmod(a: f64, b: f64) -> f64; }
        unsafe { fmod(self, other) }
    }
}

I suspect this can't be removed backwards compatibly. :(

[eefriedman]

Actually, the situation is kind of confusing: "%" doesn't typecheck unless there's an explicit implementation of Rem visible. However, trans assumes that "impl Rem for f64" is implemented the obvious way, and generates code which doesn't actually use the explicit implementation.

[alexcrichton]

Yeah I don't think that this is that serious of a decision to make either way really, just figured we may want to make a decision one way or the other to be definitive. Either way the surface language is the same and the implementation will be the same, it's just a manner of implementation details.

[alexcrichton]

Actually my initial assessment wasn't quite right, it would be a strictly-speaking backwards-incompatible change to do remove this from the language again. Right now we allow e %= e for floats but removing the language-based implementation would mean that's no longer allowed.

[ranma42]

Isn't it possible to do the same as in #27823, i.e. move the definition of the modulus operator to std?
AFAICT it would only break code which used the no_std or no_core features, which should not be available in any of the stable releases. (Also, if this would be a breaking change, wouldn't #27823 be breaking in the same way? is it because of the assumption in trans?)

[nikomatsakis]

So -- the reason this changed behavior is because I forgot about this case. I think we could restore the old behavior by modifying fix_scalar_binary_expr in writeback.rs so that it doesn't fallback to builtin semantics. However, there would still be a dependency from libcore to libm.

UPDATE -- what the compiler does now is use the impls during typechecking, but then if the impl is between two scalar types, fallback to builtin semantics.

[alexcrichton]

Ah yeah I'm fine with libcore having a dependency on fmod and fmodf, I don't think we're going to get away from that any time soon.

[nikomatsakis]

We discussed this in the language subteam meeting. The general conclusion was that the current behavior is OK, and in particular that the older behavior (in an impl but not "the language") was not especially coherent or interesting. In particular, the motivating problem seems to be that LLVM inserts a call to fmod and may remove support in the future. Removing support is not an issue, because we could call just lower into a call to fmod ourselves. That would still leave a dependency on fmod, but of course such a dependency exists through the impl as well; since core is (in my view) a core part of the language (pun intended), this is not an improvement. But we can solve the dependency on fmod entirely by reimplementing it in Rust as a lang item and calling that instead (according to @huonw this is not a ton of code).

[nikomatsakis]

I'm removing the nominating tag and closing this issue, but please feel free to re-open if you think the summary and conclusions from #27824 (comment) are not sufficient.

[ranma42]

Would it be possible to remove the double implementation in libcore and trans and only keep a single one?
Given that the implementation in libcore is currently ignored, it looks misleading to have both, as shown in the first comments of this issue. Specifically, I would propose to remove the cfg machinery and write a "trivial" implementation just as it currently happens for div. If needed, I can do a PR (I stubbed out what it might look like here: ranma42@152c76e )

[eefriedman]

@ranma42 Sure... but we probably can't merge that until we have a snapshot which includes #27875.