runtime: Use of `ref readonly` instead of `in` in libraries is a breaking change for F# callers

Description

With the switch from in to ref readonly in Unsafe.AsRef (#89736), since as I understand it F# doesn’t support ref readonly, it becomes impossible to take an unsafe mutable reference to an immutable record field.

Reproduction Steps

type MyRecord =
    { Value : int }

    member this.SetValue(v: int) =
        (Unsafe.AsRef<int> &this.Value) <- v

Expected behavior

The code compiles successfully, and calling SetValue() mutates the field.

Actual behavior

The code fails to compile:

Error FS0041 : No overloads match for method 'AsRef'.

Known type of argument: inref<'T>

Available overloads:
 - Unsafe.AsRef<'T>(source: byref<'T>) : byref<'T> // Argument 'source' doesn't match
 - Unsafe.AsRef<'T>(source: voidptr) : byref<'T> // Argument 'source' doesn't match

Regression?

This is a regression from .NET 7.

Known Workarounds

No response

Configuration

Using .NET 8 RC2 and the associated F# compiler.

Other information

No response

About this issue

Original URL
State: closed
Created 8 months ago
Reactions: 7
Comments: 30 (22 by maintainers)

Most upvoted comments

Yes, I considered posting to F# too, and I probably will. I do find it odd though that only C# was taken into consideration to determine whether this would be a breaking change.

+16

Tarmil on Nov 2, 2023

This looks like it needs to be reverted. Thanks for reporting this.

We can fix it, however it will take much more time than revert.

Thanks @vzarytovskii. Having F# fix it seems like general goodness, but this is likely blocking people from adopting .NET 8, which is a red flag in general. If someone wants to try this API change again in .NET 9, that is fine but for now this shouldn’t have been changed. The PR that did this has had other unfortunage side effects that caused other reverts. Unless there is push back from @jkotas or @stephentoub I am going to revert the change.

+12

AaronRobinsonMSFT on Nov 2, 2023

F# very much is considered with regards to breaking changes, and we do our best to avoid breaking any language.

But, avoiding all breaks is effectively impossible as simply exposing a new API can break user code. It can change overload resolution in C#, it can change type inference in F#, it can cause an extension API to no longer be invoked, it can cause an ambiguity with certain types of edge cases, and so on.

We ultimately try to move the core .NET Libraries forward in a direction that is a positive for the ecosystem as a whole, and that includes changes like https://github.com/dotnet/runtime/pull/89736 which are designed to improve safety, remove reliance on unsafe code, and help users avoid pits of failure.

We will need to do better about testing such changes in the future to ensure that the two edge cases caught don’t happen again.

The first edge was that it caused issues in the dotnet/sdk because the SDK hadn’t quite updated to the newer C# compiler version yet and this negatively impacted source build. Due to the general timeframe difference between the runtime and the source build ingesting new versions of Roslyn, this means we need to be mindful of any new C# features used in the .NET Libraries. This was a relatively new thing and not something that we had to be mindful of in previous years
The second edge was this case in F#, which we believe was tested but apparently wasn’t tested sufficiently. It has negatively impacted the ability to unsafely convert an inref to a byref, which was already a dangerous operation. There are then 2 other cases that moved from in to ref readonly that would also be impacted. The first is new ReadOnlySpan(ref readonly T) and the other Nullable.GetValueRefOrDefaultRef(ref readonly T?). The change from ref to ref readonly would be invisible to F# users (it currently sees it as byref still, and will see it as inref after the language support is added), as will the change from ref to in (since F# allows implicit byref -> inref)

+10

tannergooding on Nov 3, 2023

If F# doesn’t support ref readonly, wouldn’t just every single API using it be broken there, unless the language is updated?

In other words: isn’t this an issue that should be in the F# repo? The API is working as intended.

Sergio0694 on Nov 2, 2023

No, it’s explicitly not a breaking change in C# 12, as we introduced compatibility rules for in/ref/ref readonly for this.

The goal was explicitly to allow “fixing” the signatures of existing APIs such as this, without it being a breaking change. Which is the case, at least for C# only. You can read the spec here.

Sergio0694 on Nov 2, 2023

Right, I think it may be too late to revert this change. It is a discussion for shiproom.

#89736 was rushed in with a weak justification at the very last minute that I was not happy with (we had internal teams discussion about that). The change caused number of issues, and it is not surprising that we are discovering more issues with it. Something to learn from for next time.

cc @jeffhandley @artl93

jkotas on Nov 2, 2023

@charlesroddie I think you’re focusing too much on the fact that I used a record field in my example. The same issue happens no matter what is behind the inref.

Tarmil on Nov 3, 2023

F# would like to define the pattern as impossible.

All bets are off with Unsafe, System.Reflection or Unchecked

Szer on Nov 3, 2023

Unless there is push back from @jkotas or @stephentoub I am going to revert the change.

How do you propose to do that?. NET 8 is done, and changing it in the reverse direction is a breaking change.

stephentoub on Nov 2, 2023

It should be part of the 8.0.101 servicing release that’ll drop next month.

baronfel on Nov 13, 2023

This looks like it needs to be reverted. Thanks for reporting this.

AaronRobinsonMSFT on Nov 2, 2023

No, it’s explicitly not a breaking change in C# 12, as we introduced compatibility rules for in/ref/ref readonly for this.

BCL is not “for C#” though, but “for .NET” (written in C# yes)

Therefore if changes in BCL are breaking for .NET they are the breaking change by definition

Szer on Nov 2, 2023