Remove CS 1612 for readonly structs

[gafter]

@AlgorithmsAreCool commented on Tue Jun 05 2018

Version Used:
LangVersion =7.3

.NET Core SDK (reflecting any global.json):
 Version:   2.1.300
 Commit:    adab45bf0c

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.16299
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\2.1.300\

Host (useful for support):
  Version: 2.1.0
  Commit:  caa7b7e2ba

.NET Core SDKs installed:
  2.1.300 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 2.1.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]

Steps to Reproduce:

using System;
using System.Collections.Generic;
using System.Timers;

namespace Junk
{
    public class TimerCollection<TKey>
    {
        private Dictionary<TKey, Timer> Timers { get; } = new Dictionary<TKey, Timer>();

        public IntervalAccessor Intervals => new IntervalAccessor(this);

        public void Create(TKey key) => Timers.Add(key, new Timer());

        public readonly struct IntervalAccessor
        {
            private TimerCollection<TKey> Parent { get; }
            public IntervalAccessor(TimerCollection<TKey> parent)
            {
                Parent = parent;
            }

            public TimeSpan this[TKey key]
            {
                get => TimeSpan.FromMilliseconds(Parent.Timers[key].Interval);
                set => Parent.Timers[key].Interval = value.TotalMilliseconds;
            }
        }
    }

    public static class Repro
    {
        public static void Example()
        {
            var collection = new TimerCollection<string>();

            collection.Create("test");

            // error CS1612: Cannot modify the return value of 'TimerCollection<string>.Intervals' because it is not a variable
            collection.Intervals["test"] = TimeSpan.FromSeconds(1);
        }
    }
}

Expected Behavior:
No Error
Actual Behavior:
Very Error

So since C# doesn't have Indexed Properties I like to sometimes use this flyweight accessor pattern to simulate it.

But C# doesn't allow this because of reasons @gafter explained in #22592

Specifically @gafter says that

Calling a property set method requires passing a this parameter by reference, which can only be done for an lvalue.

I'm not clear on the how using a struct vs a class effects if something is considered an lvalue or a rvalue but now that we have readonly structs, can't the compiler see that modifying the struct is not possible and therefore whatever the setter is doing will be safe?

---

@Ollhax commented on Sat Oct 06 2018

Just want to add another case here. I'm working on a Entity Component System and I'm trying to hide the component data layout behind a readonly ref struct. Code:

public class HealthManager
{
	// Component is a "view" to the internal systems, allowing modification
	// while allowing us to reorder the internal data in whatever way is the most efficient.
	public readonly ref struct Component
	{
		private readonly HealthManager manager;
		private readonly int index;
		
		internal Component(HealthManager manager, int index)
		{
			this.manager = manager;
			this.index = index;
		}

		public int Health
		{
			get => manager.healthComponents[index].Health;
			set => manager.healthComponents[index].Health = value;
		}
	}
	
	private struct HealthComponentData
	{
		// Could store this as a Structure of Arrays or whatever makes the most sense
		public int Health;
	}
	
	private HealthComponentData[] healthComponents = ...;

	public Component Get(Entity entity)
	{
		// Return a component if we have one, or throw an exception
	}
}

// I'd like this to work:
HealthManager.Get(entity).Health = 3; // Error CS1612: Cannot modify the return value of 'HealthManager.Get(Entity)' because it is not a variable

// Workaround:
var c = HealthManager.Get(entity);
c.Health = 3;

[AlgorithmsAreCool]

I didn't realize this was a language issue, does the spec forbid this currently?

[DavidArno]

I'm completely confused by this. According to all documentation around readonly structs, "The compiler checks that the struct is indeed immutable and consists only of readonly fields and/or readonly properties" (quote taken from The ‘in’-modifier and the readonly structs in C#).

Yet the setter in the OP's code seems to be allowed and I checked this on sharplab and setters seem to be allowed in read-only structs, which surely makes a totally mockery of the "readonly" label?

[YairHalberstadt]

@DavidArno
The struct is shallowly Immutable, as it's fields are immutable. It is not however deeply immutable, not is it guaranteed to be.

[DavidArno]

The struct is shallowly Immutable

If it allows setters, then it is not even shallowly immutable. For it to be shallowly immutable to should force the developer to jump through hoops to mutate things at a deeper level. Allowing setters right there in the public API makes any immutable claims a complete farce. Sure people will always abuse language features, but I'm really shocked and disappointed that such abuse is so openly encouraged by allowing any setters of any form to exist in any indexers and properties of a readonly struct.

[AlgorithmsAreCool]

I get what you mean about setters, but if the setters cannot actually modify the underlying field(is there is one) is it actually an issue?

I'm just abusing the indexer syntax here but the struct is doesn't ever change after construction.

[PathogenDavid]

I'd like to see this applied to CS1654 ("mutating" foreach iteration variables) and readonly struct members (#1710) as well.

I ran into this limitation working on a wrapper for native code that stores data in several large arrays rather than one array of structs. We're using a thin view struct to make accessing this data more natural. (Much like @Ollhax is with his ECS.)

The implementations generally look something like this:

internal unsafe struct NativePolygonMesh
{
    public readonly ushort* Flags;
    public readonly byte* AreaIds;
    public readonly int PolygonCount;
}

public unsafe class PolygonMesh : IEnumerable<Polygon>
{
    internal readonly NativePolygonMesh* Native;
    public int PolygonCount => Native->PolygonCount;

    public Polygon this[int index]
    {
        get
        {
            if (index < 0 || index >= Native->PolygonCount)
            { throw new IndexOutOfRangeException(); }

            return new Polygon(this, index);
        }
    }

    // Omitted: IEnumerable and enumerator implementation.
}

public unsafe readonly struct Polygon
{
    private readonly PolygonMesh mesh;
    private readonly int index;

    internal Polygon(PolygonMesh mesh, int index)
    {
        this.mesh = mesh;
        this.index = index;
    }

    public byte AreaId => mesh.Native->AreaIds[index];

    public ushort Flags
    {
        get => mesh.Native->Flags[index];
        set => mesh.Native->Flags[index] = value;
    }
}

Ideally I'd like to access the data like in these examples, but CS1654/CS1612 prevent it:

public void UpdateFlags(PolygonMesh polygonMesh)
{
    foreach (Polygon polygon in polygonMesh)
    {
        polygon.Flags = 100; // error CS1654: Cannot modify members of 'polygon' because it is a 'foreach iteration variable'
    }
}

public void UpdateFlags2(PolygonMesh polygonMesh)
{
    for (int i = 0; i < polygonMesh.PolygonCount; i++)
    {
        polygonMesh[i].Flags = 100; // error CS1612: Cannot modify the return value of 'polygonMesh.this[int]' because it is not a variable
    }
}

We're currently using the workaround below, but it leaks the fact that Polygon is a view of the data instead of the actual data. (If the native library stored the data in a struct, I'd just have my iterators return a ref Polygon instead.)

foreach (Polygon _polygon in polygonMesh)
{
    Polygon polygon = _polygon;
    polygon.Flags = 100;
}

[shadow-cs]

Any news? This would certainly improve usage of struct wrappers in certain scenarios where we try to reduce allocations.

[lixiss]

Another example:

    // Some wrapper. It always just forward operations.
    // No any mutable state stored, except subject identification.
    public readonly struct A
    {
        private readonly object _objectStore;
        private readonly int _objectIndex;

        public int SomeProperty
        {
            readonly get => throw new System.NotImplementedException();
            set => throw new System.NotImplementedException();
        }
    }

    // Client's code (also a wrapper).
    public readonly struct B // wraps A
    {
        private readonly A _target;

        public readonly A Target => _target;

        public readonly A GetTarget() => _target;
    }

    public class Sample
    {
        public void DoSomething()
        {
            var x = new B();

            x.Target.SomeProperty = 1; // CS1612: ...

            x.GetTarget().SomeProperty = 1; // That's compiled fine.
        }
    }

So, why compiler ever generate error?
Doesn't property getter is just get_Target() method?

Looks like CS1612 was designed to protect from struct hidden errors, but instead it protect from right code design, while can't protect about hidden copies generally... so, IMO, is bug/unwanted feature.

[JustNrik]

Not allowing setters in autoproperties makes sense, but in normal properties it makes zero sense since they don't necessarily (actually, they simply can't) mutate the readonly struct since fields are all readonly.

[Chicken-Bones]

Linking dotnet/roslyn#45284

Seems like making readonly struct wrappers for ECS-like implementations is a shared idea

[jnm2]

Proposal to lift this restriction: #9174

[jnm2]

See also discussion at #8364

[TahirAhmadov]

@jnm2 can we also remove CS1918 while we're at it?

#8427

[jnm2]

We'll see! Check out https://github.com/dotnet/csharplang/blob/main/proposals/readonly-setter-calls-on-non-variables.md#expansions (#8427 (comment)).

[TahirAhmadov]

Oh yeah, I missed that somehow.
The collection initializer syntax relies on Add method, would it be made legal at the same time?

[jnm2]

Good point, worth adding to that section I think.

[RikkiGibson]

https://github.com/dotnet/csharplang/pull/9233/files indicates how behavior of a?.b = c could change if #9174 is accepted.