[WIP] Add rule for detecting certain readonly mutable structs #2831

mareklinka · 2019-09-08T13:57:58Z

As proposed in corefx/issues/40739 and #2811, this PR adds a new rule for detecting readonly fields of type System.Threading.SpinLock and System.Runtime.InteropServices.GCHandle.

Both C# and VB versions are implemented but VB is not my strong suite so it might not be entirely idiomatic VB.NET.

Tests for both analyzers and fixers are there but if I managed to omit some cases, let me know and I'll add them.

I'm also not sure I'm defensive enough (it's been a while since I worked with Roslyn so I might have missed some recommended practices).

ID of the new rule is Performance/CA1829.

Fixes #2811

dotnet#2811

mavasani · 2019-09-08T15:21:30Z

...crosoft.NetCore.Analyzers/Core/Performance/MutableStructsShouldNotBeUsedForReadonlyFields.cs

+
+namespace Microsoft.NetCore.Analyzers.Performance
+{
+    public abstract class MutableStructsShouldNotBeUsedForReadonlyFieldsAnalyzer : DiagnosticAnalyzer


This analyzer can be written as a language agnostic symbol analyzer, which just registers a SymbolAction with SymbolKind.Field. For example, look at https://github.com/dotnet/roslyn-analyzers/blob/master/src/Microsoft.CodeQuality.Analyzers/Core/ApiDesignGuidelines/DoNotDeclareVisibleInstanceFields.cs.

mavasani · 2019-09-08T15:25:29Z

...NetCore.Analyzers/CSharp/Performance/CSharpMutableStructsShouldNotBeUsedForReadonlyFields.cs

+                return;
+            }
+
+            var typesToCheck = MutableValueTypesOfInterest.Select(typeName => context.Compilation.GetTypeByMetadataName(typeName)).Where(type => type != null).ToList();


GetTypeByMetadataName should only be invoked during CompilationStartAction callback. You should check for null values of the required well known type(s) and bail out if the analyzer is not applicable for the current compilation, and register nested symbol action if it is applicable. For example:

roslyn-analyzers/src/Microsoft.NetCore.Analyzers/Core/Tasks/DoNotCreateTasksWithoutPassingATaskScheduler.cs

Lines 42 to 54 in 3e4ef01

context.RegisterCompilationStartAction(compilationContext =>

{

// Check if TPL is available before actually doing the searches

var taskType = compilationContext.Compilation.GetTypeByMetadataName("System.Threading.Tasks.Task");

var taskFactoryType = compilationContext.Compilation.GetTypeByMetadataName("System.Threading.Tasks.TaskFactory");

var taskSchedulerType = compilationContext.Compilation.GetTypeByMetadataName("System.Threading.Tasks.TaskScheduler");

if (taskType == null || taskFactoryType == null || taskSchedulerType == null)

{

return;

}

compilationContext.RegisterOperationAction(operationContext =>

{

mavasani · 2019-09-08T15:28:06Z

...NetCore.Analyzers/CSharp/Performance/CSharpMutableStructsShouldNotBeUsedForReadonlyFields.cs

+            context.RegisterSyntaxNodeAction(NodeAction, SyntaxKind.FieldDeclaration);
+        }
+
+        private static void NodeAction(SyntaxNodeAnalysisContext context)


In general, we strongly recommend not using SyntaxNodeAction in this repo. Majority of analyzers should fall under the below 2 buckets:

Analyze the attributes/properties of a symbols: Prefer RegisterSymbolAction

Analyze executable code: Prefer RegisterOperationAction

SyntaxNodeActions are only required for analyses related to syntax nodes that are not part of any symbol's declaration OR executable code, which is extremely rare for an analyzers in this repo.

📝 Note that syntax node actions are not inherently bad. However, the GetSymbolInfo method is very expensive, and the cost can be avoided by using one of the other approaches. If you need to operate on symbols (as opposed to just syntax), symbol and operation actions are more efficient. In some cases, operation actions also simplify the work required to support both C# and VB.

@mareklinka regarding the "rewrite" of this analyzer, you can have a look at this PR I started: https://github.com/dotnet/roslyn-analyzers/pull/3405/files

mavasani · 2019-09-08T15:37:54Z

...e.Analyzers/CSharp/Performance/CSharpMutableStructsShouldNotBeUsedForReadonlyFields.Fixer.cs

+            FieldDeclarationSyntax fieldDeclarationSyntax)
+        {
+            var editor = await DocumentEditor.CreateAsync(context.Document, context.CancellationToken).ConfigureAwait(false);
+            var withoutReadonly = fieldDeclarationSyntax.WithModifiers(


A slighly simpler implementation would be: http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp.Features/CodeFixes/HideBase/HideBaseCodeFixProvider.AddNewKeywordAction.cs,37

This also allows you to make the fixer language agnostic (you don't need language specific SyntaxKind anymore)

I tried approaching it as you suggested but I think I'm missing something. The situation:

The analyzer now operates on symbols and reports the location of the diagnostic as fieldSymbol.Locations.First(). This means the location of the symbol name is reported.

That means the fix will get a span that points to a VariableDeclaratorSyntax, not the actual FieldDeclarationSyntax. That works fine for the SyntaxGenerator, which ascends the tree correctly and returns a new FieldDeclarationSyntax node, but then I can't simply call root.ReplaceNode because the nodes are of different types.

Furthermore, this complicates executing the fix on statement private readonly SpinLock _sl1 = new SpinLock(), sl2 = new SpinLock();. Here the generator doesn't work because it creates a new FieldDeclarationSyntax but without the second variable declaration.

Either I'm missing the correct API/usage of it or it looks like it will be easier to make the fix language-specific and simply use:

root.FindNode(context.Span).FirstAncestorOrSelf<FieldDeclarationSyntax>() // this is language-specific ... var generator = SyntaxGenerator.GetGenerator(context.Document); var nodeWithoutReadonly = generator.WithModifiers(targetNode, generator.GetModifiers(targetNode).WithIsReadOnly(false)); var newRoot = root.ReplaceNode(originalFieldDeclaration, nodeWithoutReadonly); return Task.FromResult(context.Document.WithSyntaxRoot(newRoot));

Testing this, it leads to correct behavior even in case of multi-declaration.

Thanks, I meant to recommend the exact same implementation as your comment, but keep it in the shared layer with a type parameter TFieldDeclarationSyntax, where TFieldDeclarationSyntax: SyntaxNode. Language specific subtypes will just need to provide the language specific type argument FieldDeclarationSyntax and have no code within.

mavasani · 2019-09-08T15:47:10Z

...Analyzers/UnitTests/Performance/MutableStructsShouldNotBeUsedForReadonlyFieldsTests.Fixer.cs

+    private GCHandle _gch_noinit;
+}
+";
+            await CSharpCodeFixVerifier<CSharpMutableStructsShouldNotBeUsedForReadonlyFieldsAnalyzer, CSharpMutableStructsShouldNotBeUsedForReadonlyFieldsFixer>


Kindly follow the pattern used by all the existing unit tests in the repo and define usings to avoid clutter in individial unit tests such as

roslyn-analyzers/src/Microsoft.NetCore.Analyzers/UnitTests/Runtime/TestForNaNCorrectlyTests.Fixer.cs

Lines 8 to 52 in c4e5473

using VerifyCS = Test.Utilities.CSharpSecurityCodeFixVerifier<

Microsoft.NetCore.Analyzers.Runtime.TestForNaNCorrectlyAnalyzer,

Microsoft.NetCore.CSharp.Analyzers.Runtime.CSharpTestForNaNCorrectlyFixer>;

using VerifyVB = Test.Utilities.VisualBasicSecurityCodeFixVerifier<

Microsoft.NetCore.Analyzers.Runtime.TestForNaNCorrectlyAnalyzer,

Microsoft.NetCore.VisualBasic.Analyzers.Runtime.BasicTestForNaNCorrectlyFixer>;

namespace Microsoft.NetCore.Analyzers.Runtime.UnitTests

{

public class TestForNaNCorrectlyFixerTests

{

[Fact]

public async Task CA2242_FixFloatForEqualityWithFloatNaN()

{

await VerifyCS.VerifyCodeFixAsync(@"

public class A

{

public bool Compare(float f)

{

return [|f == float.NaN|];

}

}

", @"

public class A

{

public bool Compare(float f)

{

return float.IsNaN(f);

}

}

");

await VerifyVB.VerifyCodeFixAsync(@"

Public Class A

Public Function Compare(s As Single) As Boolean

Return [|s = Single.NaN|]

End Function

End Class

", @"

Public Class A

Public Function Compare(s As Single) As Boolean

Return Single.IsNaN(s)

End Function

End Class

");

mavasani · 2019-09-08T15:57:56Z

...Analyzers/UnitTests/Performance/MutableStructsShouldNotBeUsedForReadonlyFieldsTests.Fixer.cs

+
+public class C
+{
+    private [|readonly|] SpinLock _sl = new SpinLock();


Can you add C# and VB unit tests where you have a field declaration with multiple fields declared, such as private readonly SpinLock _sl1 = new SpinLock(), sl2 = new SpinLock();

mavasani · 2019-09-08T15:59:07Z

...Analyzers/UnitTests/Performance/MutableStructsShouldNotBeUsedForReadonlyFieldsTests.Fixer.cs

+Imports System.Runtime.InteropServices
+
+Public Class Class1
+    Public [|ReadOnly|] _sl As SpinLock = New SpinLock()


Can you add VB unit test with field declaration that using AsNew clause? For example, Private Readonly _sl As New SpinLock()

mavasani · 2019-09-08T16:00:49Z

...t.NetCore.Analyzers/Core/Performance/MutableStructsShouldNotBeUsedForReadonlyFields.Fixer.cs

+
+        protected abstract void AnalyzeCodeFix(CodeFixContext context, SyntaxNode targetNode);
+
+        public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(MutableStructsShouldNotBeUsedForReadonlyFieldsAnalyzer.RuleId);


Nit: Move the property overrides before the method overrides.

mavasani · 2019-09-08T16:01:15Z

...t.NetCore.Analyzers/Core/Performance/MutableStructsShouldNotBeUsedForReadonlyFields.Fixer.cs

+
+        public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(MutableStructsShouldNotBeUsedForReadonlyFieldsAnalyzer.RuleId);
+
+        public override FixAllProvider GetFixAllProvider()


Nit: Move this override ahead of RegisterCodeFixesAsync override.

mavasani · 2019-09-08T16:08:05Z

src/Microsoft.NetCore.Analyzers/Core/MicrosoftNetCoreAnalyzersResources.resx

+    <value>Certain value types (e.g. SpinLock, GCHandle) are explicitly designed to be mutable. Declaring a field of such type as readonly might cause unintentional shadow copying by the compiler, leading to performance degradation or bugs.</value>
+  </data>
+  <data name="MutableStructsShouldNotBeUsedForReadonlyFieldsMessage" xml:space="preserve">
+    <value>Field {0} of type {1} should not be readonly.</value>


Field '{0}' of type '{1}' should not be readonly.

mavasani · 2019-09-08T16:22:42Z

src/Microsoft.NetCore.Analyzers/Core/MicrosoftNetCoreAnalyzersResources.resx

+    <value>Certain readonly structs should not be used as readonly fields</value>
+  </data>
+  <data name="MutableStructsShouldNotBeUsedForReadonlyFieldsDescription" xml:space="preserve">
+    <value>Certain value types (e.g. SpinLock, GCHandle) are explicitly designed to be mutable. Declaring a field of such type as readonly might cause unintentional shadow copying by the compiler, leading to performance degradation or bugs.</value>


Given that SpinLock and GCHandle are just couple of potentially many such types for which this rule can be applicable, I think we should make this configurable for end users to allow specifying the set of disallowed types such that mutable readonly fields of all of those types would be flagged. If we agree, then I would suggest the following:

Use the existing option disallowed_symbol_names

See how this option is used to make CA1031 configurable with this commit: 40cd301. I would suggest a similar implementation and unit test for it.

If you feel this would overload your PR, I can do this as a followup PR as well. In this case, please file a separate tracking issue to make this rule configurable.

I agree that configuring this could be useful but I'd opt into enabling this configuration in a separate PR. I'll file it once this one is merged, if that's ok.

Sounds reasonable!

sharwell

Marking as Request Changes to give me a chance to meet with @mavasani and discuss the distinction between non-readonly and non-copyable. The latter is a more complicated rule, but much better at detecting potential bugs arising from misuse of mutable value types.

Evangelink · 2019-11-27T12:34:41Z

@sharwell, @mavasani have you been able to meet and discuss about the direction this PR should go to?

Evangelink · 2020-03-20T14:44:21Z

@mareklinka Is there something I can do to help moving forward with this new analyzer?

mareklinka · 2020-03-21T01:42:21Z

@Evangelink Well, as this was originally waiting for a clarification meeting and there was no update for a long time, I stopped really paying attention after a while.

As @sharwell mentioned, if the analyzer is supposed to be detecting copying of structs, it will be much more complicated. In that case, I think I will leave the implementation to someone with more time for it.

If the readonly semantics are fine, I think I should be able to revisit the PR and solve the merge conflicts so that it could be reviewed and merged.

Evangelink · 2020-03-23T12:38:11Z

@mavasani, @sharwell have you been able to discuss #2831 (review)? The original ticket is talking about non-readonly IMO, the non-copyable part would also make sense but could be either an update to this rule or a separate rule.

sharwell · 2020-03-31T07:28:12Z

@mareklinka I implemented the initial non-copyable analysis in #3420

sharwell · 2021-05-20T23:54:40Z

@mavasani My recommendation for this is to use it as more evidence in support of copy value callbacks integrated directly into the analyzer driver as part of preparing for dotnet/runtime#50389.

mareklinka added 2 commits September 8, 2019 15:41

Merge remote-tracking branch 'dotnet/master'

d5d613f

Add an analyzer to check for readonly SpinLock and GChandle fields

b4bd2f2

dotnet#2811

mavasani reviewed Sep 8, 2019

View reviewed changes

sharwell requested changes Sep 8, 2019

View reviewed changes

mavasani mentioned this pull request Mar 2, 2020

Readonly Structs vs Classes have dangerous inconsistency - failed spin lock dotnet/roslyn#17310

Closed

mavasani mentioned this pull request Mar 20, 2020

CA1070: Do not use mutable struct in readonly field #3405

Closed

sharwell mentioned this pull request Mar 31, 2020

Implement RS0042 (Do not copy value) #3420

Merged

carlossanlop mentioned this pull request Nov 30, 2020

Do not hold SpinLock in fields marked as readonly dotnet/runtime#33773

Open

Base automatically changed from master to main March 5, 2021 02:20

jinujoseph requested a review from a team as a code owner March 5, 2021 02:20

mavasani assigned sharwell May 20, 2021

sharwell added the Blocked label May 20, 2021

stephentoub mentioned this pull request Apr 12, 2024

Regression in R2R compilation time dotnet/runtime#100995

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add rule for detecting certain readonly mutable structs #2831

[WIP] Add rule for detecting certain readonly mutable structs #2831

mareklinka commented Sep 8, 2019 •

edited

Loading

mavasani Sep 8, 2019

mavasani Sep 8, 2019

mavasani Sep 8, 2019 •

edited

Loading

sharwell Sep 8, 2019

Evangelink Mar 23, 2020

mavasani Sep 8, 2019

mareklinka Sep 8, 2019 •

edited

Loading

mavasani Sep 8, 2019

mavasani Sep 8, 2019

mavasani Sep 8, 2019

mavasani Sep 8, 2019 •

edited

Loading

mavasani Sep 8, 2019

mavasani Sep 8, 2019

mavasani Sep 8, 2019 •

edited

Loading

mavasani Sep 8, 2019 •

edited

Loading

mareklinka Sep 8, 2019

mavasani Sep 8, 2019

sharwell left a comment •

edited

Loading

Evangelink commented Nov 27, 2019

Evangelink commented Mar 20, 2020

mareklinka commented Mar 21, 2020

Evangelink commented Mar 23, 2020

sharwell commented Mar 31, 2020

sharwell commented May 20, 2021

	context.RegisterCompilationStartAction(compilationContext =>
	{
	// Check if TPL is available before actually doing the searches
	var taskType = compilationContext.Compilation.GetTypeByMetadataName("System.Threading.Tasks.Task");
	var taskFactoryType = compilationContext.Compilation.GetTypeByMetadataName("System.Threading.Tasks.TaskFactory");
	var taskSchedulerType = compilationContext.Compilation.GetTypeByMetadataName("System.Threading.Tasks.TaskScheduler");
	if (taskType == null \|\| taskFactoryType == null \|\| taskSchedulerType == null)
	{
	return;
	}

	compilationContext.RegisterOperationAction(operationContext =>
	{

	using VerifyCS = Test.Utilities.CSharpSecurityCodeFixVerifier<
	Microsoft.NetCore.Analyzers.Runtime.TestForNaNCorrectlyAnalyzer,
	Microsoft.NetCore.CSharp.Analyzers.Runtime.CSharpTestForNaNCorrectlyFixer>;
	using VerifyVB = Test.Utilities.VisualBasicSecurityCodeFixVerifier<
	Microsoft.NetCore.Analyzers.Runtime.TestForNaNCorrectlyAnalyzer,
	Microsoft.NetCore.VisualBasic.Analyzers.Runtime.BasicTestForNaNCorrectlyFixer>;

	namespace Microsoft.NetCore.Analyzers.Runtime.UnitTests
	{
	public class TestForNaNCorrectlyFixerTests
	{
	[Fact]
	public async Task CA2242_FixFloatForEqualityWithFloatNaN()
	{
	await VerifyCS.VerifyCodeFixAsync(@"
	public class A
	{
	public bool Compare(float f)
	{
	return [\|f == float.NaN\|];
	}
	}
	", @"
	public class A
	{
	public bool Compare(float f)
	{
	return float.IsNaN(f);
	}
	}
	");

	await VerifyVB.VerifyCodeFixAsync(@"
	Public Class A
	Public Function Compare(s As Single) As Boolean
	Return [\|s = Single.NaN\|]
	End Function
	End Class
	", @"
	Public Class A
	Public Function Compare(s As Single) As Boolean
	Return Single.IsNaN(s)
	End Function
	End Class
	");


		protected abstract void AnalyzeCodeFix(CodeFixContext context, SyntaxNode targetNode);

		public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(MutableStructsShouldNotBeUsedForReadonlyFieldsAnalyzer.RuleId);


		public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(MutableStructsShouldNotBeUsedForReadonlyFieldsAnalyzer.RuleId);

		public override FixAllProvider GetFixAllProvider()

[WIP] Add rule for detecting certain readonly mutable structs #2831

Are you sure you want to change the base?

[WIP] Add rule for detecting certain readonly mutable structs #2831

Conversation

mareklinka commented Sep 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani Sep 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mareklinka Sep 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani Sep 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani Sep 8, 2019 • edited Loading

Choose a reason for hiding this comment

mavasani Sep 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell left a comment • edited Loading

Choose a reason for hiding this comment

Evangelink commented Nov 27, 2019

Evangelink commented Mar 20, 2020

mareklinka commented Mar 21, 2020

Evangelink commented Mar 23, 2020

sharwell commented Mar 31, 2020

sharwell commented May 20, 2021

mareklinka commented Sep 8, 2019 •

edited

Loading

mavasani Sep 8, 2019 •

edited

Loading

mareklinka Sep 8, 2019 •

edited

Loading

mavasani Sep 8, 2019 •

edited

Loading

mavasani Sep 8, 2019 •

edited

Loading

mavasani Sep 8, 2019 •

edited

Loading

sharwell left a comment •

edited

Loading