-
Notifications
You must be signed in to change notification settings - Fork 793
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
Comments
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 ...unless it's a record type. 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 Thankfully we can catch this, as it appears let getX a =
try
if a = Unchecked.defaultof<A> then -1
else a.X
with _ -> -2
getX Unchecked.defaultof<A> // returns -2 |
Ok I'll calm down now. After some reading I see that it's recommended to do null checks with There is also our new Regardless, this try/catch removal optimization still seems risky/bogus. |
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. |
(sorry - there's some github-on-mobile problem where pressing enter closes the bug!) |
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
|
Came across this when investigating #367. Consider the following code:
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>
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.
The text was updated successfully, but these errors were encountered: