New rule: using mutable structs in readonly fields

[mareklinka]

A split off issue from corefx/issues/40739 to track the suggested rule

readonly SpinLock fields. SpinLock is a mutable struct, meant only for advanced scenarios. Accidentally making a SpinLock field readonly can result in silent but significant problems, as any mutations to the instance (e.g. Enter, Exit) will be done on a compiler-generated copy and thus be ignored, making the lock an expensive nop. (It might make sense to extend this analyzer to additional mutable struct types where storing them in a readonly field is likely a bug, e.g. GCHandle.)

---

Detecting this should be easy - for each class/struct find all fields that are readonly and of a type that is designed to be mutable (for starters, the GCHandle and SpinLock).

Questions:

1. Can we think of more types that should be included in this list?
2. An automated fix should just remove the readonly modified from the field, right?
3. What would be the default severity for the rule? As there is a rather large chance this situation will lead to a bug, is a warning sufficient?
4. Are there any edge-cases where having a readonly mutable-by-design struct might actually be desired?

[stephentoub]

An automated fix should just remove the readonly modified from the field, right?

Yes

As there is a rather large chance this situation will lead to a bug, is a warning sufficient?

For SpinLock, I can't think of any real situation where it's not definitively a bug. Calling Enter will report that the lock was successfully entered, but the SpinLock itself will have no record of that, which means everyone trying to enter the lock will do so successfully, and the lock will effectively become a silent nop... not what you want for something that's meant to ensure thread safety. If the SpinLock was constructed with its default ctor or with enableThreadOwnerTracking:true as its ctor argument, then at least Exit will end up throwing an exception (because the SpinLock will think the calling thread doesn't actually own the lock), but a) the damage may already be done, b) Exit won't throw if enableThreadOwnerTracking:false was passed to the ctor (which is the high-performance thing to do and thus what most folks using SpinLock do, at least in release builds), and c) regardless it's a bug.

For GCHandle, it may not be a real buge.g. if it's stored into an object that needs access to what's in the handle, but, for example, the handle will never be Free'd, then you could imagine readonly being used with it, though I still think a warning would be useful.

Are there any edge-cases where having a readonly mutable-by-design struct might actually be desired?

Yes. If the object storing the struct has no plans or need to mutate it and is just reading from it, you could imagine someone wanting to make it readonly. But I think that's still worth a suggestion/warning/whatever default level is chosen. However, this is also why IMO any such rule should focus only on a specific list of hand-picked types, rather than, say, warning on any struct that's not readonly or whose fields aren't all readonly... that would almost certainly result in a ton of false positives.

[mareklinka]

What package/project should this go in? I don't think this is an FxCop rule so maybe Microsoft.CodeQuality.Analyzers or Microsoft.NetCore.Analyzers?

[mareklinka]

I have started looking into the implementation here and came up with a few more questions:

1. Is there a rule/convention for creating RuleIds? What ID should we give to this analyzer?
2. The rule will need to check if a field is of a specific type. The easiest option I found to get the SpinLock or GCHandle types in the analyzer is to call context.Compilation.GetTypeByMetadataName("System.Threading.SpinLock"). However, in here there is a similar approach used, with the addition of a fallback strategy that tries to find the type not by its metadata name but by going through namespaces manually. I think this is a fallback for situations where the type in question is "replaced" by a local implementation. The question is, do we need to do something similar?

[mavasani]

Is there a rule/convention for creating RuleIds? What ID should we give to this analyzer?

We generally follow the below steps:

1. Decide the applicable "category" for the new rule. See https://github.com/dotnet/roslyn-analyzers/blob/master/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt for current categories, and the CAxxxx IDs reserved for each category. Note that DiagnosticCategoryAndIdRanges.txt is fed as an additional file into an analyzer that runs on our repo, that verifies there are no duplicate IDs in each analyzer package and the ID is in the registered diagnostic descriptor's range for the provided category.
2. Refer to the current documentation/reference section for all CA rules by the chosen category over here: https://docs.microsoft.com/en-us/visualstudio/code-quality/code-analysis-for-managed-code-warnings?view=vs-2019. For example, this rule will likely fall in the "Performance" category, so you can navigate to https://docs.microsoft.com/en-us/visualstudio/code-quality/performance-warnings?view=vs-2019. You can find that CA1824 is the last documented rule ID in this range, and we also have couple of doc issues on that page for very recently added rules CA1825 and CA1826 for which we documentation still needs to be added. You can also search for the CA ID in the repo to confirm it is not being already used.
3. Once the rule is merged, it is expected that you file an issue on the docs page for the rules category to ensure it gets documented. For example, Add documentation for new Performance rule CA1825 (AvoidZeroLengthArrayAllocations) in FxCop analyzers MicrosoftDocs/visualstudio-docs#3454.

The rule will need to check if a field is of a specific type. The easiest option I found to get the SpinLock or GCHandle types in the analyzer is to call context.Compilation.GetTypeByMetadataName("System.Threading.SpinLock"). However, in here there is a similar approach used, with the addition of a fallback strategy that tries to find the type not by its metadata name but by going through namespaces manually. I think this is a fallback for situations where the type in question is "replaced" by a local implementation. The question is, do we need to do something similar?

I would recommend just using GetTypeByMetadataName. The workaround/fallback strategy was primarily added for ImmutableArray due to the fact that it exists in couple of different assemblies and the test framework was including both of them. It will likely be removed in future.

[mavasani]

I don't think this is an FxCop rule so maybe Microsoft.CodeQuality.Analyzers or Microsoft.NetCore.Analyzers?

FxCopAnalyzers package does not have any rules by itself. It is just a master package that includes both Microsoft.CodeQuality.Analyzers and Microsoft.NetCore.Analyzers as dependencies for ease of install and migration for legacy FxCop users. I hope the description over https://github.com/dotnet/roslyn-analyzers#microsoftcodequalityanalyzers and https://github.com/dotnet/roslyn-analyzers#microsoftnetcoreanalyzers helps you decide the correct package to choose.

Bottom line is analyzers related to purely code quality improvements, but are not specific to any API will go into Microsoft.CodeQuality.Analyzers. Analyzers specific to usage of any API would go into Microsoft.NetCore.Analyzers. A good rule of thumb is that if your analyzer needs to invoke GetTypeByMetadataName, then most likely it is an API specific analyzer and belongs to Microsoft.NetCore.Analyzers.

[mavasani]

FYI: I submitted a PR to document the above guidelines in the repo.

[mavasani]

Guidelines are now at https://github.com/dotnet/roslyn-analyzers/blob/master/GuidelinesForNewRules.md

[mareklinka]

Awesome, thanks for the guidance!

[mavasani]

Reading the comments above another time, it seems like this is not a performance rule, but indicates a very likely functional bug. Is that correct? If so, this should likely not be a Performance rule, but must have a different category and rule ID. Thoughts?

[mareklinka]

Yeah, I'm on the fence here. On one side, using the readonly modifier degrades performance because of the shadow copying by the compiler. On the other, using a readonly SpinLock is pretty much a definite bug. However, for GCHandle it might make sense to have it readonly in situations when the class just gets it from outside and doesn't plan to modify/free it.

So it's kind of Performance and kind of Usage or Maintainability. Not sure which would be best but I agree the performance aspect is rather marginal here.

[stephentoub]

I don't think it should be performance. Can you come up with a benchmark where adding readonly measurably improves performance with a correct implementation?

[mareklinka]

Also a good point. New idea: since such a usage likely (at least for the SpinLock) constitutes a bug, maybe the best category would be Reliability?

[stephentoub]

What's the definition of the correctness category vs the reliability category?

[sharwell]

Ideally we would have the ability to mark SpinLock as non-copyable (dotnet/csharplang#859), which covers a much broader set of misuse cases than an analyzer only focused on read-only.

I would argue that for the rule in question here, the importance of non-copyable is sufficient to make that the target behavior of the rule instead of limiting it to whether or not the storage is readonly.

[mareklinka]

What's the definition of the correctness category vs the reliability category?

As far as I can see in the docs there is no correctness category. Which is consistent with the DiagnosticCategory enum which only has MicrosoftCodeAnalysisCorrectness (which I assume is specific to diagnostics and code fixes?).

From the description of the Reliability category, I think it might be a good fit:

Warnings that support library and application reliability, such as correct memory and thread usage.

[carlossanlop]

@mavasani I noticed this proposal was marked as approved in this repo. I'll remove the label since we want it to go through API review via dotnet/runtime#33773

Edit: Maybe we can make an exception with this one since the discussion is large and there's a PR out already. I'll wait for confirmation on the runtime issue.