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

Readonly members metadata #34260

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
181 changes: 181 additions & 0 deletions docs/features/readonly-instance-members.md
Original file line number Diff line number Diff line change
@@ -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.
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved

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.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;

Expand All @@ -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;

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -130,6 +136,13 @@ public void InitializeIsExtensionMethod(bool isExtensionMethod)
ThreadSafeFlagOperations.Set(ref _bits, bitsToSet);
}

public void InitializeIsReadOnly(bool isReadOnly)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
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));
Expand Down Expand Up @@ -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 (IsValidReadOnlyTarget)
{
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))
{
Expand Down
4 changes: 3 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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) && IsValidReadOnlyTarget;

protected bool IsValidReadOnlyTarget => !IsStatic && ContainingType.IsStructType() && MethodKind != MethodKind.Constructor;

/// <summary>
/// Returns interface methods explicitly implemented by this method.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
Loading