Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use in Async throws on null #1231

Closed
polytypic opened this issue May 31, 2016 · 11 comments · Fixed by #1233
Closed

use in Async throws on null #1231

polytypic opened this issue May 31, 2016 · 11 comments · Fixed by #1233

Comments

@polytypic
Copy link

I just noticed the discrepancy demonstrated below:

$ fsharpi
F# Interactive for F# 4.0 (Open Source Edition)
//...
> Async.RunSynchronously (async { use x = null in return () }) ;;
System.NullReferenceException: Object reference not set to an instance of an object
//...
> use x = (null: System.IDisposable) in () ;;
val it : unit = ()

It might make sense to change the Async.Using method to better match core language use expression semantics. The fix is trivial null check before calling Dispose.

@forki
Copy link
Contributor

forki commented Jun 1, 2016

Are you talking about Async.usingA in control.fs?

    /// Implement use/Dispose
    let usingA (r:'T :> IDisposable) f =  
        tryFinallyA (fun () -> r.Dispose()) (callA f r)

If I change it to

    let usingA (r:'T :> IDisposable) f =  
        tryFinallyA (fun () -> if not (isNull r) then r.Dispose()) (callA f r)

then we change the signature. Specifically the type paramter 'T is now restricted to have null as value.

If I change it to

    let usingA (r:'T :> IDisposable) f =  
        tryFinallyA (fun () -> if r <> Unchecked.defaultof<'T> then r.Dispose()) (callA f r)

then we restrict 'T to implement equality.

Both create compile errors.

Any ideas?

@forki
Copy link
Contributor

forki commented Jun 1, 2016

OK,

    /// Implement use/Dispose
    let usingA (r:'T :> IDisposable) f =  
        tryFinallyA (fun () -> if not (Unchecked.equals r Unchecked.defaultof<'T>) then r.Dispose()) (callA f r)

compiles and seems to work.

image

I'm feeling dirty now and need to take a shower - but after that I will probably send a PR ;-)

@polytypic
Copy link
Author

Yes, and I noticed the same difficulty after posting the issue. I used a C# primitive to work around it. The C# primitive seems to produce the desired compile outcome on .Net (no code on a struct, simple pointer test on a class). The same CIL code sequence can probably be emitted in F# somehow.

I think the defaultof<_> comparison will not work as desired on structs.

@forki
Copy link
Contributor

forki commented Jun 1, 2016

@polytypic can you come up with a minimal struct sample that I can add as additional unit test to verify if it works?

@eiriktsarpalis
Copy link
Member

If you don't mind the boxing on structs there's also
Object.ReferenceEquals()
On Wed, Jun 1, 2016 at 10:19 Steffen Forkmann notifications@github.com
wrote:

@polytypic https://github.com/polytypic can you come up with a minimal
struct sample that I can add as additional unit test to verify if it works?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1231 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ACrts1xNL-CtM8DXpEriyL8l3mCfODycks5qHTKRgaJpZM4IrATm
.

forki added a commit to forki/visualfsharp that referenced this issue Jun 1, 2016
@forki
Copy link
Contributor

forki commented Jun 1, 2016

mhm I think we need a primitive like isNull that doesn't restrict the type and does the same as polytypic's C# primitive. That one could also be exposed so that we can use it in other libs.

@polytypic
Copy link
Author

I just used a Dummy struct like here. You probably also want to test that Dispose is actually called (do some side-effect in Dispose).

@forki
Copy link
Contributor

forki commented Jun 1, 2016

I just found this little gimmick in prim-types.fs:

let inline notnullPrim<'T when 'T : not struct>(x:'T) = (# "ldnull cgt.un" x : bool #)"

that's not too bad, right?

forki added a commit to forki/visualfsharp that referenced this issue Jun 1, 2016
@forki
Copy link
Contributor

forki commented Jun 1, 2016

forki added a commit to forki/visualfsharp that referenced this issue Jun 1, 2016
forki added a commit to forki/visualfsharp that referenced this issue Jun 1, 2016
forki added a commit to forki/visualfsharp that referenced this issue Jun 1, 2016
@forki
Copy link
Contributor

forki commented Jun 1, 2016

Ok this is weird.

I created a new function:

    let inline notnullFast (x: ^T) = 
         (# "ldnull cgt.un" x : bool #)
         when ^T struct = true

and it seems the function has the correct type:

image

but somehow it's changing the usingA signature to something completely silly:

image

forki added a commit to forki/visualfsharp that referenced this issue Jun 1, 2016
@forki
Copy link
Contributor

forki commented Jun 2, 2016

So @dsyme helped me and the PR is ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants