roslyn: IDE0044 conflicts with CA2104: Do not declare read only mutable reference types

Version Used: Visual Studio 2017 15.8.2

Steps to Reproduce:

        public class Foo
        {
            private Bar bar; // IDE0044: Make field readonly

            public Foo()
            {
                this.bar = new Bar();
            }

            private class Bar
            {
                public int Mutable { get; set; }
            }
        }

Expected Behavior: Microsoft issues consistent guidance on best practices for C# coding. Generating an IDE0044: Make field readonly on fields which are of reference type likely contradicts CA2014.

Actual Behavior: Based on discussion of CA2104 in #27212, it appears the Roslyn team is reversing the best practices established for use of readonly in the .NET 2.0 coding guidelines. Some quick testing suggests CA2104 has been removed, possibly between VS 15.6 and 15.8. As such, Visual Studio and docs.microsoft.com are out of sync with each other. This is a confusing way to roll out a change and greater clarity on Microsoft’s position would be helpful. The pattern in the repro steps is a common one so the reversal of 13 years of guidance leads to a substantial number of code points where the readonly keyword has to be added to clear IDE0044 messages.

I know Microsoft developers and technical writers often work rather independently but, in this case, a trip down the hall wouldn’t be excessive. CA2104 and CA2105 derive from quite a bit of discussion back in 2004 and, considering that historical context, I’m surprised not to turn up a blog post from the Roslyn team explaining the change of position.

About this issue

Original URL
State: closed
Created 6 years ago
Comments: 18 (16 by maintainers)

Most upvoted comments

We concluded a few things in the design meeting:

Since IDE0044 (private only) and CA2104 (exposed only) do not overlap, there is no current conflict.
If in the future IDE0044 considered exposed fields, we would prefer to implement the analysis to the most accurate allowable extent, even if that goes against CA2104.
A documentation bug should be filed to update the description of CA2104, as it doesn’t seem to be a consistent point of guidance anymore (a legacy rule that is likely to be removed over time, but still exists in some form today which is confusing to users).

sharwell on Sep 14, 2018

Actual Behavior: Based on discussion of CA2104 in #27212, it appears the Roslyn team is reversing the best practices established for use of readonly in the .NET 2.0 coding guidelines.

The .net 2.0 guideline is non-sensical. Being a mutable reference-type has no bearing on whether or not a field/variable/whatever of that type should be read-only or not. This part of the description is correct:

The read-only modifier … prevents the field from being replaced by a different instance of the reference type. However, the modifier does not prevent the instance data of the field from being modified through the reference type.

That is true. But there is not justification for why that is a problem and why should not do this. The purpose of ‘readonly’ has never been to prevent an instance being modified. It’s to prevent a variable being overwritten. Telling people they should not do something which is perfectly sensible and works exactly as intended is strange.

This rule would actually lead to making it easier to mess things up as you would have writable-fields that you might unintentionally overwrite. It does not surprise me when you say:

Some quick testing suggests CA2104 has been removed, possibly between VS 15.6 and 15.8.

That’s good 😃

This is a confusing way to roll out a change and greater clarity on Microsoft’s position would be helpful.

Perhaps consider passing along this feedback to dotnet/roslyn-analyzers. They might have a good idea how to go get these docs and whatnot updated. 😃

Good luck!

CyrusNajmabadi on Sep 1, 2018