Add S.R.CS.Unsafe.AsRef<T>(ref readonly T) #24479

ektrah · 2017-10-06T17:22:49Z

Resolves #23916

jkotas · 2017-10-06T18:44:03Z

src/System.Runtime.CompilerServices.Unsafe/src/System.Runtime.CompilerServices.Unsafe.il

+
+#ifdef netcoreapp
+#else
+.class private auto ansi sealed beforefieldinit System.Runtime.CompilerServices.IsReadOnlyAttribute


@VSadov I see that Roslyn is marking the type with Microsoft.CodeAnalysis.EmbeddedAttribute. Is it ok to skip it here?

Yes, you don't need to mark as embedded (since this type was not embedded by the compiler).

In reply to: 143267591 [](ancestors = 143267591)

I think @OmarTawfik already added the IsReadOnlyAttribute type to coreCLR and coreRT repos. Can you reference it instead of defining it again? #Closed

This package targets older runtimes too. The local definition is needed for targeting of the older runtimes. ifdef netcoreapp avoids the local definition for .NET Core.

It is also added to desktop 4.7.1. Not sure if this infrastructure takes that into account:
https://apisof.net/catalog/System.Runtime.CompilerServices.IsReadOnlyAttribute

jkotas · 2017-10-06T18:44:45Z

src/System.Runtime.CompilerServices.Unsafe/tests/UnsafeTests.cs

+            int[] a = new int[] { 0x123, 0x234, 0x345, 0x456 };
+
+            ref int r = ref Unsafe.AsRef<int>(a[0]);
+            Assert.Equal(0x123, r);


It maybe also useful to try to assign to r and verify that the value shows up in a[0].

Isn't that only guaranteed if the in keyword is specified at the call-site?

The in syntax for readonly refs was removed (dotnet/roslyn#22182) because of it was redundant. I do not think it is coming back.

This was changed to be in rather than ref readonly approx 10 days after 22182: dotnet/roslyn#22387

‘in’ provides guarantees of aliasing. No modifier at callsite just makes the best effort. In simple cases there is no difference. Complex cases are not interesting here since we are not testing the compiler.

It would be interesting though to get a ref to something readonly - like a readonly field, assign a new value, and observe the changed value by directly reading the field (not via AsRef).

@ektrah Do you plan to make this addition to the test? E.g. something like:

r = 0x42; Assert.Equal(0x42, a[0]);

@jkotas Given that "No modifier at callsite just makes the best effort." it seems that such a test could randomly fail with the current compiler version. So I would keep the test as it is for now. I'm happy to provide an update later when a syntax to guarantee aliasing is available.

It cannot fail randomly. The compiler has to be deterministic.

The behavior is not random :). Lvalues of compatible type will not be passed via copy.

It would be a good practice to use “in” at call site for methods like this though - since accidentally getting in situation when copy is made would be undesirable.

OK, I'm convinced. I've added the above check to the test.

tannergooding · 2017-10-06T20:04:33Z

src/System.Runtime.CompilerServices.Unsafe/src/System.Runtime.CompilerServices.Unsafe.il

@@ -281,6 +281,20 @@
 #endif
  } // end of method Unsafe::AsRef

+  .method public hidebysig static !!T& AsRef<T>(!!T& source) cil managed aggressiveinlining


Isn't this supposed to also have a modreq: https://github.com/dotnet/csharplang/blob/ac995bd794be5a17acb52c30dfa4d61a1bfd15c9/proposals/csharp-7.2/readonly-ref.md#metadata-representation?

modregs are used on return ref readonly only.

Doesn't that make it impossible to have both ref and ref read-only overloads, since the would compile down to the same il signature? As far as I know, the latest decision was to allow both ovetloads and only require the in keyword if the decision was ambiguous.

In order to make this change work it has to match what the current compiler does. If there are plans to keep tweaking the implementation, this will need to be updated to match. It is common for compiler upgrade in corefx to require accompanying source changes.

Looking at the current bits/tests in master, a modreq is required: https://github.com/dotnet/roslyn/blob/1fd9a04c8c0d13afdd7af546fa7dce83549d6aba/src/Compilers/CSharp/Test/Emit/Emit/InAttributeModifierTests.cs#L205 (others as well, if you just search for modreq).

@VSadov or @jcouv could probably comment more.

I do not think modreq is required in this case. It is not a return or a parameter of something overridable.
Try compiling similar signature in c# just in case.

Compiling the signature in C# doesn't seem to generate a modreq; see #24479 (comment).

@VSadov, I would fully expect the modreq to be required, otherwise you cannot disambiguate (from the IL method signatures alone) whether it is ref or ref readonly (and therefore cannot overload between the two).

The C# signature, as it is currently defined, would be: public static ref T AsRef<T>([IsReadOnly] ref T source). Given that the attribute is not part of the IL signature, it is not-distinguishable from public static ref T AsRef<T>(ref T source) (both compile to an IL signature of .method public hidebysig static !!T& AsRef<T>(!!T& source) cil managed aggressiveinlining.

@VSadov, I checked the C# behavior in current master and decided to send a mail.

I think there are some clear issues with what we are currently doing and some not so clear (or just plain missing) documents on what the final expected behavior is.

From a basic standpoint:

Converting an existing method from ref to in should not be breaking (it currently is)

This only applies if you can't have both ref and in overloads (as it is currently)

The marshaller should not need to be updated in order to work with in parameters

The compiler should be emitting the [In] attribute, as it does the [Out] attribute for out parameters

@tannergooding I believe there is some confusion here. The modreq is not used to distinguish between in parameters and other ref kinds. The attribute IsReadOnly is used for that purpose (and emitted in all cases). The modreq is only used in cases where we want to prevent older compilers/other languages to compile implementations/overrides without understanding the semantics. Am I missing something?

jkotas · 2017-10-07T07:01:08Z

@dotnet-bot test this please

ektrah · 2017-10-08T14:00:50Z

For reference:

namespace System.Runtime.CompilerServices
{
    public static class Unsafe
    {
        public static ref T AsRef<T>(ref readonly T source)
        {
            throw null;
        }
    }
}

in a .NET Standard 2.0 library project is compiled by Microsoft.NETCore.Compilers 2.6.0-beta1-62126-01 to

// =============== CLASS MEMBERS DECLARATION ===================

.class private auto ansi sealed beforefieldinit Microsoft.CodeAnalysis.EmbeddedAttribute
       extends [netstandard]System.Attribute
{
  .custom instance void [netstandard]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 ) 
  .custom instance void Microsoft.CodeAnalysis.EmbeddedAttribute::.ctor() = ( 01 00 00 00 ) 
  .method public hidebysig specialname rtspecialname 
          instance void  .ctor() cil managed
  {
    // Code size       7 (0x7)
    .maxstack  8
    IL_0000:  ldarg.0
    IL_0001:  call       instance void [netstandard]System.Attribute::.ctor()
    IL_0006:  ret
  } // end of method EmbeddedAttribute::.ctor

} // end of class Microsoft.CodeAnalysis.EmbeddedAttribute

.class private auto ansi sealed beforefieldinit System.Runtime.CompilerServices.IsReadOnlyAttribute
       extends [netstandard]System.Attribute
{
  .custom instance void [netstandard]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 ) 
  .custom instance void Microsoft.CodeAnalysis.EmbeddedAttribute::.ctor() = ( 01 00 00 00 ) 
  .method public hidebysig specialname rtspecialname 
          instance void  .ctor() cil managed
  {
    // Code size       7 (0x7)
    .maxstack  8
    IL_0000:  ldarg.0
    IL_0001:  call       instance void [netstandard]System.Attribute::.ctor()
    IL_0006:  ret
  } // end of method IsReadOnlyAttribute::.ctor

} // end of class System.Runtime.CompilerServices.IsReadOnlyAttribute

.class public abstract auto ansi sealed beforefieldinit System.Runtime.CompilerServices.Unsafe
       extends [netstandard]System.Object
{
  .method public hidebysig static !!T&  AsRef<T>(!!T& source) cil managed
  {
    .param [1]
    .custom instance void System.Runtime.CompilerServices.IsReadOnlyAttribute::.ctor() = ( 01 00 00 00 ) 
    // Code size       2 (0x2)
    .maxstack  8
    IL_0000:  ldnull
    IL_0001:  throw
  } // end of method Unsafe::AsRef

} // end of class System.Runtime.CompilerServices.Unsafe


// =============================================================

jkotas

LGTM, modulo test nit.

jcouv · 2017-10-09T22:23:50Z

src/System.Runtime.CompilerServices.Unsafe/ref/System.Runtime.CompilerServices.Unsafe.cs

@@ -16,6 +16,7 @@ public static partial class Unsafe
        public static bool AreSame<T>(ref T left, ref T right) { throw null; }
        public unsafe static void* AsPointer<T>(ref T value) { throw null; }
        public unsafe static ref T AsRef<T>(void* source) { throw null; }
+        public static ref T AsRef<T>(ref readonly T source) { throw null; }


Note: when you next update your compiler toolset, this will have to be changed to use in instead of ref readonly. I don't see the LDM notes on this posted yet, but here's a summary.

VSadov

LGTM

jkotas · 2017-10-10T19:49:35Z

Thank you!

Commit migrated from dotnet/corefx@7223105

Add S.R.CS.Unsafe.AsRef<T>(readonly ref T)

7c795c3

dnfclas added the cla-already-signed label Oct 6, 2017

jkotas reviewed Oct 6, 2017

View reviewed changes

tannergooding reviewed Oct 6, 2017

View reviewed changes

ektrah changed the title ~~[WIP] Add S.R.CS.Unsafe.AsRef<T>(readonly ref T)~~ Add S.R.CS.Unsafe.AsRef<T>(ref readonly T) Oct 6, 2017

jkotas approved these changes Oct 9, 2017

View reviewed changes

jcouv reviewed Oct 9, 2017

View reviewed changes

Update test

bc843b7

VSadov approved these changes Oct 10, 2017

View reviewed changes

jkotas merged commit 7223105 into dotnet:master Oct 10, 2017

ektrah deleted the readonly-ref-asref branch October 10, 2017 19:53

karelz added this to the 2.1.0 milestone Oct 11, 2017

pjanotti pushed a commit to pjanotti/corefx that referenced this pull request Oct 31, 2017

Add S.R.CS.Unsafe.AsRef<T>(ref readonly T) (dotnet#24479)

fc8af50

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Add S.R.CS.Unsafe.AsRef<T>(ref readonly T) (dotnet/corefx#24479)

9bb0a5f

Commit migrated from dotnet/corefx@7223105

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add S.R.CS.Unsafe.AsRef<T>(ref readonly T) #24479

Add S.R.CS.Unsafe.AsRef<T>(ref readonly T) #24479

ektrah commented Oct 6, 2017 •

edited

Loading

jkotas Oct 6, 2017

jcouv Oct 9, 2017

jcouv Oct 9, 2017 •

edited

Loading

jkotas Oct 9, 2017

OmarTawfik Oct 9, 2017

jkotas Oct 6, 2017

ektrah Oct 6, 2017 •

edited

Loading

jkotas Oct 7, 2017

tannergooding Oct 7, 2017

VSadov Oct 8, 2017

jkotas Oct 9, 2017

ektrah Oct 9, 2017 •

edited

Loading

jkotas Oct 9, 2017

VSadov Oct 10, 2017

ektrah Oct 10, 2017

tannergooding Oct 6, 2017

jkotas Oct 7, 2017 •

edited

Loading

tannergooding Oct 7, 2017

jkotas Oct 7, 2017

tannergooding Oct 7, 2017

VSadov Oct 8, 2017

ektrah Oct 8, 2017

tannergooding Oct 8, 2017

tannergooding Oct 8, 2017

OmarTawfik Oct 9, 2017 •

edited

Loading

jkotas commented Oct 7, 2017

ektrah commented Oct 8, 2017 •

edited

Loading

jkotas left a comment

jcouv Oct 9, 2017

VSadov left a comment

jkotas commented Oct 10, 2017

Add S.R.CS.Unsafe.AsRef<T>(ref readonly T) #24479

Add S.R.CS.Unsafe.AsRef<T>(ref readonly T) #24479

Conversation

ektrah commented Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ektrah Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ektrah Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarTawfik Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

jkotas commented Oct 7, 2017

ektrah commented Oct 8, 2017 • edited Loading

jkotas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov left a comment

Choose a reason for hiding this comment

jkotas commented Oct 10, 2017

ektrah commented Oct 6, 2017 •

edited

Loading

jcouv Oct 9, 2017 •

edited

Loading

ektrah Oct 6, 2017 •

edited

Loading

ektrah Oct 9, 2017 •

edited

Loading

jkotas Oct 7, 2017 •

edited

Loading

OmarTawfik Oct 9, 2017 •

edited

Loading

ektrah commented Oct 8, 2017 •

edited

Loading