Skip to content

Commit 21b4b7c

Browse files
sharwellsharwell
committed
Move language-specific functionality to derived types
1 parent ff4091f commit 21b4b7c

File tree

5 files changed

+157
-73
lines changed

5 files changed

+157
-73
lines changed

‎src/Roslyn.Diagnostics.Analyzers/CSharp/CSharpDoNotCopyValue.cs

+49
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
2+
3+
using System.Diagnostics.CodeAnalysis;
4+
using Microsoft.CodeAnalysis;
5+
using Microsoft.CodeAnalysis.CSharp;
6+
using Microsoft.CodeAnalysis.CSharp.Syntax;
7+
using Microsoft.CodeAnalysis.Diagnostics;
8+
using Microsoft.CodeAnalysis.Operations;
9+
using Roslyn.Diagnostics.Analyzers;
10+
11+
namespace Roslyn.Diagnostics.CSharp.Analyzers
12+
{
13+
[DiagnosticAnalyzer(LanguageNames.CSharp)]
14+
public sealed class CSharpDoNotCopyValue : AbstractDoNotCopyValue
15+
{
16+
protected override NonCopyableWalker CreateWalker(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
17+
=> new CSharpNonCopyableWalker(context, cache);
18+
19+
private sealed class CSharpNonCopyableWalker : NonCopyableWalker
20+
{
21+
public CSharpNonCopyableWalker(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
22+
: base(context, cache)
23+
{
24+
}
25+
26+
protected override bool CheckForEachGetEnumerator(IForEachLoopOperation operation, [DisallowNull] ref IConversionOperation? conversion, [DisallowNull] ref IOperation? instance)
27+
{
28+
if (operation.Syntax is CommonForEachStatementSyntax syntax
29+
&& operation.SemanticModel.GetForEachStatementInfo(syntax).GetEnumeratorMethod is { } getEnumeratorMethod)
30+
{
31+
CheckMethodSymbolInUnsupportedContext(operation, getEnumeratorMethod);
32+
33+
if (instance is not null
34+
&& Cache.IsNonCopyableType(getEnumeratorMethod.ReceiverType)
35+
&& !getEnumeratorMethod.IsReadOnly
36+
&& Acquire(instance) == RefKind.In)
37+
{
38+
// mark the instance as not checked by this method
39+
instance = null;
40+
}
41+
42+
return true;
43+
}
44+
45+
return false;
46+
}
47+
}
48+
}
49+
}

‎src/Roslyn.Diagnostics.Analyzers/Core/DoNotCopyValue.cs ‎src/Roslyn.Diagnostics.Analyzers/Core/AbstractDoNotCopyValue.cs

+23-35
Original file line numberDiff line numberDiff line change
@@ -9,16 +9,13 @@
99
using Analyzer.Utilities;
1010
using Analyzer.Utilities.Extensions;
1111
using Microsoft.CodeAnalysis;
12-
using Microsoft.CodeAnalysis.CSharp;
13-
using Microsoft.CodeAnalysis.CSharp.Syntax;
1412
using Microsoft.CodeAnalysis.Diagnostics;
1513
using Microsoft.CodeAnalysis.FlowAnalysis;
1614
using Microsoft.CodeAnalysis.Operations;
1715

1816
namespace Roslyn.Diagnostics.Analyzers
1917
{
20-
[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
21-
public sealed class DoNotCopyValue : DiagnosticAnalyzer
18+
public abstract class AbstractDoNotCopyValue : DiagnosticAnalyzer
2219
{
2320
private static readonly LocalizableString s_localizableTitle = new LocalizableResourceString(nameof(RoslynDiagnosticsAnalyzersResources.DoNotCopyValueTitle), RoslynDiagnosticsAnalyzersResources.ResourceManager, typeof(RoslynDiagnosticsAnalyzersResources));
2421
private static readonly LocalizableString s_localizableMessage = new LocalizableResourceString(nameof(RoslynDiagnosticsAnalyzersResources.DoNotCopyValueMessage), RoslynDiagnosticsAnalyzersResources.ResourceManager, typeof(RoslynDiagnosticsAnalyzersResources));
@@ -93,6 +90,8 @@ public sealed class DoNotCopyValue : DiagnosticAnalyzer
9390

9491
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule, UnsupportedUseRule, NoBoxingRule, NoUnboxingRule);
9592

93+
protected abstract NonCopyableWalker CreateWalker(OperationBlockAnalysisContext context, NonCopyableTypesCache cache);
94+
9695
public override void Initialize(AnalysisContext context)
9796
{
9897
context.EnableConcurrentExecution();
@@ -105,9 +104,9 @@ public override void Initialize(AnalysisContext context)
105104
});
106105
}
107106

108-
private static void AnalyzeOperationBlock(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
107+
private void AnalyzeOperationBlock(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
109108
{
110-
var walker = new NonCopyableWalker(context, cache);
109+
var walker = CreateWalker(context, cache);
111110
foreach (var operation in context.OperationBlocks)
112111
{
113112
walker.Visit(operation);
@@ -149,18 +148,21 @@ public void Dispose()
149148
}
150149
}
151150

152-
private sealed class NonCopyableWalker : OperationWalker
151+
protected abstract class NonCopyableWalker : OperationWalker
153152
{
154153
private readonly OperationBlockAnalysisContext _context;
155-
private readonly NonCopyableTypesCache _cache;
156154
private readonly HashSet<IOperation> _handledOperations = new HashSet<IOperation>();
157155

158-
public NonCopyableWalker(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
156+
protected NonCopyableWalker(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
159157
{
160158
_context = context;
161-
_cache = cache;
159+
Cache = cache;
162160
}
163161

162+
protected NonCopyableTypesCache Cache { get; }
163+
164+
protected abstract bool CheckForEachGetEnumerator(IForEachLoopOperation operation, [DisallowNull] ref IConversionOperation? conversion, [DisallowNull] ref IOperation? instance);
165+
164166
public override void VisitAddressOf(IAddressOfOperation operation)
165167
{
166168
CheckTypeInUnsupportedContext(operation);
@@ -222,11 +224,11 @@ public override void VisitAwait(IAwaitOperation operation)
222224
{
223225
// Treat await of ValueTask<T> the same way handling of a return
224226
if (operation.Type is { } type
225-
&& _cache.IsNonCopyableType(type)
227+
&& Cache.IsNonCopyableType(type)
226228
&& operation.Operation.Type is INamedTypeSymbol { OriginalDefinition: var taskType })
227229
{
228-
if (!SymbolEqualityComparer.Default.Equals(taskType, _cache.ValueTaskT)
229-
&& !SymbolEqualityComparer.Default.Equals(taskType, _cache.ConfiguredValueTaskAwaitableT))
230+
if (!SymbolEqualityComparer.Default.Equals(taskType, Cache.ValueTaskT)
231+
&& !SymbolEqualityComparer.Default.Equals(taskType, Cache.ConfiguredValueTaskAwaitableT))
230232
{
231233
CheckTypeInUnsupportedContext(operation);
232234
}
@@ -566,21 +568,7 @@ public override void VisitForEachLoop(IForEachLoopOperation operation)
566568
else
567569
{
568570
// Treat this as an invocation of the GetEnumerator method.
569-
if (operation.Syntax is CommonForEachStatementSyntax syntax
570-
&& operation.SemanticModel.GetForEachStatementInfo(syntax).GetEnumeratorMethod is { } getEnumeratorMethod)
571-
{
572-
CheckMethodSymbolInUnsupportedContext(operation, getEnumeratorMethod);
573-
574-
if (instance2 is not null
575-
&& _cache.IsNonCopyableType(getEnumeratorMethod.ReceiverType)
576-
&& !getEnumeratorMethod.IsReadOnly
577-
&& Acquire(instance) == RefKind.In)
578-
{
579-
// mark the instance as not checked by this method
580-
instance2 = null;
581-
}
582-
}
583-
else
571+
if (!CheckForEachGetEnumerator(operation, ref instance, ref instance2))
584572
{
585573
// Not supported
586574
instance = null;
@@ -669,7 +657,7 @@ public override void VisitInvocation(IInvocationOperation operation)
669657

670658
var instance = operation.Instance;
671659
if (instance is object
672-
&& _cache.IsNonCopyableType(operation.TargetMethod.ReceiverType)
660+
&& Cache.IsNonCopyableType(operation.TargetMethod.ReceiverType)
673661
&& !operation.TargetMethod.IsReadOnly
674662
&& Acquire(instance) == RefKind.In)
675663
{
@@ -837,7 +825,7 @@ public override void VisitPropertyReference(IPropertyReferenceOperation operatio
837825

838826
var instance = operation.Instance;
839827
if (instance is object
840-
&& _cache.IsNonCopyableType(operation.Property.ContainingType)
828+
&& Cache.IsNonCopyableType(operation.Property.ContainingType)
841829
&& Acquire(instance) == RefKind.In)
842830
{
843831
if (operation.IsSetMethodInvocation())
@@ -1144,7 +1132,7 @@ private static bool CanAssign(RefKind sourceRefKind, RefKind targetRefKind)
11441132
};
11451133
}
11461134

1147-
private RefKind Acquire(IOperation? operation)
1135+
protected RefKind Acquire(IOperation? operation)
11481136
{
11491137
if (operation is null)
11501138
return RefKind.RefReadOnly;
@@ -1338,7 +1326,7 @@ private void CheckTypeSymbolInUnsupportedContext(IOperation operation, ITypeSymb
13381326
if (type.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T)
13391327
{
13401328
var nullableUnderlyingType = ((INamedTypeSymbol)type).TypeArguments.FirstOrDefault();
1341-
if (_cache.IsNonCopyableType(nullableUnderlyingType))
1329+
if (Cache.IsNonCopyableType(nullableUnderlyingType))
13421330
{
13431331
_context.ReportDiagnostic(operation.Syntax.CreateDiagnostic(AvoidNullableWrapperRule, type, operation.Kind));
13441332
}
@@ -1380,7 +1368,7 @@ private void CheckLocalSymbolInUnsupportedContext(IOperation operation, ILocalSy
13801368
CheckTypeSymbolInUnsupportedContext(operation, local.Type);
13811369
}
13821370

1383-
private void CheckMethodSymbolInUnsupportedContext(IOperation operation, IMethodSymbol? symbol)
1371+
protected void CheckMethodSymbolInUnsupportedContext(IOperation operation, IMethodSymbol? symbol)
13841372
{
13851373
if (symbol is null)
13861374
return;
@@ -1424,7 +1412,7 @@ private void CheckTypeInUnsupportedContext(IOperation operation)
14241412
return;
14251413
}
14261414

1427-
if (!_cache.IsNonCopyableType(symbol))
1415+
if (!Cache.IsNonCopyableType(symbol))
14281416
{
14291417
// Copies of this type are allowed
14301418
return;
@@ -1434,7 +1422,7 @@ private void CheckTypeInUnsupportedContext(IOperation operation)
14341422
}
14351423
}
14361424

1437-
private sealed class NonCopyableTypesCache
1425+
protected sealed class NonCopyableTypesCache
14381426
{
14391427
private readonly ConcurrentDictionary<INamedTypeSymbol, bool> _typesToNonCopyable
14401428
= new ConcurrentDictionary<INamedTypeSymbol, bool>();

‎src/Roslyn.Diagnostics.Analyzers/Roslyn.Diagnostics.Analyzers.sarif

+34-18
Original file line numberDiff line numberDiff line change
@@ -134,24 +134,6 @@
134134
]
135135
}
136136
},
137-
"RS0042": {
138-
"id": "RS0042",
139-
"shortDescription": "Do not copy value",
140-
"fullDescription": "Do not unbox non-copyable value types.",
141-
"defaultLevel": "warning",
142-
"properties": {
143-
"category": "RoslynDiagnosticsReliability",
144-
"isEnabledByDefault": true,
145-
"typeName": "DoNotCopyValue",
146-
"languages": [
147-
"C#",
148-
"Visual Basic"
149-
],
150-
"tags": [
151-
"Telemetry"
152-
]
153-
}
154-
},
155137
"RS0043": {
156138
"id": "RS0043",
157139
"shortDescription": "Do not call 'GetTestAccessor()'",
@@ -248,6 +230,23 @@
248230
]
249231
}
250232
},
233+
"RS0042": {
234+
"id": "RS0042",
235+
"shortDescription": "Do not copy value",
236+
"fullDescription": "Do not unbox non-copyable value types.",
237+
"defaultLevel": "warning",
238+
"properties": {
239+
"category": "RoslynDiagnosticsReliability",
240+
"isEnabledByDefault": true,
241+
"typeName": "CSharpDoNotCopyValue",
242+
"languages": [
243+
"C#"
244+
],
245+
"tags": [
246+
"Telemetry"
247+
]
248+
}
249+
},
251250
"RS0046": {
252251
"id": "RS0046",
253252
"shortDescription": "Avoid the 'Opt' suffix",
@@ -394,6 +393,23 @@
394393
]
395394
}
396395
},
396+
"RS0042": {
397+
"id": "RS0042",
398+
"shortDescription": "Do not copy value",
399+
"fullDescription": "Do not unbox non-copyable value types.",
400+
"defaultLevel": "warning",
401+
"properties": {
402+
"category": "RoslynDiagnosticsReliability",
403+
"isEnabledByDefault": true,
404+
"typeName": "VisualBasicDoNotCopyValue",
405+
"languages": [
406+
"Visual Basic"
407+
],
408+
"tags": [
409+
"Telemetry"
410+
]
411+
}
412+
},
397413
"RS0101": {
398414
"id": "RS0101",
399415
"shortDescription": "Avoid multiple blank lines",

0 commit comments

Comments
 (0)