csharpstandard icon indicating copy to clipboard operation
csharpstandard copied to clipboard

Spec seems to allow readonly fields of instances other than 'this' to be assigned in constructors

Open jnm2 opened this issue 1 year ago • 4 comments

https://github.com/dotnet/csharpstandard/blob/standard-v7/standard/classes.md#1553-readonly-fields:

When a field_declaration includes a readonly modifier, the fields introduced by the declaration are readonly fields. Direct assignments to readonly fields can only occur as part of that declaration or in an instance constructor or static constructor in the same class. (A readonly field can be assigned to multiple times in these contexts.) Specifically, direct assignments to a readonly field are permitted only in the following contexts:

  • In the variable_declarator that introduces the field (by including a variable_initializer in the declaration).
  • For an instance field, in the instance constructors of the class that contains the field declaration; for a static field, in the static constructor of the class that contains the field declaration. These are also the only contexts in which it is valid to pass a readonly field as an out or ref parameter.

Attempting to assign to a readonly field or pass it as an out or ref parameter in any other context is a compile-time error.

I didn't see how this spec text disallows the following assignment to a readonly field, which occurs in an instance constructor in the same class:

public class C
{
    private readonly int f;
    
    public C(C other)
    {
        other.f = 42;
    }
}

Which in practice (SharpLab) gives:

error CS0191: A readonly field cannot be assigned to (except in a constructor or init-only setter of the type in which the field is defined or a variable initializer)

jnm2 avatar Dec 12 '24 03:12 jnm2

This is not as precise as it could be and a little wordsmithing might not go amiss.

I’ll offer replacing “in the instance constructors of the class that contains the field declaration” with “in the instance constructors of the class that contains the field declaration when invoked on the instance containing the field” as a starter.

Nigel-Ecma avatar Jan 21 '25 02:01 Nigel-Ecma

I assume we want to disallow the following code though:

public class C
{
    private readonly int f;
    
    public C()
    {
        C other = this;
        other.f = 42;
    }
}

That is in an instance constructor invoked on the instance containing the field - but indirectly.

I suspect we really want to describe it in terms of "implicitly or explicitly this", i.e. in terms of the qualification of the field. I wouldn't be surprised if deconstruction etc makes this more complicated, mind you.

jskeet avatar Jan 21 '25 09:01 jskeet

@jskeet wrote:

I suspect we really want to describe it in terms of "implicitly or explicitly this", i.e. in terms of the qualification of the field. I wouldn't be surprised if deconstruction etc makes this more complicated, mind you.

Agree.

My draft suggestion is effectively an upper bound and would require a compiler to track copies & aliases, but only in a limited scope. Jon’s implicit/explicit this is the lower bound. With minimal checking only, and not looking at deconstruction, I’d say that the well known compiler supports at least the lower bound and less than the upper bound.

Nigel-Ecma avatar Jan 22 '25 08:01 Nigel-Ecma

Assigned to @jnm2 - in the meeting we couldn't think of anything other than just f = ... or this.f = ... that would be be valid - but potentially with parentheses involved as well. ("Implicit or explicit use of this" or something similar, without going into parentheses may be enough?)

(Let's check base as well though.)

jskeet avatar Jan 22 '25 20:01 jskeet