From 23051b6386ebf95d869e962a94ad2a12f08a33af Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Wed, 13 Sep 2023 14:20:42 -0700 Subject: [PATCH 1/2] Basic support for ownership tracking analyzers --- src/Directory.Build.props | 2 +- .../CompilationExtensions.cs | 2 +- .../SymbolAnalysisContextExtensions.cs | 8 +- src/ErrorProne.NET.Core/SymbolExtensions.cs | 180 +++++ src/ErrorProne.NET.Core/TypeExtensions.cs | 82 +++ .../WellKnownTypesProvider.cs | 6 + ...LoosingScopeAnalyzerTests.MoveSemantics.cs | 224 +++++++ .../DisposeBeforeLoosingScopeAnalyzerTests.cs | 507 ++++++++++++++ .../ErrorProne.NET.CoreAnalyzers.Tests.csproj | 2 +- .../DiagnosticDescriptors.cs | 12 + .../DisposableAttributes.cs | 19 + .../DisposeAnalysisHelper.cs | 84 +++ .../DisposeBeforeLoosingScopeAnalyzer.cs | 634 ++++++++++++++++++ .../ErrorProne.NET.CoreAnalyzers.csproj | 18 +- .../SwallowAllExceptionsAnalyzer.cs | 1 + ...rrorProne.NET.StructAnalyzers.Tests.csproj | 2 +- 16 files changed, 1771 insertions(+), 12 deletions(-) create mode 100644 src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs create mode 100644 src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.cs create mode 100644 src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposableAttributes.cs create mode 100644 src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeAnalysisHelper.cs create mode 100644 src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzer.cs diff --git a/src/Directory.Build.props b/src/Directory.Build.props index 415d40a..624b202 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -2,7 +2,7 @@ - 9.0 + latest Enable embedded diff --git a/src/ErrorProne.NET.Core/CompilationExtensions.cs b/src/ErrorProne.NET.Core/CompilationExtensions.cs index 846d5f3..32a7cbf 100644 --- a/src/ErrorProne.NET.Core/CompilationExtensions.cs +++ b/src/ErrorProne.NET.Core/CompilationExtensions.cs @@ -23,7 +23,7 @@ public static class CompilationExtensions public static INamedTypeSymbol? TaskOfTType(this Compilation compilation) => compilation.GetTypeByFullName(typeof(Task<>).FullName); - + public static INamedTypeSymbol? ValueTaskOfTType(this Compilation compilation) => compilation.GetTypeByFullName("System.Threading.Tasks.ValueTask`1"); diff --git a/src/ErrorProne.NET.Core/SymbolAnalysisContextExtensions.cs b/src/ErrorProne.NET.Core/SymbolAnalysisContextExtensions.cs index d386b5b..bf80835 100644 --- a/src/ErrorProne.NET.Core/SymbolAnalysisContextExtensions.cs +++ b/src/ErrorProne.NET.Core/SymbolAnalysisContextExtensions.cs @@ -1,10 +1,4 @@ -// -------------------------------------------------------------------- -// -// Copyright (c) Microsoft Corporation. All rights reserved. -// -// -------------------------------------------------------------------- - -using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; using System.Diagnostics.CodeAnalysis; diff --git a/src/ErrorProne.NET.Core/SymbolExtensions.cs b/src/ErrorProne.NET.Core/SymbolExtensions.cs index 581aa5c..b395c56 100644 --- a/src/ErrorProne.NET.Core/SymbolExtensions.cs +++ b/src/ErrorProne.NET.Core/SymbolExtensions.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Linq; @@ -7,6 +8,8 @@ using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.FlowAnalysis; +using Microsoft.CodeAnalysis.Operations; namespace ErrorProne.NET.Core { @@ -20,6 +23,11 @@ public enum SymbolVisibility /// public static class SymbolExtensions { + public static bool HasAttributeWithName(this ISymbol? symbol, string attributeName) + { + return symbol?.GetAttributes().Any(a => a.AttributeClass.Name == attributeName) == true; + } + public static bool IsConstructor(this ISymbol symbol) { return (symbol is IMethodSymbol methodSymbol && methodSymbol.MethodKind == MethodKind.Constructor); @@ -264,5 +272,177 @@ public static bool ExceptionFromCatchBlock(this ISymbol symbol) // Use following code if the trick with DeclaredSyntaxReferences would not work properly! // return (bool?)(symbol.GetType().GetRuntimeProperty("IsCatch")?.GetValue(symbol)) == true; } + + public static bool IsPrivate(this ISymbol symbol) + { + return symbol.DeclaredAccessibility == Accessibility.Private; + } + } + + public static class MethodSymbolExtensions + { + /// + /// Checks if the given method matches Dispose method convention and can be recognized by "using". + /// + public static bool HasDisposeSignatureByConvention(this IMethodSymbol method) + { + return method.HasDisposeMethodSignature() + && !method.IsStatic + && !method.IsPrivate(); + } + + /// + /// Checks if the given method has the signature "void Dispose()". + /// + private static bool HasDisposeMethodSignature(this IMethodSymbol method) + { + return method.Name == "Dispose" && method.MethodKind == MethodKind.Ordinary && + method.ReturnsVoid && method.Parameters.IsEmpty; + } + } + + // TODO: move to another file + internal static class ControlFlowGraphExtensions + { + public static BasicBlock GetEntry(this ControlFlowGraph cfg) => cfg.Blocks.Single(b => b.Kind == BasicBlockKind.Entry); + public static BasicBlock GetExit(this ControlFlowGraph cfg) => cfg.Blocks.Single(b => b.Kind == BasicBlockKind.Exit); + public static IEnumerable DescendantOperations(this ControlFlowGraph cfg) + { + foreach (BasicBlock block in cfg.Blocks) + { + foreach (IOperation operation in block.DescendantOperations()) + { + yield return operation; + } + } + } + + public static bool IsFinallyBlock(this BasicBlock block) + { + return block.Kind == BasicBlockKind.Block && block.EnclosingRegion.Kind == ControlFlowRegionKind.Finally; + } + + public static bool IsTryBlock(this BasicBlock block) + { + return block.Kind == BasicBlockKind.Block && block.EnclosingRegion.Kind == ControlFlowRegionKind.Try; + } + + public static bool IsCatchBlock(this BasicBlock block) + { + return block.Kind == BasicBlockKind.Block && block.EnclosingRegion.Kind == ControlFlowRegionKind.Catch; + } + + public static IEnumerable DescendantOperations(this BasicBlock basicBlock) + { + foreach (var statement in basicBlock.Operations) + { + foreach (var operation in statement.DescendantsAndSelf()) + { + yield return operation; + } + } + + if (basicBlock.BranchValue != null) + { + foreach (var operation in basicBlock.BranchValue.DescendantsAndSelf()) + { + yield return operation; + } + } + } + + public static IEnumerable DescendantOperations(this ControlFlowGraph cfg, OperationKind operationKind) + where T : IOperation + { + foreach (var descendant in cfg.DescendantOperations()) + { + if (descendant?.Kind == operationKind) + { + yield return (T)descendant; + } + } + } + + internal static bool SupportsFlowAnalysis(this ControlFlowGraph cfg) + { + // Skip flow analysis for following root operation blocks: + // 1. Null root operation (error case) + // 2. OperationKindEx.Attribute or OperationKind.None (used for attributes before IAttributeOperation support). + // 3. OperationKind.ParameterInitialzer (default parameter values). + if (cfg.OriginalOperation == null || + cfg.OriginalOperation.Kind is OperationKindEx.Attribute or OperationKind.None or OperationKind.ParameterInitializer) + { + return false; + } + + // Skip flow analysis for code with syntax/semantic errors + if (cfg.OriginalOperation.Syntax.GetDiagnostics().Any(d => d.DefaultSeverity == DiagnosticSeverity.Error) || + cfg.OriginalOperation.HasAnyOperationDescendant(o => o is IInvalidOperation)) + { + return false; + } + + return true; + } + + public static bool HasAnyOperationDescendant(this ImmutableArray operationBlocks, Func predicate) + { + foreach (var operationBlock in operationBlocks) + { + if (operationBlock.HasAnyOperationDescendant(predicate)) + { + return true; + } + } + + return false; + } + + public static bool HasAnyOperationDescendant(this ImmutableArray operationBlocks, Func predicate, [NotNullWhen(returnValue: true)] out IOperation? foundOperation) + { + foreach (var operationBlock in operationBlocks) + { + if (operationBlock.HasAnyOperationDescendant(predicate, out foundOperation)) + { + return true; + } + } + + foundOperation = null; + return false; + } + + public static bool HasAnyOperationDescendant(this ImmutableArray operationBlocks, OperationKind kind) + { + return operationBlocks.HasAnyOperationDescendant(predicate: operation => operation.Kind == kind); + } + + public static bool HasAnyOperationDescendant(this IOperation operationBlock, Func predicate) + { + return operationBlock.HasAnyOperationDescendant(predicate, out _); + } + + public static bool HasAnyOperationDescendant(this IOperation operationBlock, Func predicate, [NotNullWhen(returnValue: true)] out IOperation? foundOperation) + { + foreach (var descendant in operationBlock.DescendantsAndSelf()) + { + if (predicate(descendant)) + { + foundOperation = descendant; + return true; + } + } + + foundOperation = null; + return false; + } + } + + internal static class OperationKindEx + { + public const OperationKind FunctionPointerInvocation = (OperationKind)0x78; + public const OperationKind ImplicitIndexerReference = (OperationKind)0x7b; + public const OperationKind Utf8String = (OperationKind)0x7c; + public const OperationKind Attribute = (OperationKind)0x7d; } } \ No newline at end of file diff --git a/src/ErrorProne.NET.Core/TypeExtensions.cs b/src/ErrorProne.NET.Core/TypeExtensions.cs index e38df93..54f1aa3 100644 --- a/src/ErrorProne.NET.Core/TypeExtensions.cs +++ b/src/ErrorProne.NET.Core/TypeExtensions.cs @@ -202,5 +202,87 @@ public static bool OverridesToString(this ITypeSymbol type) .Any(t => t.GetMembers(nameof(ToString)).Any(m => m.IsOverride)); } + /// + /// Return true if a given derives from . + /// + public static bool DerivesFrom([NotNullWhen(returnValue: true)] this ITypeSymbol? symbol, [NotNullWhen(returnValue: true)] ITypeSymbol? candidateBaseType, bool baseTypesOnly = false, bool checkTypeParameterConstraints = true) + { + if (candidateBaseType == null || symbol == null) + { + return false; + } + + if (!baseTypesOnly && candidateBaseType.TypeKind == TypeKind.Interface) + { + var allInterfaces = symbol.AllInterfaces.OfType(); + if (SymbolEqualityComparer.Default.Equals(candidateBaseType.OriginalDefinition, candidateBaseType)) + { + // Candidate base type is not a constructed generic type, so use original definition for interfaces. + allInterfaces = allInterfaces.Select(i => i.OriginalDefinition); + } + + if (allInterfaces.Contains(candidateBaseType)) + { + return true; + } + } + + if (checkTypeParameterConstraints && symbol.TypeKind == TypeKind.TypeParameter) + { + var typeParameterSymbol = (ITypeParameterSymbol)symbol; + foreach (var constraintType in typeParameterSymbol.ConstraintTypes) + { + if (constraintType.DerivesFrom(candidateBaseType, baseTypesOnly, checkTypeParameterConstraints)) + { + return true; + } + } + } + + while (symbol != null) + { + if (SymbolEqualityComparer.Default.Equals(symbol, candidateBaseType)) + { + return true; + } + + symbol = symbol.BaseType; + } + + return false; + } + + /// + /// Indicates if the given is disposable, + /// and thus can be used in a using or await using statement. + /// + public static bool IsDisposable(this ITypeSymbol type, + INamedTypeSymbol? iDisposable, + INamedTypeSymbol? iAsyncDisposable, + INamedTypeSymbol? configuredAsyncDisposable) + { + if (type.IsReferenceType) + { + return IsInterfaceOrImplementsInterface(type, iDisposable) + || IsInterfaceOrImplementsInterface(type, iAsyncDisposable); + } + else if (SymbolEqualityComparer.Default.Equals(type, configuredAsyncDisposable)) + { + return true; + } + + if (type.IsRefLikeType) + { + return type.GetMembers("Dispose").OfType() + .Any(method => method.HasDisposeSignatureByConvention()); + } + + return false; + + static bool IsInterfaceOrImplementsInterface(ITypeSymbol type, INamedTypeSymbol? interfaceType) + => interfaceType != null && + (SymbolEqualityComparer.Default.Equals(type, interfaceType) || type.AllInterfaces.Contains(interfaceType)); + } + } } \ No newline at end of file diff --git a/src/ErrorProne.NET.Core/WellKnownTypesProvider.cs b/src/ErrorProne.NET.Core/WellKnownTypesProvider.cs index 6d5c184..abbc544 100644 --- a/src/ErrorProne.NET.Core/WellKnownTypesProvider.cs +++ b/src/ErrorProne.NET.Core/WellKnownTypesProvider.cs @@ -42,4 +42,10 @@ public static WellKnownTypeProvider GetOrCreate(Compilation compilation) v => Compilation.GetBestTypeByMetadataName(v)); } } + + public static class WellKnownTypeNames + { + public const string SystemIAsyncDisposable = "System.IAsyncDisposable"; + public const string SystemIDisposable = "System.IDisposable"; + } } \ No newline at end of file diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs new file mode 100644 index 0000000..475c567 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs @@ -0,0 +1,224 @@ +using System; +using NUnit.Framework; +using System.Threading.Tasks; +using Verify = ErrorProne.NET.TestHelpers.CSharpCodeFixVerifier< + ErrorProne.NET.DisposableAnalyzers.DisposeBeforeLoosingScopeAnalyzer, + Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>; + +namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers +{ + [TestFixture] + public partial class DisposeBeforeLoosingScopeAnalyzerTests + { + private const string AcquiresOwnershipAttribute = + @" +[System.AttributeUsage(System.AttributeTargets.Parameter)] +public class AcquiresOwnershipAttribute : System.Attribute { } + +[System.AttributeUsage(System.AttributeTargets.All)] +public class ReleasesOwnershipAttribute : System.Attribute { } + +[System.AttributeUsage(System.AttributeTargets.Method)] +public class KeepsOwnershipAttribute : System.Attribute { } + +public static class DisposableExtensions +{ + /// + /// A special method that allows to release the current ownership. + /// + [KeepsOwnership] + public static T ReleaseOwnership([AcquiresOwnership]this T disposable) where T : System.IDisposable + { + return disposable; + } +}"; + + private const string Disposable = + @" +public class Disposable : System.IDisposable + { + public void Dispose() { } + }"; + + [Test] + public async Task NoWarn_On_Move() + { + var test = @" +public class Test +{ + public static void Moves() + { + var d = new Disposable(); + TakesOwnership(d); + } + + private static void TakesOwnership([AcquiresOwnership] Disposable d2) { d2.Dispose(); } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Move_To_List() + { + var test = @" +public class Test +{ + public static void Moves() + { + var d = new Disposable(); + var list = new System.Collections.Generic.List(){ d.ReleaseOwnership() }; + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Move_With_ReleaseOwnership() + { + var test = @" +public class Test +{ + public static void Moves() + { + var d = new Disposable().ReleaseOwnership(); + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Non_FactoryMethod() + { + var test = @" +public class Test +{ + public static void Moves() + { + var d = NoOwnership(); + } + + [KeepsOwnershipAttribute] + private static Disposable NoOwnership() => new Disposable().ReleaseOwnership(); +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task Warn_On_Taken_Ownership() + { + var test = @" +public class Test +{ + private static void TakesOwnership([AcquiresOwnership] Disposable [|d|]) { } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task Warn_On_Taken_Ownership_With_Usage() + { + var test = @" +public class Test +{ + private static string TakesOwnership([AcquiresOwnership] Disposable [|d|]) + { + return d.ToString(); + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Taken_Ownership_With_Dispose() + { + var test = @" +public class Test +{ + private static string TakesOwnership([AcquiresOwnership] Disposable d) + { + var r = d.ToString(); + d.Dispose(); + return r; + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Taken_Ownership_With_Using() + { + var test = @" +public class Test +{ + private static string TakesOwnership([AcquiresOwnership] Disposable d) + { + using (d) + { + return d.ToString(); + } + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Taken_Ownership_With_Moved_Ownership() + { + var test = @" +public class Test +{ + private static string TakesOwnershipAndDisposes([AcquiresOwnership] Disposable d) + { + using (d) + { + return d.ToString(); + } + } + + private static string TakesOwnership([AcquiresOwnership] Disposable d) + { + return TakesOwnershipAndDisposes(d); + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task Warn_On_Taken_Ownership_With_Incorrect_Moved_Ownership() + { + var test = @" +public class Test +{ + private static string TakesOwnershipAndDisposes(Disposable d) + { + using (d) + { + return d.ToString(); + } + } + + private static string TakesOwnership([AcquiresOwnership] Disposable [|d|]) + { + return TakesOwnershipAndDisposes(d); + } +} +"; + await VerifyAsync(test); + } + + private static Task VerifyAsync(string code) + { + code += $"{Environment.NewLine}{AcquiresOwnershipAttribute}{Environment.NewLine}{Disposable}"; + return Verify.VerifyAsync(code); + } + } +} \ No newline at end of file diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.cs new file mode 100644 index 0000000..b88b0cf --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.cs @@ -0,0 +1,507 @@ +using NUnit.Framework; +using System.Threading.Tasks; +using ErrorProne.NET.DisposableAnalyzers; +using Verify = ErrorProne.NET.TestHelpers.CSharpCodeFixVerifier< + ErrorProne.NET.DisposableAnalyzers.DisposeBeforeLoosingScopeAnalyzer, + Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>; + +namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers +{ + [TestFixture] + public partial class DisposeBeforeLoosingScopeAnalyzerTests + { + [Test] + public async Task Warn_On_No_Dispose() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + var [|d1|] = new Disposable(); + var nd = new NonDisposable(); + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + } + + public class NonDisposable { } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task Warn_On_No_Dispose_With_Cast() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + var [|d1|] = new Disposable() as object; + var [|d2|] = (object)new Disposable(); + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task Warn_On_No_Dispose_In_If_Block() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + if ([|new Disposable()|] is null) + { + } + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task Warn_On_No_Dispose_With_Factory_Method() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + var [|d|] = Disposable.Create(); + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + public static Disposable Create() => new Disposable(); + } + + public class NonDisposable { } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_No_Dispose_With_Property() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + var d = Instance; + } + + public static Disposable Instance => new Disposable().ReleaseOwnership(); +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task Warn_On_No_Dispose_With_Factory_Property() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + var [|d|] = Disposable.Instance; + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + + [ReleasesOwnership] + public static Disposable Instance => new Disposable(); + } + + public class NonDisposable { } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_UsingDeclaration() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + using var d = new Disposable(); + using var _ = new Disposable(); + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Manual_Dispose() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + var d = new Disposable(); + d.Dispose(); + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Usings() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + using var d1 = new Disposable(); + using var d2 = Disposable.Create(); + using var d3 = Disposable.Instance; + using var _ = new Disposable(); + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + public static Disposable Create() => new Disposable(); + public static Disposable Instance => new Disposable(); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_SimpleReturn() + { + var test = @" +public class Test +{ + public class Disposable : System.IDisposable + { + public void Dispose() { } + public static Disposable Create() => new Disposable(); + public static Disposable Create2() + { + return new Disposable(); + } + public static Disposable Instance => new Disposable(); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Conditional_Return() + { + var test = @" +public class Test +{ + public class Disposable : System.IDisposable + { + public void Dispose() { } + public static Disposable Create2(bool check) + { + var result = new Disposable(); + if (check) + { + return result; + } + else + { + return result; + } + } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Conditional_ReturnInCatchFinally() + { + var test = @" +public class Test +{ + public class Disposable : System.IDisposable + { + public void Dispose() { } + public static Disposable Create2(bool check) + { + var result = new Disposable(); + try + { + return result; + } + catch + { + return result; + } + } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Conditional_Return_Conditionally_In_All_Branches() + { + var test = @" +public class Test +{ + public class Disposable : System.IDisposable + { + public void Dispose() { } + public static Disposable Create2(bool check) + { + var result = new Disposable(); + try + { + if (check) + { + return result; + } + else + { + return result; + } + } + catch + { + return result; + } + } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_UsingStatement() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + using (var d = new Disposable()) + { + } + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Dispose_In_Finally() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + var d = new Disposable(); + try + { + } + finally + { + d.Dispose(); + } + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Dispose_In_Try_And_Catch() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + var d = new Disposable(); + try + { + d.Dispose(); + } + catch + { + d.Dispose(); + } + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + } +} +"; + await Verify.VerifyAsync(test); + } + + // [Test] Not supported yet. + public async Task Warn_On_DisposeInFinally_Conditionally() + { + var test = @" +public class Test +{ + public static void ShouldDispose(bool shouldDispose) + { + var [|d|] = new Disposable(); + try + { + } + finally + { + if (shouldDispose) + d.Dispose(); + } + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Dispose_In_Try_Catch() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + var d = new Disposable(); + try + { + } + finally + { + d.Dispose(); + } + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_UsingStatement_With_No_Local() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + using (new Disposable()) + { + } + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Field() + { + var test = @" +public class Test +{ + private readonly Disposable _d = new Disposable(); + + public class Disposable : System.IDisposable + { + public void Dispose() { } + } +} +"; + await Verify.VerifyAsync(test); + } + + + // Positive test cases: + // * var d = new Disposable() + // * var d = CreateDisposable(); + // * var d = await CreateDisposableAsync(); + // * var d = DisposableProperty; + // * var d = FooBar.DisposableProperty; + + // Disposed only in 'catch' block + + // Dispose if a type has 'Dispose' method and is ref struct? + + // Negative test cases + + // Configure a list of disposable types that should not be disposed. + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/ErrorProne.NET.CoreAnalyzers.Tests.csproj b/src/ErrorProne.NET.CoreAnalyzers.Tests/ErrorProne.NET.CoreAnalyzers.Tests.csproj index 1b5e148..b9eeb97 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/ErrorProne.NET.CoreAnalyzers.Tests.csproj +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/ErrorProne.NET.CoreAnalyzers.Tests.csproj @@ -1,7 +1,7 @@  - netcoreapp2.1 + net7.0 false diff --git a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs index 343fd4f..7de4b9e 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs @@ -9,6 +9,7 @@ internal static class DiagnosticDescriptors { private const string CodeSmellCategory = "CodeSmell"; private const string ConcurrencyCategory = "Concurrency"; + private const string ReliabilityCategory = "Reliability"; private static readonly string[] UnnecessaryTag = new [] { WellKnownDiagnosticTags.Unnecessary }; /// @@ -119,5 +120,16 @@ internal static class DiagnosticDescriptors messageFormat: "The API is not thread-safe.{0}", ConcurrencyCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, description: "The API is not thread safe and can cause runtime failures."); + + /// + /// A CA2000-like rule. + /// + public static readonly DiagnosticDescriptor ERP041 = new DiagnosticDescriptor( + nameof(ERP041), + title: "Dispose objects before losing scope", + messageFormat: "A local variable {0} must be disposed before losing scope", + ReliabilityCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "If a disposable object is not explicitly disposed before all references to it are out of scope, " + + "the object will be disposed at some indeterminate time when the garbage collector runs the finalizer of the object."); } } \ No newline at end of file diff --git a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposableAttributes.cs b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposableAttributes.cs new file mode 100644 index 0000000..a6eb41e --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposableAttributes.cs @@ -0,0 +1,19 @@ +namespace ErrorProne.NET.DisposableAnalyzers; + +public static class DisposableAttributes +{ + // TODO: document here? or in some other place? + public const string AcquiresOwnershipAttribute = "AcquiresOwnershipAttribute"; + + // For methods that want to emphasize that the ownership is not transferred. + public const string KeepsOwnershipAttribute = "KeepsOwnershipAttribute"; + + // For properties to emphasize that the ownership is transferred. + public const string ReleasesOwnershipAttribute = "ReleasesOwnershipAttribute"; + + + // For fields/properties, whe the ownership belongs to another type. + public const string NoOwnershipAttribute = "NoOwnershipAttribute"; + + +} \ No newline at end of file diff --git a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeAnalysisHelper.cs b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeAnalysisHelper.cs new file mode 100644 index 0000000..a37b2cf --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeAnalysisHelper.cs @@ -0,0 +1,84 @@ +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using ErrorProne.NET.Core; +using Microsoft.CodeAnalysis; + +namespace ErrorProne.NET.DisposableAnalyzers; + +internal sealed class DisposeAnalysisHelper +{ + private readonly List _disposableExceptions; + + public INamedTypeSymbol? IDisposable { get; } + public INamedTypeSymbol? IAsyncDisposable { get; } + public INamedTypeSymbol? IConfigureAsyncDisposable { get; } + + public static readonly ImmutableHashSet DisposableCreationKinds = ImmutableHashSet.Create( + OperationKind.ObjectCreation, + OperationKind.TypeParameterObjectCreation, + OperationKind.DynamicObjectCreation, // What's that? + OperationKind.Invocation + ); + + public DisposeAnalysisHelper(Compilation compilation) + { + _disposableExceptions = CreateExceptions(compilation); + + IDisposable = compilation.GetTypeByFullName(WellKnownTypeNames.SystemIDisposable); + IAsyncDisposable = compilation.GetTypeByFullName(WellKnownTypeNames.SystemIAsyncDisposable); + IConfigureAsyncDisposable = compilation.GetTypeByFullName("System.Runtime.CompilerServices.ConfiguredAsyncDisposable"); + } + + private static List CreateExceptions(Compilation compilation) + { + var result = new List(); + + // TODO: probably this list should be configurable + // and maybe we could have a special attribute and if the type is + addIfNotNull(compilation.TaskType()); + addIfNotNull(compilation.TaskOfTType()); + addIfNotNull(compilation.GetTypeByFullName("System.IO.StringReader")); + addIfNotNull(compilation.GetTypeByFullName("System.IO.MemoryStream")); + + return result; + + void addIfNotNull(INamedTypeSymbol? type) + { + if (type is not null) + { + result.Add(type); + } + } + } + + public bool IsDisposableTypeNotRequiringToBeDisposed(ITypeSymbol? typeSymbol) + { + if (typeSymbol is null) + { + return false; + } + + return _disposableExceptions.Any(e => typeSymbol.DerivesFrom(e)); + } + + public bool IsDisposable(ITypeSymbol? typeSymbol) + { + if (typeSymbol is null) + { + return false; + } + + return typeSymbol.IsDisposable(IDisposable, IAsyncDisposable, IConfigureAsyncDisposable); + } + + public bool ShouldBeDisposed(ITypeSymbol? typeSymbol) + { + if (typeSymbol is null) + { + return false; + } + + return IsDisposable(typeSymbol) && !IsDisposableTypeNotRequiringToBeDisposed(typeSymbol); + } +} \ No newline at end of file diff --git a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzer.cs new file mode 100644 index 0000000..fe8796c --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzer.cs @@ -0,0 +1,634 @@ +using System; +using System.Collections; +using System.Collections.Generic; +using System.Diagnostics.ContractsLight; +using System.Linq; +using ErrorProne.NET.CoreAnalyzers; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.FindSymbols; +using Microsoft.CodeAnalysis.FlowAnalysis; +using Microsoft.CodeAnalysis.Operations; +using ErrorProne.NET.Core; +using System.Collections.Immutable; +using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; +using System.Reflection.Metadata; +using System.Data.Common; +using ErrorProne.NET.Extensions; + +namespace ErrorProne.NET.DisposableAnalyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class DisposeBeforeLoosingScopeAnalyzer : DiagnosticAnalyzerBase +{ + /// + internal static readonly DiagnosticDescriptor Rule = DiagnosticDescriptors.ERP041; + + public override bool ReportDiagnosticsOnGeneratedCode { get; } = false; + + /// + public DisposeBeforeLoosingScopeAnalyzer() + : base(Rule) + { + } + + /// + protected override void InitializeCore(AnalysisContext context) + { + context.RegisterOperationAction(AnalyzeObjectCreation, OperationKind.ObjectCreation); + context.RegisterOperationAction(AnalyzeMethodCall, OperationKind.Invocation); + + context.RegisterOperationAction(AnalyzePropertyReference, OperationKind.PropertyReference); + + context.RegisterOperationAction(AnalyzeMethodBody, OperationKind.MethodBody); + + } + + /// + /// Analyzes method implementations for method that acquiring ownership via one of the arguments. + /// + private void AnalyzeMethodBody(OperationAnalysisContext context) + { + if (context.ContainingSymbol is not IMethodSymbol method + || !HasAcquiresOwnershipParameters(method, out var parameter) + // Skipping methods marked with 'KeepOwnership' attribute. + || method.HasAttributeWithName(DisposableAttributes.KeepsOwnershipAttribute)) + { + return; + } + + var syntax = (ParameterSyntax)parameter.DeclaringSyntaxReferences[0].GetSyntax(); + + if (!context.Operation.HasAnyOperationDescendant(o => o.Kind == OperationKind.ParameterReference, out var operation)) + { + context.ReportDiagnostic(Diagnostic.Create(Rule, syntax.Identifier.GetLocation(), parameter.Name)); + return; + } + + // It seems that the parameter was used. Checking how. + var cfg = context.GetControlFlowGraph(); + + if (IsDisposed(parameter, cfg)) + { + // Parameter is disposed. We're good! + return; + } + + if (OwnershipIsMoved(LocalOrParameterReference.Create(parameter), cfg)) + { + // The ownership is moved again. + return; + } + + if (HasParent(operation)) + { + // We have 'using(param) {} ' + return; + } + + context.ReportDiagnostic(Diagnostic.Create(Rule, syntax.Identifier.GetLocation(), parameter.Name)); + } + + private static bool HasAcquiresOwnershipParameters(IMethodSymbol method, [NotNullWhen(true)]out IParameterSymbol? result) + { + result = null; + foreach (var p in method.Parameters) + { + if (p.HasAttributeWithName(DisposableAttributes.AcquiresOwnershipAttribute)) + { + result = p; + return true; + } + } + + return false; + } + + private void AnalyzePropertyReference(OperationAnalysisContext context) + { + var propertyReference = (IPropertyReferenceOperation)context.Operation; + + var helper = new DisposeAnalysisHelper(context.Compilation); + if (!ReleasesOwnership(propertyReference.Property, helper)) + { + // We're not calling a factory method, so we're not taking an ownership of the object. + return; + } + + AnalyzeCore(context, propertyReference.Type, propertyReference); + } + + private void AnalyzeMethodCall(OperationAnalysisContext context) + { + var invocation = (IInvocationOperation)context.Operation; + + var helper = new DisposeAnalysisHelper(context.Compilation); + if (!ReleasesOwnership(invocation.TargetMethod, helper)) + { + // We're not calling a factory method, so we're not taking an ownership of the object. + return; + } + + AnalyzeCore(context, invocation.Type, invocation); + } + + private void AnalyzeObjectCreation(OperationAnalysisContext context) + { + var objectCreation = (IObjectCreationOperation)context.Operation; + AnalyzeCore(context, objectCreation.Type, objectCreation); + } + + private void AnalyzeCore(OperationAnalysisContext context, ITypeSymbol expressionType, IOperation operation) + { + if (operation.Parent is IFieldInitializerOperation or IPropertyInitializerOperation) + { + return; + } + + var helper = new DisposeAnalysisHelper(context.Compilation); + + if (helper.ShouldBeDisposed(expressionType)) + { + // The type is disposable, but its possible that the operation points not to a factory method. + // Meaning that we're not taking an ownership of the object. + + // The operation might be a variable declaration. + // Trying to figure it out since we have a ton of logic relying on that. + var variableDeclaration = EnumerateParents(operation).OfType().FirstOrDefault(); + + var controlFlowGraph = context.GetControlFlowGraph(); + if (IsDisposedOrOwnershipIsMoved(variableDeclaration, operation, controlFlowGraph)) + { + return; + } + + + // Analyzing body of the method to see if all the arguments are disposed. + if (IsFactoryMethodLikeSignature(context.ContainingSymbol, helper) && IsFactoryMethodImpl(operation, variableDeclaration, controlFlowGraph)) + { + // This is factory method/property so we can't dispose an argument since we're returning it. + return; + } + + if (variableDeclaration != null) + { + // Checking if the parent is 'using' declaration or 'using' statement, then we're good + + if (HasParent(variableDeclaration) || + HasParent(variableDeclaration)) + { + // Current object creation is a part of 'using' declaration. + return; + } + + if (variableDeclaration.Syntax is VariableDeclaratorSyntax vds) + { + var identifierLocation = vds.Identifier.GetLocation(); + context.ReportDiagnostic(Diagnostic.Create(Rule, identifierLocation, variableDeclaration.Symbol.Name)); + return; + } + + Contract.Assert(false, "Should not get here!"); + } + + // Variable declaration is null + if (HasParent(operation)) + { + // We're good, this is 'using(new Disposable())' statement + return; + } + + var location = operation.Syntax.GetLocation(); + context.ReportDiagnostic(Diagnostic.Create(Rule, location, operation.Syntax.ToString())); + } + } + + /// + /// Method returns true if a variable's ownership is moved to another place (method). + /// + private bool OwnershipIsMoved(LocalOrParameterReference localOrParam, ControlFlowGraph cfg) + { + return OwnershipIsMoved(cfg, argumentMatches: a => localOrParam.IsReferenced(a.Value)); + } + + /// + /// Method returns true if a variable's ownership is moved to another place (method). + /// + private bool OwnershipIsMoved(ControlFlowGraph cfg, Func argumentMatches) + { + foreach (var invocation in cfg.DescendantOperations().OfType()) + { + for (int i = 0; i < invocation.Arguments.Length; i++) + { + var a = invocation.Arguments[i]; + + if (argumentMatches(a)) + { + var p = invocation.TargetMethod.Parameters[i]; + if (p.HasAttributeWithName(DisposableAttributes.AcquiresOwnershipAttribute)) + { + return true; + } + } + } + } + + return false; + } + + + + // TODO: move to another place. + + + private static bool IsLocal(IVariableDeclaratorOperation variableDeclaration, IOperation operation) + { + return operation is ILocalReferenceOperation lro && + lro.Local.Equals(variableDeclaration.Symbol, SymbolEqualityComparer.Default); + } + + // + public readonly record struct LocalOrParameterReference + { + private readonly IVariableDeclaratorOperation? _variableDeclaration; + private readonly IParameterSymbol? _parameter; + + public LocalOrParameterReference(IVariableDeclaratorOperation variableDeclaration) + { + _variableDeclaration = variableDeclaration; + } + + public LocalOrParameterReference(IParameterSymbol parameter) + { + _parameter = parameter; + } + + public static LocalOrParameterReference Create(IVariableDeclaratorOperation variableDeclaration) => new (variableDeclaration); + public static LocalOrParameterReference Create(IParameterSymbol parameter) => new (parameter); + + public bool IsReferenced(IOperation operation) + { + if (operation is ILocalReferenceOperation lro && _variableDeclaration is not null) + { + return lro.Local.Equals(_variableDeclaration.Symbol, SymbolEqualityComparer.Default); + } + else if (operation is IParameterReferenceOperation pro && _parameter is not null) + { + return pro.Parameter.Equals(_parameter, SymbolEqualityComparer.Default); + } + + return false; + } + } + + /// + /// Returns true when a given looks like a factory method from a return type perspective. + /// + private bool IsFactoryMethodLikeSignature(ISymbol methodOrProperty, DisposeAnalysisHelper helper) + { + if (!helper.IsDisposable(TryFindReturnType(methodOrProperty))) + { + return false; + } + + // This is not a factory if it keeps the ownership. It might be, but we're not going to dispose the results. + if (methodOrProperty.HasAttributeWithName(DisposableAttributes.KeepsOwnershipAttribute)) + { + return false; + } + + // Special case for methods + // If there is an argument, marked with `AcquiresOwnershipAttribute`, then it's not a factory method. + if (methodOrProperty is IMethodSymbol ms && HasAcquiresOwnershipParameters(ms, out _)) + { + return false; + } + + return true; + } + + /// + /// Returns true when a given looks like a factory method and probably returns an ownership. + /// + private bool ReleasesOwnership(ISymbol methodOrProperty, DisposeAnalysisHelper helper) + { + if (!helper.IsDisposable(TryFindReturnType(methodOrProperty))) + { + return false; + } + + if (methodOrProperty.HasAttributeWithName(DisposableAttributes.KeepsOwnershipAttribute)) + { + return false; + } + + if (methodOrProperty.HasAttributeWithName(DisposableAttributes.ReleasesOwnershipAttribute)) + { + return true; + } + + // By default properties do not release ownership unless they are marked with `ReleasesOwnershipAttribute`. + if (methodOrProperty is IPropertySymbol) + { + return false; + } + + // Special case for methods + // If there is an argument, marked with `AcquiresOwnershipAttribute`, then it's not a factory method. + if (methodOrProperty is IMethodSymbol ms && HasAcquiresOwnershipParameters(ms, out _)) + { + return false; + } + + return true; + } + + private bool IsFactoryMethodImpl(IOperation operation, IVariableDeclaratorOperation? variableDeclaration, ControlFlowGraph cfg) + { + if (variableDeclaration is null && HasParent(operation)) + { + return true; + } + + if (variableDeclaration is null) + { + return false; + } + + var localVariable = variableDeclaration.Symbol; + var branches = cfg.GetExit().Predecessors.Select(b => new BranchWithInfo(b)).ToArray(); + + // We're fine if all the branches are returns and the return value is the local variable. + if (branches.All(b => + b.Kind == ControlFlowBranchSemantics.Return && b.BranchValue is ILocalReferenceOperation lro && + lro.Local.Equals(localVariable, SymbolEqualityComparer.Default))) + { + return true; + } + + + return false; + } + + // TODO: move this to helpers + private static ITypeSymbol? TryFindReturnType(ISymbol operation) + { + if (operation is IMethodSymbol ms && ms.MethodKind != MethodKind.Constructor) + { + return ms.ReturnType; + } + else if (operation is IPropertySymbol ps) + { + return ps.Type; + } + + return null; + } + + private bool IsDisposedOrOwnershipIsMoved(IVariableDeclaratorOperation? variableDeclaration, IOperation operation, ControlFlowGraph cfg) + { + if (variableDeclaration != null) + { + if (IsDisposed(variableDeclaration.Symbol, cfg) || + OwnershipIsMoved(LocalOrParameterReference.Create(variableDeclaration), cfg)) + { + return true; + } + } + + // It is possible that the ownership is moved, but the variable is not null, + // like 'new X().ReleaseOwnership()'. + if (OwnershipIsMoved(cfg, + // Using syntax node for comparison. Which is weird, since we'll get here + // two different object creation operations. + a => { return operation.Syntax.IsEquivalentTo(a.Value.Syntax); })) + { + return true; + } + + // Checking if the ownership is moved. + return false; + } + + private bool IsDisposed(ISymbol localVariable, ControlFlowGraph cfg) + { + var branches = cfg.GetExit().Predecessors.Select(b => new BranchWithInfo(b)).ToArray(); + + // Using a naive approach for now: if the variable is disposed in finally + // or in both: try and catch blocks, then we're good. + bool disposedInTry = false; + bool disposedInCatch = false; + foreach (var block in cfg.Blocks) + { + if (block.IsReachable && block.Kind == BasicBlockKind.Block && + block.ConditionKind == ControlFlowConditionKind.None && DisposedUnconditionallyIn(block, localVariable)) + { + // We have an unconditional Dispose in one of the blocks. + return true; + } + + if (block.IsFinallyBlock() && DisposedUnconditionallyIn(block, localVariable)) + { + return true; + } + + if (block.IsTryBlock() && DisposedUnconditionallyIn(block, localVariable)) + { + disposedInTry = true; + + if (disposedInCatch) + { + return true; + } + } + + if (block.IsCatchBlock() && DisposedUnconditionallyIn(block, localVariable)) + { + disposedInCatch = true; + + if (disposedInTry) + { + return true; + } + } + } + + return false; + } + + private bool DisposedUnconditionallyIn(BasicBlock block, ISymbol localVariable) + { + var invocations = block.DescendantOperations().OfType(); + foreach (var invocation in invocations) + { + if ((invocation.Instance is ILocalReferenceOperation lro && lro.Local.Equals(localVariable, SymbolEqualityComparer.Default)) || + (invocation.Instance is IParameterReferenceOperation pro && pro.Parameter.Equals(localVariable, SymbolEqualityComparer.Default))) + { + // TODO: cover if blocks. + if (invocation.TargetMethod.Name == "Dispose") + { + return true; + } + } + } + + return false; + } + + private static bool HasParent(IOperation operation) where T: IOperation + { + return EnumerateParents(operation).OfType().FirstOrDefault() != null; + } + + private static IEnumerable EnumerateParents(IOperation? operation) + { + while (operation != null) + { + operation = operation.Parent; + yield return operation; + } + } +} + +/// +/// Contains aggregated information about a control flow branch. +/// +/// TODO: move to a common place. +public sealed class BranchWithInfo +{ + private static readonly Func> s_getTransitiveNestedRegions = GetTransitiveNestedRegions; + + internal BranchWithInfo(ControlFlowBranch branch) + : this(branch.Destination!, branch.EnteringRegions, branch.LeavingRegions, branch.FinallyRegions, + branch.Semantics, branch.Source.BranchValue, + GetControlFlowConditionKind(branch), + leavingRegionLocals: ComputeLeavingRegionLocals(branch.LeavingRegions), + leavingRegionFlowCaptures: ComputeLeavingRegionFlowCaptures(branch.LeavingRegions)) + { + } + + internal BranchWithInfo(BasicBlock destination) + : this(destination, + enteringRegions: ImmutableArray.Empty, + leavingRegions: ImmutableArray.Empty, + finallyRegions: ImmutableArray.Empty, + kind: ControlFlowBranchSemantics.Regular, + branchValue: null, + controlFlowConditionKind: ControlFlowConditionKind.None, + leavingRegionLocals: ImmutableHashSet.Empty, + leavingRegionFlowCaptures: ImmutableHashSet.Empty) + { + } + + private BranchWithInfo( + BasicBlock destination, + ImmutableArray enteringRegions, + ImmutableArray leavingRegions, + ImmutableArray finallyRegions, + ControlFlowBranchSemantics kind, + IOperation? branchValue, + ControlFlowConditionKind controlFlowConditionKind, + IEnumerable leavingRegionLocals, + IEnumerable leavingRegionFlowCaptures) + { + Destination = destination; + Kind = kind; + EnteringRegions = enteringRegions; + LeavingRegions = leavingRegions; + FinallyRegions = finallyRegions; + BranchValue = branchValue; + ControlFlowConditionKind = controlFlowConditionKind; + LeavingRegionLocals = leavingRegionLocals; + LeavingRegionFlowCaptures = leavingRegionFlowCaptures; + } + + public BasicBlock Destination { get; } + public ControlFlowBranchSemantics Kind { get; } + public ImmutableArray EnteringRegions { get; } + public ImmutableArray FinallyRegions { get; } + public ImmutableArray LeavingRegions { get; } + public IOperation? BranchValue { get; } + + public ControlFlowConditionKind ControlFlowConditionKind { get; } + + public IEnumerable LeavingRegionLocals { get; } + public IEnumerable LeavingRegionFlowCaptures { get; } + + internal BranchWithInfo WithEmptyRegions(BasicBlock destination) + { + return new BranchWithInfo( + destination, + enteringRegions: ImmutableArray.Empty, + leavingRegions: ImmutableArray.Empty, + finallyRegions: ImmutableArray.Empty, + kind: Kind, + branchValue: BranchValue, + controlFlowConditionKind: ControlFlowConditionKind, + leavingRegionLocals: ImmutableHashSet.Empty, + leavingRegionFlowCaptures: ImmutableHashSet.Empty); + } + + internal BranchWithInfo With( + IOperation? branchValue, + ControlFlowConditionKind controlFlowConditionKind) + { + return new BranchWithInfo(Destination, EnteringRegions, LeavingRegions, + FinallyRegions, Kind, branchValue, controlFlowConditionKind, + LeavingRegionLocals, LeavingRegionFlowCaptures); + } + + private static IEnumerable GetTransitiveNestedRegions(ControlFlowRegion region) + { + yield return region; + + foreach (var nestedRegion in region.NestedRegions) + { + foreach (var transitiveNestedRegion in GetTransitiveNestedRegions(nestedRegion)) + { + yield return transitiveNestedRegion; + } + } + } + + private static IEnumerable ComputeLeavingRegionLocals(ImmutableArray leavingRegions) + { + return leavingRegions.SelectMany(s_getTransitiveNestedRegions).Distinct().SelectMany(r => r.Locals); + } + + private static IEnumerable ComputeLeavingRegionFlowCaptures(ImmutableArray leavingRegions) + { + return leavingRegions.SelectMany(s_getTransitiveNestedRegions).Distinct().SelectMany(r => r.CaptureIds); + } + + private static ControlFlowConditionKind GetControlFlowConditionKind(ControlFlowBranch branch) + { + if (branch.IsConditionalSuccessor || + branch.Source.ConditionKind == ControlFlowConditionKind.None) + { + return branch.Source.ConditionKind; + } + + return branch.Source.ConditionKind.Negate(); + } +} + +internal static class ControlFlowConditionKindExtensions +{ + public static ControlFlowConditionKind Negate(this ControlFlowConditionKind controlFlowConditionKind) + { + switch (controlFlowConditionKind) + { + case ControlFlowConditionKind.WhenFalse: + return ControlFlowConditionKind.WhenTrue; + + case ControlFlowConditionKind.WhenTrue: + return ControlFlowConditionKind.WhenFalse; + + default: + Debug.Fail($"Unsupported conditional kind: '{controlFlowConditionKind}'"); + return controlFlowConditionKind; + } + } +} \ No newline at end of file diff --git a/src/ErrorProne.NET.CoreAnalyzers/ErrorProne.NET.CoreAnalyzers.csproj b/src/ErrorProne.NET.CoreAnalyzers/ErrorProne.NET.CoreAnalyzers.csproj index 201c0b0..7c3f7e1 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/ErrorProne.NET.CoreAnalyzers.csproj +++ b/src/ErrorProne.NET.CoreAnalyzers/ErrorProne.NET.CoreAnalyzers.csproj @@ -12,7 +12,23 @@ incompatible changes on the "core" level --> - + + + + + + + + + + + + + + + + + diff --git a/src/ErrorProne.NET.CoreAnalyzers/ExceptionsAnalyzers/SwallowAllExceptionsAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/ExceptionsAnalyzers/SwallowAllExceptionsAnalyzer.cs index 5fd8f2c..0531b39 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/ExceptionsAnalyzers/SwallowAllExceptionsAnalyzer.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/ExceptionsAnalyzers/SwallowAllExceptionsAnalyzer.cs @@ -47,6 +47,7 @@ private static void AnalyzeSyntax(SyntaxNodeAnalysisContext context) StatementSyntax syntax = catchBlock.Block; var controlFlow = context.SemanticModel.AnalyzeControlFlow(syntax); + if (controlFlow == null) { return; diff --git a/src/ErrorProne.NET.StructAnalyzers.Tests/ErrorProne.NET.StructAnalyzers.Tests.csproj b/src/ErrorProne.NET.StructAnalyzers.Tests/ErrorProne.NET.StructAnalyzers.Tests.csproj index aed1fd9..a1373fb 100644 --- a/src/ErrorProne.NET.StructAnalyzers.Tests/ErrorProne.NET.StructAnalyzers.Tests.csproj +++ b/src/ErrorProne.NET.StructAnalyzers.Tests/ErrorProne.NET.StructAnalyzers.Tests.csproj @@ -1,7 +1,7 @@  - netcoreapp2.1 + net7.0 false From 332eb7b42fe0a558375fefb8488dda2a0900b65b Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Thu, 16 Nov 2023 09:21:33 -0800 Subject: [PATCH 2/2] WIP --- .../DisoseTaskAnalyzerTests.cs | 68 +++ ...LoosingScopeAnalyzerTests.MoveSemantics.cs | 51 ++- .../DisposeBeforeLoosingScopeAnalyzerTests.cs | 314 ++++++++++++- .../DiagnosticDescriptors.cs | 26 +- .../DisposableAttributes.cs | 2 +- .../DisposeBeforeLoosingScopeAnalyzer.cs | 412 +++++++++++++----- .../DisposeTaskAnalyzer.cs | 87 ++++ .../CSharpCodeFixVerifier`2+Test.cs | 3 +- 8 files changed, 848 insertions(+), 115 deletions(-) create mode 100644 src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisoseTaskAnalyzerTests.cs create mode 100644 src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeTaskAnalyzer.cs diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisoseTaskAnalyzerTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisoseTaskAnalyzerTests.cs new file mode 100644 index 0000000..7d4169b --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisoseTaskAnalyzerTests.cs @@ -0,0 +1,68 @@ +using NUnit.Framework; +using System.Threading.Tasks; +using ErrorProne.NET.DisposableAnalyzers; +using Verify = ErrorProne.NET.TestHelpers.CSharpCodeFixVerifier< + ErrorProne.NET.DisposableAnalyzers.DisposeTaskAnalyzer, + Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>; +using System; + +namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers +{ + [TestFixture] + public partial class DisoseTaskAnalyzerTests + { + private const string Disposable = + @" +public class Disposable : System.IDisposable + { + public void Dispose() { } + public void Close() {} + public static Disposable Create() => new Disposable(); + }"; + + private static Task VerifyAsync(string code) + { + + code = $"using System.Threading.Tasks;{Environment.NewLine}{code}{Environment.NewLine}{Disposable}"; + return Verify.VerifyAsync(code); + } + + [Test] + public async Task Warn_On_Using_Var_On_Task() + { + var test = @" +public class Test +{ + public static async Task ShouldDispose() + { + using var x = GetDisposableAsync(); + await Task.Yield(); + } + + private static Task GetDisposableAsync() => null; +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task Warn_On_Using_On_Task() + { + var test = @" +public class Test +{ + public static async Task ShouldDispose() + { + await Task.Yield(); + using (GetDisposableAsync()) + { + } + } + + private static Task GetDisposableAsync() => null; +} +"; + await VerifyAsync(test); + } + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs index 475c567..ed56c48 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs @@ -16,7 +16,7 @@ public partial class DisposeBeforeLoosingScopeAnalyzerTests public class AcquiresOwnershipAttribute : System.Attribute { } [System.AttributeUsage(System.AttributeTargets.All)] -public class ReleasesOwnershipAttribute : System.Attribute { } +public class ReturnsOwnershipAttribute : System.Attribute { } [System.AttributeUsage(System.AttributeTargets.Method)] public class KeepsOwnershipAttribute : System.Attribute { } @@ -31,6 +31,8 @@ public static T ReleaseOwnership([AcquiresOwnership]this T disposable) where { return disposable; } + + public static T ThrowIfNull(this T t) => t; }"; private const string Disposable = @@ -38,6 +40,8 @@ public static T ReleaseOwnership([AcquiresOwnership]this T disposable) where public class Disposable : System.IDisposable { public void Dispose() { } + public void Close() {} + public static Disposable Create() => new Disposable(); }"; [Test] @@ -54,6 +58,36 @@ public static void Moves() private static void TakesOwnership([AcquiresOwnership] Disposable d2) { d2.Dispose(); } } +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Out_Variable() + { + var test = @" +public class Test +{ + public void Moves(out Disposable d) + { + d = new Disposable(); + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_ActivitySource_StartActivity() + { + var test = @" +public class Test +{ + public static void StartActivityCase() + { + new System.Diagnostics.ActivitySource(""name"").StartActivity(); + } +} "; await VerifyAsync(test); } @@ -85,6 +119,21 @@ public static void Moves() var d = new Disposable().ReleaseOwnership(); } } +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_ThrowIf() + { + var test = @" +public class Test +{ + public static void Moves(Disposable d) + { + d.ThrowIfNull(); + } +} "; await VerifyAsync(test); } diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.cs index b88b0cf..d5c8f02 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.cs @@ -131,7 +131,7 @@ public class Disposable : System.IDisposable { public void Dispose() { } - [ReleasesOwnership] + [ReturnsOwnership] public static Disposable Instance => new Disposable(); } @@ -326,13 +326,138 @@ public class Test public static void ShouldDispose() { using (var d = new Disposable()) + using (var d2 = new Disposable()) + using (var d3 = Disposable.Create()) { } + } public class Disposable : System.IDisposable { public void Dispose() { } + public static Disposable Create() => new Disposable(); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_UsingStatement_In_Local() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + void useInHelper() + { + using (var d4 = Disposable.Create()) + { + } + } + + System.Threading.Tasks.Task.Run(() => + { + using (var d5 = Disposable.Create()) + { + } + }); + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_NullableDispose() + { + var test = @" +public class Test +{ + public static void ShouldDispose(bool shouldCreate) + { + Disposable d = null; + try + { + if (shouldCreate) + { + d = new Disposable(); + } + } + finally + { + d?.Dispose(); + } + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Nullable_Factory() + { + var test = @" +public class Test +{ + public static Disposable TryCreate(bool shouldCreate) + { + + try + { + Disposable d = new Disposable(); + return d; + } + catch + { + return null; + } + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_StreamReader() + { + var test = @" +public class Test +{ + public static void ShouldDispose() + { + using (var fs = new System.IO.FileStream(string.Empty, System.IO.FileMode.Open, System.IO.FileAccess.Read, System.IO.FileShare.ReadWrite)) + using (var sr = new System.IO.StreamReader(fs)) + { + } + } + + public class Disposable : System.IDisposable + { + public void Dispose() { } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Activity() + { + var test = @" +public class Test +{ + public class Activity : System.IDisposable + { + public Activity SetTag(string key, object value) => this; + public void Dispose() { } + } + + public static void ActivityCase(Activity a) + { + a.SetTag(""42"", 42); } } "; @@ -366,6 +491,130 @@ public void Dispose() { } await Verify.VerifyAsync(test); } + [Test] + public async Task NoWarn_On_Close_In_Finally() + { + var test = @" +public class Test +{ + public static void ShouldDispose(bool create) + { + Disposable d = null; + + try + { + if (create) + { + d = new Disposable(); + } + } + finally + { + if (d != null) + d.Close(); + } + + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Nested_Using() + { + var test = @" +public class Test +{ + public static void ShouldDispose(bool create) + { + Disposable d = null; + + if (create) + { + d = new Disposable(); + } + + using(d) + { + } + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Assigning_To_Field() + { + var test = @" +public class Test +{ + private Disposable _d; + public void ShouldDispose(bool create) + { + Disposable d = null; + + if (create) + { + d = new Disposable(); + } + + _d = d; + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Assigning_To_Field_With_Interlocked() + { + var test = @" +public class Test +{ + private Disposable _d; + public void ShouldDispose(bool create) + { + Disposable d = null; + + if (create) + { + d = new Disposable(); + } + + System.Threading.Interlocked.Exchange(ref _d, d); + } +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Assigning_To_Field_In_Constructor() + { + var test = @" +public class Test +{ + private Disposable _d; + private Disposable _d2; + public Test(bool create) + { + Disposable d = null; + + if (create) + { + d = new Disposable(); + } + + _d = d; + _d2 = new Disposable(); + } +} +"; + await VerifyAsync(test); + } + [Test] public async Task NoWarn_On_Dispose_In_Try_And_Catch() { @@ -394,6 +643,69 @@ public void Dispose() { } await Verify.VerifyAsync(test); } + [Test] + public async Task NoWarn_On_Disposable_Returned_In_Func() + { + var test = @" +public class Test +{ + internal static System.Func CreateInstance = () => new Disposable(); +} +"; + await VerifyAsync(test); + } + + // await using (var buildCoordinator = new BuildCoordinator( + + [Test] + public async Task NoWarn_On_Disposable_Returned_In_Func_InTask() + { + var test = @" +public class Test +{ + public static void TestTask() + { + System.Threading.Tasks.Task.Run(() => + { + var d = new Disposable(); + return d; + }); + } +}"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Type_Erasure() + { + // This should be covered by another rule. + var test = @" +public interface IFoo { } + +public class Foo : IFoo, System.IDisposable +{ + public void Dispose() { } + public static IFoo Create() => new Foo(); +} +"; + await VerifyAsync(test); + } + + [Test] + public async Task NoWarn_On_Yield_Return() + { + var test = @" +public class Test +{ + public static System.Collections.Generic.IEnumerable Get() + { + yield return new Disposable(); + } +} +"; + await VerifyAsync(test); + } + // [Test] Not supported yet. public async Task Warn_On_DisposeInFinally_Conditionally() { diff --git a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs index 7de4b9e..90eccf6 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs @@ -1,5 +1,7 @@ -using System.Collections.Immutable; +using System; +using System.Collections.Immutable; using System.Linq; +using System.Threading.Tasks; using ErrorProne.NET.CoreAnalyzers; using Microsoft.CodeAnalysis; @@ -127,9 +129,29 @@ internal static class DiagnosticDescriptors public static readonly DiagnosticDescriptor ERP041 = new DiagnosticDescriptor( nameof(ERP041), title: "Dispose objects before losing scope", - messageFormat: "A local variable {0} must be disposed before losing scope", + messageFormat: "A local variable '{0}' of type '{1}' must be disposed before losing scope", ReliabilityCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, description: "If a disposable object is not explicitly disposed before all references to it are out of scope, " + "the object will be disposed at some indeterminate time when the garbage collector runs the finalizer of the object."); + + /// + /// A rule that warns when a instance is disposed. + /// + public static readonly DiagnosticDescriptor ERP042 = new DiagnosticDescriptor( + nameof(ERP042), + title: "Do not dispose a Task instance", + messageFormat: "Task instances should not be disposed{0}", + ReliabilityCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "Task class implements IDisposable but the instances should not be disposed since they don't have actual managed resources"); + + /// + /// A rule that warns when a instance is disposed for Task<T> where T is . + /// + public static readonly DiagnosticDescriptor ERP043 = new DiagnosticDescriptor( + nameof(ERP043), + title: "Do not dispose a Task instance", + messageFormat: "Do not dispose a Task<{0}> instance. Do you want to dispose the result of the task instead?", + ReliabilityCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "It is possible that the intend is to dispose the result of a task but not the task itself."); } } \ No newline at end of file diff --git a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposableAttributes.cs b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposableAttributes.cs index a6eb41e..9fca2b5 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposableAttributes.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposableAttributes.cs @@ -9,7 +9,7 @@ public static class DisposableAttributes public const string KeepsOwnershipAttribute = "KeepsOwnershipAttribute"; // For properties to emphasize that the ownership is transferred. - public const string ReleasesOwnershipAttribute = "ReleasesOwnershipAttribute"; + public const string ReturnsOwnershipAttribute = "ReturnsOwnershipAttribute"; // For fields/properties, whe the ownership belongs to another type. diff --git a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzer.cs index fe8796c..12fc9dc 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzer.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzer.cs @@ -16,6 +16,7 @@ using System.Diagnostics.CodeAnalysis; using System.Reflection.Metadata; using System.Data.Common; +using System.Runtime.CompilerServices; using ErrorProne.NET.Extensions; namespace ErrorProne.NET.DisposableAnalyzers; @@ -53,7 +54,7 @@ private void AnalyzeMethodBody(OperationAnalysisContext context) { if (context.ContainingSymbol is not IMethodSymbol method || !HasAcquiresOwnershipParameters(method, out var parameter) - // Skipping methods marked with 'KeepOwnership' attribute. + // Skipping methods marked with 'KeepsOwnership' attribute. || method.HasAttributeWithName(DisposableAttributes.KeepsOwnershipAttribute)) { return; @@ -63,7 +64,7 @@ private void AnalyzeMethodBody(OperationAnalysisContext context) if (!context.Operation.HasAnyOperationDescendant(o => o.Kind == OperationKind.ParameterReference, out var operation)) { - context.ReportDiagnostic(Diagnostic.Create(Rule, syntax.Identifier.GetLocation(), parameter.Name)); + context.ReportDiagnostic(Diagnostic.Create(Rule, syntax.Identifier.GetLocation(), parameter.Name, parameter.Type.Name)); return; } @@ -88,7 +89,7 @@ private void AnalyzeMethodBody(OperationAnalysisContext context) return; } - context.ReportDiagnostic(Diagnostic.Create(Rule, syntax.Identifier.GetLocation(), parameter.Name)); + context.ReportDiagnostic(Diagnostic.Create(Rule, syntax.Identifier.GetLocation(), parameter.Name, parameter.Type.Name)); } private static bool HasAcquiresOwnershipParameters(IMethodSymbol method, [NotNullWhen(true)]out IParameterSymbol? result) @@ -111,7 +112,7 @@ private void AnalyzePropertyReference(OperationAnalysisContext context) var propertyReference = (IPropertyReferenceOperation)context.Operation; var helper = new DisposeAnalysisHelper(context.Compilation); - if (!ReleasesOwnership(propertyReference.Property, helper)) + if (!ReturnsOwnership(propertyReference.Property, helper)) { // We're not calling a factory method, so we're not taking an ownership of the object. return; @@ -125,7 +126,7 @@ private void AnalyzeMethodCall(OperationAnalysisContext context) var invocation = (IInvocationOperation)context.Operation; var helper = new DisposeAnalysisHelper(context.Compilation); - if (!ReleasesOwnership(invocation.TargetMethod, helper)) + if (!ReturnsOwnership(invocation.TargetMethod, helper)) { // We're not calling a factory method, so we're not taking an ownership of the object. return; @@ -140,9 +141,40 @@ private void AnalyzeObjectCreation(OperationAnalysisContext context) AnalyzeCore(context, objectCreation.Type, objectCreation); } + private static (ILocalSymbol? local, ISymbol? member) TryFindAssignmentTarget(IOperation operation) + { + foreach (var p in EnumerateParents(operation)) + { + if (p is IVariableDeclaratorOperation vdo) + { + return (local: vdo.Symbol, member: null); + } + + // TODO: can we support parameters as well? + //if (p is IParameterReferenceOperation pro) + //{ + // return p.par + //} + + if (p is IAssignmentOperation ao) + { + if (ao.Target is ILocalReferenceOperation lro) + { + return (local: lro.Local, member: null); + } + else if (ao.Target is IMemberReferenceOperation mro) + { + return (local: null, member: mro.Member); + } + } + } + + return (local: null, member: null); + } + private void AnalyzeCore(OperationAnalysisContext context, ITypeSymbol expressionType, IOperation operation) { - if (operation.Parent is IFieldInitializerOperation or IPropertyInitializerOperation) + if (EnumerateParents(operation).Any(o => o is IFieldInitializerOperation or IPropertyInitializerOperation)) { return; } @@ -156,83 +188,139 @@ private void AnalyzeCore(OperationAnalysisContext context, ITypeSymbol expressio // The operation might be a variable declaration. // Trying to figure it out since we have a ton of logic relying on that. - var variableDeclaration = EnumerateParents(operation).OfType().FirstOrDefault(); - var controlFlowGraph = context.GetControlFlowGraph(); - if (IsDisposedOrOwnershipIsMoved(variableDeclaration, operation, controlFlowGraph)) + var (localSymbol, member) = TryFindAssignmentTarget(operation); + + if (member is not null) + { + // Assigning this to a member. Don't need to dispose it. + return; + } + + // Checking for stuff like 'Activity.SetTag' that returns the same instance. + if (IsFluentApi(operation)) { return; } + // TODO BUG: it seems that for the following case we're getting the wrong control flow graph: + // public static void TestTask() + // { + // System.Threading.Tasks.Task.Run(() => + // { + // var d = new Disposable(); + // return d; + // }); + // } + // In this case for 'new Disposable' operation we're getting the control graph for the entire 'TestTask'. + // So skipping this case for now. + if (HasParent(operation)) + { + return; + } + + var controlFlowGraph = context.GetControlFlowGraph(); + if (IsDisposedOrOwnershipIsMoved(localSymbol, operation, controlFlowGraph)) + { + return; + } // Analyzing body of the method to see if all the arguments are disposed. - if (IsFactoryMethodLikeSignature(context.ContainingSymbol, helper) && IsFactoryMethodImpl(operation, variableDeclaration, controlFlowGraph)) + if ( + // Ignoring the signature for now, since we could have this: object FooBar() => new Disposable(); + // IsFactoryMethodLikeSignature(context.ContainingSymbol, helper) && + IsFactoryMethodImpl(operation, localSymbol, controlFlowGraph)) { // This is factory method/property so we can't dispose an argument since we're returning it. return; } - if (variableDeclaration != null) + if (DisposedInUsingOperation(localSymbol, controlFlowGraph) + || HasParent(operation)) { - // Checking if the parent is 'using' declaration or 'using' statement, then we're good + // We're good, this is 'using(new Disposable())' statement (this is the HasParent case) + // or 'using(disposable) {}' + return; + } - if (HasParent(variableDeclaration) || - HasParent(variableDeclaration)) - { - // Current object creation is a part of 'using' declaration. - return; - } + Location? location = null; + var variableDeclaration = EnumerateParents(operation).OfType().FirstOrDefault(); + if (variableDeclaration != null) + { if (variableDeclaration.Syntax is VariableDeclaratorSyntax vds) { - var identifierLocation = vds.Identifier.GetLocation(); - context.ReportDiagnostic(Diagnostic.Create(Rule, identifierLocation, variableDeclaration.Symbol.Name)); - return; + location = vds.Identifier.GetLocation(); } - - Contract.Assert(false, "Should not get here!"); } - // Variable declaration is null - if (HasParent(operation)) + location ??= localSymbol?.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax().GetLocation() + ?? operation.Syntax.GetLocation(); + context.ReportDiagnostic(Diagnostic.Create(Rule, location, localSymbol?.Name ?? operation.Syntax.ToString(), expressionType.Name)); + } + } + + private bool DisposedInUsingOperation(ILocalSymbol? localSymbol, ControlFlowGraph cfg) + { + if (localSymbol is null) + { + return false; + } + + foreach (var usingOperation in cfg.DescendantOperations().OfType()) + { + if (usingOperation.Locals.Any(lro => lro.Equals(localSymbol, SymbolEqualityComparer.Default))) { - // We're good, this is 'using(new Disposable())' statement - return; + return true; } - - var location = operation.Syntax.GetLocation(); - context.ReportDiagnostic(Diagnostic.Create(Rule, location, operation.Syntax.ToString())); } - } + + return false; + } /// /// Method returns true if a variable's ownership is moved to another place (method). /// private bool OwnershipIsMoved(LocalOrParameterReference localOrParam, ControlFlowGraph cfg) { - return OwnershipIsMoved(cfg, argumentMatches: a => localOrParam.IsReferenced(a.Value)); + return OwnershipIsMoved(cfg, + argumentMatches: a => localOrParam.IsReferenced(a.Value, cfg), + assignmentMatches: a => localOrParam.IsReferenced(a.Value, cfg)); } /// /// Method returns true if a variable's ownership is moved to another place (method). /// - private bool OwnershipIsMoved(ControlFlowGraph cfg, Func argumentMatches) + private bool OwnershipIsMoved(ControlFlowGraph cfg, + // Callback to handle invocations + Func argumentMatches, + Func assignmentMatches) { - foreach (var invocation in cfg.DescendantOperations().OfType()) + foreach (var descendant in cfg.DescendantOperations()) { - for (int i = 0; i < invocation.Arguments.Length; i++) + if (descendant is IInvocationOperation invocation) { - var a = invocation.Arguments[i]; - - if (argumentMatches(a)) + for (int i = 0; i < invocation.Arguments.Length; i++) { - var p = invocation.TargetMethod.Parameters[i]; - if (p.HasAttributeWithName(DisposableAttributes.AcquiresOwnershipAttribute)) + var a = invocation.Arguments[i]; + + if (argumentMatches(a)) { - return true; + var p = invocation.TargetMethod.Parameters[i]; + if (p.HasAttributeWithName(DisposableAttributes.AcquiresOwnershipAttribute)) + { + return true; + } } } } + else if (descendant is IAssignmentOperation ao) + { + if (assignmentMatches(ao)) + { + return true; + } + } } return false; @@ -252,12 +340,12 @@ private static bool IsLocal(IVariableDeclaratorOperation variableDeclaration, IO // public readonly record struct LocalOrParameterReference { - private readonly IVariableDeclaratorOperation? _variableDeclaration; + private readonly ILocalSymbol? _local; private readonly IParameterSymbol? _parameter; - public LocalOrParameterReference(IVariableDeclaratorOperation variableDeclaration) + public LocalOrParameterReference(ILocalSymbol local) { - _variableDeclaration = variableDeclaration; + _local = local; } public LocalOrParameterReference(IParameterSymbol parameter) @@ -265,19 +353,50 @@ public LocalOrParameterReference(IParameterSymbol parameter) _parameter = parameter; } - public static LocalOrParameterReference Create(IVariableDeclaratorOperation variableDeclaration) => new (variableDeclaration); + public static LocalOrParameterReference Create(ILocalSymbol variableDeclaration) => new (variableDeclaration); public static LocalOrParameterReference Create(IParameterSymbol parameter) => new (parameter); - public bool IsReferenced(IOperation operation) + public static LocalOrParameterReference Create(ISymbol symbol) { - if (operation is ILocalReferenceOperation lro && _variableDeclaration is not null) + return symbol switch { - return lro.Local.Equals(_variableDeclaration.Symbol, SymbolEqualityComparer.Default); + ILocalSymbol ls => new LocalOrParameterReference(ls), + IParameterSymbol ps => new LocalOrParameterReference(ps), + _ => default, + }; + } + + public bool IsReferenced(IOperation operation, ControlFlowGraph cfg) + { + if (operation is ILocalReferenceOperation lro && _local is not null) + { + return lro.Local.Equals(_local, SymbolEqualityComparer.Default); } else if (operation is IParameterReferenceOperation pro && _parameter is not null) { return pro.Parameter.Equals(_parameter, SymbolEqualityComparer.Default); } + else if (operation is IFlowCaptureReferenceOperation fcro) + { + var id = fcro.Id; + // Now we need to find FLowCaptureOperation with the same id. + foreach (var flowCaptureOperation in cfg.DescendantOperations(OperationKind + .FlowCapture)) + { + if (flowCaptureOperation.Id.Equals(id)) + { + if (IsReferenced(flowCaptureOperation.Value, cfg)) + { + return true; + } + } + } + } + + if (operation is IConversionOperation c) + { + return IsReferenced(c.Operand, cfg); + } return false; } @@ -288,7 +407,10 @@ public bool IsReferenced(IOperation operation) /// private bool IsFactoryMethodLikeSignature(ISymbol methodOrProperty, DisposeAnalysisHelper helper) { - if (!helper.IsDisposable(TryFindReturnType(methodOrProperty))) + var returnType = TryFindReturnType(methodOrProperty); + + if (!helper.IsDisposable(returnType) && + returnType?.Name != "IEnumerable") // TODO: do this properly { return false; } @@ -309,10 +431,27 @@ private bool IsFactoryMethodLikeSignature(ISymbol methodOrProperty, DisposeAnaly return true; } + /// + /// Returns true when a given looks like a fluent API, like Activity.SetTag that returns an existing activity. + /// + private bool IsFluentApi(IOperation operation) + { + (ISymbol? Symbol, bool IsStatic) methodOrProperty = operation switch + { + IInvocationOperation invocation => (invocation.TargetMethod, invocation.TargetMethod.IsStatic), + IPropertyReferenceOperation propertyReference => (propertyReference.Property, propertyReference.Property.IsStatic), + _ => default, + }; + + var returnType = TryFindReturnType(methodOrProperty.Symbol); + + return !methodOrProperty.IsStatic && returnType?.Equals(methodOrProperty.Symbol?.ContainingType, SymbolEqualityComparer.Default) == true; + } + /// /// Returns true when a given looks like a factory method and probably returns an ownership. /// - private bool ReleasesOwnership(ISymbol methodOrProperty, DisposeAnalysisHelper helper) + private bool ReturnsOwnership(ISymbol methodOrProperty, DisposeAnalysisHelper helper) { if (!helper.IsDisposable(TryFindReturnType(methodOrProperty))) { @@ -324,74 +463,91 @@ private bool ReleasesOwnership(ISymbol methodOrProperty, DisposeAnalysisHelper h return false; } - if (methodOrProperty.HasAttributeWithName(DisposableAttributes.ReleasesOwnershipAttribute)) + if (methodOrProperty.HasAttributeWithName(DisposableAttributes.ReturnsOwnershipAttribute)) { return true; } - // By default properties do not release ownership unless they are marked with `ReleasesOwnershipAttribute`. + // By default properties do not release ownership unless they are marked with `ReturnsOwnershipAttribute`. if (methodOrProperty is IPropertySymbol) { return false; } - // Special case for methods + // Special cases for methods + // If there is an argument, marked with `AcquiresOwnershipAttribute`, then it's not a factory method. - if (methodOrProperty is IMethodSymbol ms && HasAcquiresOwnershipParameters(ms, out _)) + + // Ignore generic methods without IDisposable constraint. + // Like x.ThrowIfNull() or something like that. + // Its not possible that such a generic method can actually return an ownership to call site. + if (methodOrProperty is IMethodSymbol ms) { - return false; + if (HasAcquiresOwnershipParameters(ms, out _)) + { + return false; + } + + var od = ms.OriginalDefinition; + if (od.IsGenericMethod && od.ReturnType is ITypeParameterSymbol tps && + !tps.ConstraintTypes.Any(helper.IsDisposable)) + { + // This is a generic method without IDisposable constraint. + return false; + } } return true; } - private bool IsFactoryMethodImpl(IOperation operation, IVariableDeclaratorOperation? variableDeclaration, ControlFlowGraph cfg) + private bool IsFactoryMethodImpl(IOperation operation, ILocalSymbol? localVariable, ControlFlowGraph cfg) { - if (variableDeclaration is null && HasParent(operation)) + if (localVariable is null && HasParent(operation)) { return true; } - if (variableDeclaration is null) + if (localVariable is null) { return false; } - var localVariable = variableDeclaration.Symbol; var branches = cfg.GetExit().Predecessors.Select(b => new BranchWithInfo(b)).ToArray(); - // We're fine if all the branches are returns and the return value is the local variable. - if (branches.All(b => - b.Kind == ControlFlowBranchSemantics.Return && b.BranchValue is ILocalReferenceOperation lro && - lro.Local.Equals(localVariable, SymbolEqualityComparer.Default))) + // OLD: We're fine if all the branches are returns and the return value is the local variable. + // Simplifying logic for now: If we hae any returns, then we're good! + // Since we have a ton of cases here! + if (branches.Any(b => + b.Kind == ControlFlowBranchSemantics.Return && isLocalReference(b.BranchValue) )) { return true; } - return false; + + bool isLocalReference(IOperation? branch) + { + return branch is ILocalReferenceOperation lro && lro.Local.Equals(localVariable, SymbolEqualityComparer.Default); + } } // TODO: move this to helpers - private static ITypeSymbol? TryFindReturnType(ISymbol operation) + private static ITypeSymbol? TryFindReturnType(ISymbol? operation) { - if (operation is IMethodSymbol ms && ms.MethodKind != MethodKind.Constructor) - { - return ms.ReturnType; - } - else if (operation is IPropertySymbol ps) + return operation switch { - return ps.Type; - } - - return null; + IMethodSymbol ms when ms.MethodKind != MethodKind.Constructor => ms.ReturnType, + IPropertySymbol ps => ps.Type, + IFieldSymbol fs => fs.Type, + _ => null + }; } - private bool IsDisposedOrOwnershipIsMoved(IVariableDeclaratorOperation? variableDeclaration, IOperation operation, ControlFlowGraph cfg) + private bool IsDisposedOrOwnershipIsMoved(ILocalSymbol? variableDeclaration, IOperation operation, ControlFlowGraph cfg) { if (variableDeclaration != null) { - if (IsDisposed(variableDeclaration.Symbol, cfg) || + if (IsDisposed(variableDeclaration, cfg) || OwnershipIsMoved(LocalOrParameterReference.Create(variableDeclaration), cfg)) { return true; @@ -403,7 +559,8 @@ private bool IsDisposedOrOwnershipIsMoved(IVariableDeclaratorOperation? variable if (OwnershipIsMoved(cfg, // Using syntax node for comparison. Which is weird, since we'll get here // two different object creation operations. - a => { return operation.Syntax.IsEquivalentTo(a.Value.Syntax); })) + argumentMatches: a => { return operation.Syntax.IsEquivalentTo(a.Value.Syntax); }, + assignmentMatches: a => false)) { return true; } @@ -411,51 +568,88 @@ private bool IsDisposedOrOwnershipIsMoved(IVariableDeclaratorOperation? variable // Checking if the ownership is moved. return false; } - + private bool IsDisposed(ISymbol localVariable, ControlFlowGraph cfg) { - var branches = cfg.GetExit().Predecessors.Select(b => new BranchWithInfo(b)).ToArray(); + // Relatively naive approach that checks if 'Dispose' or 'Close' was ever called. + // There are no checks on whether the calls happened in all branches. - // Using a naive approach for now: if the variable is disposed in finally - // or in both: try and catch blocks, then we're good. - bool disposedInTry = false; - bool disposedInCatch = false; - foreach (var block in cfg.Blocks) + foreach (var invocation in cfg.DescendantOperations(OperationKind.Invocation)) { - if (block.IsReachable && block.Kind == BasicBlockKind.Block && - block.ConditionKind == ControlFlowConditionKind.None && DisposedUnconditionallyIn(block, localVariable)) + if (invocation.TargetMethod.Name is "Dispose" or "Close") { - // We have an unconditional Dispose in one of the blocks. - return true; - } - - if (block.IsFinallyBlock() && DisposedUnconditionallyIn(block, localVariable)) - { - return true; - } - - if (block.IsTryBlock() && DisposedUnconditionallyIn(block, localVariable)) - { - disposedInTry = true; - - if (disposedInCatch) + if (LocalOrParameterReference.Create(localVariable).IsReferenced(invocation.Instance, cfg)) { return true; } - } - if (block.IsCatchBlock() && DisposedUnconditionallyIn(block, localVariable)) - { - disposedInCatch = true; - - if (disposedInTry) + // This looks overly complicated to but here me out. + // Disposable d = null; + // using (d) {} + // Is lowered to something an IInvocationOperation with some special stuff around it like conversion. + // And such lowered 'IInvocationOperation' doesn't reference a local variable directly, + // instead it goes through IFlowCaptureOperation and + if (invocation.Instance is IConversionOperation co && co.Operand is IFlowCaptureReferenceOperation fcro) { - return true; + var id = fcro.Id; + // Now we need to find FLowCaptureOperation with the same id. + foreach (var flowCaptureOperation in cfg.DescendantOperations(OperationKind.FlowCapture)) + { + if (flowCaptureOperation.Id.Equals(id)) + { + if (LocalOrParameterReference.Create(localVariable).IsReferenced(flowCaptureOperation.Value, cfg)) + { + return true; + } + } + } } } } - + return false; + //var branches = cfg.GetExit().Predecessors.Select(b => new BranchWithInfo(b)).ToArray(); + + //// Using a naive approach for now: if the variable is disposed in finally + //// or in both: try and catch blocks, then we're good. + //bool disposedInTry = false; + //bool disposedInCatch = false; + //foreach (var block in cfg.Blocks) + //{ + // if (block.IsReachable && block.Kind == BasicBlockKind.Block && + // block.ConditionKind == ControlFlowConditionKind.None && DisposedUnconditionallyIn(block, localVariable)) + // { + // // We have an unconditional Dispose in one of the blocks. + // return true; + // } + + // if (block.IsFinallyBlock() && DisposedUnconditionallyIn(block, localVariable)) + // { + // return true; + // } + + // if (block.IsTryBlock() && DisposedUnconditionallyIn(block, localVariable)) + // { + // disposedInTry = true; + + // if (disposedInCatch) + // { + // return true; + // } + // } + + // if (block.IsCatchBlock() && DisposedUnconditionallyIn(block, localVariable)) + // { + // disposedInCatch = true; + + // if (disposedInTry) + // { + // return true; + // } + // } + //} + + //return false; } private bool DisposedUnconditionallyIn(BasicBlock block, ISymbol localVariable) @@ -467,7 +661,7 @@ private bool DisposedUnconditionallyIn(BasicBlock block, ISymbol localVariable) (invocation.Instance is IParameterReferenceOperation pro && pro.Parameter.Equals(localVariable, SymbolEqualityComparer.Default))) { // TODO: cover if blocks. - if (invocation.TargetMethod.Name == "Dispose") + if (invocation.TargetMethod.Name is "Dispose" or "Close") { return true; } diff --git a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeTaskAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeTaskAnalyzer.cs new file mode 100644 index 0000000..32b6022 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeTaskAnalyzer.cs @@ -0,0 +1,87 @@ +using ErrorProne.NET.CoreAnalyzers; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Operations; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using ErrorProne.NET.Core; + +namespace ErrorProne.NET.DisposableAnalyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class DisposeTaskAnalyzer : DiagnosticAnalyzerBase +{ + /// + public override bool ReportDiagnosticsOnGeneratedCode => false; + + /// + public DisposeTaskAnalyzer() + : base(DiagnosticDescriptors.ERP042, DiagnosticDescriptors.ERP043) + { + } + + /// + protected override void InitializeCore(AnalysisContext context) + { + context.RegisterOperationAction(AnalyzeUsing, OperationKind.Using); + context.RegisterOperationAction(AnalyzeUsingDeclaration, OperationKind.UsingDeclaration); + } + + private void AnalyzeUsingDeclaration(OperationAnalysisContext context) + { + var usingDeclaration = (IUsingDeclarationOperation)context.Operation; + + // looking for a variable initializer inside 'using var xxx = ...' + var variableInitializer = usingDeclaration.EnumerateChildren().OfType().FirstOrDefault(); + + if (variableInitializer == null) + { + return; + } + + if (variableInitializer.Value.Type is INamedTypeSymbol t && t.IsTaskLike(context.Compilation)) + { + if (t.TypeArguments.Length == 0) + { + + } + } + + + + + } + + private void AnalyzeUsing(OperationAnalysisContext context) + { + + } + +} + +public static class OperationsExtensions +{ + public static IEnumerable EnumerateParents(this IOperation? operation) + { + while (operation != null) + { + operation = operation.Parent; + yield return operation; + } + } + + public static IEnumerable EnumerateChildren(this IOperation? operation) + { + if (operation is null) + { + yield break; + } + + yield return operation; + foreach (var o in operation.Children.SelectMany(c => c.EnumerateChildren())) + { + yield return o; + } + } +} diff --git a/src/RoslynNunitTestRunner/CSharpCodeFixVerifier`2+Test.cs b/src/RoslynNunitTestRunner/CSharpCodeFixVerifier`2+Test.cs index bbeb3e5..81b6113 100644 --- a/src/RoslynNunitTestRunner/CSharpCodeFixVerifier`2+Test.cs +++ b/src/RoslynNunitTestRunner/CSharpCodeFixVerifier`2+Test.cs @@ -15,7 +15,8 @@ public static Task VerifyAsync(string code) { return new Test { - TestState = { Sources = { code } } + TestState = { Sources = { code } }, + LanguageVersion = LanguageVersion.Latest }.WithoutGeneratedCodeVerification().RunAsync(); }