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

SkipLocalsInit is an unsafe operation, but does not give warning FS0009 #17116

Open
abelbraaksma opened this issue May 2, 2024 · 4 comments
Open
Labels
Analyzers Area-Diagnostics mistakes and possible improvements to diagnostics Feature Improvement
Milestone

Comments

@abelbraaksma
Copy link
Contributor

abelbraaksma commented May 2, 2024

Applying the attribute SkipLocalsInitAttribute is an unsafe operation, but does not raise the FS0009 warning.

Repro steps

type Rain() =
    [<SkipLocalsInit>]
    member _.ItPours(?msg: string) =
        let message= defaultArg mname "absent"
        $"Message: {message}"

Expected behavior

According to the docs, this is an unsafe operation.

F# issues the FS0009 warning for unsafe operations (like fixed and using nativeptr and the like). This warning should be thrown when using SkipLocalsInit as well.

This is akin to using the unsafe keyword in C#, which is indeed required for this attribute.

Actual behavior

The warning is not thrown.

Related information

The suggestion does not mention the term unsafe, but @dsyme mentioned

It's also in the realm of a "allow generation of unverifiable code for performance reasons" feature

which further cements that this is unsafe and that FS0009 should be shown here.

Since this is just a warning, I hope this can be added to F# still, without being considered a backward compatibility issue.

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented May 3, 2024

Actually, trying out some scenarios:

  • In C#, if you use pointers to access locals, you will get an error for the unsafe pointer code
  • If you do not use other unsafe constructs, the attribute alone does not raise the unsafe error and you do not need the unsafe keyword
  • If you do not use the unsafe switch, by default, C# prevents you from using unassigned variables (but there are workarounds with structs).
  • In F#, if you use any pointers, you will of course get the FS0009
  • In F#, if you do not use pointers, F# prevents you from using unassigned (mutable) let bindings. I am not aware of a constructs that would access uninitialized stack memory without getting an FS0009

In other words, the raised issue may not be an issue in practice, as the FS0009 will be raised as soon as you would access the unassigned memory.

@abonie abonie added Feature Improvement Area-Diagnostics mistakes and possible improvements to diagnostics Analyzers and removed Bug Needs-Triage labels May 6, 2024
@kerams
Copy link
Contributor

kerams commented May 6, 2024

perhaps you know whether there was a decision here?

No decision on my part. It never occurred to me to add a warning.

I am not aware of a constructs that would access uninitialized stack memory without getting an FS0009

Uninitialized local.

> [<System.Runtime.CompilerServices.SkipLocalsInit>]
let a () =
    match Map.empty.TryGetValue "" with
    | false, (x: float) -> printfn "%A" x
    | _ -> ();;
val a: unit -> unit

> a ();;
6.951863497e-310

@abelbraaksma
Copy link
Contributor Author

That looks like a bug, @kerams. I believe F# normally initializes any let bound value, and here it's implicitly let bound (I don't think this is possible with normal let or val keywords).

I would have expected pointers or references to be required to access initialized memory.

@dsyme even mentioned something along the lines to use this attribute in F# internally, as F# itself forces initialization.

@abelbraaksma
Copy link
Contributor Author

Oh wait. This is actually an outref, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analyzers Area-Diagnostics mistakes and possible improvements to diagnostics Feature Improvement
Projects
Status: New
Development

No branches or pull requests

3 participants