Readonly members metadata

docs/features/readonly-instance-members.md

@@ -0,0 +1,181 @@
+# Readonly Instance Members
+
+Championed Issue: <https://github.com/dotnet/csharplang/issues/1710>
+
+## Summary
+[summary]: #summary
+
+Provide a way to specify individual instance members on a struct do not modify state, in the same way that `readonly struct` specifies no instance members modify state.
+
+It is worth noting that `readonly instance member` != `pure instance member`. A `pure` instance member guarantees no state will be modified. A `readonly` instance member only guarantees that instance state will not be modified.
+
+All instance members on a `readonly struct` are implicitly `readonly instance members`. Explicit `readonly instance members` declared on non-readonly structs would behave in the same manner. For example, they would still create hidden copies if you called an instance member (on the current instance or on a field of the instance) which was itself not-readonly.
+
+## Design
+[design]: #design
+
+Allow a user to specify that an instance member is, itself, `readonly` and does not modify the state of the instance (with all the appropriate verification done by the compiler, of course). For example:
+
+```csharp
+public struct Vector2
+{
+    public float x;
+    public float y;
+
+    public readonly float GetLengthReadonly()
+    {
+        return MathF.Sqrt(LengthSquared);
+    }
+
+    public float GetLength()
+    {
+        return MathF.Sqrt(LengthSquared);
+    }
+
+    public readonly float GetLengthIllegal()
+    {
+        var tmp = MathF.Sqrt(LengthSquared);
+
+        x = tmp;    // Compiler error, cannot write x
+        y = tmp;    // Compiler error, cannot write y
+
+        return tmp;
+    }
+
+    public float LengthSquared
+    {
+        readonly get
+        {
+            return (x * x) +
+                   (y * y);
+        }
+    }
+}
+
+public static class MyClass
+{
+    public static float ExistingBehavior(in Vector2 vector)
+    {
+        // This code causes a hidden copy, the compiler effectively emits:
+        //    var tmpVector = vector;
+        //    return tmpVector.GetLength();
+        //
+        // This is done because the compiler doesn't know that `GetLength()`
+        // won't mutate `vector`.
+
+        return vector.GetLength();
+    }
+
+    public static float ReadonlyBehavior(in Vector2 vector)
+    {
+        // This code is emitted exactly as listed. There are no hidden
+        // copies as the `readonly` modifier indicates that the method
+        // won't mutate `vector`.
+
+        return vector.GetLengthReadonly();
+    }
+}
+```
+
+Readonly can be applied to property accessors to indicate that `this` will not be mutated in the accessor.
+
+```csharp
+public int Prop
+{
+    readonly get
+    {
+        return this._prop1;
+    }
+}
+```
+
+When `readonly` is applied to the property syntax, it means that all accessors are `readonly`.
+
+```csharp
+public readonly int Prop
+{
+    get
+    {
+        return this._store["Prop2"];
+    }
+    set
+    {
+        this._store["Prop2"] = value;
+    }
+}
+```
+
+Similar to the rules for property accessibility modifiers, redundant `readonly` modifiers are not allowed on properties.
+
+```csharp
+public readonly int Prop1 { readonly get => 42; } // Not allowed
+public int Prop2 { readonly get => this._store["Prop2"]; readonly set => this._store["Prop2"]; } // Not allowed
+```
+
+Readonly can only be applied to accessors which do not mutate the containing type.
+
+```csharp
+public int Prop
+{
+    readonly get
+    {
+        return this._prop3;
+    }
+    set
+    {
+        this._prop3 = value;
+    }
+}
+```
+
+### Auto-properties
+Readonly cannot be applied to auto-implemented properties or their accessors. The compiler will treat all auto-implemented getters as readonly.
+
+### Events
+Readonly can be applied to manually-implemented events, but not field-like events. Readonly cannot be applied to individual event accessors (add/remove).
+
+```csharp
+// Allowed
+public readonly event Action<EventArgs> Event1
+{
+    add { }
+    remove { }
+}
+
+// Not allowed
+public readonly event Action<EventArgs> Event2;
+public event Action<EventArgs> Event3
+{
+    readonly add { }
+    remove { }
+}
+public static readonly event Event4
+{
+    add { }
+    remove { }
+}
+```
+
+Some other syntax examples:
+
+* Expression bodied members: `public readonly float ExpressionBodiedMember => (x * x) + (y * y);`
+* Generic constraints: `public readonly void GenericMethod<T>(T value) where T : struct { }`
+
+The compiler would emit the instance member, as usual, and would additionally emit a compiler recognized attribute indicating that the instance member does not modify state. This effectively causes the hidden `this` parameter to become `in T` instead of `ref T`.
+
+This would allow the user to safely call said instance method without the compiler needing to make a copy.
+
+The restrictions would include:
+
+* The `readonly` modifier cannot be applied to static methods, constructors or destructors.
+* The `readonly` modifier cannot be applied to delegates.
+* The `readonly` modifier cannot be applied to members of class or interface.
+
+## Compiler API
+
+The following public API will be added:
+
+- `bool IMethodSymbol.IsDeclaredReadOnly { get; }` indicates that the member is readonly due to having a readonly keyword, or due to being an auto-implemented instance getter on a struct.
+- `bool IMethodSymbol.IsEffectivelyReadOnly { get; }` indicates that the member is readonly due to IsDeclaredReadOnly, or by the containing type being readonly, except if this method is a constructor.
+
+It may be necessary to add additional public API to properties and events. This poses a challenge for properties because `ReadOnly` properties have an existing, different meaning in VB.NET and the `IMethodSymbol.IsReadOnly` API has already shipped to describe that scenario. This specific issue is tracked in https://github.com/dotnet/roslyn/issues/34213.

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs

@@ -42,7 +42,7 @@ private struct PackedFlags
         {
             // We currently pack everything into a 32-bit int with the following layout:
             //
-            // |            m|l|k|j|i|h|g|f|e|d|c|b|aaaaa|
+            // |          n|m|l|k|j|i|h|g|f|e|d|c|b|aaaaa|
             // 
             // a = method kind. 5 bits.
             // b = method kind populated. 1 bit.
@@ -58,7 +58,9 @@ private struct PackedFlags
             // j = isUseSiteDiagnostic populated. 1 bit
             // k = isConditional populated. 1 bit
             // l = isOverriddenOrHiddenMembers populated. 1 bit
-            // 16 bits remain for future purposes.
+            // m = isReadOnly. 1 bit.
+            // n = isReadOnlyPopulated. 1 bit.
+            // 14 bits remain for future purposes.
 
             private const int MethodKindOffset = 0;
 
@@ -75,6 +77,8 @@ private struct PackedFlags
             private const int IsUseSiteDiagnosticPopulatedBit = 0x1 << 13;
             private const int IsConditionalPopulatedBit = 0x1 << 14;
             private const int IsOverriddenOrHiddenMembersPopulatedBit = 0x1 << 15;
+            private const int IsReadOnlyBit = 0x1 << 16;
+            private const int IsReadOnlyPopulatedBit = 0x1 << 17;
 
             private int _bits;
 
@@ -103,6 +107,8 @@ public MethodKind MethodKind
             public bool IsUseSiteDiagnosticPopulated => (_bits & IsUseSiteDiagnosticPopulatedBit) != 0;
             public bool IsConditionalPopulated => (_bits & IsConditionalPopulatedBit) != 0;
             public bool IsOverriddenOrHiddenMembersPopulated => (_bits & IsOverriddenOrHiddenMembersPopulatedBit) != 0;
+            public bool IsReadOnly => (_bits & IsReadOnlyBit) != 0;
+            public bool IsReadOnlyPopulated => (_bits & IsReadOnlyPopulatedBit) != 0;
 
 #if DEBUG
             static PackedFlags()
@@ -130,6 +136,13 @@ public void InitializeIsExtensionMethod(bool isExtensionMethod)
                 ThreadSafeFlagOperations.Set(ref _bits, bitsToSet);
             }
 
+            public void InitializeIsReadOnly(bool isReadOnly)
+            {
+                int bitsToSet = (isReadOnly ? IsReadOnlyBit : 0) | IsReadOnlyPopulatedBit;
+                Debug.Assert(BitsAreUnsetOrSame(_bits, bitsToSet));
+                ThreadSafeFlagOperations.Set(ref _bits, bitsToSet);
+            }
+
             public void InitializeMethodKind(MethodKind methodKind)
             {
                 Debug.Assert((int)methodKind == ((int)methodKind & MethodKindMask));
@@ -1027,8 +1040,23 @@ public override ImmutableArray<MethodSymbol> ExplicitInterfaceImplementations
             }
         }
 
-        // PROTOTYPE: need to round trip readonly attribute in metadata
-        internal override bool IsDeclaredReadOnly => false;
+        internal override bool IsDeclaredReadOnly
+        {
+            get
+            {
+                if (!_packedFlags.IsReadOnlyPopulated)
+                {
+                    bool isReadOnly = false;
+                    if (CouldBeReadOnly)
+                    {
+                        var moduleSymbol = _containingType.ContainingPEModule;
+                        isReadOnly = moduleSymbol.Module.HasIsReadOnlyAttribute(_handle);
+                    }
+                    _packedFlags.InitializeIsReadOnly(isReadOnly);
+                }
+                return _packedFlags.IsReadOnly;
+            }
+        }
 
         public override string GetDocumentationCommentXml(CultureInfo preferredCulture = null, bool expandIncludes = false, CancellationToken cancellationToken = default(CancellationToken))
         {

src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs

@@ -315,7 +315,9 @@ internal virtual bool IsExplicitInterfaceImplementation
         /// Indicates whether the method is effectively readonly,
         /// by either the method or the containing type being marked readonly.
         /// </summary>
-        internal bool IsEffectivelyReadOnly => (IsDeclaredReadOnly || ContainingType?.IsReadOnly == true) && MethodKind != MethodKind.Constructor;
+        internal bool IsEffectivelyReadOnly => (IsDeclaredReadOnly || ContainingType?.IsReadOnly == true) && CouldBeReadOnly;
+
+        protected bool CouldBeReadOnly => !IsStatic && ContainingType.IsStructType() && MethodKind != MethodKind.Constructor;
 
         /// <summary>
         /// Returns interface methods explicitly implemented by this method.

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs

@@ -1618,6 +1618,11 @@ internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, r
         {
             base.AddSynthesizedAttributes(moduleBuilder, ref attributes);
 
+            if (IsDeclaredReadOnly)
+            {
+                AddSynthesizedAttribute(ref attributes, moduleBuilder.SynthesizeIsReadOnlyAttribute(this));
+            }
+
             bool isAsync = this.IsAsync;
             bool isIterator = this.IsIterator;
 

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

@@ -1047,7 +1047,7 @@ internal override void AfterAddingTypeMembersChecks(ConversionsBase conversions,
                 PartialMethodChecks(this, implementingPart, diagnostics);
             }
 
-            if (_refKind == RefKind.RefReadOnly)
+            if (_refKind == RefKind.RefReadOnly || IsDeclaredReadOnly)
             {
                 this.DeclaringCompilation.EnsureIsReadOnlyAttributeExists(diagnostics, location, modifyCompilation: true);
             }

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyAccessorSymbol.cs

@@ -219,6 +219,12 @@ private SourcePropertyAccessorSymbol(
             }
             declarationModifiers |= propertyModifiers & ~DeclarationModifiers.Indexer;
 
+            // auto-implemented struct getters are implicitly 'readonly'
+            if (containingType.IsStructType() && !property.IsStatic && isAutoPropertyAccessor && methodKind == MethodKind.PropertyGet)
+            {
+                declarationModifiers |= DeclarationModifiers.ReadOnly;
+            }
+
             // ReturnsVoid property is overridden in this class so
             // returnsVoid argument to MakeFlags is ignored.
             this.MakeFlags(methodKind, declarationModifiers, returnsVoid: false, isExtensionMethod: false,
@@ -565,6 +571,16 @@ private ImmutableArray<ParameterSymbol> ComputeParameters(DiagnosticBag diagnost
             return parameters.ToImmutableAndFree();
         }
 
+        internal override void AfterAddingTypeMembersChecks(ConversionsBase conversions, DiagnosticBag diagnostics)
+        {
+            base.AfterAddingTypeMembersChecks(conversions, diagnostics);
+
+            if (IsDeclaredReadOnly)
+            {
+                this.DeclaringCompilation.EnsureIsReadOnlyAttributeExists(diagnostics, locations[0], modifyCompilation: true);
+            }
+        }
+
         internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, ref ArrayBuilder<SynthesizedAttributeData> attributes)
         {
             base.AddSynthesizedAttributes(moduleBuilder, ref attributes);