Make it an error to invoke non-readonly methods on a readonly field

[andre-ss6]

Hey folks, there may be a legitimate use case for this that I'm missing, if so, sorry in advance. I also tried looking for an existing discussion on this, but couldn't find one.

My suggestion is to make this an error:

struct Bar
{
    void NonReadonlyMethod() {}
}

/*readonly*/ struct Foo 
{
    /*readonly*/ Bar _bar;
    readonly void ReadonlyMethod() => _bar.NonReadonlyMethod(); // error
    
    readonly void ReadonlyMethod()
    {
        var copy = _bar;
        copy.NonReadonlyMethod(); // legal
    }
}

I have fallen quite a few times into the trap of introducing readonly to a field of a non-readonly struct type on code that relies on the mutability of that field, thus breaking the code. Related dotnet/roslyn#30240

Unless I'm missing something obvious here, even when it's not a bug I think I'd much prefer the compiler force me to make an explicit copy (such as when I know an API that I don't control does not change state but hasn't been marked readonly, or that does change state, but I purposefully don't care/want the field being updated), than having a hidden copy made for me.

These issues are currently solved by analyzers, and AFAIK there is no official one that checks for this shipped with the .NET SDK.

[CyrusNajmabadi]

There is nothing wrong calling a non-readonly method on a readonly field. Indeed, it's super common. Consider how often people may call things like GetHashCode, ToString, or even Equals on these methods. All of which are not commonly exported as something readonly.

These issues are currently solved by analyzers

Yes, this is the right way to do things. Having the language fail here would break everything.

[TahirAhmadov]

Now, whether my proposed rule should apply inside of the Bar and Bar2 methods in my SharpLab is a different question. I would err on the side of "yes" - you have an in or ref readonly parameter, which means it cannot be modified. If you are only reading that struct, then you can either 1) use a readonly member, or 2) make a copy yourself for clarity and explicit intent. The performance issue can be significant, because if the struct is on the larger size, then it's much easier to modify an existing member (if possible) to be readonly and keep high performance, as opposed to copying.
PS. To clarify, my point is that the reason somebody would typically use in is to avoid copying a large struct with the default by-value semantics. So the developer uses in, but neglects to mark a struct member as readonly, and then falls right back to copying, completely undoing the benefit of in in that scenario.

[TahirAhmadov]

there is no copying at the call site, and instead it copies inside the method

the in method obviously doesn't copy inside the method. If so, that would break everything, as you'd now be getting copies of large objects slowing down everything :)

Yes it does. Please check the SharpLab.

[TahirAhmadov]

causes a silent copy

It is silent. You literally cannot see it when you make the call. Just like you cannot see the copy when you do x.Foo();

Whenever the compiler needs to emit a new local behind the scenes without a user doing so explicitly, should be illegal.

Note: by your logic x.Foo(); is not a silent copy either. because you can go to 'x' and see that it is readonly and thus know it will cause copies.

Incorrect. Not only I need to know that the field - which can be many LOCs away, in a different file, or in a 3rd-party assembly - is readonly, which requires me to check manually, but I also need to know that the method is not readonly, again, which requires me to navigate to it to check. That's the entire point - given how broken the behavior of the silent copying is, the compiler should make our lives easier and prohibit this anti-pattern.

PS. Basically, the current behavior is a perfect example of "pit of failure".

[CyrusNajmabadi]

You're not understanding what is going on here. There is no copy inside the method caused by a eitehr a non-in or in method. In your example, tehre is a copy because you're calling a non-readonly method on a parameter.

Whenever the compiler needs to emit a new local behind the scenes without a user doing so explicitly, should be illegal.

Again, that's the exact case i showed. With the code examples, some of those cases will end up with a copy and some will not. And you cannot tell from the call site. Teh copy is silent.

[CyrusNajmabadi]

PS. To clarify, my point is that the reason somebody would typically use in is to avoid copying a large struct with the default by-value semantics. So the developer uses in, but neglects to mark a struct member as readonly, and then falls right back to copying, completely undoing the benefit of in in that scenario.

There is no copy at the callsite here. There's a copy in your sharplab case inside the method due to the parameter being 'in' and the method not being readonly.

which requires me to check manually, but I also need to know that the method is not readonly, again, which requires me to navigate to it to check.

Yes. That's the definition of 'silent'. You can't tell at the use site. You must see the declaration. THat's literally why these are silent copies, as i've been saying.

should be illegal.

I really don't know what to say at this point. We're going in circles. We will not break the world like this. I'm sorry if that's not something you are ok with, but it's the stated and affirmed philosophy of the team many times over. Heck, even with field, where we are willing to take a breaking change, we've tried to push it into the tiniest scope ever to avoid the pain this would cause. Making it so that "[w]henever the compiler needs to emit a new local behind the scenes without a user doing so explicitly" is now illegal, breaks the world. It is a non-starter. Full stop. Asking us to do this goes nowhere.

If you want this this is what analyzers are for. Then you can break your world, and others can opt into breaking theirs.