dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.86k stars 777 forks source link

Units of Measure - non-Measure type parameter can be passed as a Measure parameter #10824

Open bzuu opened 3 years ago

bzuu commented 3 years ago

The following code compiles and works fine in debug mode, however in release mode it compiles, but then throws a MissingMethodException when run.

Given a file in project Lib:

module Lib

open System

type Id<[<Measure>] 'T> =
    private
        { Id: Guid }

    static member Create<'T> id: Id<'T> =
        if id = Guid.Empty then
            invalidArg (nameof (id)) "ID cannot be empty"
        else
            { Id = id }

    static member New<'T>(): Id<'T> = { Id = Guid.NewGuid() }

    member this.Value = this.Id

And the following file in project App with a reference to project Lib:

module App

open System
open Lib

[<Measure>] type product

type Product = { Id: Id<product>; Name: string }

let displayProduct p = printfn $"{p.Id.Value} - {p.Name}"

[<EntryPoint>]
let main argv =

    let bananas = { Id = Id.New<product>(); Name = "bananas" }
    displayProduct bananas

    let pancakes = { Id = Guid("47367575-1ea9-49b8-9720-9f9b869ddf17") |> Id.Create<product>; Name = "pancakes" }
    displayProduct pancakes
    0

Provide the steps required to reproduce the problem:

  1. You can successfully compile this code with the debug or release configuration:

    
    dotnet build App --configuration Release          
    Microsoft (R) Build Engine version 16.8.0+126527ff1 for .NET
    Copyright (C) Microsoft Corporation. All rights reserved.
    
    Determining projects to restore...
    All projects are up-to-date for restore.
    Lib -> C:\Code\uom-issue\Lib\bin\Release\net5.0\Lib.dll
    App -> C:\Code\uom-issue\App\bin\Release\net5.0\App.dll

Build succeeded. 0 Warning(s) 0 Error(s)

2. You can also run the code in debug mode without issues:

C:\Code\uom-issue> dotnet run --project App --configuration Debug
c94d0902-7476-454e-bfac-e0cddd42372b - bananas 47367575-1ea9-49b8-9720-9f9b869ddf17 - pancakes

3. However in release mode it throws an exception:

C:\Code\uom-issue> dotnet run --project App --configuration Release
Unhandled exception. System.MissingMethodException: Method not found: 'Id Id.New()'. at App.main(String[] argv)


[Sample.zip](https://github.com/dotnet/fsharp/files/5755639/Sample.zip)

**Expected behavior**

I would expect the code to either fail to compile in both configuration OR run fine in both configurations.

**Known workarounds**

When the type annotations on the `Create` and `New` method are removed everything works fine, i.e. when the code in the library is changed to this:

```fsharp
module Lib

open System

type Id<[<Measure>] 'T> =
    private
        { Id: Guid }

    static member Create id =
        if id = Guid.Empty then
            invalidArg (nameof (id)) "ID cannot be empty"
        else
            { Id = id }

    static member New() = { Id = Guid.NewGuid() }

    member this.Value = this.Id

Related information

Provide any related information (optional):

cartermp commented 3 years ago

Thanks for filing, we should take a look at this

vzarytovskii commented 3 years ago

Minimal repro:

module Program =

    [<Measure>] type userId

    [<NoEquality; NoComparison>]
    type Id<[<Measure>] 'T> = { Id: int } with
        static member Create<'T> id : Id<'T> = failwithf "%A" id

    [<EntryPoint>]
    let main argv =
        {| Id = Id.Create<userId> 42 |} |> ignore
        0

However if you replace

        static member Create<'T> id : Id<'T> = failwithf "%A" id

with

        static member Create<'T> id : Id<'T> = { Id = id }

It works just fine. It seems optimizer is getting confused.

Gonna look into it.

vzarytovskii commented 3 years ago
module Program =

    [<Measure>] type meters
    type Square<[<Measure>] 'T> = 
        private { Side: int } with
        static member Create<'T> side : Square<'T> = { Side = side }
        static member CreateInMeters side = Square.Create<meters> side

(sharplab) Even more interesting, the code above will fail in both Debug and Release with the following error:

Error in pass3 for type Program, error: Error in pass3 for type Program, error: Error in pass3 for type Square, error: Error in GetMethodRefAsMethodDefIdx for mref = ("Create", "Square"), error: Exception of type 'FSharp.Compiler.AbstractIL.ILBinaryWriter+MethodDefNotFound' was thrown.

Which is exactly the same error as the original issue. Removing static member CreateInMeters side = Square.Create<meters> side will compile just fine (sharplab).

Calling .Create without type parameter compiles just fine:

module Program =

    [<Measure>] type meters
    type Square<[<Measure>] 'T> = 
        private { Side: int } with
        static member Create<'T> side : Square<'T> = { Side = side }
        static member CreateInMeters side = Square.Create side

(sharplab)

Using .Create<T> from the let binding will also work just fine:

module Program =

    [<Measure>] type meters
    type Square<[<Measure>] 'T> = 
        private { Side: int } with
        static member Create<'T> side : Square<'T> = { Side = side }

    let createInMeters side = Square.Create<meters> side

(sharplab)

However, if you force .Create to not inline, it fails even from let binding:

open System
open System.Runtime.CompilerServices
module Program =   
    [<Measure>] type userId   

    [<NoEquality; NoComparison>]
    type Id<[<Measure>] 'T> = { Id: int } with       

        [<MethodImpl(MethodImplOptions.NoInlining)>]
        static member Create<'T> id : Id<'T> = { Id = id }

    let f() = Id.Create<userId> 123

(sharplab)

Note, that, if we use normal types as oppose of UOM (i.e. it's not erased), it works fine:

module Program =

    type Square<'T> = 
        private { Side: 'T } with
        static member Create<'T> side : Square<'T> = { Side = side }
        static member CreateInt side = Square.Create<int> side

(sharplab)

bzuu commented 3 years ago

@vzarytovskii interesting to see how many different ways there are to trigger this. I hadn't realised it when I filed the issue originally.

vzarytovskii commented 3 years ago

Lastly, after poking around the code and talking to Will, the following code will work as expected:

open System
module Program =
    type Id<[<Measure>] 'T> =
        private
            { Id: Guid }

        static member Create<[<Measure>] 'T> id: Id<'T> =
            if id = Guid.Empty then
                invalidArg (nameof (id)) "ID cannot be empty"
            else
                { Id = id }

        static member New<'T>(): Id<'T> = { Id = Guid.NewGuid() }

        member this.Value = this.Id

    [<Measure>] type product

    type Product = { Id: Id<product>; Name: string }

    let displayProduct p = printfn $"{p.Id.Value} - {p.Name}"

    [<EntryPoint>]
    let main argv =
        let bananas = { Id = Id.New<product>(); Name = "bananas" }
        displayProduct bananas

        let pancakes = { Id = Guid("47367575-1ea9-49b8-9720-9f9b869ddf17") |> Id.Create<product>; Name = "pancakes" }
        displayProduct pancakes
        0

Notice that Create method now has the [<Measure>] attribute, which makes sense.

I think we should produce proper error: Expected unit-of-measure parameter, not type parameter. Explicit unit-of-measure parameters must be marked with the [<Measure>] attribute. like we do in other cases when we use UOMs.

cc @bzuu

bzuu commented 3 years ago

@vzarytovskii interestingly the code with the [<Measure>] attribute on the Create method works when it is all in a single project, however it still fails when the code is split in two projects... When you also add the [<Measure>] attribute on the New method it does work also when split in two projects.

vzarytovskii commented 3 years ago

@vzarytovskii interestingly the code with the [<Measure>] attribute on the Create method works when it is all in a single project, however it still fails when the code is split in two projects... When you also add the [<Measure>] attribute on the New method it does work also when split in two projects.

Huh interesting.

I will look into it, however it shouldn't pass typecheker if it lacks MeasureAttribute and when it is used as measure, I think. The same way we disallow it here:

    let beef<[<Measure>] 'T>() = ()
    let beef2<'T>() = beef<'T>() // this will fail to compile.
bzuu commented 3 years ago

Perhaps it has to do with how the typechecker works across projects? As the uom types are removed at compile time this is perhaps why it doesn't correctly detect the mismatch?

vzarytovskii commented 3 years ago

@bzuu

Perhaps it has to do with how the typechecker works across projects? As the uom types are removed at compile time this is perhaps why it doesn't correctly detect the mismatch?

I think it related to inlining and how it works when one module vs different ones.

For example, the following will fail, since it's forced not to inline:

open System
open System.Runtime.CompilerServices
module Program =
    type Id<[<Measure>] 'T> =
        private
            { Id: Guid }

        static member Create<[<Measure>] 'T> id: Id<'T> =
            if id = Guid.Empty then
                invalidArg (nameof (id)) "ID cannot be empty"
            else
                { Id = id }

        [<MethodImpl(MethodImplOptions.NoInlining)>]
        static member New<'T>(): Id<'T> = { Id = Guid.NewGuid() }

        member this.Value = this.Id

    [<Measure>] type product

    type Product = { Id: Id<product>; Name: string }

    let displayProduct p = printfn $"{p.Id.Value} - {p.Name}"

    [<EntryPoint>]
    let main argv =
        let bananas = { Id = Id.New<product>(); Name = "bananas" }
        displayProduct bananas

        let pancakes = { Id = Guid("47367575-1ea9-49b8-9720-9f9b869ddf17") |> Id.Create<product>; Name = "pancakes" }
        displayProduct pancakes
        0
dsyme commented 2 years ago

Yes, this is a bug in checking where [<Measure>] is not being required, e.g. in the last repro this fixes it:

    static member New<[<Measure>] 'T>(): Id<'T> = { Id = Guid.NewGuid() }