fsharp / fslang-suggestions

The place to make suggestions, discuss and vote on F# language and core library features
344 stars 21 forks source link

Warning (off by default) on integer division #1161

Open charlesroddie opened 2 years ago

charlesroddie commented 2 years ago

When a programmer writes 1/2, he or she could mean:

  1. 1 div 2, i.e. the integer 0.
  2. A fraction representing a half in some user-defined type, or the floating point number 0.5.

In our codebase 99% of integer division are mistakes, and the 1% of cases where they are intentional, they are better written using a more verbose and intentional syntax.

The formal justification for special-casing / for integer division is that / is not a standard mathematical division, which is defined in terms of multiplicative inverses. 1./3. is, approximately, the number which when multiplied by 3. gives 1.. 1/3 is not.

However there may be more valid uses of /, such as manually implementing multidimensional arrays or sorting algorithms.

With a warning, off by default, there is no backwards incompatibility, users doing a lot of low level CS work where integer division is common are unaffected, and users doing mathematical work where / as integer division causes problems can enable the warning.

vzarytovskii commented 2 years ago

Technically, you can redefine it (and then autoopen it):

open System
open System.Runtime.CompilerServices
[<NoDynamicInvocation;MethodImpl(MethodImplOptions.AggressiveInlining)>]
[<CompilerMessage("Operator '/' shall not be used", 1337, IsError = true)>]
let inline (/) _ _ = failwith "Operator '/' shall not be used."

this will result into:

image

gusty commented 2 years ago

@vzarytovskii that would avoid using / for all types, not just integers.

Something like this will do the trick:

open System
open System.Runtime.CompilerServices

type DivExtension = DivExtension with

    // Fallback case
    static member inline (=>) (x, DivExtension) = fun y -> x / y

    [<CompilerMessage("Operator '/' shall not be used for ints", 1337, IsError = true)>]
    static member (=>) (x: int   , DivExtension) = fun (y: int) -> failwith "Operator '/' shall not be used for ints."

    [<CompilerMessage("Operator '/' shall not be used for bigints", 1337, IsError = true)>]
    static member (=>) (x: bigint, DivExtension) = fun (y: bigint) -> failwith "Operator '/' shall not be used for bigints."

    [<CompilerMessage("Operator '/' shall not be used for int64s", 1337, IsError = true)>]
    static member (=>) (x: int64 , DivExtension) = fun (y: int64) -> failwith "Operator '/' shall not be used for int64s."

    [<CompilerMessage("Operator '/' shall not be used for int16s", 1337, IsError = true)>]
    static member (=>) (x: int16 , DivExtension) = fun (y: int16) -> failwith "Operator '/' shall not be used for int16s."

    [<CompilerMessage("Operator '/' shall not be used for uint64s", 1337, IsError = true)>]
    static member (=>) (x: uint64, DivExtension) = fun (y: uint64) -> failwith "Operator '/' shall not be used for uint64s."

    [<CompilerMessage("Operator '/' shall not be used for uint32s", 1337, IsError = true)>]
    static member (=>) (x: uint32, DivExtension) = fun (y: uint32) -> failwith "Operator '/' shall not be used for uint32s."

    [<CompilerMessage("Operator '/' shall not be used for uint16s", 1337, IsError = true)>]
    static member (=>) (x: uint16, DivExtension) = fun (y: uint16) -> failwith "Operator '/' shall not be used for uint16s."

let inline (/) x y = (x => DivExtension) y

I definitely agree in that / shouldn't be used for integer division which is a completely different type of division, sometimes called Euclidean division (division with remainder). Some languages adopted div for that operation and I think even Python did a breaking change at v3 to adjust this.

I wrote long time ago a module to be used in generic math operations in F#+ as FSharPlus.Math.Generic which among other things does this adjustment.

Happypig375 commented 2 years ago

What happens in generic code? IDivisionOperators or inline.

jasiozet commented 2 years ago

I think gusty nails it here.

Python 2: The single front-slash operator a/b performs integer division. Python 2: The double front-slash operator a//b performs integer division. Python 3: The single front-slash operator a/b performs float division. Python 3: The double front-slash operator a//b performs integer division. I'm no python expert, but in Python2 you had to do: (float)1/2 or 1./2

Integer division is not something that you would usually want. Maybe it is time for similar approach in future versions of F#?

Happypig375 commented 2 years ago

@jasiozet For generic code involving division, will integers as a type argument be warned as well?

jasiozet commented 2 years ago

@jasiozet For generic code involving division, will integers as a type argument be warned as well?

I would say for this use case I would imagine the warning is only produced if a type

  1. is declared as integer
  2. currently specifically inferred as integer.

Another option I could see is as worth considering is an ability to use this explicitily:

  1. in a file open Fsharp.FutureDivision
  2. in a .fsproj
    <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <UseIntegerDivision>true</UseIntegerDivision>
    </PropertyGroup>

Just for clarification C# still uses Integer division in almost the same way, but I don't think there is a reason F# cannot try to do better.

Happypig375 commented 2 years ago

I wonder if we can introduce a div operator alongside https://github.com/fsharp/fslang-suggestions/issues/1065

jasiozet commented 2 years ago

I wonder if we can introduce a div operator alongside #1065

I think this could be an alternative solution. I think

/

and

//

are more discoverable, but some improvements are better than none 🙂

Happypig375 commented 2 years ago

// is already the "start of comment" in F#.

l3m commented 2 years ago

I wonder if we can introduce a div operator alongside #1065

Wouldn't this clash with HTML DSLs, widely used with fable?

Just having a warning might be safer.

jasiozet commented 2 years ago

If I'm not wrong in HTML DSLs it is used as Html.div

jcmrva commented 2 years ago

If it's not going to be an operator, it might as well be an extension on Math.

l3m commented 2 years ago

If I'm not wrong in HTML DSLs it is used as Html.div

Well, this is an example from Bolero:

let myElement (name: string) : Node =
    div {
        h1 { "My app" }
        p { $"Hello {name} and welcome to my app!" }
    }
voronoipotato commented 1 year ago

I think the suggestion is fantastic and doesn't need additional features. Lets not make this harder than it has to be. The suggestion a minimally invasive change. Everything else in this thread is a breaking change. If you want to do something other than a warning, I think it would be best to submit another suggestion that you can reference this issue with unless you have an objection to adding a warning. That way we don't clutter up this suggestion with discussion with essentially other suggestions.

cartermp commented 1 year ago

I would personally support an off-by-default warning here.

voronoipotato commented 1 year ago

Same. I'd personally turn it on and use #nowarn in the very unlikely event I really mean it, but I wouldn't complain terribly about having it off-by-default.

dsyme commented 1 year ago

I'm fine with an off by default warning, given the discussion above.