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.9k stars 783 forks source link

DefaultAugmentation on DU leads to badly generated type #9178

Open GratianPlume opened 4 years ago

GratianPlume commented 4 years ago

I fell into a pit. exp: Writting a DU in Liberary1:

[<DefaultAugmentation false>] // focus on it
type Bar =
| Test1 of  int
| Test 2

module Bar =
    let inline  foo x = // focus on "inline" 
         match x with
         | Test1 x -> x
         | Test2 -> 0

and then write code in Liberary2

let foo2 x = Bar.foo x

As foo2 is called, some exception is happend. Because DefaultAugmentation false don't generate property Tag, the inline function foo use _tag for judgment. And when we using Bar.foo in another liberary, the code inline to foo2. It visit _tag, bug it's an internal value...

abelbraaksma commented 4 years ago

Something seems to have gone wrong with the layout of the code sections of your post, can you edit for readability?

And you mention "some exception", can you paste the explicit English exception message and exception code? Otherwise it's hard to find out what went wrong, or whether it's a bug.

abelbraaksma commented 4 years ago

Btw, why do you want to use DefaultAugmentationAttribute with false? It removes the code normally generated for proper operation of the DU, and I think you'd then have to write the augmentation by hand (there's little info on that attribute, but that's what I'd expect, and I think it's primarily used inside the compiler, i.e. with the Option type).

GratianPlume commented 4 years ago

Btw, why do you want to use DefaultAugmentationAttribute with false? It removes the code normally generated for proper operation of the DU, and I think you'd then have to write the augmentation by hand (there's little info on that attribute, but that's what I'd expect, and I think it's primarily used inside the compiler, i.e. with the Option type).

Well, I'd made a custom Option shared in C# and F#, so that the C# project won't import the FSharp.Core.dll, and when I update the 'FSharp.Core.dll' in F#, the related C# project do not have to NuGet a new one. Btw I want it to have a friendly name and easier using in C#,etc. I use DefaultAugmentationAttribute with false to avoid exposing unnecessary interfaces. I don't know that I made the mistake.

the exception code is simple , it just told the the visit for _tag fail. I copy it flow:

Attempt by method 'MiroIotHelper.Security.CreateRsaKeyPair(System.String, System.Net.WebSockets.WebSocket, System.Threading.CancellationToken)' to access field 'System.Option1<System.__Canon>._tag' failed. at MiroIotHelper.Security.CreateRsaKeyPair(String key, WebSocket websocket, CancellationToken cancellationToken) in D:\Git\MiroIotCore3\MiroIotHelper\Security.fs:line 88 at MiroIotCore.WebSockets.Controllers.WsWebController.GetRsaKey() in D:\Git\MiroIotCore3\MiroIotCore3\WebSockets\Controllers\WsWebController.cs:line 76 at MiroIotCore.WebSockets.Controllers.WsWebController.WebEvents_OnGetRsaKey(Object sender, WsWebCommand msg) in D:\Git\MiroIotCore3\MiroIotCore3\WebSockets\Controllers\WsWebController.cs:line 51 at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__139_1(Object state) at System.Threading.QueueUserWorkItemCallback.<>c.<.cctor>b__6_0(QueueUserWorkItemCallback quwi) at System.Threading.ExecutionContext.RunForThreadPoolUnsafe[TState](ExecutionContext executionContext, Action1 callback, TState& state) at System.Threading.QueueUserWorkItemCallback.Execute() at System.Threading.ThreadPoolWorkQueue.Dispatch() at System.Threading._ThreadPoolWaitCallback.PerformWaitCallback()

TIHan commented 4 years ago

@greatim

Thank you for reporting this. I am having trouble reproducing your issue. I created two libraries like you said and the code generation appears to be correct; it's not emitting _tag.

Is it possible that you could share an example solution? What version of the F# compiler are you using?

Also, I'm not sure what you are trying to do with your project by creating your own option type. I understand you are trying to keep the FSharp.Core.dll dependency away from your C# project. My recommendation is that if you are building an API in C# to be consumed in F#, I would build your C# project and API just like any other C# project would, even if it means returning null. On the F# side, you could create a shim on top of it. This might be painful if the API has a large surface area, but at least it makes the C# project predictable.

abelbraaksma commented 4 years ago

Well, I'd made a custom Option shared in C# and F#, so that the C# project won't import the FSharp.Core.dll, and when I update the 'FSharp.Core.dll' in F#, the related C# project do not have to NuGet a new one.

About this, as I've once tried something similar (ridding of the dependency, not mimicking existing functionality):

  1. Fsc has a compiler option --standalone, this will create new types for anything from Fsharp.Core.dll. However, you'll have to be careful, as your dll can be used from C#, but its use from F# is limited, due to the obvious type collisions (same names and namespaces, different types).
  2. Fsc has the command line option --static-link, which will statically link any dll you reference. I've no experience with this, but the difference should be that types are not copied to new types, but keep the same identity.
  3. You say you need to update Fsharp.Core.dll in C#. You don't. If you reference the F# project, or dll, it'll include this automatically. Just like other dependencies. In fact, C#, VB and F# rely heavily on component based development. Dependencies are a normal thing in that world. Try, for instance, to reference System.Collections.Immutable.dll, it'll add a dependency to System.Memory.dll as well. Same is true for System.Json.dll. Conflicts in versions are resolved through app.config with binding redirects. All MS libraries are designed to be backwards compatible, so even if your C# library depends on an older F# library, that won't give problems, as long as you use binding redirects. These can be added automatically to your projects by NuGet.
  4. It's next to impossible to manually create an F# library that doesn't depend on FSharp.Core.dll, unless you use the aforementioned compiler options. The minute you use a higher order function (and you almost always do), or use raise for exceptions, you create such a dependency. Just recreating core types like option, apart from that potentially causing a lot of extra work and issues, isn't going to cut it. Btw, your type will not work with existing types, a simple Seq.choose won't work, so you'll have to recreate all these functions too. You'll lose a lot of performance, as internally those functions call Adapt, which speeds up iterative operations, but that function exists in FSharp.Core.dll too.
  5. Instead of trying to break iron with your bare hands, several tools exist to create single file executables or libraries. Think of ILMerge, Warp, monolinker. They're free and the latter too open source. They work well.
  6. An alternative to dll merging is dll embedding. In this scenario, you embed the dlls you need as an embedded resource, and you register an event to the (iirc) AssemblyLoadEvent to manually load the dll you need. To make sure it's done in time, add a stub to the hidden Module.cctor,which is called before anything else. You can weave this with Fody and its ModuleInit module. It's what I used to do in the past, when I used a library that caused conflicts regardless of binding redirects (the affecting library is fixed, and I can rely safely on binding redirects again). You can also use this solution to log assembly binding issues (to some extend, at least).

I can't really judge why you need to do what you do, perhaps it really is needed, but I can show you the drawbacks and pitfalls. If you can choose any of the other existing solutions out there, you'll have a much cleaner and more pleasant coding experience.

GratianPlume commented 4 years ago

@TIHan @abelbraaksma Thanks for your suggestions.@abelbraaksma gived me so many tips, and they are great.

I have to build a complete agent library for downstream engineers. In this magical country, 99.99% engineers using .net are using C #, I can't count more than 100 people using F #. C#'s world is not the F#'s world. People feel Unit``FSharpFunc``FSharp*** strange -- they look bad, they look dangerous. If I let them see these, I should explain a lot.

Option type is an exception, it's simple and important. Because F# is inconvenient to handle refrence type null and Nullable(). If I don't want to write too many AllowNullLiteral or Unchecked.default<_>, I should make a hard sell on Option , and guide other engineers avoid to include a null value.

I know, when my libray is used, it surely depend on FSharp.Core.dll, But I hope it linked in more silence, less nuget, less configuration. That's enough.

GratianPlume commented 4 years ago

@greatim

Thank you for reporting this. I am having trouble reproducing your issue. I created two libraries like you said and the code generation appears to be correct; it's not emitting _tag.

Is it possible that you could share an example solution? What version of the F# compiler are you using?

Also, I'm not sure what you are trying to do with your project by creating your own option type. I understand you are trying to keep the FSharp.Core.dll dependency away from your C# project. My recommendation is that if you are building an API in C# to be consumed in F#, I would build your C# project and API just like any other C# project would, even if it means returning null. On the F# side, you could create a shim on top of it. This might be painful if the API has a large surface area, but at least it makes the C# project predictable.

I make a solution that can reproduce the error. I build it with vs2019: http://www.miro-iot.com:5000/DefAugFalseTest.zip

abelbraaksma commented 4 years ago

Option type is an exception, it's simple and important. Because F# is inconvenient to handle refrence type null and Nullable().

Instead of exposing option, may I suggest to expose normal classes. C# now supports nullability, which you aan leverage to avoid nulls.

Alternatively, use Option.ofObj for anything that comes in from exposed interfaces, it's safe. Don't expose Option itself, indeed, that's confusing for C# developers.

Don't use defaultOf, unless you really have to. Certainly, C# programmers should never use it.

Expose interfaces, not modules, functions or classes. Though classes is possible, age well understood by C# programmers, interfaces are cleaner, and very easy to work with from F#.

Use tupled arguments for anything you expose, this keeps things simple for C# guys.

Write a bunch of partial patterns if you need to match over some unsafe input from C# classes or BCL input. That way you prevent yourself from having to write too many cluttering if statements.

These are just some thoughts, but most F# coders that create libs to interact with C# take this route. Sometimes they also expose a set of module functions, but put them into a specific namespace ending with "FSharp", to emphasize that they should not be used by C# programmers.

I have many libraries that interact with C# and I share your pain, but much becomes easy with a few wrappers and a handful interfaces to expose stuff to the outside world.

GratianPlume commented 4 years ago

@abelbraaksma

use Option.ofObj for anything that comes in from exposed interfaces, it's safe.

But how to give a option value to C#? If use Option.toObj, I should use AllowNullLiteral.Even using tuple, it also needs a value of null/defaultOf . Btw, will F# provide a easy way to make a TupleElementNames attribute in the future?

C# now supports nullability, which you aan leverage to avoid nulls.

When I first time here that, I am full of expectations. But when C#8.0 released. The new asp.net core 3 project doesn't enable it by default. I suddenly realized, it need a lot of courage to open the box. This means many changes from the old project. Models need to add a ? to the field, they are models for json or ef. How much will it affect if we miss something? It takes a long time for taste, before we can evaluate it, either to find a correctly posture for open .

abelbraaksma commented 4 years ago

But how to give a option value to C#? If use Option.toObj, I should use AllowNullLiteral

I don't. You shouldn't expose Option. If you need to return a value that's an option type, use Option.toObj. It'll return null for None, just as C# programmers expect. Better yet to don't return anything that can be null, but that's not always possible.

A good alternative is to use a byref that holds a boolean, if you want to return value types that can be null, or to indicate success/fail, i.e. if the semantics are similar to methods like Int32.TryParse.

The trick is to check how C# best practices go for libraries, and adopt that in your interfaces and signatures.

Even using tuple, it also needs a value of null/defaultOf .

Also no. Stay clear of such practices, it'll clutter your code and you'll lose a lot of the benefits of using F#. What I meant was: when you declare a method that's to be used from C#, comma-separate the arguments. This is what "tupled arguments" means. C# won't see the tuples, just a normal method for them.

I.e.: instead of member this.Foo a b = a + b, use member this.Foo(a, b) = a + b.

Btw, will F# provide a easy way to make a TupleElementNames attribute in the future?

I don't understand this, but the same rule should apply: don't expose F# specific things, keep them internal. This includes:

A notable exception is records, they're quite natural to use from C#.

All this doesn't mean you cannot use it, it only means you shouldn't expose them.

Models need to add a ? to the field, they are models for json or ef. How much will it affect if we miss something?

I cannot help you with that. But we're talking about interfacing here, between two projects. You can only enable it for those, and not for all projects at once. However, it's probably worth the effort, as it makes C# programming a lot safer.

abelbraaksma commented 4 years ago

But when C#8.0 released.

It was released in September 2019, along with Visual Studio 2019, 16.3. That it isn't enabled by default allows you to migrate one by one. And as you saw in that link, you can migrate slowly, since it'll only raise warnings for mistakes. This too will help programmers improve their code, but they don't have to do it all at once, it'll continue to compile just fine.

GratianPlume commented 4 years ago

If you need to return a value that's an option type, use Option.toObj. It'll return null for None, just as C# programmers expect. Better yet to don't return anything that can be null, but that's not always possible.

I mean this:

    type IAh = interface end

    let Foo (x: IAh Option) =
        Option.toObj x    // FS0001

WIthout Option I can't easily express None case.

abelbraaksma commented 4 years ago

@greatim, perhaps I was describing it all a little too abstract. Interfaces in F# are guarded for null, but in C# they aren't, so when you have a function that operates on input from an unknown source (i.e., C#), you'll need to check for null regardless.

Since you already need to do that, and because you don't want C# people to have to deal with Some/None, here's one way you could code that in a way that is convenient for you. All you basically need is two helper functions:

  1. The first one, here getUnsafe, operates on input from an unknown source and turns it into a safe option type for further processing.
  2. The second one, here convertForCSharpUnsafe, takes a safe option of any type, and it turns it into either a null reference, or the contents of Some, of the same type.

This way you maintain type-safety throughout and you can ease the burden on the C# people, in the sense that they can continue to use their unsafe coding practices with null references etc. They won't have to learn anything new.

/// Public, shared type between you and others (i.e., both F# and C#)
type IAh = 
    abstract member CallHome: unit -> bool

/// An example function that uses F# style of coding
let internal internalSafeFunction (x: IAh option) =
    match x with
    | Some x -> x.CallHome()
    | None -> false

/// The F# way of safely creating an instance of the interface
let internal internalSafeCreateFunction x =
    if x > 0 then Some { new IAh with member _.CallHome() = true }
    else None

/// A helper that can get any type of object, if input is null, returns a typed None value.
let internal getUnsafe (x: 'V) =
    box x 
    |> Option.ofObj 
    |> Option.map (fun x -> x :?> 'V)

/// A helper that will create null if None is given. This prevents C# people to have to work with Option, instead they can continue to work with null.
/// (you may want to guard this against value types, though).
let internal convertForCSharpUnsafe (x: 'V option) =
    match x with
    | Some v -> v
    | None -> Unchecked.defaultof<'V>       // the only place where you need defaultof<_>.

/// The actual class or classes that you expose to C#. This should only contain simple wrappers into your safe functions.
type ExternalExposedToCSharpClass =
    /// Instead of taking an option, it safely operates on null if a C# user hands in a null, while you can still work null-safe.
    static member tryCallHome(x: IAh) =
        getUnsafe x 
        |> internalSafeFunction

    /// Instead of returning an option, it returns null to C# if you internally were working with None.
    static member createMaybeInterface x =
        internalSafeCreateFunction x
        |> convertForCSharpUnsafe

Usage (as if from C#):

> let var = ExternalExposedToCSharpClass.createMaybeInterface 0;;
val var : IAh = null

> let var = ExternalExposedToCSharpClass.tryCallHome var;;
val var : bool = false

> let var = ExternalExposedToCSharpClass.createMaybeInterface 1;;
val var : IAh

> let var = ExternalExposedToCSharpClass.tryCallHome var;;
val var : bool = true

Note that you still cannot call the function directly in F# with null, which is good, it'll protect you from misuse (but C# can, and you can too, in F#, by using a variable of the proper type, as shown in the first two lines above):

> let var = ExternalExposedToCSharpClass.tryCallHome null;;

  let var = ExternalExposedToCSharpClass.tryCallHome null;;
  ---------------------------------------------------^^^^

C:\Users\Abel\AppData\Local\Temp\stdin(7,52): error FS0071: Type constraint mismatch when applying the default type 'IAh' for a type inference variable. The type 'IAh' does not have 'null' as a proper value Consider adding further type constraints
GratianPlume commented 4 years ago

@abelbraaksma OK, I got it now. I should take a unsafe handle in a helper module one time, but not to use Unchecked.defaultof<_> every where.

abelbraaksma commented 4 years ago

Indeed, and clearly separate what you're exposing to C#, and what you're internally using. That way you keep all the freedom and don't have to worry about what you can and cannot expose to C#, since you will only expose clean static classes, "normal" classes and interfaces. From the C# point of view, this will look just as if they use any other normal library.

You may want to add another helper function: one that raises an ArgumentNullException for those methods that really shouldn't accept null as an alternative. This too, is common practice for C# (i.e., most Linq functions work this way).

NinoFloris commented 4 years ago

Running into this as well, this is a bug in the compiler. (in my case I wanted to redefine the case helpers IsFoo so they are visible to F# code)

This returns nasty exceptions: System.FieldAccessException: Attempt by method 'x' to access field 'Lib.Skippable`1<System.__Canon>._tag' failed.

abelbraaksma commented 4 years ago

@ninofloris? You mean when using DefaultAugmentation? I don't know if it's possible to create a useful DU if you have to create all helpers by hand. Why not just use isFoo instead?

There's a proposal out there that would expose some of these default properties to F#.

NinoFloris commented 4 years ago

@abelbraaksma every important bit of a DU is still defined (Tag, NewX etc.) DefaultAugmentations is really for the C# interop helpers as I understand it. There just seems to be a failing of the compiler somewhere to respect visibility of the _tag field backing Tag and as such access to _tag ends up getting emitted into other assemblies

abelbraaksma commented 4 years ago

Ah, I didn't know that. There's really very little info about that attribute ;)

dsyme commented 4 years ago

TBH I don't recommend using DefaultAugmentation(false).

However it should still work if used.