Skip to content

Commit 7ce3b00

Browse files
Merge pull request #74170 from CyrusNajmabadi/useLock
2 parents 9457049 + 37af8ff commit 7ce3b00

File tree

58 files changed

+2274
-23
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

58 files changed

+2274
-23
lines changed

eng/Directory.Packages.props

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
<!-- Versions used by several individual references below -->
44
<RoslynDiagnosticsNugetPackageVersion>3.11.0-beta1.24081.1</RoslynDiagnosticsNugetPackageVersion>
55
<MicrosoftCodeAnalysisNetAnalyzersVersion>8.0.0-preview.23468.1</MicrosoftCodeAnalysisNetAnalyzersVersion>
6-
<MicrosoftCodeAnalysisTestingVersion>1.1.2-beta1.24121.1</MicrosoftCodeAnalysisTestingVersion>
6+
<MicrosoftCodeAnalysisTestingVersion>1.1.3-beta1.24319.1</MicrosoftCodeAnalysisTestingVersion>
77
<MicrosoftVisualStudioExtensibilityTestingVersion>0.1.187-beta</MicrosoftVisualStudioExtensibilityTestingVersion>
88
<_BasicReferenceAssembliesVersion>1.7.2</_BasicReferenceAssembliesVersion>
99
<!-- CodeStyleAnalyzerVersion should we updated together with version of dotnet-format in dotnet-tools.json -->

src/Analyzers/CSharp/Analyzers/CSharpAnalyzers.projitems

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,7 @@
136136
<Compile Include="$(MSBuildThisFileDirectory)UseIsNullCheck\CSharpUseNullCheckOverTypeCheckDiagnosticAnalyzer.cs" />
137137
<Compile Include="$(MSBuildThisFileDirectory)UseIsNullCheck\CSharpUseIsNullCheckForCastAndEqualityOperatorDiagnosticAnalyzer.cs" />
138138
<Compile Include="$(MSBuildThisFileDirectory)UseIsNullCheck\CSharpUseIsNullCheckForReferenceEqualsDiagnosticAnalyzer.cs" />
139+
<Compile Include="$(MSBuildThisFileDirectory)UseSystemThreadingLock\CSharpUseSystemThreadingLockDiagnosticAnalyzer.cs" />
139140
<Compile Include="$(MSBuildThisFileDirectory)UseNameofInNullableAttribute\CSharpUseNameofInNullableAttributeDiagnosticAnalyzer.cs" />
140141
<Compile Include="$(MSBuildThisFileDirectory)UseObjectInitializer\CSharpUseNamedMemberInitializerAnalyzer.cs" />
141142
<Compile Include="$(MSBuildThisFileDirectory)UsePatternMatching\CSharpAsAndMemberAccessDiagnosticAnalyzer.cs" />

src/Analyzers/CSharp/Analyzers/CSharpAnalyzersResources.resx

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -418,4 +418,8 @@
418418
<data name="Make_anonymous_function_static" xml:space="preserve">
419419
<value>Make anonymous function static</value>
420420
</data>
421+
<data name="Use_System_Threading_Lock" xml:space="preserve">
422+
<value>Use 'System.Threading.Lock'</value>
423+
<comment>{Locked="System.Threading.Lock"} "System.Threading.Lock" is the name of a .Net type and should not be localized.</comment>
424+
</data>
421425
</root>

src/Analyzers/CSharp/Analyzers/CodeStyle/CSharpAnalyzerOptionsProvider.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ internal CSharpSimplifierOptions GetSimplifierOptions()
8282
public CodeStyleOption2<UnusedValuePreference> UnusedValueAssignment => GetOption(CSharpCodeStyleOptions.UnusedValueAssignment, FallbackCodeStyleOptions.UnusedValueAssignment);
8383
public CodeStyleOption2<bool> PreferMethodGroupConversion => GetOption(CSharpCodeStyleOptions.PreferMethodGroupConversion, FallbackCodeStyleOptions.PreferMethodGroupConversion);
8484
public CodeStyleOption2<bool> PreferPrimaryConstructors => GetOption(CSharpCodeStyleOptions.PreferPrimaryConstructors, FallbackCodeStyleOptions.PreferPrimaryConstructors);
85+
public CodeStyleOption2<bool> PreferSystemThreadingLock => GetOption(CSharpCodeStyleOptions.PreferSystemThreadingLock, FallbackCodeStyleOptions.PreferSystemThreadingLock);
8586

8687
// CodeGenerationOptions
8788

Lines changed: 249 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,249 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System.Linq;
6+
using System.Runtime.CompilerServices;
7+
using Microsoft.CodeAnalysis.CodeStyle;
8+
using Microsoft.CodeAnalysis.Collections;
9+
using Microsoft.CodeAnalysis.CSharp.CodeStyle;
10+
using Microsoft.CodeAnalysis.CSharp.Extensions;
11+
using Microsoft.CodeAnalysis.CSharp.Shared.Extensions;
12+
using Microsoft.CodeAnalysis.CSharp.Syntax;
13+
using Microsoft.CodeAnalysis.Diagnostics;
14+
using Microsoft.CodeAnalysis.Operations;
15+
using Microsoft.CodeAnalysis.PooledObjects;
16+
using Microsoft.CodeAnalysis.Shared.Collections;
17+
using Microsoft.CodeAnalysis.Shared.Extensions;
18+
using Roslyn.Utilities;
19+
20+
namespace Microsoft.CodeAnalysis.CSharp.UseSystemThreadingLock;
21+
22+
using static SegmentedCollectionsMarshal;
23+
24+
/// <summary>
25+
/// Looks for code of the form:
26+
///
27+
/// private ... object _gate = new object();
28+
/// ...
29+
/// lock (_gate)
30+
/// {
31+
/// }
32+
///
33+
/// and converts it to:
34+
///
35+
/// private ... Lock _gate = new Lock();
36+
/// </summary>
37+
[DiagnosticAnalyzer(LanguageNames.CSharp)]
38+
internal sealed class CSharpUseSystemThreadingLockDiagnosticAnalyzer()
39+
: AbstractBuiltInCodeStyleDiagnosticAnalyzer(
40+
IDEDiagnosticIds.UseSystemThreadingLockDiagnosticId,
41+
EnforceOnBuildValues.UseSystemThreadingLock,
42+
CSharpCodeStyleOptions.PreferSystemThreadingLock,
43+
new LocalizableResourceString(
44+
nameof(CSharpAnalyzersResources.Use_System_Threading_Lock), CSharpAnalyzersResources.ResourceManager, typeof(CSharpAnalyzersResources)))
45+
{
46+
/// <summary>
47+
/// A method body edit anywhere in a type will force us to reanalyze the whole type.
48+
/// </summary>
49+
/// <returns></returns>
50+
public override DiagnosticAnalyzerCategory GetAnalyzerCategory()
51+
=> DiagnosticAnalyzerCategory.SemanticDocumentAnalysis;
52+
53+
protected override void InitializeWorker(AnalysisContext context)
54+
{
55+
context.RegisterCompilationStartAction(compilationContext =>
56+
{
57+
var compilation = compilationContext.Compilation;
58+
59+
// The new 'Lock' feature is only supported in C# 13 and above, and only if we actually have a definition of
60+
// System.Threading.Lock available.
61+
if (!compilation.LanguageVersion().IsCSharp13OrAbove())
62+
return;
63+
64+
var lockType = compilation.GetBestTypeByMetadataName("System.Threading.Lock");
65+
if (lockType is not { DeclaredAccessibility: Accessibility.Public })
66+
return;
67+
68+
if (lockType.GetTypeMembers("Scope").FirstOrDefault() is not { TypeKind: TypeKind.Struct, IsRefLikeType: true, DeclaredAccessibility: Accessibility.Public })
69+
return;
70+
71+
context.RegisterSymbolStartAction(AnalyzeNamedType, SymbolKind.NamedType);
72+
});
73+
}
74+
75+
private void AnalyzeNamedType(SymbolStartAnalysisContext context)
76+
{
77+
var cancellationToken = context.CancellationToken;
78+
79+
var namedType = (INamedTypeSymbol)context.Symbol;
80+
if (namedType is not { TypeKind: TypeKind.Class or TypeKind.Struct })
81+
return;
82+
83+
SyntaxTree? currentSyntaxTree = null;
84+
CodeStyleOption2<bool>? currentOption = null;
85+
86+
// Needs to have a private field that is exactly typed as 'object'. This way we can analyze all usages of it to
87+
// be sure it's completely safe to move to the new lock type.
88+
using var fieldsArray = TemporaryArray<(IFieldSymbol field, VariableDeclaratorSyntax declarator, CodeStyleOption2<bool> option)>.Empty;
89+
90+
foreach (var member in namedType.GetMembers())
91+
{
92+
if (member is not IFieldSymbol
93+
{
94+
Type.SpecialType: SpecialType.System_Object,
95+
DeclaredAccessibility: Accessibility.Private,
96+
DeclaringSyntaxReferences: [var fieldSyntaxReference],
97+
} field)
98+
{
99+
continue;
100+
}
101+
102+
var fieldSyntaxTree = fieldSyntaxReference.SyntaxTree;
103+
if (fieldSyntaxTree != currentSyntaxTree)
104+
{
105+
currentSyntaxTree = fieldSyntaxTree;
106+
107+
currentOption = context.GetCSharpAnalyzerOptions(currentSyntaxTree).PreferSystemThreadingLock;
108+
109+
// Ignore this field if it is is in a file that should be skipped.
110+
if (!currentOption.Value || ShouldSkipAnalysis(currentSyntaxTree, context.Options, context.Compilation.Options, currentOption.Notification, cancellationToken))
111+
continue;
112+
}
113+
114+
if (fieldSyntaxReference.GetSyntax(cancellationToken) is not VariableDeclaratorSyntax variableDeclarator)
115+
continue;
116+
117+
// For simplicity, only offer this for fields with a single declarator.
118+
if (variableDeclarator.Parent is not VariableDeclarationSyntax { Parent: FieldDeclarationSyntax, Variables.Count: 1 })
119+
return;
120+
121+
// Looks like something that could be converted to a System.Threading.Lock if we see that this field is used
122+
// in a compatible fashion.
123+
fieldsArray.Add((field, variableDeclarator, currentOption!));
124+
}
125+
126+
if (fieldsArray.Count == 0)
127+
return;
128+
129+
// The set of fields we think could be converted to `System.Threading.Lock` from `object`.
130+
//
131+
// 'wasLocked' tracks whether or not we saw this field used in a `lock (obj)` statement. If not, we do not want
132+
// to convert this as the user wasn't using this as a lock. Note: we can consider expanding the set of patterns
133+
// we detect (like Monitor.Enter + Monitor.Exit) if we think it's worthwhile.
134+
//
135+
// Note: both this dictionary is written to concurrently in the analysis pass below. This is safe as we never
136+
// are actually mutating the dictionary itself (we're not adding/removing items). We're just getting a
137+
// reference to its tuple value and overwriting a bool in place within that tuple with the new value. And we
138+
// only move hte value in one direction. For example 'canUse' only moved from 'true' to 'false' and 'wasLocked'
139+
// only moves the value from 'false' to 'true'.
140+
var potentialLockFields = new SegmentedDictionary<
141+
IFieldSymbol,
142+
(VariableDeclaratorSyntax declarator, CodeStyleOption2<bool> option, bool canUse, bool wasLocked)>(capacity: fieldsArray.Count);
143+
144+
foreach (var (field, declarator, option) in fieldsArray)
145+
potentialLockFields[field] = (declarator, option, canUse: true, wasLocked: false);
146+
147+
// Register a callback to ensure the field's initializer is either missing, or is only instantiating the field
148+
// with a new object.
149+
context.RegisterOperationAction(context =>
150+
{
151+
var cancellationToken = context.CancellationToken;
152+
if (context.ContainingSymbol is not IFieldSymbol fieldSymbol)
153+
return;
154+
155+
ref var valueRef = ref GetValueRefOrNullRef(potentialLockFields, fieldSymbol);
156+
if (Unsafe.IsNullRef(ref valueRef))
157+
return;
158+
159+
var fieldInitializer = (IFieldInitializerOperation)context.Operation;
160+
if (fieldInitializer.Value is null)
161+
return;
162+
163+
if (!IsObjectCreationOperation(fieldInitializer.Value))
164+
valueRef.canUse = false;
165+
}, OperationKind.FieldInitializer);
166+
167+
// Now go see how the code within this named type actually uses any fields within.
168+
context.RegisterOperationAction(context =>
169+
{
170+
var cancellationToken = context.CancellationToken;
171+
var fieldReferenceOperation = (IFieldReferenceOperation)context.Operation;
172+
var fieldReference = fieldReferenceOperation.Field.OriginalDefinition;
173+
174+
// We only care about examining field references to the fields we're considering converting to System.Threading.Lock.
175+
ref var valueRef = ref GetValueRefOrNullRef(potentialLockFields, fieldReference);
176+
if (Unsafe.IsNullRef(ref valueRef))
177+
return;
178+
179+
// If some other analysis already determined we can't convert this field, then no point continuing.
180+
if (!valueRef.canUse)
181+
return;
182+
183+
if (fieldReferenceOperation.Parent is ILockOperation lockOperation)
184+
{
185+
// Locking on the the new lock type disallows yielding inside the lock. So if we see that, immediately
186+
// consider this not applicable.
187+
if (lockOperation.Syntax.ContainsYield())
188+
{
189+
valueRef.canUse = false;
190+
return;
191+
}
192+
193+
// We did lock on this field, mark as such as its now something we'd def like to convert to a
194+
// System.Threading.Lock if possible.
195+
valueRef.wasLocked = true;
196+
return;
197+
}
198+
199+
// It's ok to assign to the field, as long as we're assigning a new lock object to it. e.g. `_gate = new
200+
// object()` is fine to continue converting over to System.Threading.Lock. But an assignment of something
201+
// else is not.
202+
if (fieldReferenceOperation.Parent is IAssignmentOperation assignment &&
203+
assignment.Target == fieldReferenceOperation &&
204+
IsObjectCreationOperation(assignment.Value))
205+
{
206+
return;
207+
}
208+
209+
// Fine to use `nameof(someLock)` as that's not actually using the lock.
210+
if (fieldReferenceOperation.Parent is INameOfOperation)
211+
return;
212+
213+
// Note: More patterns can be added here as needed.
214+
215+
// This wasn't a supported case. Immediately disallow conversion of this field.
216+
valueRef.canUse = false;
217+
}, OperationKind.FieldReference);
218+
219+
// Finally, once we're done analyzing the symbol body, report diagnostics for any fields that we determined are
220+
// locks and can be safely converted.
221+
context.RegisterSymbolEndAction(context =>
222+
{
223+
var cancellationToken = context.CancellationToken;
224+
225+
foreach (var (lockField, (declarator, option, canUse, wasLocked)) in potentialLockFields)
226+
{
227+
// If we blocked this field in our analysis pass, can immediately skip.
228+
if (!canUse)
229+
continue;
230+
231+
// Has to at least see this field locked on to offer to convert it to a Lock.
232+
if (!wasLocked)
233+
continue;
234+
235+
context.ReportDiagnostic(DiagnosticHelper.Create(
236+
Descriptor,
237+
declarator.Identifier.GetLocation(),
238+
option.Notification,
239+
context.Options,
240+
additionalLocations: null,
241+
properties: null));
242+
}
243+
});
244+
}
245+
246+
private static bool IsObjectCreationOperation(IOperation value)
247+
// unwrap the implicit conversion around `new()` if necessary.
248+
=> value.UnwrapImplicitConversion() is IObjectCreationOperation { Type.SpecialType: SpecialType.System_Object };
249+
}

src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.cs.xlf

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.de.xlf

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.es.xlf

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.fr.xlf

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.it.xlf

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)