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

Optimizer incorrectly assumes immutable field accesses are side-effect free #368

Closed
latkin opened this issue Apr 16, 2015 · 5 comments
Closed
Labels

Comments

@latkin
Copy link
Contributor

latkin commented Apr 16, 2015

Came across this when investigating #367. Consider the following code:

[<AllowNullLiteral>]
type A(x:int) =
    member __.X = x

let getX (a:A) =
    try a.X
    with _ -> -1

getX null

This code crashes with nullref, because the optimizer has stripped away the try/catch block completely, assuming that immutable field accesses are side-effect free and cannot trigger an exception.

See culprit code here and here.

I can understand intent of this - within pure-F# code, use of immutable F#-defined types should prevent stuff like nullrefs. However this breaks pretty easily in some cases:

  • [<AllowNullLiteral>]
  • Unchecked.defaultof<T>
  • C# interop

Perhaps we can blame the developer for shooting themselves in the foot on nos. 1 and 2. But for no. 3, it seems very terrifying that we silently remove the error handling.

@latkin
Copy link
Contributor Author

latkin commented Apr 16, 2015

The rabbit hole goes deeper...

If I'm a good library dev I won't be null-checking by catching nullref, I'll just check for null directly. Well, I can't do that unless I add [<AllowNullLiteral>] to my type. Bummer! Workaround would be to check input against Unchecked.defaultof< >. Ok that works fine...

...unless it's a record type. [<AllowNullLiteral>] isn't allowed on record types, so my only option is to compare against Unchecked.defaultof. Trying it:

type A = { X : int }

let getX a =
    if a = Unchecked.defaultof<A> then -1
    else a.X

getX Unchecked.defaultof<A> // or C# call passing null

But this crashes with nullref, as the "null check" codegens as a call to a.Equals( ... )!

Thankfully we can catch this, as it appears Unchecked.defaultof is assumed to have side effects, so try/catch is not removed this time.

let getX a =
    try
        if a = Unchecked.defaultof<A> then -1
        else a.X
    with _ -> -2

getX Unchecked.defaultof<A> // returns -2

@latkin
Copy link
Contributor Author

latkin commented Apr 16, 2015

Ok I'll calm down now. After some reading I see that it's recommended to do null checks with match box x with null -> ... and that will work for everything.

There is also our new isNull, though now I'm thinking this API would be more useful if it were applicable even to non-nullable types. Something like internal helper notnullPrim.

Regardless, this try/catch removal optimization still seems risky/bogus.

@dsyme
Copy link
Contributor

dsyme commented Apr 16, 2015

I'll follow up in more detail tomorrow, but there is a bit in the F# language spec about what assumptions can/can't be made about nullness. In general the compiler is allowed to assume that a range of values for which null is an abnormal value (functions, tuples, records, unions etc) are non-null.

For AllowNullLiteral types, we should regard dereferencing a field as an effect and we should do that for F# 4.0.

@dsyme dsyme closed this as completed Apr 16, 2015
@dsyme dsyme reopened this Apr 16, 2015
@dsyme
Copy link
Contributor

dsyme commented Apr 16, 2015

(sorry - there's some github-on-mobile problem where pressing enter closes the bug!)

@latkin
Copy link
Contributor Author

latkin commented Apr 17, 2015

Fair enough. Very interesting scenarios to ponder... I assume this is maybe related:

[<AllowNullLiteral>]
type A() =
    member this.IsNull() = (this = null)

(null : A).IsNull()  // returns true, no nullref

IsNull gets completely inlined, based (presumably) on the same logic of "this can't be null, so there is no chance of a nullref, so it is semantically equivalent to just inline everything"

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

No branches or pull requests

2 participants