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

Disallow "mutable" on byref<'T> locals #677

Open
TIHan opened this Issue Jun 20, 2018 · 8 comments

Comments

Projects
None yet
6 participants
@TIHan

TIHan commented Jun 20, 2018

Disallow "mutable" on byref<'T> locals

Currently, we can write code like this:

let mutable x = 1
let mutable y = &x
y <- 4

y is of type byref<int>. This means we can assign a value to the byref as such, y <- 4. But, we can do this even if y wasn't mutable: let y = &x; y <- 4. So what does mutable do here? It actually doesn't do anything as making this mutable gives intent of assigning byrefs with other byrefs, but we have no way to express that in F# (in fact we may never want to do that in F#). Therefore, mutable is moot at this point.

For devs to avoid this confusion, we should never allow mutable byref locals, by throwing an error stating that you cannot do this as it serves no purpose. Therefore, the code above should fail to compile. To fix it, write as such:

let mutable x = 1
let y = &x
y <- 4

In the future we may or may not to allow assigning byrefs, so it might look like this:

let mutable x = 1
let mutable w = 1
let mutable y = &x
y <- &w

But that is not part of this suggestion.

Pros

Better consistency. Less confusion.

Cons

This is technically a breaking change, but we think most people do not make this mutable because it doesn't do anything.

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • This is not a breaking change to the F# language design
  • I or my company would be willing to help implement and/or test this
@matthid

This comment has been minimized.

matthid commented Jun 20, 2018

Another option would be to force/require the mutable keyword, correct? Or add a warning if the mutable keyword is missing.

@TIHan

This comment has been minimized.

TIHan commented Jun 20, 2018

That would make it more of a breaking change.

let mutable x : byref<int> ... What does this mean to you? If it means that you can assign a value to the byref, then how would this work let f (x: byref<int>) = x <- 1. x is not mutable.

@smoothdeveloper

This comment has been minimized.

smoothdeveloper commented Jun 20, 2018

There might be some consideration to gather wrt tooling which shows mutable variables in specific color.

If I can assign using <-, I expect to see the same color as a proper mutable symbol.

@realvictorprm

This comment has been minimized.

Member

realvictorprm commented Jun 20, 2018

What about finally supporting mutable and non mutable byref?

@cartermp

This comment has been minimized.

Member

cartermp commented Jun 20, 2018

@realvictorprm Could you clarify what you mean?

@matthid

This comment has been minimized.

matthid commented Jun 22, 2018

But the problem here is that <- is ambiguous, correct? Your suggestion is (maybe in the future) to basically overload the <- depending on the types, correct? This sounds surprisingly dangerous to me. But also it feels like byref somehow "works around" the design idea of F# here (to annotate mutability)

@zpodlovics

This comment has been minimized.

zpodlovics commented Jul 3, 2018

It seems this is a continuation of F# optimalizations - earlier there was an optimization for laddrof (&&): Microsoft/visualfsharp#5148 In fact this is explicitly noted in the commit:

"2. In optimization, &x (i.e. LAddrOf) is a constant value even if x is mutable and so can be propagated (so for let y = &x in ... y ... the occurrences of y get replaced by &x and y gets removed)"

My concern is as usual native memory & atomic operations with F#. Assigning byrefs (aliases) have some legitimate usage there. What about byrefs that points to unmanaged memory? This is why the NativePtr.toByRef "invented" Microsoft/visualfsharp#409 (comment)

From one of my related issue comment (dotnet/coreclr#916 (comment)):

"Unique advantage of managed pointers (ref in C#) is that they can point to unmanaged memory, and be used in many of the same ways as unmanaged pointers. For example, check this code fragment:

IntPtr p = ...
Interlocked.CompareExchange(ref *(Int32*)p, 1, 0);

Thus the existing volatile and atomic operations defined on managed pointers should work fine for your scenario. The code is not as straightforward as it could be with unsafe helper method - the unsafe constructs are not straightforward in C# and .NET Core libraries on purpose, to encourage writing safe code."

Related Issue:
Microsoft/visualfsharp#409

@cartermp

This comment has been minimized.

Member

cartermp commented Sep 10, 2018

I think this should be implemented as a warning rather than an error. It's redundant code, but it doesn't affect you negatively if you do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment