Add rules for readonly members

[BillWagner]

Fixes #1339

Add a new section for the rules on readonly members of structs.

I didn't write a specific rule that readonly members can modify static fields. There's no rule prohibiting it, so it should be allowed.

I chose language consistent with the future work on #1053 to consider. One popular implementation creates a "defensive copy" of the receiver to invoke a non-readonly member.

Finally, add an example that demonstrates the "direct member of a struct" rule by having a readonly property and readonly method modify the members of a direct member, which is allowed.

[Nigel-Ecma]

Missing definition(s)?

[BillWagner]

This is now ready for final review @jskeet @Nigel-Ecma

[jnm2]

What does a readonly member do to ensure this? I would have thought it was an error to call a non-readonly member using the receiver instance.

[BillWagner]

The roslyn implementation creates a defensive copy, and calls the non-readonly member using that copy as this.

There are also instances where static analysis determines that the call doesn't modify state, so the copy is elided.

That's the reason for the language that doesn't require an implementation choice.

[jnm2]

What do you think about having it spec clearly that the compiler must perform this defensive copy? As written it sounds ambiguous, as though there might be something the language allows the user to do in order to achieve this.

[BillWagner]

In the meeting, @Nigel-Ecma pointed out that saying it must be a defensive copy is standardizing one implementation choice. While it's an obvious choice, it's not the only possible implementation. I see your point. How about this alternative wording:

Suggested change

	- A readonly member may call a non-readonly member using the receiver instance only if it ensures no modifications to the receiver are observable after the readonly member returns.
	- A readonly member may call a non-readonly member using the receiver instance. A conforming implementation must ensure no modifications to the receiver are observable after the readonly member returns.

[jnm2]

It's the only possible implementation that does not result in a compiler error, isn't it? I would think for portability reasons, all compilers should agree on whether or not it's an error. If it's not, then defensive copy is the only possible approach.

[Nigel-Ecma]

@jnm2 & @BillWagner why not just:

Suggested change

	- A readonly member may call a non-readonly member using the receiver instance only if it ensures no modifications to the receiver are observable after the readonly member returns.
	- A readonly member may call a non-readonly member using the receiver instance; however no modifications to the receiver are observable after the readonly member returns.

Which just sticks to the required language semantics – which implementations are required to provide.

Note: I followed @BillWagner’s suggestion for the semantics, which are presumably this way as there is no concern over with possible observation between the call & return – if that presumption is incorrect then the wording will need modifying appropriately, e.g. by dropping “after the readonly member returns”.

[jnm2]

Okay, I'm following and I agree. I like Nigel's suggestion. The main things I wanted the spec to be clear on is that it is never a compiler error to make that call, while simultaneously no writes are being made to the current instance.

[jnm2]

Okay, I'm following and I agree. I like Nigel's suggestion. The main things I wanted the spec to be clear on is that it is never a compiler error to make that call, while simultaneously no writes are being made to the current instance.

[jnm2]

I do have one quibble about "after it returns"- I want a guarantee that there are no writes before it returns, either. I think I can make an example with a callback that shows the difference being observable.

[jnm2]

Here is the example. It's not sufficient to say that readonly means that no writes are observable after the method returns. No writes should happen at all, within the memory of the this instance, if the method is readonly. I believe "observable" is also tricky to define and I would leave it out. If we keep "observable," I'd want to carefully define it, since at the level of subtle threading behaviors and such, there can be observable differences if you do even a "no-op" write, differences which go beyond observing the eventual value.

using System;

class Program
{
    static void Main()
    {
        var storageLocation = new S();

        Console.WriteLine(storageLocation.SomeField); // 0

        storageLocation.M(() =>
        {
            Console.WriteLine(storageLocation.SomeField); // 1
        });

        Console.WriteLine(storageLocation.SomeField); // 0
    }
}

struct S
{
    public int SomeField;

    public void M(Action someCallback)
    {
        // No writes are observable *after* M returns, yet crucially,
        // writes are observable. Readonly should disallow this scenario.

        SomeField++;
        someCallback();
        SomeField--;
    }
}

[jnm2]

I just remembered refs!

Suggested change

	- The member shall not reassign the value of `this` or an instance field of the receiver.
	- The member shall not reassign the value of an instance field-like event (§15.8.2) of the receiver.
	- The member shall not reassign the value of or take a writable reference to `this`, to an instance field of the receiver, or to an instance field-like event (§15.8.2) of the receiver.

Example (and same for field or field-like event as for this itself):

struct S
{
    readonly void M()
    {
        ref readonly S s = ref this; // Allowed
        ref S s = ref this; // Not allowed

        M2(ref this); // Not allowed
        M3(in this); // Allowed
    }

    void M2(ref S p) { }
    void M3(in S p) { }
}

[jnm2]

What does "obfuscating the intent" mean in this context, and what is being discouraged? Looking at this from my own context, both approaches seem (individually) appropriate to me. I know when I'd use one and I know when I'd use the other.

[BillWagner]

I'll defer to @jskeet and @Nigel-Ecma Both thought the example demonstrated a practice they wanted "strongly discouraged".

[Nigel-Ecma]

@jnm2 – I can’t speak for @jskeet, for myself:

This is very much subjective. The usual understanding of readonly is, unsurprisingly, that state is not altered – this example alters what is clearly private state. It would be better without the set, as it is the readonly feels a bit like a false flag operation 😉 (you may collectively groan)

It feels a bit worse here as, depending on language, arrays may be value or reference types but are often thought of as more like values regardless – here flags certainly has the flavour of local value state of the struct.

It’s a subjective boundary – if the struct stored a reference to, say, a GUI window then not being able to change what window was reference, but being able to change the window’s content would likely not be viewed negatively.

[jnm2]

A reader should never gather more from the readonly modifier than its technical meaning, which is "the struct instance will not have changed due to this call." It does not imply anything more broad, such as that no mutations are being made in private state through an indirection.

For example, CancellationToken.Register is readonly (the whole struct is readonly), but no one would imagine that this means that no mutations are being done at all within indirect private state during the Register call.

[jskeet]

I think this is worth discussing more. I would be happy with softening it to something like "used with care and with clear documentation" rather than outright discouragement. CancellationToken is a good example to discuss.

[BillWagner]

How about this slight modification to Nigel's suggestion:

Suggested change

	- A readonly member may call a non-readonly member using the receiver instance only if it ensures no modifications to the receiver are observable after the readonly member returns.
	- A readonly member may call a non-readonly member using the receiver instance; however no modifications to the receiver are observable outside the scope of the readonly member.

[jskeet]

It feels a little odd in terms of scope usually being about the meaning of names, but I'm ambivalent.

[Nigel-Ecma]

Seeing @jskeet’s above comment in my email I spotted that all three of the original lines 147-149 appear to have the wrong actor. Back here on github I see proposed modifications to 149 do not, but that leaves 147 & 148. It is not “The member shall not reassign” – the member is not the actor, the language semantics are by preventing such an assignment. So all three lines need rewording, I have not checked whether the same issue appears elsewhere in the PR.

[jnm2]

I'd rather not mention observability because I think that is a thorny topic (see #1497 (comment)). I believe the fact that a write has happened can be observable through other effects besides reading the eventual value.

[jnm2]

Actually, we can be far more definite. A readonly member is passed a ref readonly this, and therefore it cannot write to fields of this, nor can it call a non-readonly member on this because that would require passing ref this which it does not have. It must create a copy in order to do so, and this is exactly the concept of "defensive copy" which is demanded by the fact that a readonly member never gains a writeable reference to this.

I have more details now too on how any mutation that a readonly member somehow managed to do could cause access violations, since the this reference may be within readonly memory when calling a readonly member, but luckily we don't have to discuss that (and other side effects of mutations) because mutations are not legal through a readonly ref this.

[jskeet]

We decided that we should say a copy is taken, and the non-readonly member is invoked on the copy. We don't need to call it "defensive" and we don't need to explicitly say that an implementation can optimize the copy away.