Skip to content

Commit 129a02d

Browse files
sharwellsharwell
authored
Merge pull request #4374 from sharwell/foreach-readonly
Support foreach over parameters in DoNotCopyValue
2 parents a235f3f + 21b4b7c commit 129a02d

File tree

5 files changed

+228
-60
lines changed

5 files changed

+228
-60
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

+42-23
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,7 @@
1515

1616
namespace Roslyn.Diagnostics.Analyzers
1717
{
18-
[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
19-
public sealed class DoNotCopyValue : DiagnosticAnalyzer
18+
public abstract class AbstractDoNotCopyValue : DiagnosticAnalyzer
2019
{
2120
private static readonly LocalizableString s_localizableTitle = new LocalizableResourceString(nameof(RoslynDiagnosticsAnalyzersResources.DoNotCopyValueTitle), RoslynDiagnosticsAnalyzersResources.ResourceManager, typeof(RoslynDiagnosticsAnalyzersResources));
2221
private static readonly LocalizableString s_localizableMessage = new LocalizableResourceString(nameof(RoslynDiagnosticsAnalyzersResources.DoNotCopyValueMessage), RoslynDiagnosticsAnalyzersResources.ResourceManager, typeof(RoslynDiagnosticsAnalyzersResources));
@@ -91,6 +90,8 @@ public sealed class DoNotCopyValue : DiagnosticAnalyzer
9190

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

93+
protected abstract NonCopyableWalker CreateWalker(OperationBlockAnalysisContext context, NonCopyableTypesCache cache);
94+
9495
public override void Initialize(AnalysisContext context)
9596
{
9697
context.EnableConcurrentExecution();
@@ -103,9 +104,9 @@ public override void Initialize(AnalysisContext context)
103104
});
104105
}
105106

106-
private static void AnalyzeOperationBlock(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
107+
private void AnalyzeOperationBlock(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
107108
{
108-
var walker = new NonCopyableWalker(context, cache);
109+
var walker = CreateWalker(context, cache);
109110
foreach (var operation in context.OperationBlocks)
110111
{
111112
walker.Visit(operation);
@@ -147,18 +148,21 @@ public void Dispose()
147148
}
148149
}
149150

150-
private sealed class NonCopyableWalker : OperationWalker
151+
protected abstract class NonCopyableWalker : OperationWalker
151152
{
152153
private readonly OperationBlockAnalysisContext _context;
153-
private readonly NonCopyableTypesCache _cache;
154154
private readonly HashSet<IOperation> _handledOperations = new HashSet<IOperation>();
155155

156-
public NonCopyableWalker(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
156+
protected NonCopyableWalker(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
157157
{
158158
_context = context;
159-
_cache = cache;
159+
Cache = cache;
160160
}
161161

162+
protected NonCopyableTypesCache Cache { get; }
163+
164+
protected abstract bool CheckForEachGetEnumerator(IForEachLoopOperation operation, [DisallowNull] ref IConversionOperation? conversion, [DisallowNull] ref IOperation? instance);
165+
162166
public override void VisitAddressOf(IAddressOfOperation operation)
163167
{
164168
CheckTypeInUnsupportedContext(operation);
@@ -220,11 +224,11 @@ public override void VisitAwait(IAwaitOperation operation)
220224
{
221225
// Treat await of ValueTask<T> the same way handling of a return
222226
if (operation.Type is { } type
223-
&& _cache.IsNonCopyableType(type)
227+
&& Cache.IsNonCopyableType(type)
224228
&& operation.Operation.Type is INamedTypeSymbol { OriginalDefinition: var taskType })
225229
{
226-
if (!SymbolEqualityComparer.Default.Equals(taskType, _cache.ValueTaskT)
227-
&& !SymbolEqualityComparer.Default.Equals(taskType, _cache.ConfiguredValueTaskAwaitableT))
230+
if (!SymbolEqualityComparer.Default.Equals(taskType, Cache.ValueTaskT)
231+
&& !SymbolEqualityComparer.Default.Equals(taskType, Cache.ConfiguredValueTaskAwaitableT))
228232
{
229233
CheckTypeInUnsupportedContext(operation);
230234
}
@@ -367,7 +371,7 @@ bool IsSupportedConversion()
367371
switch (Acquire(operation.Operand))
368372
{
369373
case RefKind.None:
370-
case RefKind.Ref when operation.Conversion.IsIdentity:
374+
case RefKind.Ref or RefKind.RefReadOnly when operation.Conversion.IsIdentity:
371375
return true;
372376

373377
default:
@@ -545,17 +549,32 @@ public override void VisitForEachLoop(IForEachLoopOperation operation)
545549
CheckLocalSymbolInUnsupportedContext(operation, local);
546550
}
547551

548-
var instance = operation.Collection;
552+
// 'foreach' operations have an identity conversion for the collection property, and then invoke the
553+
// GetEnumerator method.
554+
var instance = operation.Collection as IConversionOperation;
549555
var instance2 = (operation.Collection as IConversionOperation)?.Operand;
550-
if (Acquire(operation.Collection) != RefKind.Ref)
556+
557+
if (instance2 is null)
551558
{
559+
// Didn't match the known pattern
552560
instance = null;
553-
instance2 = null;
554561
}
555-
else if (Acquire(instance2) != RefKind.Ref)
562+
else if (instance?.Conversion is not { IsIdentity: true, MethodSymbol: null })
556563
{
564+
// Not a supported conversion
565+
instance = null;
557566
instance2 = null;
558567
}
568+
else
569+
{
570+
// Treat this as an invocation of the GetEnumerator method.
571+
if (!CheckForEachGetEnumerator(operation, ref instance, ref instance2))
572+
{
573+
// Not supported
574+
instance = null;
575+
instance2 = null;
576+
}
577+
}
559578

560579
using var releaser = TryAddForVisit(_handledOperations, instance, out _);
561580
using var releaser2 = TryAddForVisit(_handledOperations, instance2, out _);
@@ -638,7 +657,7 @@ public override void VisitInvocation(IInvocationOperation operation)
638657

639658
var instance = operation.Instance;
640659
if (instance is object
641-
&& _cache.IsNonCopyableType(operation.TargetMethod.ReceiverType)
660+
&& Cache.IsNonCopyableType(operation.TargetMethod.ReceiverType)
642661
&& !operation.TargetMethod.IsReadOnly
643662
&& Acquire(instance) == RefKind.In)
644663
{
@@ -806,7 +825,7 @@ public override void VisitPropertyReference(IPropertyReferenceOperation operatio
806825

807826
var instance = operation.Instance;
808827
if (instance is object
809-
&& _cache.IsNonCopyableType(operation.Property.ContainingType)
828+
&& Cache.IsNonCopyableType(operation.Property.ContainingType)
810829
&& Acquire(instance) == RefKind.In)
811830
{
812831
if (operation.IsSetMethodInvocation())
@@ -1113,7 +1132,7 @@ private static bool CanAssign(RefKind sourceRefKind, RefKind targetRefKind)
11131132
};
11141133
}
11151134

1116-
private RefKind Acquire(IOperation? operation)
1135+
protected RefKind Acquire(IOperation? operation)
11171136
{
11181137
if (operation is null)
11191138
return RefKind.RefReadOnly;
@@ -1307,7 +1326,7 @@ private void CheckTypeSymbolInUnsupportedContext(IOperation operation, ITypeSymb
13071326
if (type.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T)
13081327
{
13091328
var nullableUnderlyingType = ((INamedTypeSymbol)type).TypeArguments.FirstOrDefault();
1310-
if (_cache.IsNonCopyableType(nullableUnderlyingType))
1329+
if (Cache.IsNonCopyableType(nullableUnderlyingType))
13111330
{
13121331
_context.ReportDiagnostic(operation.Syntax.CreateDiagnostic(AvoidNullableWrapperRule, type, operation.Kind));
13131332
}
@@ -1349,7 +1368,7 @@ private void CheckLocalSymbolInUnsupportedContext(IOperation operation, ILocalSy
13491368
CheckTypeSymbolInUnsupportedContext(operation, local.Type);
13501369
}
13511370

1352-
private void CheckMethodSymbolInUnsupportedContext(IOperation operation, IMethodSymbol? symbol)
1371+
protected void CheckMethodSymbolInUnsupportedContext(IOperation operation, IMethodSymbol? symbol)
13531372
{
13541373
if (symbol is null)
13551374
return;
@@ -1393,7 +1412,7 @@ private void CheckTypeInUnsupportedContext(IOperation operation)
13931412
return;
13941413
}
13951414

1396-
if (!_cache.IsNonCopyableType(symbol))
1415+
if (!Cache.IsNonCopyableType(symbol))
13971416
{
13981417
// Copies of this type are allowed
13991418
return;
@@ -1403,7 +1422,7 @@ private void CheckTypeInUnsupportedContext(IOperation operation)
14031422
}
14041423
}
14051424

1406-
private sealed class NonCopyableTypesCache
1425+
protected sealed class NonCopyableTypesCache
14071426
{
14081427
private readonly ConcurrentDictionary<INamedTypeSymbol, bool> _typesToNonCopyable
14091428
= 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)