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/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 new file mode 100644 index 0000000..ed56c48 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs @@ -0,0 +1,273 @@ +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 ReturnsOwnershipAttribute : 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; + } + + public static T ThrowIfNull(this T t) => t; +}"; + + private const string Disposable = + @" +public class Disposable : System.IDisposable + { + public void Dispose() { } + public void Close() {} + public static Disposable Create() => new Disposable(); + }"; + + [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_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); + } + + [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_ThrowIf() + { + var test = @" +public class Test +{ + public static void Moves(Disposable d) + { + d.ThrowIfNull(); + } +} +"; + 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..d5c8f02 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.cs @@ -0,0 +1,819 @@ +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() { } + + [ReturnsOwnership] + 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()) + 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); + } +} +"; + 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_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() + { + 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] + 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() + { + 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..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; @@ -9,6 +11,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 +122,36 @@ 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}' 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 new file mode 100644 index 0000000..9fca2b5 --- /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 ReturnsOwnershipAttribute = "ReturnsOwnershipAttribute"; + + + // 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..12fc9dc --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzer.cs @@ -0,0 +1,828 @@ +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 System.Runtime.CompilerServices; +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 'KeepsOwnership' 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, parameter.Type.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, parameter.Type.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 (!ReturnsOwnership(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 (!ReturnsOwnership(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 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 (EnumerateParents(operation).Any(o => o 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 (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 ( + // 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 (DisposedInUsingOperation(localSymbol, controlFlowGraph) + || HasParent(operation)) + { + // We're good, this is 'using(new Disposable())' statement (this is the HasParent case) + // or 'using(disposable) {}' + return; + } + + Location? location = null; + + var variableDeclaration = EnumerateParents(operation).OfType().FirstOrDefault(); + if (variableDeclaration != null) + { + if (variableDeclaration.Syntax is VariableDeclaratorSyntax vds) + { + location = vds.Identifier.GetLocation(); + } + } + + 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))) + { + return true; + } + } + + 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, 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, + // Callback to handle invocations + Func argumentMatches, + Func assignmentMatches) + { + foreach (var descendant in cfg.DescendantOperations()) + { + if (descendant is IInvocationOperation invocation) + { + 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; + } + } + } + } + else if (descendant is IAssignmentOperation ao) + { + if (assignmentMatches(ao)) + { + 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 ILocalSymbol? _local; + private readonly IParameterSymbol? _parameter; + + public LocalOrParameterReference(ILocalSymbol local) + { + _local = local; + } + + public LocalOrParameterReference(IParameterSymbol parameter) + { + _parameter = parameter; + } + + public static LocalOrParameterReference Create(ILocalSymbol variableDeclaration) => new (variableDeclaration); + public static LocalOrParameterReference Create(IParameterSymbol parameter) => new (parameter); + + public static LocalOrParameterReference Create(ISymbol symbol) + { + return symbol switch + { + 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; + } + } + + /// + /// Returns true when a given looks like a factory method from a return type perspective. + /// + private bool IsFactoryMethodLikeSignature(ISymbol methodOrProperty, DisposeAnalysisHelper helper) + { + var returnType = TryFindReturnType(methodOrProperty); + + if (!helper.IsDisposable(returnType) && + returnType?.Name != "IEnumerable") // TODO: do this properly + { + 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 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 ReturnsOwnership(ISymbol methodOrProperty, DisposeAnalysisHelper helper) + { + if (!helper.IsDisposable(TryFindReturnType(methodOrProperty))) + { + return false; + } + + if (methodOrProperty.HasAttributeWithName(DisposableAttributes.KeepsOwnershipAttribute)) + { + return false; + } + + if (methodOrProperty.HasAttributeWithName(DisposableAttributes.ReturnsOwnershipAttribute)) + { + return true; + } + + // By default properties do not release ownership unless they are marked with `ReturnsOwnershipAttribute`. + if (methodOrProperty is IPropertySymbol) + { + return false; + } + + // Special cases for methods + + // If there is an argument, marked with `AcquiresOwnershipAttribute`, then it's not a factory method. + + // 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) + { + 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, ILocalSymbol? localVariable, ControlFlowGraph cfg) + { + if (localVariable is null && HasParent(operation)) + { + return true; + } + + if (localVariable is null) + { + return false; + } + + var branches = cfg.GetExit().Predecessors.Select(b => new BranchWithInfo(b)).ToArray(); + + // 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) + { + return operation switch + { + IMethodSymbol ms when ms.MethodKind != MethodKind.Constructor => ms.ReturnType, + IPropertySymbol ps => ps.Type, + IFieldSymbol fs => fs.Type, + _ => null + }; + } + + private bool IsDisposedOrOwnershipIsMoved(ILocalSymbol? variableDeclaration, IOperation operation, ControlFlowGraph cfg) + { + if (variableDeclaration != null) + { + if (IsDisposed(variableDeclaration, 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. + argumentMatches: a => { return operation.Syntax.IsEquivalentTo(a.Value.Syntax); }, + assignmentMatches: a => false)) + { + return true; + } + + // Checking if the ownership is moved. + return false; + } + + private bool IsDisposed(ISymbol localVariable, ControlFlowGraph cfg) + { + // Relatively naive approach that checks if 'Dispose' or 'Close' was ever called. + // There are no checks on whether the calls happened in all branches. + + foreach (var invocation in cfg.DescendantOperations(OperationKind.Invocation)) + { + if (invocation.TargetMethod.Name is "Dispose" or "Close") + { + if (LocalOrParameterReference.Create(localVariable).IsReferenced(invocation.Instance, cfg)) + { + return true; + } + + // 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) + { + 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) + { + 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 is "Dispose" or "Close") + { + 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/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/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 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(); }