Roslyn: "Make field readonly" should not fire if the field is a non-readonly struct and there are invocations on it

[Neme12]

"Make field readonly" should not be offered on a field whose type is a non-readonly struct if there are any invocations or property accesses to the field. Doing so is very dangerous because it silently changes semantics from invoking a method on the original field to invoking it on a copy. Example of bad behavior:

```c#
using System;

struct S
{
public readonly int Value;

public S(int value) => this.Value = value;

public void Mutate()
{
    this = new S(Value + 1);
}

}

static class C
{
static S v;

static void Main()
{
    v.Mutate();
    Console.WriteLine(v.Value);
}

}
```

This program prints 1, but if we make the field readonly, it prints 0.

[Neme12]

@sharwell I see you went with the solution of not offering MakeFieldReadonly, when the struct has any mutable fields. That not only misses some scenarios where the field could be converted to readonly even though its mutable (if there are no writes and no invocations), but it is also wrong, as you can see in the example above. Any method in a struct can directly assign to this unless it is a readonly struct. The proper check should be; are there any invocations on a non-readonly struct? if so, bail out (but actually more complex because of extension methods).

I believe this could be implemented very easily using ValueUsageInfo once it is fixed. See the cases outlined in https://github.com/dotnet/roslyn/pull/30155#issuecomment-425120266 and also https://github.com/dotnet/roslyn/issues/24877#issuecomment-425236303.

Also tagging @mavasani. Once you fix those bugs, could you modify MakeFieldReadonly to use ValueUsageInfo exclusively and finally remove any of its own checks for checking whether the field is a struct, whether it is mutable etc, because this will be covered by ValueUsageInfo?

[sharwell]

@Neme12 Is this currently broken somewhere? The following test is supposed to be verifying this case already:

https://github.com/dotnet/roslyn/blob/6dc2858f4a96abbaf955d572c127782c450b137a/src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs#L848-L860

[Neme12]

@sharwell Can you please take a look at my example in the issue? The key component is the assignment to this. That works even if all the fields are readonly.

[sharwell]

@Neme12 Yep, I saw the commentary after I posted 👍

[sharwell]

The proper check should be; are there any invocations on a non-readonly struct?

This will fix the false positive mentioned, but it's going to cause a bunch of new false negatives. readonly struct is a new construct that isn't widely adopted yet. I'm a bit torn here; I need to figure out the prevalence of assignments to this in structs that have all-readonly fields. Perhaps an alternate solution is to report a diagnostic for assignments to this outside the constructor if the structure matches the readonly struct pattern.

[Neme12]

it's going to cause a bunch of new false negatives

Can you please show an example?

If the feature used information from ValueUsageInfo (and it showed proper results on all invocations and all usages... I linked to a comment listing the current issues) then everything should work.

[sharwell]

Can you please show an example?

All immutable structs that compile against C# prior to the introduction of readonly struct and use fluent syntax for transforming the struct will fall into this category.

[Neme12]

Perhaps an alternate solution is to report a diagnostic for assignments to this outside the constructor if the structure matches the readonly struct pattern.

Now that we have readonly structs, I don't see a reason for this. There should instead be an analyzer to suggest making a struct readonly if all fields are readonly and there are no assignments to this inside it.

[Neme12]

Can you please show an example?

All immutable structs that compile against C# prior to the introduction of readonly struct and use fluent syntax for transforming the struct will fall into this category.

Yes, but we absolutely cannot know whether the implementation mutates the struct or not. There shouldn't be an analyzer out-of-the-box that suggests something that seems so innocent but can change the behavior. False positives are a much bigger problem than false negatives.

If Roslyn shipped with an analyzer+code fix to make structs readonly, the false negatives would be increasingly less and less of a problem in the future. It sounds like a good idea anyway because it falls into the category of suggestions to use new language features.

[Neme12]

The key issue is that methods on non-readonly structs pass this by (a non-readonly) ref. That has to be considered as a write for the analyzer.

[Neme12]

Perhaps an alternate solution is to report a diagnostic for assignments to this outside the constructor if the structure matches the readonly struct pattern.

Also, what would be the appropriate solution to fix this diagnostics? Make the fields not readonly? But then I would get the "Make field readonly" diagnostic :smile: This seems a lot like the suggestion to not use readonly for arrays. The language has well-defined semantics here, and I would prefer to use readonly whenever possible, even if the struct makes assignments to this.

[CyrusNajmabadi]

This will fix the false positive mentioned, but it's going to cause a bunch of new false negatives

Agreed. I'd be willing to accept the feature not being perfect, but being good enough for the vast majority of coding patterns out there.

[CyrusNajmabadi]

Yes, but we absolutely cannot know whether the implementation mutates the struct or not. There shouldn't be an analyzer out-of-the-box that suggests something that seems so innocent but can change the behavior. False positives are a much bigger problem than false negatives.

We have lots of analyzers that will change behavior in circumstances considered so non-common that it's not worth blocking things for. Features like 'rename' (which also has one of hte highest correctness bars out there) also have some pathological cases where it will get things wrong.

It's a non-goal for analyzers to be perfect, or to be 100% certain that they will not change behavior in any circumstance.

--

Indeed, in practice, it turns out to be far more likely that behavior breaks because of changes introduced that are observable, though do not represent an actual language change (i.e. through observing things through metadata). We definitely even allow those things, and those are much more likely.

--

In the cases presented here, i can accept the false positives. They seem unlikely enough to be acceptable, given the positive behavior it has for all the cases likely to happen in practice.

[sharwell]

Design Meeting Notes

This went to a design meeting yesterday. We decided to proceed with an interim solution, which may be adjusted in the future for increased accuracy. The primary motivating concerns were the following:

1. readonly struct is preferred, but is so new that most existing source code and libraries do not use it extensively. We should encourage a move in this direction over time.
2. The situation leading to this bug is an obscure coding pattern, similar in impact to const_cast in C++ but without the clarity of a keyword or well-established coding pattern. We have doubts that the situation occurs naturally in practice (but see the follow-up below).

The critical follow-up for this issue is adding tests to ensure that Make Field Readonly will never suggest marking a struct field readonly if the implementation contains an assignment to this outside the constructor. I will file a secondary follow-up to add a new analyzer that reports a diagnostic for any assignment to this outside a constructor if the type contains readonly instance fields.

[sharwell]

It's a non-goal for analyzers to be perfect, or to be 100% certain that they will not change behavior in any circumstance.

This is technically correct, however we do push for a very high correctness bar. This applies especially to cases like this bug, where most professional C# developers will not understand why this bug can occur or why readonly can change program behavior. Incorrect application of this code fix for the relevant scenarios is likely to go undetected indefinitely, and would be extremely difficult to track down without tools like git blame (which is also not something average users are familiar with).

[sharwell]

Closing as Won't Fix in favor of the interim solution provided by #30262 and #30263.