Utilize the new `ref readonly` language feature by tannergooding · Pull Request #89736 · dotnet/runtime

tannergooding · 2023-07-31T19:01:36Z

This resolves#85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

This is broken into 3 main commits:

in to ref readonly
ref to in
ref to ref readonly

As per the API review, none of these are binary breaking changes and none of these introduce new errors. However, users may see new warnings for two of the scenarios:

An API was changed from in to ref readonly, users will now be asked to use in explicitly
An API was changed from ref to in, users will now be asked to use in or no longer specify ref

ghost · 2023-07-31T19:01:43Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-07-31T19:02:22Z

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

Author:	tannergooding
Assignees:	tannergooding
Labels:	`area-System.Runtime.CompilerServices`, `new-api-needs-documentation`
Milestone:	-

tannergooding · 2023-07-31T19:14:04Z

 [NonVersionable]
 [MethodImpl(MethodImplOptions.AggressiveInlining)]
- public static ref T AsRef<T>(scoped in T source)
+ public static ref T AsRef<T>(scoped ref readonly T source)


For .NET 9, I think it may be worth a discussion in API review on whether we want more of Unsafe to take ref readonly.
The general consideration is that most APIs on Unsafe are "unsafe" and many, such as Add, do not actually mutate the contents of the ref but rather simply adjust the ref to point to a new address.
If we were to changes these APIs to be, for example, ref T Add(ref readonly T source, int elementOffset), then it basically has an implicit ref T AsRef<T>(ref readonly T source) built in.
The benefit of this is that it reduces "clutter" and can improve readability of such Unsafe code. Consider the simple case below:
- ref readonly T address = ref Unsafe.Add(ref Unsafe.AsRef(in source), (nint)elementOffset);+ ref readonly T address = ref Unsafe.Add(in source, (nint)elementOffset);
The downside is that any Unsafe operation can go from mutable to immutable ref and users could introduce a mutation by accident. This could be handled with an analyzer though, suggesting users preserve mutability/immutability when using Unsafe and to insert Unsafe.AsRef explicitly if it was intentional.
In an ideal world, we might have overloads that were ref T Add(ref T source, int offset) and ref readonly T Add(ref readonly T source, int offset). However, we can't due to them both being T& in IL. Alternatively we could request a language feature that basically says "I match the mutability of my input". Such a feature would also be beneficial for some ref returns. But an analyzer (potentially keyed off an attribute) seems like an acceptable solution to this problem instead and would be straightforward to implement

stephentoub · 2023-07-31T19:27:14Z

 uint address = PrivateAddress;
- MemoryMarshal.Write(destination, ref address);
+ MemoryMarshal.Write(destination, in address);


Should we change this to:
MemoryMarshal.Write(destination,PrivateAddress);
? Seems like the primary motivation behind changing the parameter from ref to in is so that it can use rvalues

We definitely can. I had just gone with the most straightforward set of changes to start since we're trying to land this in .NET 8

stephentoub · 2023-07-31T19:28:00Z

 {
 ulong true_val = BitConverter.IsLittleEndian ? 0x65007500720054ul : 0x54007200750065ul; // "True"
- MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), ref true_val);
+ MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), in true_val);


Same here and elsewhere

Meaning this?
MemoryMarshal.Write(MemoryMarshal.AsBytes(destination),BitConverter.IsLittleEndian?0x65007500720054ul:0x54007200750065ul);// "True"

jeffhandley

I approve of merging this in for RC1. It's a late-breaking language feature that we should adopt during the release, and the compiler produces compatible output.

Thanks for fielding this, @tannergooding. I've not reviewed the CI issues; please ensure those are known/tracked before merge.

tannergooding · 2023-08-02T14:10:22Z

CI issue is going to be unblocked by #89593, at which point this can rerun and then be mergeable.

Basically just need the roslyn toolset to be "properly" ingested to ensure source build remains happy.

mgravell · 2023-09-14T10:08:14Z

A bit late now, but a nasty side-effect of this is that it makes Unsafe.AsRef etc unusable from older lang-vers (and compilers);

with an up-to-date compiler but down-level lang-ver:

error CS8936: Feature 'ref readonly parameters' is not available in C# 10.0. Please use language version 12.0
or greater.

without an up-to-date compiler:

error CS1620: Argument 1 must be passed with the 'ref' keyword

(which of course it can't be; we're calling Unsafe.AsRef precisely because we don't have a ref)

At the time of writing, no devenv update is available on public preview that can use this feature, so: no Unsafe.AsRef for me until then :(

(I have 17.8.0 Preview 1.0 currently)

tannergooding · 2023-07-31T19:01:36Z

This resolves#85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

This is broken into 3 main commits:

in to ref readonly
ref to in
ref to ref readonly

As per the API review, none of these are binary breaking changes and none of these introduce new errors. However, users may see new warnings for two of the scenarios:

An API was changed from in to ref readonly, users will now be asked to use in explicitly
An API was changed from ref to in, users will now be asked to use in or no longer specify ref

ghost · 2023-07-31T19:01:43Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-07-31T19:02:22Z

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

Author:	tannergooding
Assignees:	tannergooding
Labels:	`area-System.Runtime.CompilerServices`, `new-api-needs-documentation`
Milestone:	-

tannergooding · 2023-07-31T19:14:04Z

 [NonVersionable]
 [MethodImpl(MethodImplOptions.AggressiveInlining)]
- public static ref T AsRef<T>(scoped in T source)
+ public static ref T AsRef<T>(scoped ref readonly T source)


For .NET 9, I think it may be worth a discussion in API review on whether we want more of Unsafe to take ref readonly.
The general consideration is that most APIs on Unsafe are "unsafe" and many, such as Add, do not actually mutate the contents of the ref but rather simply adjust the ref to point to a new address.
If we were to changes these APIs to be, for example, ref T Add(ref readonly T source, int elementOffset), then it basically has an implicit ref T AsRef<T>(ref readonly T source) built in.
The benefit of this is that it reduces "clutter" and can improve readability of such Unsafe code. Consider the simple case below:
- ref readonly T address = ref Unsafe.Add(ref Unsafe.AsRef(in source), (nint)elementOffset);+ ref readonly T address = ref Unsafe.Add(in source, (nint)elementOffset);
The downside is that any Unsafe operation can go from mutable to immutable ref and users could introduce a mutation by accident. This could be handled with an analyzer though, suggesting users preserve mutability/immutability when using Unsafe and to insert Unsafe.AsRef explicitly if it was intentional.
In an ideal world, we might have overloads that were ref T Add(ref T source, int offset) and ref readonly T Add(ref readonly T source, int offset). However, we can't due to them both being T& in IL. Alternatively we could request a language feature that basically says "I match the mutability of my input". Such a feature would also be beneficial for some ref returns. But an analyzer (potentially keyed off an attribute) seems like an acceptable solution to this problem instead and would be straightforward to implement

stephentoub · 2023-07-31T19:27:14Z

 uint address = PrivateAddress;
- MemoryMarshal.Write(destination, ref address);
+ MemoryMarshal.Write(destination, in address);


Should we change this to:
MemoryMarshal.Write(destination,PrivateAddress);
? Seems like the primary motivation behind changing the parameter from ref to in is so that it can use rvalues

We definitely can. I had just gone with the most straightforward set of changes to start since we're trying to land this in .NET 8

stephentoub · 2023-07-31T19:28:00Z

 {
 ulong true_val = BitConverter.IsLittleEndian ? 0x65007500720054ul : 0x54007200750065ul; // "True"
- MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), ref true_val);
+ MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), in true_val);


Same here and elsewhere

Meaning this?
MemoryMarshal.Write(MemoryMarshal.AsBytes(destination),BitConverter.IsLittleEndian?0x65007500720054ul:0x54007200750065ul);// "True"

jeffhandley

I approve of merging this in for RC1. It's a late-breaking language feature that we should adopt during the release, and the compiler produces compatible output.

Thanks for fielding this, @tannergooding. I've not reviewed the CI issues; please ensure those are known/tracked before merge.

tannergooding · 2023-08-02T14:10:22Z

CI issue is going to be unblocked by #89593, at which point this can rerun and then be mergeable.

Basically just need the roslyn toolset to be "properly" ingested to ensure source build remains happy.

mgravell · 2023-09-14T10:08:14Z

A bit late now, but a nasty side-effect of this is that it makes Unsafe.AsRef etc unusable from older lang-vers (and compilers);

with an up-to-date compiler but down-level lang-ver:

error CS8936: Feature 'ref readonly parameters' is not available in C# 10.0. Please use language version 12.0
or greater.

without an up-to-date compiler:

error CS1620: Argument 1 must be passed with the 'ref' keyword

(which of course it can't be; we're calling Unsafe.AsRef precisely because we don't have a ref)

At the time of writing, no devenv update is available on public preview that can use this feature, so: no Unsafe.AsRef for me until then :(

(I have 17.8.0 Preview 1.0 currently)

tannergooding · 2023-07-31T19:01:36Z

This resolves#85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

This is broken into 3 main commits:

in to ref readonly
ref to in
ref to ref readonly

As per the API review, none of these are binary breaking changes and none of these introduce new errors. However, users may see new warnings for two of the scenarios:

An API was changed from in to ref readonly, users will now be asked to use in explicitly
An API was changed from ref to in, users will now be asked to use in or no longer specify ref

ghost · 2023-07-31T19:01:43Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-07-31T19:02:22Z

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

Author:	tannergooding
Assignees:	tannergooding
Labels:	`area-System.Runtime.CompilerServices`, `new-api-needs-documentation`
Milestone:	-

tannergooding · 2023-07-31T19:14:04Z

 [NonVersionable]
 [MethodImpl(MethodImplOptions.AggressiveInlining)]
- public static ref T AsRef<T>(scoped in T source)
+ public static ref T AsRef<T>(scoped ref readonly T source)


For .NET 9, I think it may be worth a discussion in API review on whether we want more of Unsafe to take ref readonly.
The general consideration is that most APIs on Unsafe are "unsafe" and many, such as Add, do not actually mutate the contents of the ref but rather simply adjust the ref to point to a new address.
If we were to changes these APIs to be, for example, ref T Add(ref readonly T source, int elementOffset), then it basically has an implicit ref T AsRef<T>(ref readonly T source) built in.
The benefit of this is that it reduces "clutter" and can improve readability of such Unsafe code. Consider the simple case below:
- ref readonly T address = ref Unsafe.Add(ref Unsafe.AsRef(in source), (nint)elementOffset);+ ref readonly T address = ref Unsafe.Add(in source, (nint)elementOffset);
The downside is that any Unsafe operation can go from mutable to immutable ref and users could introduce a mutation by accident. This could be handled with an analyzer though, suggesting users preserve mutability/immutability when using Unsafe and to insert Unsafe.AsRef explicitly if it was intentional.
In an ideal world, we might have overloads that were ref T Add(ref T source, int offset) and ref readonly T Add(ref readonly T source, int offset). However, we can't due to them both being T& in IL. Alternatively we could request a language feature that basically says "I match the mutability of my input". Such a feature would also be beneficial for some ref returns. But an analyzer (potentially keyed off an attribute) seems like an acceptable solution to this problem instead and would be straightforward to implement

stephentoub · 2023-07-31T19:27:14Z

 uint address = PrivateAddress;
- MemoryMarshal.Write(destination, ref address);
+ MemoryMarshal.Write(destination, in address);


Should we change this to:
MemoryMarshal.Write(destination,PrivateAddress);
? Seems like the primary motivation behind changing the parameter from ref to in is so that it can use rvalues

We definitely can. I had just gone with the most straightforward set of changes to start since we're trying to land this in .NET 8

stephentoub · 2023-07-31T19:28:00Z

 {
 ulong true_val = BitConverter.IsLittleEndian ? 0x65007500720054ul : 0x54007200750065ul; // "True"
- MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), ref true_val);
+ MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), in true_val);


Same here and elsewhere

Meaning this?
MemoryMarshal.Write(MemoryMarshal.AsBytes(destination),BitConverter.IsLittleEndian?0x65007500720054ul:0x54007200750065ul);// "True"

jeffhandley

I approve of merging this in for RC1. It's a late-breaking language feature that we should adopt during the release, and the compiler produces compatible output.

Thanks for fielding this, @tannergooding. I've not reviewed the CI issues; please ensure those are known/tracked before merge.

tannergooding · 2023-08-02T14:10:22Z

CI issue is going to be unblocked by #89593, at which point this can rerun and then be mergeable.

Basically just need the roslyn toolset to be "properly" ingested to ensure source build remains happy.

mgravell · 2023-09-14T10:08:14Z

A bit late now, but a nasty side-effect of this is that it makes Unsafe.AsRef etc unusable from older lang-vers (and compilers);

with an up-to-date compiler but down-level lang-ver:

error CS8936: Feature 'ref readonly parameters' is not available in C# 10.0. Please use language version 12.0
or greater.

without an up-to-date compiler:

error CS1620: Argument 1 must be passed with the 'ref' keyword

(which of course it can't be; we're calling Unsafe.AsRef precisely because we don't have a ref)

At the time of writing, no devenv update is available on public preview that can use this feature, so: no Unsafe.AsRef for me until then :(

(I have 17.8.0 Preview 1.0 currently)

tannergooding · 2023-07-31T19:01:36Z

This resolves#85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

This is broken into 3 main commits:

in to ref readonly
ref to in
ref to ref readonly

As per the API review, none of these are binary breaking changes and none of these introduce new errors. However, users may see new warnings for two of the scenarios:

An API was changed from in to ref readonly, users will now be asked to use in explicitly
An API was changed from ref to in, users will now be asked to use in or no longer specify ref

ghost · 2023-07-31T19:01:43Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-07-31T19:02:22Z

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

Author:	tannergooding
Assignees:	tannergooding
Labels:	`area-System.Runtime.CompilerServices`, `new-api-needs-documentation`
Milestone:	-

tannergooding · 2023-07-31T19:14:04Z

 [NonVersionable]
 [MethodImpl(MethodImplOptions.AggressiveInlining)]
- public static ref T AsRef<T>(scoped in T source)
+ public static ref T AsRef<T>(scoped ref readonly T source)


For .NET 9, I think it may be worth a discussion in API review on whether we want more of Unsafe to take ref readonly.
The general consideration is that most APIs on Unsafe are "unsafe" and many, such as Add, do not actually mutate the contents of the ref but rather simply adjust the ref to point to a new address.
If we were to changes these APIs to be, for example, ref T Add(ref readonly T source, int elementOffset), then it basically has an implicit ref T AsRef<T>(ref readonly T source) built in.
The benefit of this is that it reduces "clutter" and can improve readability of such Unsafe code. Consider the simple case below:
- ref readonly T address = ref Unsafe.Add(ref Unsafe.AsRef(in source), (nint)elementOffset);+ ref readonly T address = ref Unsafe.Add(in source, (nint)elementOffset);
The downside is that any Unsafe operation can go from mutable to immutable ref and users could introduce a mutation by accident. This could be handled with an analyzer though, suggesting users preserve mutability/immutability when using Unsafe and to insert Unsafe.AsRef explicitly if it was intentional.
In an ideal world, we might have overloads that were ref T Add(ref T source, int offset) and ref readonly T Add(ref readonly T source, int offset). However, we can't due to them both being T& in IL. Alternatively we could request a language feature that basically says "I match the mutability of my input". Such a feature would also be beneficial for some ref returns. But an analyzer (potentially keyed off an attribute) seems like an acceptable solution to this problem instead and would be straightforward to implement

stephentoub · 2023-07-31T19:27:14Z

 uint address = PrivateAddress;
- MemoryMarshal.Write(destination, ref address);
+ MemoryMarshal.Write(destination, in address);


Should we change this to:
MemoryMarshal.Write(destination,PrivateAddress);
? Seems like the primary motivation behind changing the parameter from ref to in is so that it can use rvalues

We definitely can. I had just gone with the most straightforward set of changes to start since we're trying to land this in .NET 8

stephentoub · 2023-07-31T19:28:00Z

 {
 ulong true_val = BitConverter.IsLittleEndian ? 0x65007500720054ul : 0x54007200750065ul; // "True"
- MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), ref true_val);
+ MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), in true_val);


Same here and elsewhere

Meaning this?
MemoryMarshal.Write(MemoryMarshal.AsBytes(destination),BitConverter.IsLittleEndian?0x65007500720054ul:0x54007200750065ul);// "True"

jeffhandley

I approve of merging this in for RC1. It's a late-breaking language feature that we should adopt during the release, and the compiler produces compatible output.

Thanks for fielding this, @tannergooding. I've not reviewed the CI issues; please ensure those are known/tracked before merge.

tannergooding · 2023-08-02T14:10:22Z

CI issue is going to be unblocked by #89593, at which point this can rerun and then be mergeable.

Basically just need the roslyn toolset to be "properly" ingested to ensure source build remains happy.

mgravell · 2023-09-14T10:08:14Z

A bit late now, but a nasty side-effect of this is that it makes Unsafe.AsRef etc unusable from older lang-vers (and compilers);

with an up-to-date compiler but down-level lang-ver:

error CS8936: Feature 'ref readonly parameters' is not available in C# 10.0. Please use language version 12.0
or greater.

without an up-to-date compiler:

error CS1620: Argument 1 must be passed with the 'ref' keyword

(which of course it can't be; we're calling Unsafe.AsRef precisely because we don't have a ref)

At the time of writing, no devenv update is available on public preview that can use this feature, so: no Unsafe.AsRef for me until then :(

(I have 17.8.0 Preview 1.0 currently)

tannergooding · 2023-07-31T19:01:36Z

This resolves#85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

This is broken into 3 main commits:

in to ref readonly
ref to in
ref to ref readonly

As per the API review, none of these are binary breaking changes and none of these introduce new errors. However, users may see new warnings for two of the scenarios:

An API was changed from in to ref readonly, users will now be asked to use in explicitly
An API was changed from ref to in, users will now be asked to use in or no longer specify ref

ghost · 2023-07-31T19:01:43Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-07-31T19:02:22Z

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

Author:	tannergooding
Assignees:	tannergooding
Labels:	`area-System.Runtime.CompilerServices`, `new-api-needs-documentation`
Milestone:	-

tannergooding · 2023-07-31T19:14:04Z

 [NonVersionable]
 [MethodImpl(MethodImplOptions.AggressiveInlining)]
- public static ref T AsRef<T>(scoped in T source)
+ public static ref T AsRef<T>(scoped ref readonly T source)


For .NET 9, I think it may be worth a discussion in API review on whether we want more of Unsafe to take ref readonly.
The general consideration is that most APIs on Unsafe are "unsafe" and many, such as Add, do not actually mutate the contents of the ref but rather simply adjust the ref to point to a new address.
If we were to changes these APIs to be, for example, ref T Add(ref readonly T source, int elementOffset), then it basically has an implicit ref T AsRef<T>(ref readonly T source) built in.
The benefit of this is that it reduces "clutter" and can improve readability of such Unsafe code. Consider the simple case below:
- ref readonly T address = ref Unsafe.Add(ref Unsafe.AsRef(in source), (nint)elementOffset);+ ref readonly T address = ref Unsafe.Add(in source, (nint)elementOffset);
The downside is that any Unsafe operation can go from mutable to immutable ref and users could introduce a mutation by accident. This could be handled with an analyzer though, suggesting users preserve mutability/immutability when using Unsafe and to insert Unsafe.AsRef explicitly if it was intentional.
In an ideal world, we might have overloads that were ref T Add(ref T source, int offset) and ref readonly T Add(ref readonly T source, int offset). However, we can't due to them both being T& in IL. Alternatively we could request a language feature that basically says "I match the mutability of my input". Such a feature would also be beneficial for some ref returns. But an analyzer (potentially keyed off an attribute) seems like an acceptable solution to this problem instead and would be straightforward to implement

stephentoub · 2023-07-31T19:27:14Z

 uint address = PrivateAddress;
- MemoryMarshal.Write(destination, ref address);
+ MemoryMarshal.Write(destination, in address);


Should we change this to:
MemoryMarshal.Write(destination,PrivateAddress);
? Seems like the primary motivation behind changing the parameter from ref to in is so that it can use rvalues

We definitely can. I had just gone with the most straightforward set of changes to start since we're trying to land this in .NET 8

stephentoub · 2023-07-31T19:28:00Z

 {
 ulong true_val = BitConverter.IsLittleEndian ? 0x65007500720054ul : 0x54007200750065ul; // "True"
- MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), ref true_val);
+ MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), in true_val);


Same here and elsewhere

Meaning this?
MemoryMarshal.Write(MemoryMarshal.AsBytes(destination),BitConverter.IsLittleEndian?0x65007500720054ul:0x54007200750065ul);// "True"

jeffhandley

I approve of merging this in for RC1. It's a late-breaking language feature that we should adopt during the release, and the compiler produces compatible output.

Thanks for fielding this, @tannergooding. I've not reviewed the CI issues; please ensure those are known/tracked before merge.

tannergooding · 2023-08-02T14:10:22Z

CI issue is going to be unblocked by #89593, at which point this can rerun and then be mergeable.

Basically just need the roslyn toolset to be "properly" ingested to ensure source build remains happy.

mgravell · 2023-09-14T10:08:14Z

A bit late now, but a nasty side-effect of this is that it makes Unsafe.AsRef etc unusable from older lang-vers (and compilers);

with an up-to-date compiler but down-level lang-ver:

error CS8936: Feature 'ref readonly parameters' is not available in C# 10.0. Please use language version 12.0
or greater.

without an up-to-date compiler:

error CS1620: Argument 1 must be passed with the 'ref' keyword

(which of course it can't be; we're calling Unsafe.AsRef precisely because we don't have a ref)

At the time of writing, no devenv update is available on public preview that can use this feature, so: no Unsafe.AsRef for me until then :(

(I have 17.8.0 Preview 1.0 currently)

tannergooding · 2023-07-31T19:01:36Z

This resolves#85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

This is broken into 3 main commits:

in to ref readonly
ref to in
ref to ref readonly

As per the API review, none of these are binary breaking changes and none of these introduce new errors. However, users may see new warnings for two of the scenarios:

An API was changed from in to ref readonly, users will now be asked to use in explicitly
An API was changed from ref to in, users will now be asked to use in or no longer specify ref

ghost · 2023-07-31T19:01:43Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-07-31T19:02:22Z

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

Author:	tannergooding
Assignees:	tannergooding
Labels:	`area-System.Runtime.CompilerServices`, `new-api-needs-documentation`
Milestone:	-

tannergooding · 2023-07-31T19:14:04Z

 [NonVersionable]
 [MethodImpl(MethodImplOptions.AggressiveInlining)]
- public static ref T AsRef<T>(scoped in T source)
+ public static ref T AsRef<T>(scoped ref readonly T source)


For .NET 9, I think it may be worth a discussion in API review on whether we want more of Unsafe to take ref readonly.
The general consideration is that most APIs on Unsafe are "unsafe" and many, such as Add, do not actually mutate the contents of the ref but rather simply adjust the ref to point to a new address.
If we were to changes these APIs to be, for example, ref T Add(ref readonly T source, int elementOffset), then it basically has an implicit ref T AsRef<T>(ref readonly T source) built in.
The benefit of this is that it reduces "clutter" and can improve readability of such Unsafe code. Consider the simple case below:
- ref readonly T address = ref Unsafe.Add(ref Unsafe.AsRef(in source), (nint)elementOffset);+ ref readonly T address = ref Unsafe.Add(in source, (nint)elementOffset);
The downside is that any Unsafe operation can go from mutable to immutable ref and users could introduce a mutation by accident. This could be handled with an analyzer though, suggesting users preserve mutability/immutability when using Unsafe and to insert Unsafe.AsRef explicitly if it was intentional.
In an ideal world, we might have overloads that were ref T Add(ref T source, int offset) and ref readonly T Add(ref readonly T source, int offset). However, we can't due to them both being T& in IL. Alternatively we could request a language feature that basically says "I match the mutability of my input". Such a feature would also be beneficial for some ref returns. But an analyzer (potentially keyed off an attribute) seems like an acceptable solution to this problem instead and would be straightforward to implement

stephentoub · 2023-07-31T19:27:14Z

 uint address = PrivateAddress;
- MemoryMarshal.Write(destination, ref address);
+ MemoryMarshal.Write(destination, in address);


Should we change this to:
MemoryMarshal.Write(destination,PrivateAddress);
? Seems like the primary motivation behind changing the parameter from ref to in is so that it can use rvalues

We definitely can. I had just gone with the most straightforward set of changes to start since we're trying to land this in .NET 8

stephentoub · 2023-07-31T19:28:00Z

 {
 ulong true_val = BitConverter.IsLittleEndian ? 0x65007500720054ul : 0x54007200750065ul; // "True"
- MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), ref true_val);
+ MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), in true_val);


Same here and elsewhere

Meaning this?
MemoryMarshal.Write(MemoryMarshal.AsBytes(destination),BitConverter.IsLittleEndian?0x65007500720054ul:0x54007200750065ul);// "True"

jeffhandley

I approve of merging this in for RC1. It's a late-breaking language feature that we should adopt during the release, and the compiler produces compatible output.

Thanks for fielding this, @tannergooding. I've not reviewed the CI issues; please ensure those are known/tracked before merge.

tannergooding · 2023-08-02T14:10:22Z

CI issue is going to be unblocked by #89593, at which point this can rerun and then be mergeable.

Basically just need the roslyn toolset to be "properly" ingested to ensure source build remains happy.

mgravell · 2023-09-14T10:08:14Z

A bit late now, but a nasty side-effect of this is that it makes Unsafe.AsRef etc unusable from older lang-vers (and compilers);

with an up-to-date compiler but down-level lang-ver:

error CS8936: Feature 'ref readonly parameters' is not available in C# 10.0. Please use language version 12.0
or greater.

without an up-to-date compiler:

error CS1620: Argument 1 must be passed with the 'ref' keyword

(which of course it can't be; we're calling Unsafe.AsRef precisely because we don't have a ref)

At the time of writing, no devenv update is available on public preview that can use this feature, so: no Unsafe.AsRef for me until then :(

(I have 17.8.0 Preview 1.0 currently)

tannergooding · 2023-07-31T19:01:36Z

This resolves#85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

This is broken into 3 main commits:

in to ref readonly
ref to in
ref to ref readonly

As per the API review, none of these are binary breaking changes and none of these introduce new errors. However, users may see new warnings for two of the scenarios:

An API was changed from in to ref readonly, users will now be asked to use in explicitly
An API was changed from ref to in, users will now be asked to use in or no longer specify ref

ghost · 2023-07-31T19:01:43Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-07-31T19:02:22Z

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

Author:	tannergooding
Assignees:	tannergooding
Labels:	`area-System.Runtime.CompilerServices`, `new-api-needs-documentation`
Milestone:	-

tannergooding · 2023-07-31T19:14:04Z

 [NonVersionable]
 [MethodImpl(MethodImplOptions.AggressiveInlining)]
- public static ref T AsRef<T>(scoped in T source)
+ public static ref T AsRef<T>(scoped ref readonly T source)


For .NET 9, I think it may be worth a discussion in API review on whether we want more of Unsafe to take ref readonly.
The general consideration is that most APIs on Unsafe are "unsafe" and many, such as Add, do not actually mutate the contents of the ref but rather simply adjust the ref to point to a new address.
If we were to changes these APIs to be, for example, ref T Add(ref readonly T source, int elementOffset), then it basically has an implicit ref T AsRef<T>(ref readonly T source) built in.
The benefit of this is that it reduces "clutter" and can improve readability of such Unsafe code. Consider the simple case below:
- ref readonly T address = ref Unsafe.Add(ref Unsafe.AsRef(in source), (nint)elementOffset);+ ref readonly T address = ref Unsafe.Add(in source, (nint)elementOffset);
The downside is that any Unsafe operation can go from mutable to immutable ref and users could introduce a mutation by accident. This could be handled with an analyzer though, suggesting users preserve mutability/immutability when using Unsafe and to insert Unsafe.AsRef explicitly if it was intentional.
In an ideal world, we might have overloads that were ref T Add(ref T source, int offset) and ref readonly T Add(ref readonly T source, int offset). However, we can't due to them both being T& in IL. Alternatively we could request a language feature that basically says "I match the mutability of my input". Such a feature would also be beneficial for some ref returns. But an analyzer (potentially keyed off an attribute) seems like an acceptable solution to this problem instead and would be straightforward to implement

stephentoub · 2023-07-31T19:27:14Z

 uint address = PrivateAddress;
- MemoryMarshal.Write(destination, ref address);
+ MemoryMarshal.Write(destination, in address);


Should we change this to:
MemoryMarshal.Write(destination,PrivateAddress);
? Seems like the primary motivation behind changing the parameter from ref to in is so that it can use rvalues

We definitely can. I had just gone with the most straightforward set of changes to start since we're trying to land this in .NET 8

stephentoub · 2023-07-31T19:28:00Z

 {
 ulong true_val = BitConverter.IsLittleEndian ? 0x65007500720054ul : 0x54007200750065ul; // "True"
- MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), ref true_val);
+ MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), in true_val);


Same here and elsewhere

Meaning this?
MemoryMarshal.Write(MemoryMarshal.AsBytes(destination),BitConverter.IsLittleEndian?0x65007500720054ul:0x54007200750065ul);// "True"

jeffhandley

I approve of merging this in for RC1. It's a late-breaking language feature that we should adopt during the release, and the compiler produces compatible output.

Thanks for fielding this, @tannergooding. I've not reviewed the CI issues; please ensure those are known/tracked before merge.

tannergooding · 2023-08-02T14:10:22Z

CI issue is going to be unblocked by #89593, at which point this can rerun and then be mergeable.

Basically just need the roslyn toolset to be "properly" ingested to ensure source build remains happy.

mgravell · 2023-09-14T10:08:14Z

A bit late now, but a nasty side-effect of this is that it makes Unsafe.AsRef etc unusable from older lang-vers (and compilers);

with an up-to-date compiler but down-level lang-ver:

error CS8936: Feature 'ref readonly parameters' is not available in C# 10.0. Please use language version 12.0
or greater.

without an up-to-date compiler:

error CS1620: Argument 1 must be passed with the 'ref' keyword

(which of course it can't be; we're calling Unsafe.AsRef precisely because we don't have a ref)

At the time of writing, no devenv update is available on public preview that can use this feature, so: no Unsafe.AsRef for me until then :(

(I have 17.8.0 Preview 1.0 currently)

tannergooding · 2023-07-31T19:01:36Z

This resolves#85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

This is broken into 3 main commits:

in to ref readonly
ref to in
ref to ref readonly

As per the API review, none of these are binary breaking changes and none of these introduce new errors. However, users may see new warnings for two of the scenarios:

An API was changed from in to ref readonly, users will now be asked to use in explicitly
An API was changed from ref to in, users will now be asked to use in or no longer specify ref

ghost · 2023-07-31T19:01:43Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-07-31T19:02:22Z

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #85911. The dotnet/sdk currently has the same version of Roslyn inserted (v4.8.0-1.23378.8).

Author:	tannergooding
Assignees:	tannergooding
Labels:	`area-System.Runtime.CompilerServices`, `new-api-needs-documentation`
Milestone:	-

tannergooding · 2023-07-31T19:14:04Z

 [NonVersionable]
 [MethodImpl(MethodImplOptions.AggressiveInlining)]
- public static ref T AsRef<T>(scoped in T source)
+ public static ref T AsRef<T>(scoped ref readonly T source)


For .NET 9, I think it may be worth a discussion in API review on whether we want more of Unsafe to take ref readonly.
The general consideration is that most APIs on Unsafe are "unsafe" and many, such as Add, do not actually mutate the contents of the ref but rather simply adjust the ref to point to a new address.
If we were to changes these APIs to be, for example, ref T Add(ref readonly T source, int elementOffset), then it basically has an implicit ref T AsRef<T>(ref readonly T source) built in.
The benefit of this is that it reduces "clutter" and can improve readability of such Unsafe code. Consider the simple case below:
- ref readonly T address = ref Unsafe.Add(ref Unsafe.AsRef(in source), (nint)elementOffset);+ ref readonly T address = ref Unsafe.Add(in source, (nint)elementOffset);
The downside is that any Unsafe operation can go from mutable to immutable ref and users could introduce a mutation by accident. This could be handled with an analyzer though, suggesting users preserve mutability/immutability when using Unsafe and to insert Unsafe.AsRef explicitly if it was intentional.
In an ideal world, we might have overloads that were ref T Add(ref T source, int offset) and ref readonly T Add(ref readonly T source, int offset). However, we can't due to them both being T& in IL. Alternatively we could request a language feature that basically says "I match the mutability of my input". Such a feature would also be beneficial for some ref returns. But an analyzer (potentially keyed off an attribute) seems like an acceptable solution to this problem instead and would be straightforward to implement

stephentoub · 2023-07-31T19:27:14Z

 uint address = PrivateAddress;
- MemoryMarshal.Write(destination, ref address);
+ MemoryMarshal.Write(destination, in address);


Should we change this to:
MemoryMarshal.Write(destination,PrivateAddress);
? Seems like the primary motivation behind changing the parameter from ref to in is so that it can use rvalues

We definitely can. I had just gone with the most straightforward set of changes to start since we're trying to land this in .NET 8

stephentoub · 2023-07-31T19:28:00Z

 {
 ulong true_val = BitConverter.IsLittleEndian ? 0x65007500720054ul : 0x54007200750065ul; // "True"
- MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), ref true_val);
+ MemoryMarshal.Write(MemoryMarshal.AsBytes(destination), in true_val);


Same here and elsewhere

Meaning this?
MemoryMarshal.Write(MemoryMarshal.AsBytes(destination),BitConverter.IsLittleEndian?0x65007500720054ul:0x54007200750065ul);// "True"

jeffhandley

I approve of merging this in for RC1. It's a late-breaking language feature that we should adopt during the release, and the compiler produces compatible output.

Thanks for fielding this, @tannergooding. I've not reviewed the CI issues; please ensure those are known/tracked before merge.

tannergooding · 2023-08-02T14:10:22Z

CI issue is going to be unblocked by #89593, at which point this can rerun and then be mergeable.

Basically just need the roslyn toolset to be "properly" ingested to ensure source build remains happy.

mgravell · 2023-09-14T10:08:14Z

A bit late now, but a nasty side-effect of this is that it makes Unsafe.AsRef etc unusable from older lang-vers (and compilers);

with an up-to-date compiler but down-level lang-ver:

error CS8936: Feature 'ref readonly parameters' is not available in C# 10.0. Please use language version 12.0
or greater.

without an up-to-date compiler:

error CS1620: Argument 1 must be passed with the 'ref' keyword

(which of course it can't be; we're calling Unsafe.AsRef precisely because we don't have a ref)

At the time of writing, no devenv update is available on public preview that can use this feature, so: no Unsafe.AsRef for me until then :(

(I have 17.8.0 Preview 1.0 currently)

tannergooding added 4 commits July 31, 2023 10:41

Updating the Roslyn toolset compiler to v4.8.0-1.23378.8
077061f

Changing signatures from in to ref readonly
4a254d0

Changing signatures from ref to in
5f9222b

Changing signatures from ref to ref readonly
e23f090

tannergooding requested a review from MichalStrehovsky as a code owner July 31, 2023 19:01

ghost added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Jul 31, 2023

ghost assigned tannergooding Jul 31, 2023

tannergooding added area-System.Runtime.CompilerServices and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 31, 2023

tannergooding commented Jul 31, 2023
View reviewed changes

Fix case missed for Unix build
8b7f7db

stephentoub reviewed Jul 31, 2023
View reviewed changes

stephentoub approved these changes Jul 31, 2023
View reviewed changes

tannergooding force-pushed the fix-85911 branch from 6965d16 to d326f8eCompare July 31, 2023 22:35

Ensure an xml comment works for .NET or .NET Framework
b5435fd

tannergooding force-pushed the fix-85911 branch 3 times, most recently from ece204e to fe0dbafCompare August 1, 2023 15:16

Ensure tests properly handle ref readonly changes
6a0761a

tannergooding force-pushed the fix-85911 branch from fe0dbaf to 6a0761aCompare August 1, 2023 16:14

jeffhandley approved these changes Aug 2, 2023
View reviewed changes

Merge remote-tracking branch 'dotnet/main' into fix-85911
08823b9

build-analysisBot mentioned this pull request Aug 2, 2023
Assert in System.Net.Sockets.SocketAsyncEventArgs.CompleteAcceptOperation #89806
Closed

tannergooding merged commit ecc8ba2 into dotnet:mainAug 2, 2023

tannergooding deleted the fix-85911 branch August 2, 2023 20:51

MichaelSimons mentioned this pull request Aug 7, 2023
[main] Update dependencies from dotnet/sdk dotnet/installer#17118
Merged

EgorBo mentioned this pull request Aug 10, 2023
[Perf] Windows/arm64: 1 Improvement on 8/3/2023 2:32:33 AM dotnet/perf-autofiling-issues#20494
Closed

lewing mentioned this pull request Aug 16, 2023
[main] Update dependencies from dotnet/runtime dotnet/efcore#31421
Merged

ghost locked as resolved and limited conversation to collaborators Sep 2, 2023