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

Volatile.Read can no longer read ref (instead it wants inref) #16524

Closed
radekm opened this issue Jan 12, 2024 · 11 comments
Closed

Volatile.Read can no longer read ref (instead it wants inref) #16524

radekm opened this issue Jan 12, 2024 · 11 comments

Comments

@radekm
Copy link
Contributor

radekm commented Jan 12, 2024

Please provide a succinct description of the issue.

Repro steps

I have a following global variable defined in a module

let currentData : Data ref = ref { Timestamp = DateTimeOffset.MinValue
                                   Foo = None
                                   Bar = []
                                 }

and I read its value in some function via Volatile.Read:

// `toJson` has type `Data -> byte array`.
let json = Data.toJson (Volatile.Read currentData)

Expected behavior

The code should type check (it type checked on net7 and also initially on net8).

Actual behavior

After updating .NET 8 it doesn't type check with error:

error FS0041: No overloads match for method 'Read'. Known type of argument: Data ref
Available overloads:
- Volatile.Read(location: inref<bool>) : bool // Argument 'location' doesn't match
- Volatile.Read(location: inref<byte>) : byte // Argument 'location' doesn't match
...
- Volatile.Read<'T when 'T: not struct>(location: inref<'T>) : 'T // Argument 'location' doesn't match

I think that the last mentioned overload should work but it seems that ref<Data> argument
can't be given to inref<Data> parameter.

Known workarounds

Don't know.

Related information

Provide any related information (optional):

  • Tested under Windows, Linux, Mac (x86-64)
  • Type checked on .NET 8.0.100, doesn't work on .NET 8.0.101
  • Can it be related to 54d05be?
@vzarytovskii
Copy link
Member

vzarytovskii commented Jan 12, 2024

Duplicate of #16265
Fixed in #16232

@vzarytovskii vzarytovskii marked this as a duplicate of #16265 Jan 12, 2024
@vzarytovskii vzarytovskii closed this as not planned Won't fix, can't repro, duplicate, stale Jan 12, 2024
@vzarytovskii
Copy link
Member

Actually, let me double check, it. It sounds like a different issue. Though refs and byrefs are different things.

@radekm
Copy link
Contributor Author

radekm commented Jan 12, 2024

Is it really the same thing? Because #16265 says it has been fixed 2 days ago. But to me it seems the problem was just introduced.

@vzarytovskii
Copy link
Member

Is it really the same thing? Because #16265 says it has been fixed 2 days ago. But to me it seems the problem was just introduced.

This problem was introduced with fix of dotnet/runtime#94317, not entirely sure what's happening.

@vzarytovskii vzarytovskii reopened this Jan 12, 2024
@github-project-automation github-project-automation bot moved this from Done to In Progress in F# Compiler and Tooling Jan 12, 2024
@vzarytovskii
Copy link
Member

vzarytovskii commented Jan 12, 2024

@radekm

Immediate workaround would be to make it a byref:

open System;
open System.Threading
type Data = { Timestamp: DateTimeOffset }
let mutable currentData: Data  =
  { Timestamp = DateTimeOffset.MinValue }
let x = Volatile.Read &currentData

or

open System;
open System.Threading
type Data = { Timestamp: DateTimeOffset }
let mutable currentData: Data ref =
  ref { Timestamp = DateTimeOffset.MinValue }
let x = Volatile.Read &currentData

Until I figure out why don't we resolve the method anymore.

@vzarytovskii
Copy link
Member

vzarytovskii commented Jan 12, 2024

Ok, I think I understand what's going on, here's timeline:

  1. Before, Volatile.Read had byref parameter (or just ref in C#), it changed to ref readonly here: https://github.com/dotnet/runtime/pull/89736/files?diff=unified&w=0#diff-c2f9cef9820db1801d32a7f0da09d49669f2c49f92f833e9cbf3c2d934129956L221-R222
  2. F# compiler was treating ref readonly as byref in <=8.0.100, which was a breaking change, for all the changed places, as we needed to treat it as inref.
  3. It was fixed in 8.0.101 (here: Ad-hoc fix for ref readonly parameters #16232)
  4. Since we converted F# ref to byref, Volatile.Read worked in <=8.0.100, but stopped in 8.0.101.

I don't exactly know right now what can be done. Can we safely convert F# refs (not byrefs) to inrefs? @T-Gro @dsyme

I guess yes, if bindings are not mutable?

@vzarytovskii
Copy link
Member

vzarytovskii commented Jan 12, 2024

To recap:

  1. There are different type of "refs"

    • F# ref - just a type you prefix with ref, is a RefCell
    • .NET byref - just a byref type, denoted as ref in C#.
    • .NET inref - C# in.
    • .NET outref - C# out.
    • .NET ref readonly - C# ref readonly.
  2. Before 8.0.100 (in 7.0.xxx), Volatile.Read's had a byref parameter, and we could convert F# ref to byref<'T> (still can).

  3. In 8.0.100, Volatile.Read's parameter type changed to ref readonly, and F# compiler treated is as byref (though should've as inref) - this was a breaking change for all APIs which did such change pretty much - they stopped accepting byref params.

  4. In 8.0.101+, F# compiler started treating ref readonly params as inref - so we can't pass F# ref anymore, since there's no conversion between ref and inref, only ref and byref.

So, this is not exactly a bug in compiler, but a change in BCL which caused that (since ref params are not really equal to ref readonly).

@T-Gro
Copy link
Member

T-Gro commented Jan 12, 2024

F# compiler could convert a RefCell to inref to overcome this breaking change, that should work.

@vzarytovskii
Copy link
Member

F# compiler could convert a RefCell to inref to overcome this breaking change, that should work.

But probably non-mutable refcell to inref, right? Or any?

@vzarytovskii
Copy link
Member

vzarytovskii commented Jan 12, 2024

@T-Gro what bothers me slightly, we have the following in our method calls checking:

// If the called method argument is an inref type, then the caller may provide a byref or value

So, it was a conscious decision to restrict it to only refs and byrefs.

vzarytovskii added a commit to vzarytovskii/fsharp that referenced this issue Jan 12, 2024
@vzarytovskii
Copy link
Member

vzarytovskii commented Jan 15, 2024

I think it shouldn't be fixed and considered by design (since it is, and changing it will be changing spec/RFC), since it's a change in BCL method signature (byref -> ref readonly, and latter is equivalent of inref in F# compiler, it briefly worked in 8.0.100 because F# compiler had a bug treating ref readonly as byref, and worked in 7.0.xxx and earlier, because the signature was specifying byref there).

I am going to close it for the said reason, feel free to open language suggestion for having ref readonly/inref parameters to be "compatible" and "convertible" with ref cells.

@vzarytovskii vzarytovskii closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in F# Compiler and Tooling Jan 15, 2024
ForNeVeR added a commit to codingteam/emulsion that referenced this issue Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

3 participants