Skip to content

Commit

Permalink
Merge pull request #4374 from sharwell/foreach-readonly
Browse files Browse the repository at this point in the history 
Support foreach over parameters in DoNotCopyValue
  • Loading branch information
@sharwell
sharwell committed Oct 28, 2020
2 parents a235f3f + 21b4b7c commit 129a02d
Show file tree
Hide file tree
Showing 5 changed files with 228 additions and 60 deletions.
49 changes: 49 additions & 0 deletions src/Roslyn.Diagnostics.Analyzers/CSharp/CSharpDoNotCopyValue.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using Roslyn.Diagnostics.Analyzers;

namespace Roslyn.Diagnostics.CSharp.Analyzers
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class CSharpDoNotCopyValue : AbstractDoNotCopyValue
{
protected override NonCopyableWalker CreateWalker(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
=> new CSharpNonCopyableWalker(context, cache);

private sealed class CSharpNonCopyableWalker : NonCopyableWalker
{
public CSharpNonCopyableWalker(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
: base(context, cache)
{
}

protected override bool CheckForEachGetEnumerator(IForEachLoopOperation operation, [DisallowNull] ref IConversionOperation? conversion, [DisallowNull] ref IOperation? instance)
{
if (operation.Syntax is CommonForEachStatementSyntax syntax
&& operation.SemanticModel.GetForEachStatementInfo(syntax).GetEnumeratorMethod is { } getEnumeratorMethod)
{
CheckMethodSymbolInUnsupportedContext(operation, getEnumeratorMethod);

if (instance is not null
&& Cache.IsNonCopyableType(getEnumeratorMethod.ReceiverType)
&& !getEnumeratorMethod.IsReadOnly
&& Acquire(instance) == RefKind.In)
{
// mark the instance as not checked by this method
instance = null;
}

return true;
}

return false;
}
}
}
}
65 changes: 42 additions & 23 deletions ...gnostics.Analyzers/Core/DoNotCopyValue.cs → ....Analyzers/Core/AbstractDoNotCopyValue.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@

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

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

protected abstract NonCopyableWalker CreateWalker(OperationBlockAnalysisContext context, NonCopyableTypesCache cache);

public override void Initialize(AnalysisContext context)
{
context.EnableConcurrentExecution();
Expand All @@ -103,9 +104,9 @@ public override void Initialize(AnalysisContext context)
});
}

private static void AnalyzeOperationBlock(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
private void AnalyzeOperationBlock(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
{
var walker = new NonCopyableWalker(context, cache);
var walker = CreateWalker(context, cache);
foreach (var operation in context.OperationBlocks)
{
walker.Visit(operation);
Expand Down Expand Up @@ -147,18 +148,21 @@ public void Dispose()
}
}

private sealed class NonCopyableWalker : OperationWalker
protected abstract class NonCopyableWalker : OperationWalker
{
private readonly OperationBlockAnalysisContext _context;
private readonly NonCopyableTypesCache _cache;
private readonly HashSet<IOperation> _handledOperations = new HashSet<IOperation>();

public NonCopyableWalker(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
protected NonCopyableWalker(OperationBlockAnalysisContext context, NonCopyableTypesCache cache)
{
_context = context;
_cache = cache;
Cache = cache;
}

protected NonCopyableTypesCache Cache { get; }

protected abstract bool CheckForEachGetEnumerator(IForEachLoopOperation operation, [DisallowNull] ref IConversionOperation? conversion, [DisallowNull] ref IOperation? instance);

public override void VisitAddressOf(IAddressOfOperation operation)
{
CheckTypeInUnsupportedContext(operation);
Expand Down Expand Up @@ -220,11 +224,11 @@ public override void VisitAwait(IAwaitOperation operation)
{
// Treat await of ValueTask<T> the same way handling of a return
if (operation.Type is { } type
&& _cache.IsNonCopyableType(type)
&& Cache.IsNonCopyableType(type)
&& operation.Operation.Type is INamedTypeSymbol { OriginalDefinition: var taskType })
{
if (!SymbolEqualityComparer.Default.Equals(taskType, _cache.ValueTaskT)
&& !SymbolEqualityComparer.Default.Equals(taskType, _cache.ConfiguredValueTaskAwaitableT))
if (!SymbolEqualityComparer.Default.Equals(taskType, Cache.ValueTaskT)
&& !SymbolEqualityComparer.Default.Equals(taskType, Cache.ConfiguredValueTaskAwaitableT))
{
CheckTypeInUnsupportedContext(operation);
}
Expand Down Expand Up @@ -367,7 +371,7 @@ bool IsSupportedConversion()
switch (Acquire(operation.Operand))
{
case RefKind.None:
case RefKind.Ref when operation.Conversion.IsIdentity:
case RefKind.Ref or RefKind.RefReadOnly when operation.Conversion.IsIdentity:
return true;

default:
Expand Down Expand Up @@ -545,17 +549,32 @@ public override void VisitForEachLoop(IForEachLoopOperation operation)
CheckLocalSymbolInUnsupportedContext(operation, local);
}

var instance = operation.Collection;
// 'foreach' operations have an identity conversion for the collection property, and then invoke the
// GetEnumerator method.
var instance = operation.Collection as IConversionOperation;
var instance2 = (operation.Collection as IConversionOperation)?.Operand;
if (Acquire(operation.Collection) != RefKind.Ref)

if (instance2 is null)
{
// Didn't match the known pattern
instance = null;
instance2 = null;
}
else if (Acquire(instance2) != RefKind.Ref)
else if (instance?.Conversion is not { IsIdentity: true, MethodSymbol: null })
{
// Not a supported conversion
instance = null;
instance2 = null;
}
else
{
// Treat this as an invocation of the GetEnumerator method.
if (!CheckForEachGetEnumerator(operation, ref instance, ref instance2))
{
// Not supported
instance = null;
instance2 = null;
}
}

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

var instance = operation.Instance;
if (instance is object
&& _cache.IsNonCopyableType(operation.TargetMethod.ReceiverType)
&& Cache.IsNonCopyableType(operation.TargetMethod.ReceiverType)
&& !operation.TargetMethod.IsReadOnly
&& Acquire(instance) == RefKind.In)
{
Expand Down Expand Up @@ -806,7 +825,7 @@ public override void VisitPropertyReference(IPropertyReferenceOperation operatio

var instance = operation.Instance;
if (instance is object
&& _cache.IsNonCopyableType(operation.Property.ContainingType)
&& Cache.IsNonCopyableType(operation.Property.ContainingType)
&& Acquire(instance) == RefKind.In)
{
if (operation.IsSetMethodInvocation())
Expand Down Expand Up @@ -1113,7 +1132,7 @@ private static bool CanAssign(RefKind sourceRefKind, RefKind targetRefKind)
};
}

private RefKind Acquire(IOperation? operation)
protected RefKind Acquire(IOperation? operation)
{
if (operation is null)
return RefKind.RefReadOnly;
Expand Down Expand Up @@ -1307,7 +1326,7 @@ private void CheckTypeSymbolInUnsupportedContext(IOperation operation, ITypeSymb
if (type.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T)
{
var nullableUnderlyingType = ((INamedTypeSymbol)type).TypeArguments.FirstOrDefault();
if (_cache.IsNonCopyableType(nullableUnderlyingType))
if (Cache.IsNonCopyableType(nullableUnderlyingType))
{
_context.ReportDiagnostic(operation.Syntax.CreateDiagnostic(AvoidNullableWrapperRule, type, operation.Kind));
}
Expand Down Expand Up @@ -1349,7 +1368,7 @@ private void CheckLocalSymbolInUnsupportedContext(IOperation operation, ILocalSy
CheckTypeSymbolInUnsupportedContext(operation, local.Type);
}

private void CheckMethodSymbolInUnsupportedContext(IOperation operation, IMethodSymbol? symbol)
protected void CheckMethodSymbolInUnsupportedContext(IOperation operation, IMethodSymbol? symbol)
{
if (symbol is null)
return;
Expand Down Expand Up @@ -1393,7 +1412,7 @@ private void CheckTypeInUnsupportedContext(IOperation operation)
return;
}

if (!_cache.IsNonCopyableType(symbol))
if (!Cache.IsNonCopyableType(symbol))
{
// Copies of this type are allowed
return;
Expand All @@ -1403,7 +1422,7 @@ private void CheckTypeInUnsupportedContext(IOperation operation)
}
}

private sealed class NonCopyableTypesCache
protected sealed class NonCopyableTypesCache
{
private readonly ConcurrentDictionary<INamedTypeSymbol, bool> _typesToNonCopyable
= new ConcurrentDictionary<INamedTypeSymbol, bool>();
Expand Down
52 changes: 34 additions & 18 deletions src/Roslyn.Diagnostics.Analyzers/Roslyn.Diagnostics.Analyzers.sarif
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,24 +134,6 @@
]
}
},
"RS0042": {
"id": "RS0042",
"shortDescription": "Do not copy value",
"fullDescription": "Do not unbox non-copyable value types.",
"defaultLevel": "warning",
"properties": {
"category": "RoslynDiagnosticsReliability",
"isEnabledByDefault": true,
"typeName": "DoNotCopyValue",
"languages": [
"C#",
"Visual Basic"
],
"tags": [
"Telemetry"
]
}
},
"RS0043": {
"id": "RS0043",
"shortDescription": "Do not call 'GetTestAccessor()'",
Expand Down Expand Up @@ -248,6 +230,23 @@
]
}
},
"RS0042": {
"id": "RS0042",
"shortDescription": "Do not copy value",
"fullDescription": "Do not unbox non-copyable value types.",
"defaultLevel": "warning",
"properties": {
"category": "RoslynDiagnosticsReliability",
"isEnabledByDefault": true,
"typeName": "CSharpDoNotCopyValue",
"languages": [
"C#"
],
"tags": [
"Telemetry"
]
}
},
"RS0046": {
"id": "RS0046",
"shortDescription": "Avoid the 'Opt' suffix",
Expand Down Expand Up @@ -394,6 +393,23 @@
]
}
},
"RS0042": {
"id": "RS0042",
"shortDescription": "Do not copy value",
"fullDescription": "Do not unbox non-copyable value types.",
"defaultLevel": "warning",
"properties": {
"category": "RoslynDiagnosticsReliability",
"isEnabledByDefault": true,
"typeName": "VisualBasicDoNotCopyValue",
"languages": [
"Visual Basic"
],
"tags": [
"Telemetry"
]
}
},
"RS0101": {
"id": "RS0101",
"shortDescription": "Avoid multiple blank lines",
Expand Down
Loading

0 comments on commit 129a02d

Please sign in to comment.