From 23051b6386ebf95d869e962a94ad2a12f08a33af Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Wed, 13 Sep 2023 14:20:42 -0700 Subject: [PATCH 1/4] 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/4] 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(); } From 79c89549aec339f1f7e87f73c6ee00d8cbc9e24a Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Mon, 7 Sep 2026 08:16:40 -0700 Subject: [PATCH 3/4] Fix ownership review feedback and complete generated annotations Match constructed generic types against original definitions without conflating different type arguments. Add direct type-relationship regressions, correct ownership analyzer and test names, and remove review-reported whitespace. Complete generated non-ownership annotations with MustUseReturnValue and DoNotUseConfigureAwait, including legacy ConfigureAwait attribute reuse, metadata coverage, and documentation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/Rules/EPC34.md | 9 ++ .../AnnotationsGenerator.cs | 24 ++++ src/ErrorProne.NET.Annotations/README.md | 38 +++++- .../Annotations/AnnotationsGeneratorTests.cs | 127 +++++++++++++++--- ...foreLosingScopeAnalyzerTests.Borrowing.cs} | 2 +- ...eLosingScopeAnalyzerTests.FreshReturns.cs} | 2 +- ...foreLosingScopeAnalyzerTests.Inference.cs} | 2 +- ...LosingScopeAnalyzerTests.MoveSemantics.cs} | 4 +- ...ngScopeAnalyzerTests.ReviewRegressions.cs} | 2 +- ... DisposeBeforeLosingScopeAnalyzerTests.cs} | 4 +- ...erTests.cs => DisposeTaskAnalyzerTests.cs} | 4 +- .../OwnershipContractsTests.cs | 2 +- .../TypeExtensionsTests.cs | 87 ++++++++++++ .../AnalyzerReleases.Unshipped.md | 2 +- .../DisposableAttributes.cs | 5 +- ...cs => DisposeBeforeLosingScopeAnalyzer.cs} | 4 +- .../DisposableAnalyzers/OwnershipInference.cs | 18 +-- .../SwallowAllExceptionsAnalyzer.cs | 1 - .../TypeExtensions.cs | 6 +- 19 files changed, 297 insertions(+), 46 deletions(-) rename src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/{DisposeBeforeLoosingScopeAnalyzerTests.Borrowing.cs => DisposeBeforeLosingScopeAnalyzerTests.Borrowing.cs} (99%) rename src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/{DisposeBeforeLoosingScopeAnalyzerTests.FreshReturns.cs => DisposeBeforeLosingScopeAnalyzerTests.FreshReturns.cs} (99%) rename src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/{DisposeBeforeLoosingScopeAnalyzerTests.Inference.cs => DisposeBeforeLosingScopeAnalyzerTests.Inference.cs} (99%) rename src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/{DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs => DisposeBeforeLosingScopeAnalyzerTests.MoveSemantics.cs} (97%) rename src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/{DisposeBeforeLoosingScopeAnalyzerTests.ReviewRegressions.cs => DisposeBeforeLosingScopeAnalyzerTests.ReviewRegressions.cs} (99%) rename src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/{DisposeBeforeLoosingScopeAnalyzerTests.cs => DisposeBeforeLosingScopeAnalyzerTests.cs} (99%) rename src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/{DisoseTaskAnalyzerTests.cs => DisposeTaskAnalyzerTests.cs} (96%) create mode 100644 src/ErrorProne.NET.CoreAnalyzers.Tests/TypeExtensionsTests.cs rename src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/{DisposeBeforeLoosingScopeAnalyzer.cs => DisposeBeforeLosingScopeAnalyzer.cs} (99%) diff --git a/docs/Rules/EPC34.md b/docs/Rules/EPC34.md index ce7aaab..a52cc9d 100644 --- a/docs/Rules/EPC34.md +++ b/docs/Rules/EPC34.md @@ -6,6 +6,15 @@ This analyzer detects when methods marked with `MustUseResultAttribute` have the The analyzer warns when the return value of a method decorated with `MustUseResultAttribute` is not used. This attribute indicates that the method's return value contains important information that should always be observed by the caller. +## Getting the attribute + +The [ErrorProne.Net.Annotations source generator](../../src/ErrorProne.NET.Annotations/README.md) +provides `MustUseResultAttribute` and the recognized compatibility name +`MustUseReturnValueAttribute`. It embeds internal types in the project's root +namespace without adding a runtime annotation DLL. Attributes on public APIs +remain visible to downstream analyzers through assembly metadata. +Applications can also continue defining their own attributes, as shown below. + ## Code that triggers the analyzer ```csharp diff --git a/src/ErrorProne.NET.Annotations/AnnotationsGenerator.cs b/src/ErrorProne.NET.Annotations/AnnotationsGenerator.cs index 86b1db0..e5fb095 100644 --- a/src/ErrorProne.NET.Annotations/AnnotationsGenerator.cs +++ b/src/ErrorProne.NET.Annotations/AnnotationsGenerator.cs @@ -31,7 +31,9 @@ private static readonly (string Name, string Targets, string Summary)[] Attribut ("KeepsOwnershipAttribute", "Method | Property | ReturnValue", "Compatibility annotation for a borrowed result; prefer DoNotDispose."), ("NoOwnershipAttribute", "Parameter | Field | Property", "Compatibility annotation for borrowed parameters and members; prefer DoNotDispose."), ("MustUseResultAttribute", "Method", "Requires the caller to observe the method result."), + ("MustUseReturnValueAttribute", "Method", "Compatibility annotation for a method result that must be observed; prefer MustUseResult."), ("UseConfigureAwaitFalseAttribute", "Assembly", "Requires ConfigureAwait(false) for awaits in this assembly."), + ("DoNotUseConfigureAwaitAttribute", "Assembly", "Marks ConfigureAwait(false) as redundant for awaits in this assembly."), }; public void Initialize(IncrementalGeneratorInitializationContext context) @@ -69,6 +71,14 @@ public void Initialize(IncrementalGeneratorInitializationContext context) var metadataName = metadataNamespace.Length == 0 ? attribute.Name : metadataNamespace + "." + attribute.Name; var existing = compilation.GetTypesByMetadataName(metadataName); + if (attribute.Name is "UseConfigureAwaitFalseAttribute" or "DoNotUseConfigureAwaitAttribute") + { + // These assembly policies also recognize legacy attribute classes without the suffix. + var legacyName = metadataName.Substring(0, metadataName.Length - nameof(Attribute).Length); + existing = existing.AddRange(compilation.GetTypesByMetadataName(legacyName) + .Where(type => IsAttributeType(type, compilation))); + } + if (existing.Any(type => SymbolEqualityComparer.Default.Equals(type.ContainingAssembly, compilation.Assembly))) { continue; @@ -108,6 +118,20 @@ internal sealed class " + attribute.Name + @" : global::System.Attribute }); } + private static bool IsAttributeType(INamedTypeSymbol type, Compilation compilation) + { + var attributeType = compilation.GetTypeByMetadataName("System.Attribute"); + for (INamedTypeSymbol? current = type; current != null; current = current.BaseType) + { + if (SymbolEqualityComparer.Default.Equals(current, attributeType)) + { + return true; + } + } + + return false; + } + private static bool TryGetNamespace(string value, out string sourceNamespace, out string metadataNamespace) { value = value.Trim(); diff --git a/src/ErrorProne.NET.Annotations/README.md b/src/ErrorProne.NET.Annotations/README.md index e232faa..2f3f459 100644 --- a/src/ErrorProne.NET.Annotations/README.md +++ b/src/ErrorProne.NET.Annotations/README.md @@ -73,11 +73,47 @@ The generator provides: - `DoNotDispose`: borrowed parameters, fields, properties, and results. - `KeepsOwnership` and `NoOwnership`: compatibility ownership names. - `MustUseResult`: method results that must be observed. -- `UseConfigureAwaitFalse`: assembly-wide ConfigureAwait policy. +- `MustUseReturnValue`: a recognized compatibility name for `MustUseResult`. +- `UseConfigureAwaitFalse`: assembly-wide policy requiring ConfigureAwait. +- `DoNotUseConfigureAwait`: assembly-wide policy marking `ConfigureAwait(false)` as redundant. + +### Non-ownership annotations + +These annotations work independently of disposable ownership: + +```csharp +// With RootNamespace = MyProject, choose the assembly policy appropriate for the project: +[assembly: MyProject.UseConfigureAwaitFalse] +// Alternatively: [assembly: MyProject.DoNotUseConfigureAwait] + +namespace MyProject +{ + public static class Validation + { + [MustUseResult] + public static bool IsValid(string value) => !string.IsNullOrEmpty(value); + } +} +``` + +Ignoring an annotated method's result reports [EPC34](../../docs/Rules/EPC34.md), +including an ignored awaited result. `MustUseResult` is the preferred name; +`MustUseReturnValue` has the same analyzer behavior. Assembly-level policies +enable [EPC15](../../docs/Rules/EPC15.md) or [EPC14](../../docs/Rules/EPC14.md), +respectively. Generating the attribute types alone does not select a policy: +apply the assembly attribute explicitly. + +All of these types are internal and source-embedded, just like the ownership +attributes. They add no runtime DLL dependency. + +## Existing definitions If an accessible type with the same fully qualified name already exists, its definition is used instead of generating a duplicate. Inaccessible internal definitions in other assemblies do not prevent generating a local copy. +For the ConfigureAwait assembly policies, recognized legacy attribute classes +named `UseConfigureAwaitFalse` or `DoNotUseConfigureAwait` without the `Attribute` +suffix are also reused to avoid ambiguous attribute names. Definitions reachable only through an `extern alias` also do not prevent local generation, since the generated source cannot use those types by their namespace. Friend/test assemblies can omit the generator when `InternalsVisibleTo` already diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/Annotations/AnnotationsGeneratorTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/Annotations/AnnotationsGeneratorTests.cs index 6c50b79..1c019bd 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/Annotations/AnnotationsGeneratorTests.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/Annotations/AnnotationsGeneratorTests.cs @@ -5,6 +5,7 @@ using System.Threading; using System.Threading.Tasks; using ErrorProne.NET.Annotations.Generation; +using ErrorProne.NET.AsyncAnalyzers; using ErrorProne.NET.DisposableAnalyzers; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; @@ -18,6 +19,8 @@ namespace ErrorProne.NET.CoreAnalyzers.Tests.Annotations; [TestFixture] public sealed class AnnotationsGeneratorTests { + private const int AnnotationCount = 9; + private const string LibrarySource = @" [assembly: Library.UseConfigureAwaitFalse] namespace Library @@ -29,6 +32,7 @@ public static class Api [return: DoNotDispose] public static Resource GetShared() { return null; } public static void Take([AcquiresOwnership] Resource resource) { resource.Dispose(); } [MustUseResult] public static int Observe() { return 42; } + [MustUseReturnValue] public static int ObserveAlias() { return 42; } } }"; @@ -79,9 +83,10 @@ private static PortableExecutableReference Emit(CSharpCompilation compilation, b [TestCase(LanguageVersion.Latest)] public async Task Generates_Internal_Annotations_Without_Runtime_Dependencies(LanguageVersion languageVersion) { - var compilation = Generate(await CreateCompilation("Library", LibrarySource, languageVersion), 7); + var compilation = Generate(await CreateCompilation("Library", LibrarySource, languageVersion), AnnotationCount); foreach (var name in new[] { "AcquiresOwnership", "ReturnsOwnership", "DoNotDispose", - "KeepsOwnership", "NoOwnership", "MustUseResult", "UseConfigureAwaitFalse" }) + "KeepsOwnership", "NoOwnership", "MustUseResult", "MustUseReturnValue", + "UseConfigureAwaitFalse", "DoNotUseConfigureAwait" }) { var attribute = compilation.GetTypeByMetadataName("Library." + name + "Attribute"); Assert.That(attribute, Is.Not.Null); @@ -104,12 +109,38 @@ namespace Library { internal sealed class DoNotDisposeAttribute : System.Attribute { } }"); - Generate(compilation, 6); + Generate(compilation, AnnotationCount - 1); } - [TestCase(null, 6, "ExistingAnnotations")] - [TestCase("Existing", 7, "Consumer")] - [TestCase("global,Existing", 6, "ExistingAnnotations")] + [TestCase("UseConfigureAwaitFalse")] + [TestCase("DoNotUseConfigureAwait")] + public async Task Reuses_An_Existing_Unsuffixed_ConfigureAwait_Attribute(string name) + { + var compilation = await CreateCompilation("Library", @" +[assembly: Library." + name + @"] +namespace Library +{ + [System.AttributeUsage(System.AttributeTargets.Assembly)] + internal sealed class " + name + @" : System.Attribute { } +}"); + Generate(compilation, AnnotationCount - 1); + } + + [Test] + public async Task Does_Not_Treat_An_Unsuffixed_NonAttribute_Class_As_An_Annotation() + { + var compilation = await CreateCompilation("Library", @" +[assembly: Library.DoNotUseConfigureAwait] +namespace Library +{ + internal sealed class DoNotUseConfigureAwait { } +}"); + Generate(compilation, AnnotationCount); + } + + [TestCase(null, AnnotationCount - 1, "ExistingAnnotations")] + [TestCase("Existing", AnnotationCount, "Consumer")] + [TestCase("global,Existing", AnnotationCount - 1, "ExistingAnnotations")] public async Task Reuses_Referenced_Attributes_Only_When_Globally_Accessible( string? aliases, int expectedCount, string expectedAssembly) { @@ -142,7 +173,7 @@ public class Api [TestCase(true)] public async Task Internal_Annotations_On_Public_APIs_Survive_Assembly_Boundaries(bool referenceAssembly) { - var library = Generate(await CreateCompilation("Library", LibrarySource), 7); + var library = Generate(await CreateCompilation("Library", LibrarySource), AnnotationCount); var reference = Emit(library, referenceAssembly); var consumer = await CreateCompilation("Consumer", @" public class UseLibrary @@ -156,6 +187,7 @@ public void Run() Library.Api.Take(transferred); transferred.Dispose(); Library.Api.Observe(); + Library.Api.ObserveAlias(); } }", reference: reference); Assert.That(consumer.GetDiagnostics().Where(d => d.Severity >= DiagnosticSeverity.Warning), Is.Empty); @@ -169,9 +201,9 @@ public void Run() == "UseConfigureAwaitFalseAttribute"), Is.True); var diagnostics = await consumer.WithAnalyzers(ImmutableArray.Create( - new DisposeBeforeLoosingScopeAnalyzer(), new MustUseResultAnalyzer())).GetAnalyzerDiagnosticsAsync(); + new DisposeBeforeLosingScopeAnalyzer(), new MustUseResultAnalyzer())).GetAnalyzerDiagnosticsAsync(); Assert.That(diagnostics.Select(d => d.Id).OrderBy(id => id), - Is.EqualTo(new[] { "EPC34", "ERP044", "ERP046", "ERP046" })); + Is.EqualTo(new[] { "EPC34", "EPC34", "ERP044", "ERP046", "ERP046" })); // The consumer can also annotate its own API without reusing inaccessible library types. var annotatedConsumer = await CreateCompilation("AnnotatedConsumer", @" @@ -183,7 +215,72 @@ public class ConsumerApi public System.IDisposable GetShared() { return Library.Api.GetShared(); } } }", reference: reference); - Generate(annotatedConsumer, 7); + Generate(annotatedConsumer, AnnotationCount); + } + + [TestCase("MustUseResult")] + [TestCase("MustUseReturnValue")] + public async Task Generated_Result_Annotations_Require_Observing_Sync_And_Async_Results(string attribute) + { + var compilation = Generate(await CreateCompilation("Library", @" +using System.Threading.Tasks; +namespace Library +{ + public static class Api + { + [" + attribute + @"] public static int Observe() { return 42; } + [" + attribute + @"] public static Task ObserveAsync() { return Task.FromResult(42); } + public static async Task Run() + { + Observe(); + _ = Observe(); + await ObserveAsync(); + _ = await ObserveAsync(); + } + } +}"), AnnotationCount); + + var diagnostics = await compilation.WithAnalyzers(ImmutableArray.Create( + new MustUseResultAnalyzer())).GetAnalyzerDiagnosticsAsync(); + Assert.That(diagnostics.Select(d => d.Id), Is.EqualTo(new[] { "EPC34", "EPC34" })); + } + + [TestCase(null, null)] + [TestCase("UseConfigureAwaitFalse", "EPC15")] + [TestCase("DoNotUseConfigureAwait", "EPC14")] + public async Task Generated_Assembly_Annotations_Configure_Await_Policy(string? attribute, string? expectedDiagnostic) + { + var assemblyAttribute = attribute == null ? "" : "[assembly: Library." + attribute + "]\n"; + var compilation = Generate(await CreateCompilation("Library", "using System.Threading.Tasks;\n" + assemblyAttribute + @" +namespace Library +{ + public static class Api + { + public static async Task Run() + { + await Task.Delay(1); + await Task.Delay(1).ConfigureAwait(false); + } + } +}"), AnnotationCount); + + var diagnostics = await compilation.WithAnalyzers(ImmutableArray.Create( + new ConfigureAwaitRequiredAnalyzer(), new RedundantConfigureAwaitFalseAnalyzer())).GetAnalyzerDiagnosticsAsync(); + Assert.That(diagnostics.Select(d => d.Id), Is.EqualTo(expectedDiagnostic == null + ? System.Array.Empty() : new[] { expectedDiagnostic })); + } + + [TestCase("MustUseResult", System.AttributeTargets.Method)] + [TestCase("MustUseReturnValue", System.AttributeTargets.Method)] + [TestCase("UseConfigureAwaitFalse", System.AttributeTargets.Assembly)] + [TestCase("DoNotUseConfigureAwait", System.AttributeTargets.Assembly)] + public async Task Generated_Non_Ownership_Annotations_Have_Expected_Targets( + string name, System.AttributeTargets expectedTargets) + { + var compilation = Generate(await CreateCompilation("Library", ""), AnnotationCount); + var attribute = compilation.GetTypeByMetadataName("Library." + name + "Attribute")!; + var usage = attribute.GetAttributes().Single(a => a.AttributeClass?.Name == "AttributeUsageAttribute"); + Assert.That(usage.ConstructorArguments.Single().Value, Is.EqualTo((int)expectedTargets)); } [TestCase("Azure.Core", null, "Azure.Core")] @@ -193,7 +290,7 @@ public class ConsumerApi [TestCase("class.namespace", null, "class.namespace")] public async Task Uses_Configured_Namespace(string rootNamespace, string? namespaceOverride, string expected) { - var compilation = Generate(await CreateCompilation("DifferentAssemblyName", ""), 7, rootNamespace, namespaceOverride); + var compilation = Generate(await CreateCompilation("DifferentAssemblyName", ""), AnnotationCount, rootNamespace, namespaceOverride); var prefix = expected.Length == 0 ? "" : expected + "."; Assert.That(compilation.GetTypeByMetadataName(prefix + "DoNotDisposeAttribute"), Is.Not.Null); } @@ -241,7 +338,7 @@ public class Api public async Task Reuses_Accessible_Friend_Attribute_Instead_Of_Duplicating_It() { var compilation = await CreateCompilation("Consumer", "", reference: await CreateFriendLibrary("Library")); - var generated = Generate(compilation, 6, rootNamespace: "Azure.Core"); + var generated = Generate(compilation, AnnotationCount - 1, rootNamespace: "Azure.Core"); Assert.That(generated.GetTypeByMetadataName("Azure.Core.DoNotDisposeAttribute")! .ContainingAssembly.Name, Is.EqualTo("Library")); } @@ -253,9 +350,9 @@ public async Task Ambiguous_Friend_Attributes_Require_An_Explicit_Namespace_Over .AddReferences(await CreateFriendLibrary("FirstLibrary"), await CreateFriendLibrary("SecondLibrary")); var driver = CreateDriver(compilation, "Azure.Core").RunGenerators(compilation); Assert.That(driver.GetRunResult().Diagnostics.Select(d => d.Id), Is.EqualTo(new[] { "EPANN003" })); - Assert.That(driver.GetRunResult().GeneratedTrees.Length, Is.EqualTo(6)); + Assert.That(driver.GetRunResult().GeneratedTrees.Length, Is.EqualTo(AnnotationCount - 1)); - var generated = Generate(compilation, 7, "Azure.Core", "Azure.Core.InternalAnnotations"); + var generated = Generate(compilation, AnnotationCount, "Azure.Core", "Azure.Core.InternalAnnotations"); Assert.That(generated.GetTypeByMetadataName("Azure.Core.InternalAnnotations.DoNotDisposeAttribute")! .ContainingAssembly.Name, Is.EqualTo("Consumer")); } @@ -268,7 +365,7 @@ namespace Azure.Core { internal sealed class DoNotDisposeAttribute : System.Attribute { } }")).AddReferences(await CreateFriendLibrary("FirstLibrary"), await CreateFriendLibrary("SecondLibrary")); - var generated = Generate(compilation, 6, rootNamespace: "Azure.Core"); + var generated = Generate(compilation, AnnotationCount - 1, rootNamespace: "Azure.Core"); Assert.That(generated.GetTypeByMetadataName("Azure.Core.DoNotDisposeAttribute")! .ContainingAssembly.Name, Is.EqualTo("Consumer")); } diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.Borrowing.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.Borrowing.cs similarity index 99% rename from src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.Borrowing.cs rename to src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.Borrowing.cs index eab826a..90afe46 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.Borrowing.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.Borrowing.cs @@ -3,7 +3,7 @@ namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers; -public partial class DisposeBeforeLoosingScopeAnalyzerTests +public partial class DisposeBeforeLosingScopeAnalyzerTests { [TestCase("[return: DoNotDispose]")] [TestCase("[DoNotDispose]")] diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.FreshReturns.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.FreshReturns.cs similarity index 99% rename from src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.FreshReturns.cs rename to src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.FreshReturns.cs index 0fa50f4..7c73c46 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.FreshReturns.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.FreshReturns.cs @@ -3,7 +3,7 @@ namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers; -public partial class DisposeBeforeLoosingScopeAnalyzerTests +public partial class DisposeBeforeLosingScopeAnalyzerTests { [TestCase("=> new Disposable();")] [TestCase("{ return new Disposable(); }")] diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.Inference.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.Inference.cs similarity index 99% rename from src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.Inference.cs rename to src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.Inference.cs index 4f0e022..c055e69 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.Inference.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.Inference.cs @@ -3,7 +3,7 @@ namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers; -public partial class DisposeBeforeLoosingScopeAnalyzerTests +public partial class DisposeBeforeLosingScopeAnalyzerTests { [TestCase("var {|ERP044:d|} = new Disposable(); var alias = d;")] [TestCase("var {|ERP044:d|} = new Disposable(); _ = d;")] diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.MoveSemantics.cs similarity index 97% rename from src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs rename to src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.MoveSemantics.cs index 53f0923..4893379 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.MoveSemantics.cs @@ -1,13 +1,13 @@ using NUnit.Framework; using System.Threading.Tasks; using Verify = ErrorProne.NET.TestHelpers.CSharpCodeFixVerifier< - ErrorProne.NET.DisposableAnalyzers.DisposeBeforeLoosingScopeAnalyzer, + ErrorProne.NET.DisposableAnalyzers.DisposeBeforeLosingScopeAnalyzer, Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>; namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers { [TestFixture] - public partial class DisposeBeforeLoosingScopeAnalyzerTests + public partial class DisposeBeforeLosingScopeAnalyzerTests { private const string AcquiresOwnershipAttribute = @" diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.ReviewRegressions.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.ReviewRegressions.cs similarity index 99% rename from src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.ReviewRegressions.cs rename to src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.ReviewRegressions.cs index 5e83309..35b9d7f 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.ReviewRegressions.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.ReviewRegressions.cs @@ -3,7 +3,7 @@ namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers; -public partial class DisposeBeforeLoosingScopeAnalyzerTests +public partial class DisposeBeforeLosingScopeAnalyzerTests { [TestCase("var d = new Disposable(); using (d) { {|ERP044:d|} = new Disposable(); }")] [TestCase("var d = new Disposable(); using (var captured = d) { {|ERP044:d|} = new Disposable(); }")] diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.cs similarity index 99% rename from src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.cs rename to src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.cs index 8c9a68b..4a68081 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.cs @@ -2,13 +2,13 @@ using System.Threading.Tasks; using ErrorProne.NET.DisposableAnalyzers; using Verify = ErrorProne.NET.TestHelpers.CSharpCodeFixVerifier< - ErrorProne.NET.DisposableAnalyzers.DisposeBeforeLoosingScopeAnalyzer, + ErrorProne.NET.DisposableAnalyzers.DisposeBeforeLosingScopeAnalyzer, Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>; namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers { [TestFixture] - public partial class DisposeBeforeLoosingScopeAnalyzerTests + public partial class DisposeBeforeLosingScopeAnalyzerTests { [Test] public async Task Warn_On_No_Dispose() diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisoseTaskAnalyzerTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeTaskAnalyzerTests.cs similarity index 96% rename from src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisoseTaskAnalyzerTests.cs rename to src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeTaskAnalyzerTests.cs index d6f2308..09673a6 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisoseTaskAnalyzerTests.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeTaskAnalyzerTests.cs @@ -8,7 +8,7 @@ namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers { [TestFixture] - public partial class DisoseTaskAnalyzerTests + public partial class DisposeTaskAnalyzerTests { private const string Disposable = @" @@ -43,7 +43,7 @@ public static async Task ShouldDispose() "; await VerifyAsync(test); } - + [Test] public async Task Warn_On_Using_On_Task() { diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/OwnershipContractsTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/OwnershipContractsTests.cs index 5b5c144..f8fbf38 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/OwnershipContractsTests.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/OwnershipContractsTests.cs @@ -7,7 +7,7 @@ using Microsoft.CodeAnalysis.Testing; using NUnit.Framework; using Verify = ErrorProne.NET.TestHelpers.CSharpCodeFixVerifier< - ErrorProne.NET.DisposableAnalyzers.DisposeBeforeLoosingScopeAnalyzer, + ErrorProne.NET.DisposableAnalyzers.DisposeBeforeLosingScopeAnalyzer, Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>; namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers; diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/TypeExtensionsTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/TypeExtensionsTests.cs new file mode 100644 index 0000000..3b01689 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/TypeExtensionsTests.cs @@ -0,0 +1,87 @@ +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using ErrorProne.NET.Core; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Testing; +using NUnit.Framework; + +namespace ErrorProne.NET.CoreAnalyzers.Tests; + +[TestFixture] +public sealed class TypeExtensionsTests +{ + [TestCase("BaseInt", "BaseDefinition", true)] + [TestCase("Derived", "BaseDefinition", true)] + [TestCase("Derived", "BaseInt", true)] + [TestCase("Derived", "BaseString", false)] + [TestCase("BaseInt", "BaseString", false)] + [TestCase("BaseDefinition", "BaseInt", false)] + [TestCase("InterfaceInt", "InterfaceDefinition", true)] + [TestCase("Derived", "InterfaceDefinition", true)] + [TestCase("Derived", "InterfaceInt", true)] + [TestCase("Derived", "InterfaceString", false)] + [TestCase("Derived", "InterfaceDefinition", false, true)] + [TestCase("Constrained", "BaseDefinition", true)] + [TestCase("TransitiveConstraint", "BaseDefinition", true)] + [TestCase("TaskInt", "TaskDefinition", true)] + [TestCase("DerivedTask", "TaskDefinition", true)] + [TestCase("TaskInt", "TaskInt", true)] + [TestCase("TaskInt", "Object", true)] + public async Task DerivesFrom_Respects_Generic_Definitions_And_Constructed_Arguments( + string source, string candidate, bool expected, bool baseTypesOnly = false) + { + var types = await CreateTypes(); + Assert.That(types[source].DerivesFrom(types[candidate], baseTypesOnly), Is.EqualTo(expected)); + } + + [Test] + public async Task DerivesFrom_Rejects_Missing_Types() + { + var types = await CreateTypes(); + ITypeSymbol? missing = null; + Assert.That(missing.DerivesFrom(types["Object"]), Is.False); + Assert.That(types["Object"].DerivesFrom(missing), Is.False); + } + + private static async Task> CreateTypes() + { + var references = await ReferenceAssemblies.Net.Net80.ResolveAsync(LanguageNames.CSharp, CancellationToken.None); + var compilation = CSharpCompilation.Create("TypeRelationships", + new[] { CSharpSyntaxTree.ParseText(@" +public interface IMarker { } +public class GenericBase : IMarker { } +public class Derived : GenericBase { } +public class Constraints where T : GenericBase where U : T { } +public class DerivedTask : System.Threading.Tasks.Task +{ + public DerivedTask() : base(() => 42) { } +}") }, references, new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)); + Assert.That(compilation.GetDiagnostics().Where(d => d.Severity >= DiagnosticSeverity.Warning), Is.Empty); + + var integer = compilation.GetSpecialType(SpecialType.System_Int32); + var text = compilation.GetSpecialType(SpecialType.System_String); + var baseDefinition = compilation.GetTypeByMetadataName("GenericBase`1")!; + var interfaceDefinition = compilation.GetTypeByMetadataName("IMarker`1")!; + var taskDefinition = compilation.GetTypeByMetadataName("System.Threading.Tasks.Task`1")!; + var constraints = compilation.GetTypeByMetadataName("Constraints`2")!; + return new Dictionary + { + ["BaseDefinition"] = baseDefinition, + ["BaseInt"] = baseDefinition.Construct(integer), + ["BaseString"] = baseDefinition.Construct(text), + ["Derived"] = compilation.GetTypeByMetadataName("Derived")!, + ["InterfaceDefinition"] = interfaceDefinition, + ["InterfaceInt"] = interfaceDefinition.Construct(integer), + ["InterfaceString"] = interfaceDefinition.Construct(text), + ["Constrained"] = constraints.TypeParameters[0], + ["TransitiveConstraint"] = constraints.TypeParameters[1], + ["TaskDefinition"] = taskDefinition, + ["TaskInt"] = taskDefinition.Construct(integer), + ["DerivedTask"] = compilation.GetTypeByMetadataName("DerivedTask")!, + ["Object"] = compilation.GetSpecialType(SpecialType.System_Object), + }; + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md b/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md index 7a37e8a..d879c84 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md +++ b/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md @@ -39,6 +39,6 @@ ERP022 | ErrorHandling | Warning | SwallowAllExceptionsAnalyzer ERP031 | Concurrency | Warning | ConcurrentCollectionAnalyzer ERP041 | CodeSmell | Info | EventSourceSealedAnalyzer ERP042 | CodeSmell | Warning | EventSourceAnalyzer -ERP044 | Reliability | Warning | DisposeBeforeLoosingScopeAnalyzer +ERP044 | Reliability | Warning | DisposeBeforeLosingScopeAnalyzer ERP045 | Reliability | Warning | Invalid external ownership annotation ERP046 | Reliability | Warning | Respect disposable ownership diff --git a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposableAttributes.cs b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposableAttributes.cs index d18b668..02ea85e 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposableAttributes.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposableAttributes.cs @@ -6,16 +6,13 @@ public static class DisposableAttributes // Attribute contracts are documented in docs/Rules/ERP044.md. public const string AcquiresOwnershipAttribute = "AcquiresOwnershipAttribute"; - + // For methods that want to emphasize that the ownership is not transferred. public const string KeepsOwnershipAttribute = "KeepsOwnershipAttribute"; // For methods and properties whose results transfer ownership. public const string ReturnsOwnershipAttribute = "ReturnsOwnershipAttribute"; - // For borrowed parameters, fields and properties. public const string NoOwnershipAttribute = "NoOwnershipAttribute"; - - } \ No newline at end of file diff --git a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzer.cs similarity index 99% rename from src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzer.cs rename to src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzer.cs index 94b00f2..26c7a70 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzer.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzer.cs @@ -18,13 +18,13 @@ namespace ErrorProne.NET.DisposableAnalyzers; /// Conditional cleanup is accepted; this is not an exception-safe or path-sensitive proof. /// [DiagnosticAnalyzer(LanguageNames.CSharp)] -public sealed class DisposeBeforeLoosingScopeAnalyzer : DiagnosticAnalyzerBase +public sealed class DisposeBeforeLosingScopeAnalyzer : DiagnosticAnalyzerBase { internal static readonly DiagnosticDescriptor Rule = DiagnosticDescriptors.ERP044; public override bool ReportDiagnosticsOnGeneratedCode => false; - public DisposeBeforeLoosingScopeAnalyzer() + public DisposeBeforeLosingScopeAnalyzer() : base(Rule, DiagnosticDescriptors.ERP045, DiagnosticDescriptors.ERP046) { } diff --git a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/OwnershipInference.cs b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/OwnershipInference.cs index 62aee5b..2ae1166 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/OwnershipInference.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/OwnershipInference.cs @@ -50,7 +50,7 @@ public OwnershipInference(Compilation compilation, OwnershipContracts contracts, public ISymbol? GetAwaitedMember(IOperation operation) { - operation = DisposeBeforeLoosingScopeAnalyzer.Unwrap(operation); + operation = DisposeBeforeLosingScopeAnalyzer.Unwrap(operation); if (operation is IInvocationOperation { Instance: { } instance } invocation && _configureAwaitMethods.Contains(invocation.TargetMethod.OriginalDefinition) && _contracts.GetReturnOwnership(invocation.TargetMethod) == OwnershipKind.Unspecified) @@ -77,7 +77,7 @@ public OwnershipInference(Compilation compilation, OwnershipContracts contracts, } var target = invocation.Arguments.FirstOrDefault(a => a.Parameter?.Ordinal == 0); - return target != null && DisposeBeforeLoosingScopeAnalyzer.Unwrap(target.Value) is IMemberReferenceOperation member + return target != null && DisposeBeforeLosingScopeAnalyzer.Unwrap(target.Value) is IMemberReferenceOperation member && !_contracts.IsBorrowed(member.Member) ? invocation.Arguments.FirstOrDefault(a => a.Parameter?.Ordinal == 1)?.Value : null; } @@ -236,7 +236,7 @@ private bool InferAcquisition(IParameterSymbol parameter, HashSet(SymbolEqualityComparer.Default) { parameter }; - foreach (var operation in DisposeBeforeLoosingScopeAnalyzer.EnumerateOperations(root)) + foreach (var operation in DisposeBeforeLosingScopeAnalyzer.EnumerateOperations(root)) { if (operation is IInvocationOperation invocation) { @@ -280,7 +280,7 @@ private bool InferAcquisition(IParameterSymbol parameter, HashSet aliases) return false; } - operation = DisposeBeforeLoosingScopeAnalyzer.Unwrap(operation); - if (DisposeBeforeLoosingScopeAnalyzer.GetAliasSymbol(operation) is { } symbol) + operation = DisposeBeforeLosingScopeAnalyzer.Unwrap(operation); + if (DisposeBeforeLosingScopeAnalyzer.GetAliasSymbol(operation) is { } symbol) { return aliases.Contains(symbol); } if (operation is IConditionalAccessInstanceOperation) { - return ReferencesResource(DisposeBeforeLoosingScopeAnalyzer.GetConditionalReceiver(operation), aliases); + return ReferencesResource(DisposeBeforeLosingScopeAnalyzer.GetConditionalReceiver(operation), aliases); } if (operation is IInvocationOperation invocation && GetConfiguredDisposableResource(invocation) is { } resource) diff --git a/src/ErrorProne.NET.CoreAnalyzers/ExceptionsAnalyzers/SwallowAllExceptionsAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/ExceptionsAnalyzers/SwallowAllExceptionsAnalyzer.cs index 1477ba8..c328f3c 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/ExceptionsAnalyzers/SwallowAllExceptionsAnalyzer.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/ExceptionsAnalyzers/SwallowAllExceptionsAnalyzer.cs @@ -47,7 +47,6 @@ 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.CoreAnalyzers/TypeExtensions.cs b/src/ErrorProne.NET.CoreAnalyzers/TypeExtensions.cs index 3478262..e05b583 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/TypeExtensions.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/TypeExtensions.cs @@ -219,10 +219,11 @@ public static bool DerivesFrom([NotNullWhen(returnValue: true)] this ITypeSymbol return false; } + var candidateIsDefinition = SymbolEqualityComparer.Default.Equals(candidateBaseType.OriginalDefinition, candidateBaseType); if (!baseTypesOnly && candidateBaseType.TypeKind == TypeKind.Interface) { var allInterfaces = symbol.AllInterfaces.OfType(); - if (SymbolEqualityComparer.Default.Equals(candidateBaseType.OriginalDefinition, candidateBaseType)) + if (candidateIsDefinition) { // Candidate base type is not a constructed generic type, so use original definition for interfaces. allInterfaces = allInterfaces.Select(i => i.OriginalDefinition); @@ -248,7 +249,8 @@ public static bool DerivesFrom([NotNullWhen(returnValue: true)] this ITypeSymbol while (symbol != null) { - if (SymbolEqualityComparer.Default.Equals(symbol, candidateBaseType)) + if (SymbolEqualityComparer.Default.Equals( + candidateIsDefinition ? symbol.OriginalDefinition : symbol, candidateBaseType)) { return true; } From 64a967d73ae3d8bf34d9e1044ae73562cca1bf34 Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Tue, 8 Sep 2026 08:46:45 -0700 Subject: [PATCH 4/4] Refine ownership inference and add adoption workflows Keep ERP044 enabled while treating unknown handoffs conservatively. Preserve completed-task result ownership, enforce acquiring async-result contracts, correct review feedback, and add four evidence-driven adoption skills. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/skills/ownership-adoption/SKILL.md | 85 ++++ .../skills/ownership-contract-review/SKILL.md | 150 +++++++ .github/skills/ownership-inventory/SKILL.md | 118 ++++++ .github/skills/ownership-pilot/SKILL.md | 187 +++++++++ ReadMe.md | 13 + docs/Rules/ERP044.md | 65 ++- docs/Rules/ERP045.md | 11 +- docs/Rules/ERP046.md | 23 +- .../AnalyzerReleases.Unshipped.md | 1 + .../AnnotationsGenerator.cs | 23 +- src/ErrorProne.NET.Annotations/README.md | 12 +- .../Annotations/AnnotationsGeneratorTests.cs | 87 ++++ ...ngScopeAnalyzerTests.AdoptionBoundaries.cs | 155 ++++++++ ...ingScopeAnalyzerTests.CarriedParameters.cs | 207 ++++++++++ ...reLosingScopeAnalyzerTests.FreshReturns.cs | 2 +- ...eforeLosingScopeAnalyzerTests.Inference.cs | 29 +- ...BeforeLosingScopeAnalyzerTests.LowNoise.cs | 317 +++++++++++++++ ...LosingScopeAnalyzerTests.LowNoiseReview.cs | 191 +++++++++ ...foreLosingScopeAnalyzerTests.PrFeedback.cs | 244 ++++++++++++ ...ingScopeAnalyzerTests.ReviewRegressions.cs | 4 +- .../OwnershipContractsTests.cs | 38 +- .../DisposeBeforeLosingScopeAnalyzer.cs | 375 ++++++++++++++---- .../DisposableAnalyzers/OwnershipInference.cs | 256 +++++++++--- 23 files changed, 2431 insertions(+), 162 deletions(-) create mode 100644 .github/skills/ownership-adoption/SKILL.md create mode 100644 .github/skills/ownership-contract-review/SKILL.md create mode 100644 .github/skills/ownership-inventory/SKILL.md create mode 100644 .github/skills/ownership-pilot/SKILL.md create mode 100644 src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.AdoptionBoundaries.cs create mode 100644 src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.CarriedParameters.cs create mode 100644 src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.LowNoise.cs create mode 100644 src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.LowNoiseReview.cs create mode 100644 src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.PrFeedback.cs diff --git a/.github/skills/ownership-adoption/SKILL.md b/.github/skills/ownership-adoption/SKILL.md new file mode 100644 index 0000000..a017e5f --- /dev/null +++ b/.github/skills/ownership-adoption/SKILL.md @@ -0,0 +1,85 @@ +--- +name: ownership-adoption +description: >- + Plan and run a staged adoption of ErrorProne.NET disposable ownership in an + existing C# repository. Use for local NuGet pilots, disposable-usage audits, + finding annotation candidates, or evaluating ERP044, ERP045, and ERP046 before + rollout. Coordinate inventory, contract review, and real-project validation. +--- + +# Disposable ownership adoption + +Adoption is an evidence-gathering exercise, not a request to put `using` around +every disposable value. An analyzer finding is an unfulfilled modeled obligation, +not proof of a runtime leak. A clean build is not a proof of ownership safety. + +## Establish the boundary + +Read repository instructions and record the checkout, commit, branch, dirty +files, build entry points, compiler version, and existing analyzer configuration. +Find these facts yourself; ask the user only for material decisions: + +- Is this inventory-only, a package/configuration pilot, or an approved source fix? +- May local branches be created, and which files/projects may change? +- Should the review include real risks outside the current analyzer's coverage? + +For a large repository, agree on a broad inventory plus deeply traced, ranked +candidates. Do not quietly substitute a few interesting files for an inventory. +Independent repositories may be investigated in parallel; keep source-analysis +ownership separate from package/build work to avoid duplicated investigation. + +Keep proprietary source, paths, service names, findings, and build logs in the +consumer repository or an explicitly private artifact directory. Reusable skills +and public analyzer regressions must use independently written, generic examples. +Do not publish, commit, push, change global NuGet settings, or run live integration +tests without the corresponding authorization. + +## Workflow + +1. Read [ERP044](../../../docs/Rules/ERP044.md), + [ERP045](../../../docs/Rules/ERP045.md), and + [ERP046](../../../docs/Rules/ERP046.md). These documents are the policy authority. +2. Use [ownership-inventory](../ownership-inventory/SKILL.md) to map disposable + implementations, producer APIs, consumers, and mixed-cleanup call sites. +3. Use [ownership-pilot](../ownership-pilot/SKILL.md) to publish an immutable local + preview, establish the unchanged baseline, and exercise representative projects. +4. Use [ownership-contract-review](../ownership-contract-review/SKILL.md) to + classify findings and agree on individual ownership boundaries. +5. After source changes are authorized, annotate the smallest justified boundary, + rerun the pilot, and verify both intended diagnostics and negative controls. +6. Generalize demonstrated lessons into the adoption workflow; retain uncertainty + and untested steps rather than presenting a draft procedure as proven. + +## Non-negotiable v1 distinctions + +| Distinction | Adoption consequence | +| --- | --- | +| Disposable capability versus instance ownership | A disposable type does not make every result owning. | +| Unknown versus borrowed | Missing annotations do not prohibit disposal. | +| Input versus output ownership | A consumed parameter and a borrowed result are independent contracts. | +| Recognized transfer versus proven final cleanup | Member storage and explicit contracts are trusted boundaries. | +| Source inference versus library contracts | A simple fresh return may be inferred in one compilation but not across a project/assembly boundary. | +| Diagnostic versus defect | Missing knowledge about a task, callback, collection, or wrapper can produce a finding on correct code. | +| No diagnostic versus safety | Exceptional paths, lifecycle teardown, and general escapes remain outside a full proof. | + +Do not expand the ownership model or invent type-wide attributes as an adoption +shortcut. Separate necessary analyzer improvements from consumer-code changes. + +## Handoff + +Deliver the package IDs/version/feed, reproducible pilot commands, baseline and +pilot outcomes, inventory coverage and gaps, and a ranked review queue. Each +candidate needs a producer, representative callers, the likely lifetime owner, +confidence, current analyzer coverage, and the next decision. + +Use a concrete question, for example: + +```csharp +using var first = provider.Open(); +var second = provider.Open(); +cache.Remember(second); +``` + +Determine whether `Remember` owns and eventually releases its argument before +asking whether to add disposal. Present source evidence and a recommended +contract; never infer approval from a context-only reply. diff --git a/.github/skills/ownership-contract-review/SKILL.md b/.github/skills/ownership-contract-review/SKILL.md new file mode 100644 index 0000000..aaac16b --- /dev/null +++ b/.github/skills/ownership-contract-review/SKILL.md @@ -0,0 +1,150 @@ +--- +name: ownership-contract-review +description: >- + Review disposable ownership boundaries before adding ErrorProne.NET source or + external annotations. Use to classify ownership-oblivious APIs, borrowing, + transfers, mixed-cleanup callers, and suspected ownership analyzer false positives. +--- + +# Review an ownership contract + +Read the current [ERP044](../../../docs/Rules/ERP044.md), +[ERP045](../../../docs/Rules/ERP045.md), and +[ERP046](../../../docs/Rules/ERP046.md) contracts before proposing a change. + +## Build a lifetime record + +For one producer/consumer boundary, document: + +- Which instance is created, returned, passed, stored, or shared. +- Who is responsible before the call, after success, and after failure. +- Whether the result aliases an input, a receiver, a cached value, or a fresh value. +- Who releases retained state, including replacement and shutdown paths. +- Whether source, inherited, or external contracts already apply. +- What today's analyzer actually observes versus what manual tracing establishes. + +Treat task completion, callbacks, aggregate collections, certificate collections, +and configurable wrappers as boundaries to investigate. Do not add early `using` +to silence a finding if the value must remain live after the method returns. + +For a composite result, trace every independently acquired underlying resource. +Disposing a returned wrapper may close a stream but leave its connection owner +unreleased. A local `using` on that connection before returning the wrapper is +not a fix; the composite needs a truthful ownership and teardown design. + +## Choose the smallest truthful contract + +| Established behavior | Candidate annotation | +| --- | --- | +| Result transfers cleanup responsibility to its recipient | `ReturnsOwnership` on method/property/return | +| Recipient must neither dispose nor transfer a result | `DoNotDispose`, preferably return-targeted for methods | +| Callee acquires responsibility for an argument | `AcquiresOwnership` on that parameter | +| Callee only borrows an argument | `DoNotDispose` on that parameter | +| Ownership varies with arguments or remains uncertain | Keep unknown; document the decision or model limitation | + +Do not label all results of a disposable type as owned. Do not label unknown +results borrowed merely to reduce warning volume. Do not annotate generic +collections, task-completion methods, or callback APIs globally based on one +application's lifetime convention. + +An array or list is not automatically an ownership contract for its elements. +Trace element cleanup in the actual aggregate owner. Likewise, successful task +publication and eventual consumption are distinct: a task can be abandoned, and +`TrySetResult` can reject a value. Do not model conditional acceptance as an +unconditional acquiring parameter. + +If one local aliases a borrowed input on one branch and a newly created copy on +another, dispose only the copy. An unconditional `using` on the merged alias can +turn a real cleanup omission into disposal of somebody else's resource. + +Input and result contracts remain independent: + +```csharp +[return: DoNotDispose] +Resource Replace( + [AcquiresOwnership] Resource incoming, + [DoNotDispose] Resource shared) +{ + StoreOwned(incoming); + return shared; +} +``` + +The shared result does not discharge `incoming`; `StoreOwned` must establish +that transfer. A return contract also does not assert that the result aliases a +particular argument. + +Ask one concrete decision at a time. Present representative caller code, +the implementation evidence, the proposed owner, and the recommended answer. +A discussion or context-only reply is not approval to apply attributes. + +## Source versus external annotations + +Use source attributes for owned APIs when changes are approved. The +[annotations generator](../../../src/ErrorProne.NET.Annotations/README.md) +embeds internal types into `RootNamespace` or its explicit override; its public +API usages survive full and reference-assembly emission. No runtime annotation +DLL or public attribute mode is needed. + +Use `.ownership.xml` through the consuming project's `AdditionalFiles` when +the API cannot be edited: + +```xml + + + + + + + +``` + +Obtain declaration IDs from resolved symbols, such as +`DocumentationCommentId.CreateDeclarationId`, rather than guessing overloaded, +generic, constructor, or explicit-interface syntax. Use the assembly's simple +name and actual parameter names. Verify resolution in the intended compilation: +an absent member can be legal in an annotation pack, so no ERP045 does not prove +that an entry matched. Source contracts take precedence over XML. + +The v1 XML schema cannot express arbitrary conditional ownership. Never turn +`leaveOpen: true` or a conditional ownership flag into an unconditional transfer. + +## Validate the decision, not just compilation + +After authorization, require a positive and a negative example for the boundary: +missing cleanup reports, legitimate cleanup/transfer does not, and borrowed +cleanup reports where applicable. Check library consumers separately from +same-compilation inference. Verify independent incoming and outgoing obligations. + +If the issue is in the analyzer, create an independently written minimal +regression in ErrorProne.NET. Reproduce the diagnostic before changing analysis, +then check adjacent ownership cases and the real pilot again. + +Keep negative-control scenarios for retained task results, reusable release +tokens, explicit shutdown-callback cleanup, and collection-element transfers. +Known completed-task wrappers transport result identity; unknown publication, +collection arguments and captures make ownership uncertain and silence ERP044. +Distinguish these outcomes: silence at an unknown handoff does not prove +successful transfer, callback execution or eventual cleanup. Contrast each with +local abandonment, discarded completed wrappers, explicit borrowed boundaries +and ordinary receiver use. Failed-transfer cases can remain intentionally +undetected under the low-noise policy; record that trade-off instead of claiming +that a green build establishes safety. + +Keep fixes to consumer lifetimes, fixes to analyzer modeling, and intentional +trusted contracts separate in the review record. Never close an uncertain case +as a confirmed leak or a proven-safe transfer. + +## Record v1 gaps explicitly + +Separate a supported contract that behaves incorrectly from a documented +analysis limit, missing/incorrect consumer metadata, or an integration problem +such as stale compiler output. Keep both noisy correct lifetimes and missed +unsafe lifetimes in the review queue. + +For each gap, state the expected lifetime, observed diagnostic or absence, +minimal independently written reproduction, affected real-world pattern, +current documented boundary, and the decision needed before expanding v1. +Characterization tests can record a known limitation without changing policy; +name them accordingly. Passing such a test means the limitation was reproduced, +not that the example's lifetime is safe. diff --git a/.github/skills/ownership-inventory/SKILL.md b/.github/skills/ownership-inventory/SKILL.md new file mode 100644 index 0000000..03a9763 --- /dev/null +++ b/.github/skills/ownership-inventory/SKILL.md @@ -0,0 +1,118 @@ +--- +name: ownership-inventory +description: >- + Inventory IDisposable and IAsyncDisposable implementations and trace ownership + across C# producers and consumers. Use to find inconsistent disposal of results, + critical resource types, ownership-transfer boundaries, and adoption candidates. +--- + +# Inventory disposable ownership + +## Define and measure coverage + +Start from tracked source and project membership. Exclude generated outputs, +build caches, secrets, deployment data, and unrelated languages. Classify +production, test-only, generated, and vendored code separately. +Account for case-varied project extensions. Inspect provenance before excluding +a directory merely named `Output`: it can contain real source. A fake in a +shipping-library folder is still a fake, not proof of production ownership. + +Prefer an existing semantic index or Roslyn symbols for type relationships and +references. If only syntax/search is available, label the output accordingly. +Search is a candidate generator, not evidence that two callers invoke the same API. +Before starting an indexer or language server in a read-only audit, check whether +it triggers design-time builds, restores, or writes outside the approved scope. + +Inventory: + +- Direct and inherited `IDisposable` and `IAsyncDisposable` implementations. +- Disposable structs and lease/registration tokens. +- Pattern-disposable types as a separate category; verify analyzer support. +- `SafeHandle` descendants and resource-owning wrappers. +- Raw handles and manual release APIs as a separate, currently unmodeled category. +- Interfaces/abstract bases that carry contracts, separately from instantiable types. + +Follow in-repository base types transitively and distinguish nested types and +generic arity. Merge partial declarations by symbol when binding is available; +otherwise preserve declaration rows and qualify any unique-type count. +Retain unresolved external bases, +ambiguous type names, conditional compilation, and unavailable projects as +coverage gaps. Do not label a direct `: IDisposable` text search exhaustive. + +For every inventory entry record: + +| Field | Evidence | +| --- | --- | +| Identity | Project/assembly, qualified type, arity, declaration locations | +| Classification | Production/test/vendor; direct/inherited/pattern/unknown | +| Lifetime role | Resource owner, borrowed facade, lease, aggregate, marker | +| Resource impact | Native resource, connection, timer, subscription, lock, managed-only, unknown | +| Cleanup | Actual cleanup implementation and owned fields; not merely the method name | +| Usage | Construction/factories, returned values, parameters, member storage, representative callers | +| Confidence | Semantic resolution or bounded syntax/manual tracing; unresolved edges | + +## Find high-value producer/consumer pairs + +Group calls by resolved member identity, overload, and relevant arguments. +Include properties and awaited `Task`/`ValueTask` results. Keep disposal +of a task distinct from disposal of the resource produced by awaiting it. + +Look for an API whose result is: + +- Disposed with `using`, `await using`, `Dispose`, or `DisposeAsync` by some callers. +- Ignored, retained in a local, stored, returned, or passed onward by other callers. +- Transferred through a wrapper, callback, task completion, queue, or collection. + +For each promising pair, trace both call paths to a terminal owner. Check whether +the API allocates, returns shared state, lends a pooled object, or transfers an +existing object. Freshness is evidence, not a substitute for an ownership contract. + +```csharp +using var a = sessions.Get(); +var b = sessions.Get(); +owner.Attach(b); +``` + +This is not a leak report until `Get`, `Attach`, and the owner's teardown have +been investigated. Repeated calls may return the same object, and one caller's +existing `using` may be the bug. + +Caching does not by itself prove borrowing. A cached release token can represent +one disposal duty for each successful lock acquisition, even when several +acquisitions receive the same object. Conversely, constructing that token may not +acquire anything. Trace acquisition/release state, not just allocation and identity. + +Inspect ownership-changing arguments such as `leaveOpen`, `disposeHandler`, and +pool/retention options. Do not generalize one overload or argument combination +into an unconditional contract for every call. + +## Rank without inflating certainty + +Prioritize resource cost, call frequency, retention duration, and affected +production paths. A cancellation source that owns a timer or registrations is +different evidence from an otherwise unused disposable marker. + +Record at least one negative control: a mixed-use pattern that is safe because +of sharing or a verified transfer. Include candidates where `DoNotDispose` +would prevent an inappropriate cleanup, not only missing-dispose candidates. + +Separate results into: + +1. Evidence-backed missing cleanup or misuse. +2. Correct runtime ownership with missing analyzer-visible contracts. +3. Unresolved ownership requiring an API-owner decision. +4. Analyzer false positive or unsupported transport. +5. Real risk outside v1, such as exceptional-path or lifecycle cleanup. + +Deduplicate multi-target findings by rule, source location, and modeled resource; +retain the target frameworks as evidence rather than counting each as a new bug. + +## Deliverables + +Save a private machine-readable inventory and a code-linked review queue. +State files/projects considered, direct and transitive discovery methods, +classification counts, unresolved bases, and manually traced coverage. +Do not confuse the number of matches with the number of audited lifetimes. + +Feed the strongest candidates into +[ownership-contract-review](../ownership-contract-review/SKILL.md). diff --git a/.github/skills/ownership-pilot/SKILL.md b/.github/skills/ownership-pilot/SKILL.md new file mode 100644 index 0000000..89a55ee --- /dev/null +++ b/.github/skills/ownership-pilot/SKILL.md @@ -0,0 +1,187 @@ +--- +name: ownership-pilot +description: >- + Build and consume a local ErrorProne.NET ownership preview in real C# projects. + Use for immutable NuGet folder feeds, opt-in analyzer/source-generator wiring, + isolated MSBuild assets, baseline comparisons, and ERP044/045/046 rollout evidence. +--- + +# Run a real-project ownership pilot + +## Publish a local preview + +Record the analyzer commit and exact emitted package versions. Build and pack +`ErrorProne.Net.Annotations` and `ErrorProne.NET.CoreAnalyzers` using the existing +projects and build tools. Do not change dependency versions to hide restore issues. + +Use an explicit local folder feed. Never omit the destination in a publishing +command, publish to a remote feed, or add a global package source by default. +For this repository, the analyzer package is produced by the CoreAnalyzers +CodeFixes project, not the implementation project's placeholder package ID. + +Account for build-time versioning: inspect the actual `.nupkg` version rather +than assuming a command-line `PackageVersion` override won. Do not overwrite +different package contents under the same ID/version. Record package hashes. +Projects with `GeneratePackageOnBuild` may require an explicit configuration +build before packing; a missing Release DLL is not a NuGet restore problem. + +Inspect the package archive for analyzer DLLs and required build-transitive +assets, especially compiler-property forwarding for the generator. There must +be no `lib`/`runtimes` assets from these build-only packages. + +## Establish a baseline first + +Inspect SDK/compiler versions, central package management, current analyzer +versions, warnings-as-errors, custom output paths, and the repository's supported +build commands. The generator requires Roslyn 4.13 or newer; a project's target +framework and its compiler version are separate facts. + +Build the smallest representative real project without the preview. If the +baseline fails because dependencies are missing, restore through the repository's +existing sources and repeat. Distinguish baseline/environment failures from +preview regressions; do not declare an unchanged failure caused by the analyzer. + +Never run live integration tests, production startup, or deployment targets as +a shortcut to validation. Use existing targeted, local tests. + +## Make rollout opt-in and reversible + +Preserve normal defaults. Prefer an explicit pilot property and, for a large +graph, a project selector. Replace the selected project's existing analyzer +reference rather than loading old and new analyzer assemblies together. + +Pin both preview package versions and keep: + +```xml +PrivateAssets="all" +IncludeAssets="analyzers;build;buildtransitive" +``` + +Respect the repository's actual package-management mechanism. NuGet central +package management and `Microsoft.Build.CentralPackageVersions` do not have +identical import order or global-reference semantics. + +Add the local feed only for the pilot invocation, for example through +`RestoreAdditionalProjectSources`. Retain the established authenticated sources. +Verify NuGet's restored metadata identifies the intended local package. + +Isolate preview restore assets, intermediate compilation, generated sources, +binary outputs, package outputs, and diagnostic logs from the normal build. +Separate package selection from artifact isolation: a test project may consume +a preview-built dependency without enabling the preview analyzer itself, but +its copied dependencies and test output still belong to the isolated invocation. +Set early MSBuild path properties before they are consumed; evaluate the final +paths per project and target framework. Do not globally force all referenced +projects to share one assets file or output directory. + +Trace later output overrides, not just the pilot's first property assignment. +Evaluate representative library, application, and custom-package-ID shapes and +compare `OutputPath`, `OutDir`, `TargetDir`, and package paths with normal builds. +Limit the supported graph explicitly and fail closed for unsupported project +kinds or layouts. Keep pilot builds out of normal release package staging and +deployment targets; preserve normal output precedence when the pilot is off. + +Keep generated files outside source globs. Existing generators, such as +nullability polyfills, may emit additional attributes; count this generator's +output by producer, not every `*Attribute.g.cs` in the directory. + +Check `git status` and ignore rules for every required new import. A successful +local build can hide a broken patch when its new `.props` files are ignored. +Expose only the required files; do not unignore arbitrary build output. + +## Exercise and classify + +For a dirty-source preview, verify the effective package version after the +versioning targets execute, not just at project evaluation. Git-based version +generators can override a command-line `PackageVersion`. Pack into an isolated +staging folder, inspect the archive identity and hashes, then copy to the local +feed without overwriting an existing version. Record the base commit and dirty +source-input/diff hashes; a commit-shaped version alone is insufficient. + +Restore and build selected projects using the preview. Capture structured +diagnostics, preferably separate SARIF files per project/target. Preserve +warnings-as-errors and existing analysis; do not add broad suppressions or +disable analyzers to obtain a green result. + +Changing the project selector or whole-graph scope changes effective package +references. Restore with that same selection, and inspect the selected project's +actual assets and resolved compiler analyzers. A forced rebuild with stale assets +can succeed without loading the requested preview at all. Fresh SARIF and a +correct package archive on disk do not, by themselves, establish analyzer loading. + +A graph may stop at an early ownership diagnostic. Record that failure, then +use a clearly documented project-scoped pilot to inspect other projects without +silently weakening the original graph's policy. + +Build a small provider and a real caller when testing a cross-project contract. +Compiling only the provider can confirm annotation generation but cannot prove +that a downstream call receives the intended ownership diagnostic. + +For an annotation-driven experiment, record the same source and preview with +the contract off and on, then a correct cleanup/transfer control with it on. +An existing unrelated diagnostic can prevent an otherwise useful comparison +from becoming green. Record that baseline separately; if authorized, temporarily +discharge that independent obligation in every comparison, then restore only +your control edits. Never label an unrelated compiler failure as successful +detection of the intended ownership defect. + +Force compilation for these measurements, for example with `--no-incremental`. +Changing a conditional `AdditionalFiles` item set can reuse old successful +compiler output when the newly included file is older than that output. +Where appropriate, expose the contract-mode property through +`CompilerVisibleProperty` so the generated analyzer configuration also changes. +Verify an ordinary incremental off-to-on transition separately. Preserve each +stage's SARIF only after confirming compilation actually ran. + +When the user requests a deliberately failing pilot, retain only the opt-in +contract/configuration and the original source defect, not an injected runtime +leak. Record the expected rule, source span, and exit code. Promote a warning +to an error only in that narrowly selected experiment if needed; do not alter +normal warning policy. Restore temporary cleanup controls and confirm both the +expected opt-in failure and the unaffected normal build. + +If the user subsequently requests permanent source fixes, retain the corrected +source with the contracts enabled; do not reintroduce the original omission. +Keep historical negative-control evidence separate from the corrected build. +Choose cleanup boundaries from actual ownership: materializers consume readers, +returned streams outlive their factory call, borrowed inputs are not owned copies, +and scheduled work or retained registry aliases may outlive the current method. +Unknown contracts and unfinished asynchronous teardown remain separate questions, +not permission to add immediate disposal or claim complete lifetime safety. + +Separate: + +- ERP044/ERP046 ownership findings requiring code/contract review. +- ERP045 or EPANN configuration/annotation failures. +- AD0001 and analyzer loading or compiler compatibility failures. +- Other new diagnostics from upgrading the complete analyzer package. +- Baseline, restore, compiler, test-host, or environment failures. + +Deduplicate repeated target-framework emissions while retaining target coverage. +Route findings to [ownership-contract-review](../ownership-contract-review/SKILL.md); +do not equate the diagnostic count with a leak count. + +ERP044 remains enabled by default but quiets unresolved lifetimes at unknown +argument handoffs and captures. When comparing previews, classify disappearing +diagnostics as proven transport, conservative uncertainty, or an unintended +regression. Include annotated owned-result abandonment and receiver-only use +as positive controls, and explicit borrowed-parameter handoff as a retained +obligation. A lower warning count alone is not increased safety coverage. + +Run existing focused tests against the preview-built dependency. Verify a +nonzero matching test count and the actual assembly used. A successful test +command with no restored test SDK or no matching tests is not passing coverage. + +Inspect runtime output for accidental ErrorProne DLL dependencies. Verify the +normal build still resolves its original packages and output paths with the +pilot disabled. + +## Exit evidence + +Save exact package IDs, versions, hashes and source; baseline and pilot commands; +effective project/TFM coverage; diagnostic classifications; test counts; generated +annotation evidence; runtime-output evidence; and remaining blockers. + +Do not call a preview merge-ready because packages restored or one project +compiled. Unreviewed diagnostics and unresolved ownership decisions remain open, +even when the build treats them as warnings. diff --git a/ReadMe.md b/ReadMe.md index bf8a410..3493b2c 100644 --- a/ReadMe.md +++ b/ReadMe.md @@ -58,6 +58,11 @@ Unannotated results are ownership-oblivious by default. A simple, non-overridabl source method directly returning a new disposable can establish ownership; shared, complex, or external results need an explicit contract to establish a caller obligation. Unknown does not imply `DoNotDispose`. +ERP044 is enabled by default with a low-noise policy: unknown argument handoffs +and captures stop an unresolved local cleanup warning, without proving transfer +or safety. Explicit borrowed arguments and ordinary receiver calls retain the +caller obligation. Known completed-task wrappers carry result ownership rather +than discharging it. The rules intentionally do not attempt a complete borrow checker or proof of exception safety; see the documented limitations. This first version focuses on contracts and simple inference. Dedicated diagnostics for unknown ownership @@ -69,6 +74,14 @@ escapes are deferred; missing knowledge is not itself evidence of unsafe code. | [ERP045](docs/Rules/ERP045.md) | Invalid external ownership annotation | | [ERP046](docs/Rules/ERP046.md) | Obvious use after disposal/transfer or misuse of explicit borrowing | +For adoption in an existing codebase, start with the +[ownership-adoption skill](.github/skills/ownership-adoption/SKILL.md). +It coordinates a [disposable inventory](.github/skills/ownership-inventory/SKILL.md), +[contract review](.github/skills/ownership-contract-review/SKILL.md), and an +[opt-in local-package pilot](.github/skills/ownership-pilot/SKILL.md). +These workflows distinguish actual lifetime defects from missing contracts and +analysis gaps before applying source changes. + ### Concurrency | Id | Description | diff --git a/docs/Rules/ERP044.md b/docs/Rules/ERP044.md index f7dded5..dedb765 100644 --- a/docs/Rules/ERP044.md +++ b/docs/Rules/ERP044.md @@ -2,7 +2,8 @@ This rule looks for recognized disposal or ownership transfer of an acquired `IDisposable` or `IAsyncDisposable` resource. A diagnostic means the modeled -obligation was not discharged; it is not proof that a runtime leak occurs. +obligation was abandoned within the modeled local scope; it is not proof that a +runtime leak occurs. The rule is enabled by default and favors low noise. ```csharp var resource = new Resource(); // ERP044: no recognized disposal or transfer. @@ -18,11 +19,18 @@ This version focuses on ownership contracts, bounded local/callee inference, and obvious contract violations. It does not perform general escape analysis or issue a separate diagnostic when ownership tracking becomes unknown. -An unannotated API may genuinely take ownership even when the analyzer cannot -infer that transfer. Describe that boundary with `AcquiresOwnership` or an -external annotation rather than treating the diagnostic as proof that the API -is unsafe. Conversely, no diagnostics does not establish safety: recognized -ownership boundaries are trusted, not verified transitively. +An unannotated API may genuinely retain a resource even when the analyzer cannot +infer a transfer. Passing a resource as an unknown argument or capturing it in +a callback makes its cleanup uncertain, so ERP044 stays quiet for that lifetime. +This is not evidence of a successful transfer and does not imply +`AcquiresOwnership` or an ERP046 use-after-transfer diagnostic. Ordinary instance +receiver calls, such as `resource.Use()`, do not hide an outstanding obligation. + +Explicit contracts still matter: passing to a `DoNotDispose` parameter retains +the caller's cleanup obligation, while an acquiring contract establishes a +transfer. Describe known boundaries with attributes or external annotations. +No diagnostics does not establish safety: unknown handoffs may leak, and +recognized ownership boundaries are trusted, not verified transitively. Dedicated, opt-in diagnostics for unknown escapes are deferred to a later iteration. They should distinguish missing knowledge from an ownership bug. @@ -80,8 +88,17 @@ void Consume([AcquiresOwnership] Resource resource) // ERP044 on resource ``` Every acquiring parameter is checked, including constructor and indexer parameters. +An explicit acquiring contract also applies to `object` and unconstrained generic +parameters: erasing the static disposable type does not erase the callee's duty. +Ordinary casts and direct type-pattern aliases can carry that parameter to cleanup. Overrides and interface implementations inherit ownership contracts. +An acquiring `Task` or `ValueTask` parameter acquires the eventual result. +Its body must dispose that result after awaiting, or transfer responsibility; +disposing only the Task wrapper does not satisfy the contract. Passing a known +completed-result wrapper to such a parameter transfers its carried resource. +This is a trusted ownership boundary, not proof that asynchronous cleanup completes. + `ReturnsOwnership` marks an owning method/property result. `DoNotDispose` marks a borrowed parameter, member, or result; prefer `[return: DoNotDispose]` for method results. The recipient must not dispose the resource or transfer it @@ -195,24 +212,41 @@ borrow checker: in an owning member, and forwarding to another consumer can infer an acquiring parameter. Simple local aliases are followed. Recursive inference is capped at four parameter hops. +- Unannotated invocation, constructor, indexer and user-defined operator inputs without + recognized consumption make ownership uncertain. Dynamic argument handoffs + and lambda/local-function/method-group captures also silence ERP044. + Extension receivers are arguments and follow their parameter contracts, + except for the recognized fluent-alias inference described above. + Other arguments to a fluent call still follow their own contracts. + Uncertainty does not erase explicit borrowing or later definite disposal + and use-after-disposal checks, nor obligations for replacement allocations. - `StreamReader`, `StreamWriter`, `BinaryReader`, and `BinaryWriter` constructors are recognized as owning their stream when `leaveOpen` is absent or constant - `false`. Unknown `leaveOpen` values do not prove transfer. + `false`. Constant `true` retains the caller's obligation; unknown `leaveOpen` + makes cleanup uncertain without proving transfer. - Returning a resource, assigning it to an owning field/property or an output parameter, and `Interlocked.Exchange` into an owning member transfer ownership. - Local aliases retain the same obligation. Discarding an alias does not transfer it. Unconditional reassignment of a local or parameter does not let cleanup of the new resource hide an older leaked resource. Conditional reassignments are handled conservatively and can hide leaks. +- `Task.FromResult`, `ValueTask.FromResult` and `new ValueTask(T result)` + transport the result's responsibility through local aliases, returns, owning + member storage and `await`, including framework `ConfigureAwait`. Discarding + the wrapper, awaiting without cleanup, or disposing the task itself does not + dispose the carried resource. Task and resource identities remain separate. + Supported fluent carrier aliases and `Interlocked.Exchange` into an owning + member preserve that distinction. Bounded source consumption inference also + follows these carriers to actual cleanup or owning storage, not mere wrapping. - `using` and `await using` capture the resource before their body executes. Reassigning the original variable inside the body does not dispose the new value. The framework's `IAsyncDisposable.ConfigureAwait` wrapper preserves resource identity, including through local aliases. - User-defined conversions are separate input/result boundaries, not aliases. Explicit conversion-operator contracts are honored independently, and source - consumption can be inferred for their parameters. Dynamic conversions do not - establish identity or prove consumption, so an input acquisition can still - have an outstanding obligation even when its converted result is disposed. + consumption can be inferred for their parameters. An unknown conversion input + makes cleanup uncertain, not discharged. Dynamic conversions do not establish + identity or prove consumption. Task objects, `StringReader`, and `MemoryStream` families retain the prototype's disposal exemptions. This includes derived types, so these exemptions are @@ -230,12 +264,15 @@ This is not a proof of exception safety or a Rust borrow checker. Conditional cleanup/transfer is accepted even when another path may leak. General loops, complex aliases, closures, local-function captures, dynamic calls and collection element ownership are not modeled exhaustively. Nested lambda/local-function -bodies are not analyzed as separate lifetimes, nor used as evidence that their -enclosing function disposes a resource. +bodies are not analyzed as separate lifetimes. Captured variables make cleanup +uncertain even when the resource is assigned after the callback is created; +it is not evidence that the callback actually runs or disposes anything. Calling `DisposeAsync` is accepted as cleanup; the rule does not prove that its -completion is awaited. Ownership carried through arbitrary task/container -wrappers is not tracked. +completion is awaited. Only the completed-result wrappers listed above carry +tracked resource identity; arbitrary task/container wrappers are not modeled. +Unknown collection or task-publication arguments can silence a warning even +when the recipient rejects or never disposes the resource. Transferring to a member trusts the receiving object's ownership boundary; the rule does not prove that the containing type eventually disposes its fields. diff --git a/docs/Rules/ERP045.md b/docs/Rules/ERP045.md index b40a344..c39e9fb 100644 --- a/docs/Rules/ERP045.md +++ b/docs/Rules/ERP045.md @@ -96,10 +96,13 @@ Library.Consumer.Take(new Library.Resource()); // Valid: explicit transfer. Without these annotations, external results remain unknown: their disposable type alone does not create an ERP044 obligation or an ERP046 borrowing -restriction. This does not remove obligations established by direct creation: -`Library.Consumer.Take(new Library.Resource())` still produces ERP044 without -a known consuming contract, because passing the new resource to an unknown -consumer does not establish that its ownership was transferred. +restriction. Passing a directly created resource to an unknown parameter, as in +`Library.Consumer.Take(new Library.Resource())`, makes cleanup uncertain and is +quiet under ERP044's low-noise policy. It does not establish a successful +transfer or imply ERP046 on subsequent use. An explicit borrowed parameter +retains the caller's obligation; an owned parameter establishes a transfer. +Use contracts to make these distinctions enforceable, not just to silence a +diagnostic. Simply abandoning `new Library.Resource()` still reports ERP044. ## Diagnostics and limits diff --git a/docs/Rules/ERP046.md b/docs/Rules/ERP046.md index 5db7254..d545184 100644 --- a/docs/Rules/ERP046.md +++ b/docs/Rules/ERP046.md @@ -29,6 +29,11 @@ unambiguously within this bounded model. A condition-dependent alias is not treated as proof of misuse. Compile-time `nameof` references are not runtime uses. These exclusions do not establish that the remaining code is safe. +Unknown argument handoffs and captures are not definite moves. ERP044 can stay +quiet because the recipient might retain ownership without ERP046 claiming +that subsequent use is invalid. A later definite `Dispose` still permits an +obvious use-after-disposal diagnostic. + ## Explicit borrowing ```csharp @@ -55,7 +60,23 @@ framework's async-disposal `ConfigureAwait` wrapper. Explicit property contracts also apply to awaited `Task` and `ValueTask` results. Ordinary property getters can return borrowed values; owning properties cannot. -Simple local aliases and assignments preserve borrowing. Replacing a local +`DisposeAsync` calls count as cleanup only when they target the actual +`IAsyncDisposable` implementation or the framework's configured-disposal wrapper. +An unrelated method with that name does not discharge ERP044 or invent an ERP046 +violation. Recognizing cleanup still does not prove its asynchronous completion. + +Known completed-result wrappers (`Task.FromResult`, `ValueTask.FromResult`, +and `new ValueTask(T result)`) retain the result's borrowing through local +aliases and `await`, including framework `ConfigureAwait`. Disposing the task +object is not disposing its result. Returning a borrowed carried result through +an owning return, or disposing it after awaiting, still violates borrowing. + +Wrapping a borrowed resource cannot bypass an acquiring parameter contract. +`DoNotDispose` on `Task`/`ValueTask` parameters also preserves borrowing of +their awaited results. Source-inferred async consumers must actually consume the +result; a call that only disposes the Task wrapper is not result acquisition. + +Simple local aliases, assignments, and direct type-pattern bindings preserve borrowing. Replacing a local or parameter with a new owned resource clears borrowing for that binding, not for other aliases of the original resource. Conditional assignments are conservative; arbitrary branching and escape propagation remain out of scope. diff --git a/src/ErrorProne.NET.Annotations/AnalyzerReleases.Unshipped.md b/src/ErrorProne.NET.Annotations/AnalyzerReleases.Unshipped.md index 661b1a9..45b94a3 100644 --- a/src/ErrorProne.NET.Annotations/AnalyzerReleases.Unshipped.md +++ b/src/ErrorProne.NET.Annotations/AnalyzerReleases.Unshipped.md @@ -6,3 +6,4 @@ Rule ID | Category | Severity | Notes EPANN001 | Configuration | Error | Annotation namespace is unavailable EPANN002 | Configuration | Error | Annotation namespace is invalid EPANN003 | Configuration | Error | Existing annotation definitions are ambiguous +EPANN004 | Configuration | Error | Annotation name is occupied by a non-attribute type diff --git a/src/ErrorProne.NET.Annotations/AnnotationsGenerator.cs b/src/ErrorProne.NET.Annotations/AnnotationsGenerator.cs index e5fb095..8e12fca 100644 --- a/src/ErrorProne.NET.Annotations/AnnotationsGenerator.cs +++ b/src/ErrorProne.NET.Annotations/AnnotationsGenerator.cs @@ -23,6 +23,10 @@ public sealed class AnnotationsGenerator : IIncrementalGenerator "EPANN003", "Existing annotation definitions are ambiguous", "Multiple accessible definitions of '{0}' exist in {1}; set ErrorProneAnnotationsNamespace or remove the conflicting reference", "Configuration", DiagnosticSeverity.Error, isEnabledByDefault: true); + private static readonly DiagnosticDescriptor OccupiedByNonAttribute = new( + "EPANN004", "Annotation name is occupied by a non-attribute type", + "'{0}' already exists in this assembly but does not derive from System.Attribute; rename it or set ErrorProneAnnotationsNamespace", + "Configuration", DiagnosticSeverity.Error, isEnabledByDefault: true); private static readonly (string Name, string Targets, string Summary)[] Attributes = { ("AcquiresOwnershipAttribute", "Parameter", "Transfers cleanup responsibility to the receiving method."), @@ -71,20 +75,33 @@ public void Initialize(IncrementalGeneratorInitializationContext context) var metadataName = metadataNamespace.Length == 0 ? attribute.Name : metadataNamespace + "." + attribute.Name; var existing = compilation.GetTypesByMetadataName(metadataName); + var localConflict = existing.FirstOrDefault(type => + SymbolEqualityComparer.Default.Equals(type.ContainingAssembly, compilation.Assembly) + && !IsAttributeType(type, compilation)); + if (localConflict != null) + { + output.ReportDiagnostic(Diagnostic.Create(OccupiedByNonAttribute, + localConflict.Locations.FirstOrDefault(location => location.IsInSource) ?? Location.None, + metadataName)); + continue; + } + + IEnumerable reusableCandidates = + existing.Where(type => IsAttributeType(type, compilation)); if (attribute.Name is "UseConfigureAwaitFalseAttribute" or "DoNotUseConfigureAwaitAttribute") { // These assembly policies also recognize legacy attribute classes without the suffix. var legacyName = metadataName.Substring(0, metadataName.Length - nameof(Attribute).Length); - existing = existing.AddRange(compilation.GetTypesByMetadataName(legacyName) + reusableCandidates = reusableCandidates.Concat(compilation.GetTypesByMetadataName(legacyName) .Where(type => IsAttributeType(type, compilation))); } - if (existing.Any(type => SymbolEqualityComparer.Default.Equals(type.ContainingAssembly, compilation.Assembly))) + if (reusableCandidates.Any(type => SymbolEqualityComparer.Default.Equals(type.ContainingAssembly, compilation.Assembly))) { continue; } - var accessible = existing.Where(type => globalAssemblies.Contains(type.ContainingAssembly) + var accessible = reusableCandidates.Where(type => globalAssemblies.Contains(type.ContainingAssembly) && compilation.IsSymbolAccessibleWithin(type, compilation.Assembly)) .Distinct(SymbolEqualityComparer.Default).ToArray(); if (accessible.Length > 1) diff --git a/src/ErrorProne.NET.Annotations/README.md b/src/ErrorProne.NET.Annotations/README.md index 2f3f459..262032c 100644 --- a/src/ErrorProne.NET.Annotations/README.md +++ b/src/ErrorProne.NET.Annotations/README.md @@ -108,9 +108,11 @@ attributes. They add no runtime DLL dependency. ## Existing definitions -If an accessible type with the same fully qualified name already exists, its -definition is used instead of generating a duplicate. Inaccessible internal -definitions in other assemblies do not prevent generating a local copy. +If an accessible attribute type with the same fully qualified name already +exists, its definition is used instead of generating a duplicate. Inaccessible +internal definitions in other assemblies do not prevent generating a local copy, +and referenced non-attribute types with the same name do not suppress local +generation. For the ConfigureAwait assembly policies, recognized legacy attribute classes named `UseConfigureAwaitFalse` or `DoNotUseConfigureAwait` without the `Attribute` suffix are also reused to avoid ambiguous attribute names. @@ -122,7 +124,9 @@ makes the required annotations available. If multiple referenced definitions are accessible and ambiguous, EPANN003 requests a namespace override or removal of the conflicting reference. The generator does not guess which assembly to reuse, silently change namespace, -or switch to public attribute types. +or switch to public attribute types. If the current assembly already declares a +same-name non-attribute type in the target namespace, EPANN004 reports that +local conflict instead of generating a duplicate attribute declaration. This package does not add type-level ownership markers or generate BCL nullability attributes. Existing application-defined and legacy ErrorProne.NET diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/Annotations/AnnotationsGeneratorTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/Annotations/AnnotationsGeneratorTests.cs index 1c019bd..5cb7fad 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/Annotations/AnnotationsGeneratorTests.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/Annotations/AnnotationsGeneratorTests.cs @@ -138,6 +138,93 @@ internal sealed class DoNotUseConfigureAwait { } Generate(compilation, AnnotationCount); } + [Test] + public async Task Generates_A_Local_Attribute_When_Only_A_Referenced_NonAttribute_Exists() + { + var existing = await CreateCompilation("ExistingAnnotations", @" +namespace Library +{ + public sealed class DoNotDisposeAttribute { } +}"); + var compilation = await CreateCompilation("Consumer", @" +namespace Library +{ + public class Api + { + [return: DoNotDispose] + public System.IDisposable GetShared() { return null; } + } +}", reference: Emit(existing, referenceAssembly: false)); + var driver = CreateDriver(compilation).RunGeneratorsAndUpdateCompilation(compilation, + out var updated, out var diagnostics); + + Assert.That(diagnostics, Is.Empty); + Assert.That(driver.GetRunResult().Diagnostics, Is.Empty); + Assert.That(driver.GetRunResult().GeneratedTrees.Length, Is.EqualTo(AnnotationCount)); + Assert.That(updated.GetDiagnostics().Where(d => d.Severity == DiagnosticSeverity.Error), Is.Empty); + Assert.That(updated.GetTypeByMetadataName("Library.DoNotDisposeAttribute")! + .ContainingAssembly.Name, Is.EqualTo("Consumer")); + } + + [Test] + public async Task Reports_Local_NonAttribute_That_Occupies_The_Canonical_Annotation_Name() + { + var compilation = await CreateCompilation("Library", @" +namespace Library +{ + internal sealed class DoNotDisposeAttribute { } + + public class Api + { + [return: DoNotDispose] + public System.IDisposable GetShared() { return null; } + } +}"); + var driver = CreateDriver(compilation).RunGeneratorsAndUpdateCompilation(compilation, + out var updated, out var diagnostics); + + Assert.That(diagnostics.Select(d => d.Id), Is.EqualTo(new[] { "EPANN004" })); + Assert.That(driver.GetRunResult().Diagnostics.Select(d => d.Id), Is.EqualTo(new[] { "EPANN004" })); + Assert.That(driver.GetRunResult().GeneratedTrees.Length, Is.EqualTo(AnnotationCount - 1)); + Assert.That(updated.GetDiagnostics().Select(d => d.Id), Does.Contain("CS0616")); + Assert.That(updated.GetDiagnostics().Select(d => d.Id), Does.Not.Contain("CS0101")); + } + + [Test] + public async Task Local_NonAttribute_Takes_Precedence_Over_A_Referenced_Attribute() + { + var reference = Emit(await CreateCompilation("ExistingAnnotations", @" +namespace Library +{ + public sealed class DoNotDisposeAttribute : System.Attribute { } +}"), referenceAssembly: false); + var compilation = await CreateCompilation("Consumer", @" +namespace Library +{ + internal sealed class DoNotDisposeAttribute { } +}", reference: reference); + var driver = CreateDriver(compilation).RunGenerators(compilation); + + Assert.That(driver.GetRunResult().Diagnostics.Select(d => d.Id), Is.EqualTo(new[] { "EPANN004" })); + Assert.That(driver.GetRunResult().GeneratedTrees.Length, Is.EqualTo(AnnotationCount - 1)); + } + + [Test] + public async Task Nested_And_Generic_NonAttributes_Do_Not_Block_Generation() + { + var compilation = await CreateCompilation("Library", @" +namespace Library +{ + internal sealed class Container + { + internal sealed class DoNotDisposeAttribute { } + } + + internal sealed class DoNotDisposeAttribute { } +}"); + Generate(compilation, AnnotationCount); + } + [TestCase(null, AnnotationCount - 1, "ExistingAnnotations")] [TestCase("Existing", AnnotationCount, "Consumer")] [TestCase("global,Existing", AnnotationCount - 1, "ExistingAnnotations")] diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.AdoptionBoundaries.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.AdoptionBoundaries.cs new file mode 100644 index 0000000..36bbb85 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.AdoptionBoundaries.cs @@ -0,0 +1,155 @@ +using System.Threading.Tasks; +using NUnit.Framework; + +namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers; + +public partial class DisposeBeforeLosingScopeAnalyzerTests +{ + // These characterize v1 boundaries, including known noise and missed obligations. + // A passing test here does not mean the example has a safe resource lifetime. + [Test] + public Task Adoption_Task_Publication_Transfers_The_Carried_Resource() + { + return VerifyAsync(@" +public class Test +{ + [return: ReturnsOwnership] + private static System.Threading.Tasks.Task CreateAsync() + { + var item = new Disposable(); + return System.Threading.Tasks.Task.FromResult(item); + } + + public async System.Threading.Tasks.Task RunAsync() + { + using var item = await CreateAsync(); + } +}"); + } + + [Test] + public Task Adoption_Unannotated_Collection_Makes_Element_Ownership_Unknown() + { + return VerifyAsync(@" +public sealed class Test : System.IDisposable +{ + private readonly System.Collections.Generic.List items = new(); + + public void Add() + { + var item = new Disposable(); + items.Add(item); + } + + public void Dispose() + { + foreach (var item in items) + item.Dispose(); + items.Clear(); + } +}"); + } + + [Test] + public Task Adoption_Shutdown_Capture_Makes_Local_Ownership_Unknown() + { + return VerifyAsync(@" +public class Test +{ + public void Register() + { + var item = new Disposable(); + System.AppDomain.CurrentDomain.ProcessExit += (sender, args) => item.Dispose(); + } +}"); + } + + [Test] + public Task Adoption_Lambda_Local_Leak_Currently_Is_Not_Reported() + { + return VerifyAsync(@" +public class Test +{ + public void Run() + { + System.Action callback = () => + { + var item = new Disposable(); + item.ToString(); + }; + callback(); + } +}"); + } + + [Test] + public Task Adoption_Conditional_Cleanup_Currently_Discharges_All_Paths() + { + return VerifyAsync(@" +public class Test +{ + public void Run(bool shouldDispose) + { + var item = new Disposable(); + if (shouldDispose) + item.Dispose(); + } +}"); + } + + [Test] + public Task Adoption_Member_Transfer_Currently_Does_Not_Require_Owner_Teardown() + { + return VerifyAsync(@" +public sealed class Test : System.IDisposable +{ + private readonly Disposable item; + + public Test() + { + item = new Disposable(); + } + + public void Dispose() { } +}"); + } + + [Test] + public Task Adoption_Discarded_Async_Cleanup_Currently_Discharges_Ownership() + { + return VerifyAsync(@" +public sealed class AsyncResource : System.IAsyncDisposable +{ + public async System.Threading.Tasks.ValueTask DisposeAsync() + { + await System.Threading.Tasks.Task.Yield(); + } +} + +public class Test +{ + public void Run() + { + var item = new AsyncResource(); + _ = item.DisposeAsync(); + } +}"); + } + + [TestCase("var {|ERP044:item|} = Create();")] + [TestCase("using var item = Create();")] + [TestCase("Consume(Create());")] + public Task Adoption_Explicit_Owned_Result_Respects_Cleanup_And_Transfer(string body) + { + return VerifyAsync(@" +public class Test +{ + [return: ReturnsOwnership] + private static Disposable Create() => new Disposable(); + + private static void Consume([AcquiresOwnership] Disposable item) => item.Dispose(); + + public void Run() { " + body + @" } +}"); + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.CarriedParameters.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.CarriedParameters.cs new file mode 100644 index 0000000..966f024 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.CarriedParameters.cs @@ -0,0 +1,207 @@ +using System.Threading.Tasks; +using NUnit.Framework; + +namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers; + +public partial class DisposeBeforeLosingScopeAnalyzerTests +{ + [TestCase("Task", "Task.FromResult(item)")] + [TestCase("ValueTask", "ValueTask.FromResult(item)")] + [TestCase("ValueTask", "new ValueTask(item)")] + public Task CarriedParameters_Explicit_Transfer_Preserves_Ownership(string type, string carrier) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + private object saved; + private void Take([AcquiresOwnership] " + type + @" pending) { saved = pending; } + public void Owned() + { + var item = new Disposable(); + Take(" + carrier + @"); + {|ERP046:item|}.ToString(); + } + public void Borrowed([DoNotDispose] Disposable item) + { + Take({|ERP046:" + carrier + @"|}); + } +}"); + } + + [TestCase("Task")] + [TestCase("ValueTask")] + public Task CarriedParameters_Acquired_Result_Is_Cleaned_Up_By_Awaiting(string type) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public async Task Run([AcquiresOwnership] " + type + @" pending) + { + var alias = pending; + var item = await alias.ConfigureAwait(false); + item.Dispose(); + ({|ERP046:await pending|}).ToString(); + } +}"); + } + + [TestCase("Task", "")] + [TestCase("Task", "pending.Dispose();")] + [TestCase("Task", "using (pending) { }")] + [TestCase("ValueTask", "_ = pending;")] + public Task CarriedParameters_Wrapper_Only_Cleanup_Does_Not_Satisfy_The_Contract(string type, string body) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public void Run([AcquiresOwnership] " + type + @" {|ERP044:pending|}) + { + " + body + @" + } +}"); + } + + [TestCase("Task")] + [TestCase("ValueTask")] + public Task CarriedParameters_Borrowed_Results_Cannot_Be_Disposed(string type) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public async Task Run([DoNotDispose] " + type + @" pending) + { + var alias = pending; + ({|ERP046:await alias.ConfigureAwait(false)|}).Dispose(); + } +}"); + } + + [TestCase("new Sink(Task.FromResult(item))")] + [TestCase("sink[Task.FromResult(item)]")] + [TestCase("(Sink)Task.FromResult(item)")] + public Task CarriedParameters_Other_Acquiring_Boundaries_Check_Carriers(string expression) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Sink +{ + private object saved; + public Sink() { } + public Sink([AcquiresOwnership] Task pending) { saved = pending; } + public int this[[AcquiresOwnership] Task pending] + { + get { saved = pending; return 0; } + } + public static implicit operator Sink([AcquiresOwnership] Task pending) => new Sink(pending); +} +public class Test +{ + public void Run(Sink sink, [DoNotDispose] Disposable item) + { + _ = " + expression.Replace("Task.FromResult(item)", "{|ERP046:Task.FromResult(item)|}") + @"; + } + public void Owned(Sink sink) + { + var item = new Disposable(); + _ = " + expression + @"; + {|ERP046:item|}.ToString(); + } +}"); + } + + [TestCase("Task", "Task.FromResult(item)")] + [TestCase("ValueTask", "ValueTask.FromResult(item)")] + public Task CarriedParameters_Source_Inference_Recognizes_Awaited_Cleanup(string type, string carrier) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + private static async Task Cleanup(" + type + @" pending) { (await pending).Dispose(); } + public async Task Run([DoNotDispose] Disposable item) + { + await Cleanup({|ERP046:" + carrier + @"|}); + } + public async Task Owned() + { + var item = new Disposable(); + await Cleanup(" + carrier + @"); + {|ERP046:item|}.ToString(); + } +}"); + } + + [Test] + public Task CarriedParameters_Source_Inference_Follows_Wrapped_Arguments() + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + private static object saved; + private static void Take([AcquiresOwnership] Task pending) { saved = pending; } + private static void Forward(Disposable value) { Take(Task.FromResult(value)); } + public void Run([DoNotDispose] Disposable item) { Forward({|ERP046:item|}); } + public void Owned() + { + var item = new Disposable(); + Forward(item); + {|ERP046:item|}.ToString(); + } +}"); + } + + [Test] + public Task CarriedParameters_Disposing_Only_The_Wrapper_Does_Not_Infer_Result_Acquisition() + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + private static void WrapperOnly(Task pending) { pending.Dispose(); } + public void Run([DoNotDispose] Disposable item) { WrapperOnly(Task.FromResult(item)); } + public async Task Owned([AcquiresOwnership] Task pending) + { + (await pending).Dispose(); + pending.Dispose(); + } +}"); + } + + [TestCase("Task")] + [TestCase("ValueTask")] + public Task CarriedParameters_Capture_Remains_An_Unknown_Handoff(string type) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public System.Action Run([AcquiresOwnership] " + type + @" pending) + => () => System.GC.KeepAlive(pending); +}"); + } + + [Test] + public Task CarriedParameters_Borrowed_Reassignment_Updates_Result_Binding() + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public async Task Replace([DoNotDispose] Task pending) + { + pending = Task.FromResult(new Disposable()); + (await pending).Dispose(); + } + public async Task Preserve([DoNotDispose] Task pending, [DoNotDispose] Disposable item, bool replace) + { + if (replace) { pending = Task.FromResult(item); } + ({|ERP046:await pending|}).Dispose(); + } +}"); + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.FreshReturns.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.FreshReturns.cs index 7c73c46..9fe0ace 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.FreshReturns.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.FreshReturns.cs @@ -218,7 +218,7 @@ public Task Fresh_Return_Supports_Generic_Extensions_Without_Receiver_Aliasing() return VerifyAsync(@" public static class Factories { - public static T FreshCopy(this T value) where T : System.IDisposable, new() => new T(); + public static T FreshCopy([DoNotDispose] this T value) where T : System.IDisposable, new() => new T(); } public class Test { diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.Inference.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.Inference.cs index c055e69..4f80b40 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.Inference.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.Inference.cs @@ -62,7 +62,7 @@ public void Run() var " + (leaks ? "{|ERP044:d|}" : "d") + @" = new Disposable(); " + call + @" } - private void Consume(Disposable borrowed, [AcquiresOwnership] Disposable owned) + private void Consume([DoNotDispose] Disposable borrowed, [AcquiresOwnership] Disposable owned) { owned.Dispose(); } @@ -159,7 +159,7 @@ public Task Recursive_Inference_Is_Bounded() return VerifyAsync(@" public class Test { - public void Run() { Loop({|ERP044:new Disposable()|}); } + public void Run([DoNotDispose] Disposable borrowed) { Loop(borrowed); } private static void Loop(Disposable value) { Loop(value); } }"); } @@ -332,17 +332,20 @@ public void Consume([AcquiresOwnership] Disposable {|ERP044:value|}) }"); } - [TestCase("var alias = value; alias.Dispose();", false)] - [TestCase("using var alias = value;", false)] - [TestCase("var alias = value; using (alias) { }", false)] - [TestCase("var alias = value; Forward(alias);", false)] - [TestCase("value = new Disposable(); value.Dispose();", true)] - public Task Infers_Disposal_Through_Simple_Callee_Aliases(string body, bool leaks) + [TestCase("var alias = value; alias.Dispose();", true)] + [TestCase("using var alias = value;", true)] + [TestCase("var alias = value; using (alias) { }", true)] + [TestCase("var alias = value; Forward(alias);", true)] + [TestCase("value = new Disposable(); value.Dispose();", false)] + public Task Infers_Disposal_Through_Simple_Callee_Aliases(string body, bool consumes) { return VerifyAsync(@" public class Test { - public void Run() { Consume(" + (leaks ? "{|ERP044:new Disposable()|}" : "new Disposable()") + @"); } + public void Run([DoNotDispose] Disposable borrowed) + { + Consume(" + (consumes ? "{|ERP046:borrowed|}" : "borrowed") + @"); + } private static void Consume(Disposable value) { " + body + @" } private static void Forward([AcquiresOwnership] Disposable value) { value.Dispose(); } }"); @@ -409,7 +412,7 @@ public void Dispose() { } } public static class BorrowExtensions { - [KeepsOwnership] public static T BorrowGeneric(this T resource) => default; + [KeepsOwnership] public static T BorrowGeneric([DoNotDispose] this T resource) => default; } public class Test { @@ -431,11 +434,11 @@ public Task Source_Forwarding_Uses_The_Same_Stream_Wrapper_Policy(string creatio return VerifyAsync(@" public class Test { - public void Run(bool leaveOpen) + public void Run(bool leaveOpen, [DoNotDispose] System.IO.Stream borrowed) { - var " + (retainsStream ? "{|ERP044:stream|}" : "stream") + @" = - new System.IO.FileStream(""path"", System.IO.FileMode.Open); + var stream = new System.IO.FileStream(""path"", System.IO.FileMode.Open); using var reader = Read(stream, leaveOpen); + using var other = Read(" + (retainsStream ? "borrowed" : "{|ERP046:borrowed|}") + @", leaveOpen); } private static System.IO.StreamReader Read(System.IO.Stream stream, bool leaveOpen) => " + creation + @"; }"); diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.LowNoise.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.LowNoise.cs new file mode 100644 index 0000000..c342aa5 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.LowNoise.cs @@ -0,0 +1,317 @@ +using System.Threading.Tasks; +using NUnit.Framework; + +namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers; + +public partial class DisposeBeforeLosingScopeAnalyzerTests +{ + [TestCase("Unknown(item); item.ToString();")] + [TestCase("_ = new Holder(item);")] + [TestCase("_ = this[item];")] + [TestCase("var alias = item; Unknown(alias);")] + [TestCase("System.Action callback = () => item.ToString();")] + [TestCase("System.Action callback = item.Dispose;")] + [TestCase("void Callback() { item.ToString(); }")] + [TestCase("var pending = Task.FromResult(item); Unknown(pending);")] + [TestCase("dynamic target = this; target.Unknown(item);")] + public Task LowNoise_Unknown_Handoffs_And_Captures_Are_Not_Leaks_Or_Definite_Moves(string body) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public void Run() + { + var item = new Disposable(); + " + body + @" + item.ToString(); + } + public static void Unknown(object value) { } + public int this[Disposable value] => 0; +} +public class Holder { public Holder(Disposable value) { } } +"); + } + + [TestCase("item.ToString();")] + [TestCase("_ = item;")] + [TestCase("Borrow(item);")] + [TestCase("_ = new Borrower(item);")] + [TestCase("System.Action callback = () => System.Console.WriteLine(nameof(item));")] + [TestCase("System.Action callback = () => { var other = new Disposable(); other.Dispose(); };")] + [TestCase("var alias = item.ThrowIfNull();")] + public Task LowNoise_Receivers_Borrowing_And_NonCaptures_Keep_The_Obligation(string body) + { + return VerifyAsync(@" +public class Test +{ + public void Run() + { + var {|ERP044:item|} = new Disposable(); + " + body + @" + } + public static void Borrow([DoNotDispose] Disposable value) { } +} +public class Borrower { public Borrower([DoNotDispose] Disposable value) { } } +"); + } + + [TestCase("Unknown(item);")] + [TestCase("System.Action callback = () => item.ToString();")] + public Task LowNoise_Uncertainty_Does_Not_Hide_Later_Definite_Misuse(string handoff) + { + return VerifyAsync(@" +public class Test +{ + public void Run() + { + var item = new Disposable(); + " + handoff + @" + item.Dispose(); + {|ERP046:item|}.ToString(); + } + private static void Unknown(object value) { } +}"); + } + + [Test] + public Task LowNoise_Unknown_Consumption_Is_Not_Inferred_As_Acquiring() + { + return VerifyAsync(@" +public class Test +{ + public void Run([DoNotDispose] Disposable borrowed) + { + Forward(borrowed); + borrowed.ToString(); + } + private static void Forward(Disposable value) { Unknown(value); } + private static void Unknown(object value) { } +}"); + } + + [TestCase("Task.FromResult(item)")] + [TestCase("new ValueTask(item)")] + [TestCase("ValueTask.FromResult(item)")] + public Task LowNoise_Discarding_A_Completed_Task_Keeps_The_Resource_Obligation(string expression) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public void Run() + { + var {|ERP044:item|} = new Disposable(); + _ = " + expression + @"; + } +}"); + } + + [TestCase("var pending = Task.FromResult(item); pending.Dispose();")] + [TestCase("using var pending = Task.FromResult(item);")] + [TestCase("var pending = Task.FromResult(item); var alias = pending; _ = alias;")] + [TestCase("var pending = Task.FromResult(item); pending = Task.FromResult(null); using var value = await pending;")] + public Task LowNoise_Task_Lifetime_Is_Not_Its_Result_Lifetime(string body) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public async Task Run() + { + var {|ERP044:item|} = new Disposable(); + " + body + @" + await Task.CompletedTask; + } +}"); + } + + [TestCase("using var value = await Task.FromResult(item);")] + [TestCase("using var value = await Task.FromResult(item).ConfigureAwait(false);")] + [TestCase("using var value = await new ValueTask(item);")] + [TestCase("using var value = await ValueTask.FromResult(item);")] + [TestCase("var pending = Task.FromResult(item); var alias = pending; pending = null; using var value = await alias;")] + [TestCase("var pending = Task.FromResult(item).ConfigureAwait(false); using var value = await pending;")] + [TestCase("var pending = Task.FromResult(item); var value = await pending; value.Dispose();")] + public Task LowNoise_Completed_Task_Results_Retain_Resource_Identity(string body) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public async Task Run() + { + var item = new Disposable(); + " + body + @" + } +}"); + } + + [Test] + public Task LowNoise_Completed_Task_Result_Reports_Use_After_Disposal() + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public async Task Run() + { + var item = new Disposable(); + var pending = Task.FromResult(item); + var value = await pending; + value.Dispose(); + {|ERP046:item|}.ToString(); + } +}"); + } + + [TestCase("using var value = {|ERP046:await Task.FromResult(item)|};")] + [TestCase("var pending = Task.FromResult(item); using var value = {|ERP046:await pending|};")] + [TestCase("var pending = new ValueTask(item); using var value = {|ERP046:await pending.ConfigureAwait(false)|};")] + [TestCase("var pending = Task.FromResult(item); var value = await pending; {|ERP046:value|}.Dispose();")] + [TestCase("using var pending = Task.FromResult(item);")] + [TestCase("var pending = Task.FromResult(item); pending = Task.FromResult(new Disposable()); using var value = await pending;")] + public Task LowNoise_Completed_Task_Results_Preserve_Borrowing_Not_Task_Borrowing(string body) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public async Task Run([DoNotDispose] Disposable item) + { + " + body + @" + await Task.CompletedTask; + } +}"); + } + + [Test] + public Task LowNoise_Owning_Task_Return_Cannot_Carry_A_Borrowed_Resource() + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + [return: ReturnsOwnership] + public Task Wrap([DoNotDispose] Disposable borrowed) + { + var pending = Task.FromResult(borrowed); + return {|ERP046:pending|}; + } +}"); + } + + [Test] + public Task LowNoise_Unknown_Handoff_Does_Not_Hide_A_Replacement_Lifetime() + { + return VerifyAsync(@" +public class Test +{ + public void Run() + { + var item = new Disposable(); + Unknown(item); + {|ERP044:item|} = new Disposable(); + } + private static void Unknown(object value) { } +}"); + } + + [Test] + public Task LowNoise_Acquired_Parameter_May_Be_Handed_To_An_Unknown_Consumer() + { + return VerifyAsync(@" +public class Test +{ + public void Forward([AcquiresOwnership] Disposable item) { Unknown(item); } + private static void Unknown(object value) { } +}"); + } + + [Test] + public Task LowNoise_Explicit_Borrowing_Survives_An_Unknown_Handoff() + { + return VerifyAsync(@" +public class Test +{ + public void Run([DoNotDispose] Disposable item) + { + Unknown(item); + {|ERP046:item|}.Dispose(); + } + private static void Unknown(object value) { } +}"); + } + + [TestCase("Task.FromResult(new Disposable())", "Task")] + [TestCase("new ValueTask(new Disposable())", "ValueTask")] + [TestCase("ValueTask.FromResult(new Disposable())", "ValueTask")] + public Task LowNoise_Completed_Wrappers_Can_Be_Returned_Through_A_Local(string value, string type) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + [return: ReturnsOwnership] + public " + type + @" Create() + { + var pending = " + value + @"; + return pending; + } +}"); + } + + [TestCase("Task.FromResult(item)")] + [TestCase("new ValueTask(item)")] + public Task LowNoise_Awaiting_Without_Cleanup_Does_Not_Discharge_A_Wrapped_Resource(string value) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public async Task Run() + { + var {|ERP044:item|} = new Disposable(); + var result = await " + value + @"; + result.ToString(); + } +}"); + } + + [Test] + public Task LowNoise_UserDefined_Conversion_Is_An_Unknown_Handoff_Not_An_Alias() + { + return VerifyAsync(@" +public sealed class Other : System.IDisposable +{ + public void Dispose() { } + public static implicit operator Other(Disposable input) => new Other(); +} +public class Test +{ + public void Run() + { + var input = new Disposable(); + using var output = (Other)input; + input.ToString(); + } +}"); + } + + [Test] + public Task LowNoise_Awaiting_A_Known_Disposed_Resource_Is_Still_Misuse() + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public async Task Run() + { + var item = new Disposable(); + var pending = Task.FromResult(item); + item.Dispose(); + var value = {|ERP046:await pending|}; + } +}"); + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.LowNoiseReview.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.LowNoiseReview.cs new file mode 100644 index 0000000..ea3dee1 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.LowNoiseReview.cs @@ -0,0 +1,191 @@ +using System.Threading.Tasks; +using NUnit.Framework; + +namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers; + +public partial class DisposeBeforeLosingScopeAnalyzerTests +{ + [TestCase("registry.Register(item);")] + [TestCase("registry.Identity(item);")] + public Task LowNoiseReview_Fluent_Receivers_Do_Not_Hide_Other_Unknown_Arguments(string body) + { + return VerifyAsync(@" +public interface IRegistry { IRegistry Register(Disposable value); } +public static class Extensions { public static T Identity(this T value, Disposable other) => value; } +public class Test +{ + public void Run(IRegistry registry) + { + var item = new Disposable(); + " + body + @" + item.ToString(); + } +}"); + } + + [TestCase("var item = new Disposable();", "using var result = await alias;")] + [TestCase("var {|ERP044:item|} = new Disposable();", "alias.Dispose();")] + public Task LowNoiseReview_Fluent_Carrier_Identity_Is_Not_Resource_Identity(string creation, string cleanup) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public static class Extensions { public static T Identity(this T value) => value; } +public class Test +{ + public async Task Run() + { + " + creation + @" + var pending = Task.FromResult(item); + var alias = pending.Identity(); + " + cleanup + @" + await Task.CompletedTask; + } +}"); + } + + [Test] + public Task LowNoiseReview_Fluent_Carriers_Preserve_Borrowing() + { + return VerifyAsync(@" +using System.Threading.Tasks; +public static class Extensions { public static T Identity(this T value) => value; } +public class Test +{ + public async Task Run([DoNotDispose] Disposable item) + { + var alias = Task.FromResult(item).Identity(); + using var result = {|ERP046:await alias|}; + } +}"); + } + + [TestCase("System.Action cleanup = () => item.Dispose();")] + [TestCase("void cleanup() { item.Dispose(); }")] + public Task LowNoiseReview_Capture_Before_Acquisition_Is_Unknown(string callback) + { + return VerifyAsync(@" +public class Test +{ + public void Run() + { + Disposable item = null; + " + callback + @" + item = new Disposable(); + cleanup(); + } +}"); + } + + [Test] + public Task LowNoiseReview_Nameof_Before_Acquisition_Does_Not_Capture() + { + return VerifyAsync(@" +public class Test +{ + public void Run() + { + Disposable item = null; + System.Action callback = () => System.Console.WriteLine(nameof(item)); + {|ERP044:item|} = new Disposable(); + callback(); + } +}"); + } + + [TestCase("_ = this + item;")] + [TestCase("dynamic target = this; _ = target[item];")] + public Task LowNoiseReview_Operator_And_Dynamic_Indexer_Arguments_Are_Handoffs(string handoff) + { + return VerifyAsync(@" +public class Test +{ + public static Test operator +(Test target, Disposable value) => target; + public void Run() + { + var item = new Disposable(); + " + handoff + @" + item.ToString(); + } +}"); + } + + [Test] + public Task LowNoiseReview_Operator_Input_Contracts_Are_Independent() + { + return VerifyAsync(@" +public class Test +{ + public static int operator +(Test target, [DoNotDispose] Disposable value) => 0; + public static int operator -(Test target, [AcquiresOwnership] Disposable value) { value.Dispose(); return 0; } + public void Run([DoNotDispose] Disposable borrowed) + { + var {|ERP044:item|} = new Disposable(); + _ = this + item; + _ = this - {|ERP046:borrowed|}; + } +}"); + } + + [TestCase("_ = +item;")] + [TestCase("item++;")] + [TestCase("item += 1;")] + public Task LowNoiseReview_Unannotated_Unary_And_Compound_Operators_Are_Handoffs(string body) + { + return VerifyAsync(@" +public class Resource : System.IDisposable +{ + public void Dispose() { } + public static int operator +(Resource value) => 0; + public static Resource operator ++(Resource value) => value; + public static Resource operator +(Resource value, int count) => value; +} +public class Test +{ + public void Run() + { + var item = new Resource(); + " + body + @" + } +}"); + } + + [TestCase("Interlocked.Exchange(ref pending, {|ERP046:Task.FromResult(item)|});", true)] + [TestCase("Interlocked.Exchange(ref pending, Task.FromResult(item)); {|ERP046:item|}.Dispose();", false)] + public Task LowNoiseReview_Interlocked_Carriers_Respect_Ownership(string body, bool borrowed) + { + return VerifyAsync(@" +using System.Threading; +using System.Threading.Tasks; +public class Test +{ + private Task pending; + public void Run(" + (borrowed ? "[DoNotDispose] Disposable item" : "") + @") + { + " + (borrowed ? "" : "var item = new Disposable();") + @" + " + body + @" + } +}"); + } + + [TestCase("(await Task.FromResult(value)).Dispose();", true)] + [TestCase("var pending = Task.FromResult(value); using var result = await pending.ConfigureAwait(false);", true)] + [TestCase("var pending = Task.FromResult(value); var alias = pending; pending = null; (await alias).Dispose();", true)] + [TestCase("var pending = Task.FromResult(value); pending = Task.FromResult(null); (await pending).Dispose();", false)] + [TestCase("var pending = Task.FromResult(value); pending.Dispose(); await Task.CompletedTask;", false)] + [TestCase("_ = Task.FromResult(value); await Task.CompletedTask;", false)] + [TestCase("var pending = Task.FromResult(value); Unknown(pending); await Task.CompletedTask;", false)] + [TestCase("var pending = new ValueTask(value); (await pending).Dispose();", true)] + [TestCase("var pending = ValueTask.FromResult(value); (await pending).Dispose();", true)] + public Task LowNoiseReview_Source_Consumption_Follows_Completed_Carriers_Only_When_Consumed(string body, bool consumes) + { + return VerifyAsync(@" +using System.Threading.Tasks; +public class Test +{ + public Task Run([DoNotDispose] Disposable borrowed) => Consume(" + + (consumes ? "{|ERP046:borrowed|}" : "borrowed") + @"); + private static async Task Consume(Disposable value) { " + body + @" } + private static void Unknown(object pending) { } +}"); + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.PrFeedback.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.PrFeedback.cs new file mode 100644 index 0000000..904ddba --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.PrFeedback.cs @@ -0,0 +1,244 @@ +using System.Threading.Tasks; +using NUnit.Framework; + +namespace ErrorProne.NET.CoreAnalyzers.Tests.DisposableAnalyzers; + +public partial class DisposeBeforeLosingScopeAnalyzerTests +{ + [TestCase("int DisposeAsync() => 0")] + [TestCase("void DisposeAsync() => System.GC.KeepAlive(null)")] + [TestCase("System.Threading.Tasks.Task DisposeAsync() => System.Threading.Tasks.Task.CompletedTask")] + [TestCase("System.Threading.Tasks.ValueTask DisposeAsync() => default")] + public Task PrFeedback_Unrelated_DisposeAsync_Does_Not_End_Ownership(string method) + { + return VerifyAsync(@" +public class Resource : System.IDisposable +{ + public void Dispose() { } + public " + method + @"; +} +public class Test +{ + public void Run() + { + var {|ERP044:resource|} = new Resource(); + resource.DisposeAsync(); + resource.ToString(); + } +}"); + } + + [TestCase("int", "0")] + [TestCase("System.Threading.Tasks.ValueTask", "default")] + public Task PrFeedback_Public_Lookalike_Is_Not_An_Explicit_Async_Implementation(string returnType, string result) + { + return VerifyAsync(@" +public class Resource : System.IAsyncDisposable +{ + System.Threading.Tasks.ValueTask System.IAsyncDisposable.DisposeAsync() => default; + public " + returnType + " DisposeAsync() => " + result + @"; +} +public class Test +{ + public void Run() + { + var {|ERP044:resource|} = new Resource(); + resource.DisposeAsync(); + resource.ToString(); + } +}"); + } + + [Test] + public Task PrFeedback_Lookalike_Is_Not_Borrowed_Cleanup_Or_Inferred_Acquisition() + { + return VerifyAsync(@" +public class Resource : System.IDisposable +{ + public void Dispose() { } + public int DisposeAsync() => 0; +} +public class Test +{ + private static void Inspect(Resource resource) { resource.DisposeAsync(); } + public void Run([DoNotDispose] Resource resource) + { + resource.DisposeAsync(); + Inspect(resource); + resource.ToString(); + } +}"); + } + + [TestCase("resource.DisposeAsync();")] + [TestCase("((System.IAsyncDisposable)resource).DisposeAsync();")] + [TestCase("System.Threading.Tasks.TaskAsyncEnumerableExtensions.ConfigureAwait(resource, false).DisposeAsync();")] + public Task PrFeedback_Actual_Async_Disposal_Still_Discharges_Ownership(string cleanup) + { + return VerifyAsync(@" +public class Resource : System.IAsyncDisposable +{ + public System.Threading.Tasks.ValueTask DisposeAsync() => default; +} +public class Test +{ + public void Run() + { + var resource = new Resource(); + " + cleanup + @" + } +}"); + } + + [TestCase("")] + [TestCase("public override System.Threading.Tasks.ValueTask DisposeAsync() => base.DisposeAsync();")] + public Task PrFeedback_Inherited_And_Overridden_Async_Disposal_Are_Recognized(string implementation) + { + return VerifyAsync(@" +public class Base : System.IAsyncDisposable +{ + public virtual System.Threading.Tasks.ValueTask DisposeAsync() => default; +} +public class Resource : Base { " + implementation + @" } +public class Test +{ + public void Run() + { + var resource = new Resource(); + resource.DisposeAsync(); + } +}"); + } + + [Test] + public Task PrFeedback_Reimplemented_Async_Disposal_Does_Not_Trust_An_Inherited_Lookalike() + { + return VerifyAsync(@" +public class Base : System.IAsyncDisposable +{ + public System.Threading.Tasks.ValueTask DisposeAsync() => default; +} +public class Resource : Base, System.IAsyncDisposable +{ + System.Threading.Tasks.ValueTask System.IAsyncDisposable.DisposeAsync() => default; +} +public class Test +{ + public void Run() + { + var {|ERP044:resource|} = new Resource(); + resource.DisposeAsync(); + } +}"); + } + + [Test] + public Task PrFeedback_Generic_DisposeAsync_Lookalike_Does_Not_End_Ownership() + { + return VerifyAsync(@" +public class Resource : System.IAsyncDisposable +{ + public System.Threading.Tasks.ValueTask DisposeAsync() => default; + public System.Threading.Tasks.ValueTask DisposeAsync() => default; +} +public class Test +{ + public void Run() + { + var {|ERP044:resource|} = new Resource(); + resource.DisposeAsync(); + } +}"); + } + + [TestCase("object", "")] + [TestCase("T", "")] + public Task PrFeedback_Explicit_Acquisition_Requires_Callee_Cleanup_For_Erased_Types(string type, string typeParameters) + { + return VerifyAsync(@" +public class Test +{ + private static void Take" + typeParameters + "([AcquiresOwnership] " + type + @" {|ERP044:value|}) { } + public void Run() + { + Take(new Disposable()); + } +}"); + } + + [TestCase("object", "", "((System.IDisposable)value).Dispose();")] + [TestCase("T", "", "(value as System.IDisposable)?.Dispose();")] + [TestCase("object", "", "if (value is System.IDisposable resource) { resource.Dispose(); }")] + [TestCase("T", "", "if (value is not System.IDisposable resource) return; resource.Dispose();")] + public Task PrFeedback_Erased_Acquiring_Parameters_Can_Be_Disposed(string type, string typeParameters, string cleanup) + { + return VerifyAsync(@" +public class Test +{ + private static void Take" + typeParameters + "([AcquiresOwnership] " + type + @" value) + { + " + cleanup + @" + } + public void Run() + { + Take(new Disposable()); + } +}"); + } + + [Test] + public Task PrFeedback_Pattern_Matching_Alone_Does_Not_Discharge_An_Acquiring_Parameter() + { + return VerifyAsync(@" +public class Test +{ + public void Take([AcquiresOwnership] T {|ERP044:value|}) + { + if (value is System.IDisposable resource) { resource.ToString(); } + } +}"); + } + + [Test] + public Task PrFeedback_Pattern_Aliases_Preserve_Borrowing() + { + return VerifyAsync(@" +public class Test +{ + public void Run([DoNotDispose] object value) + { + if (value is System.IDisposable resource) { {|ERP046:resource|}.Dispose(); } + } +}"); + } + + [TestCase("Dispose", "{|ERP046:value|}")] + [TestCase("ToString", "value")] + public Task PrFeedback_Pattern_Based_Source_Acquisition_Requires_Actual_Cleanup(string method, string argument) + { + return VerifyAsync(@" +public class Test +{ + private static void Inspect(object value) + { + if (value is System.IDisposable resource) { resource." + method + @"(); } + } + public void Run([DoNotDispose] Disposable value) { Inspect(" + argument + @"); } +}"); + } + + [TestCase("object", "")] + [TestCase("T", "")] + public Task PrFeedback_Erased_Acquiring_Parameters_Can_Transfer_To_Storage(string type, string typeParameters) + { + return VerifyAsync(@" +public class Test +{ + private object stored; + public void Take" + typeParameters + "([AcquiresOwnership] " + type + @" value) + { + stored = value; + } +}"); + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.ReviewRegressions.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.ReviewRegressions.cs index 35b9d7f..9544839 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.ReviewRegressions.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzerTests.ReviewRegressions.cs @@ -124,7 +124,7 @@ public Task User_Defined_Conversions_Do_Not_Imply_Resource_Identity(string body) public sealed class Other : System.IDisposable { public void Dispose() { } - public static implicit operator Other(Disposable value) => new Other(); + public static implicit operator Other([DoNotDispose] Disposable value) => new Other(); } public class Test { public void Run([DoNotDispose] Disposable borrowed) { " + body + @" } }"); } @@ -140,7 +140,7 @@ public void Dispose() { } } public class Test { - public void Run() { Consume({|ERP044:new Disposable()|}); } + public void Run([DoNotDispose] Disposable borrowed) { Consume(borrowed); } private static void Consume(Disposable d) { using var converted = (Other)d; } }"); } diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/OwnershipContractsTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/OwnershipContractsTests.cs index f8fbf38..f2fc035 100644 --- a/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/OwnershipContractsTests.cs +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/OwnershipContractsTests.cs @@ -44,6 +44,39 @@ private static Verify.Test CreateTest(string source, string xml) return test; } + [TestCase("object", "", "M:Api.Take(System.Object)")] + [TestCase("T", "", "M:Api.Take``1(``0)")] + public Task ExternalAcquisitionChecksErasedCalleeParameters(string type, string typeParameters, string memberId) + { + return CreateTest(@" +class Api +{ + public static void Take" + typeParameters + "(" + type + @" {|ERP044:value|}) { } + public static void Run() { Take(new Resource()); } +}", $@"") + .RunAsync(); + } + + [TestCase("Task", "Task.FromResult")] + [TestCase("ValueTask", "ValueTask.FromResult")] + public Task ExternalAcquisitionTransfersAndChecksAwaitedParameterResults(string type, string factory) + { + return CreateTest(@" +using System.Threading.Tasks; +class Api +{ + public static async Task Take(" + type + @" pending) { (await pending).Dispose(); } + public static async Task Run([DoNotDispose] Resource borrowed) + { + await Take({|ERP046:" + factory + @"(borrowed)|}); + var resource = new Resource(); + await Take(" + factory + @"(resource)); + {|ERP046:resource|}.ToString(); + } +}", @"") + .RunAsync(); + } + [Test] public Task ExternalBorrowedReturnsRemainBorrowedThroughLocalAliases() { @@ -288,7 +321,7 @@ public static void Run() var transferred = Library.Factory.Create(); Library.Consumer.Take(transferred); " + (annotated ? "{|ERP046:transferred|}" : "transferred") + @".Dispose(); - Library.Consumer.Take(" + (annotated ? "new Library.Resource()" : "{|ERP044:new Library.Resource()|}") + @"); + Library.Consumer.Take(new Library.Resource()); } }", annotated ? @" @@ -352,6 +385,9 @@ public static void Run() + + + ").RunAsync(); } diff --git a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzer.cs index 26c7a70..f9a79b1 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzer.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLosingScopeAnalyzer.cs @@ -57,14 +57,18 @@ protected override void InitializeCore(AnalysisContext context) } ReportBorrowedUses(operations, contracts, inference, block); + var capturedSymbols = new HashSet(SymbolEqualityComparer.Default); + foreach (var closure in operations.Where(o => o is IAnonymousFunctionOperation or ILocalFunctionOperation)) + { + CollectCapturedSymbols(closure, capturedSymbols, block.CancellationToken); + } foreach (var parameter in method.Parameters) { - if (contracts.AcquiresOwnership(parameter) - && helper.ShouldBeDisposed(parameter.Type)) + if (contracts.AcquiresOwnership(parameter)) { - var lifetime = new Lifetime(null, parameter, operations, contracts, inference, block.CancellationToken); - if (!lifetime.IsDischarged()) + var lifetime = new Lifetime(null, parameter, operations, capturedSymbols, contracts, inference, block.CancellationToken); + if (lifetime.Analyze() == LifetimeOutcome.Abandoned) { block.ReportDiagnostic(Diagnostic.Create(Rule, parameter.Locations.FirstOrDefault(), parameter.Name, parameter.Type.Name)); @@ -84,8 +88,8 @@ protected override void InitializeCore(AnalysisContext context) continue; } - var lifetime = new Lifetime(operation, null, operations, contracts, inference, block.CancellationToken); - if (!lifetime.IsDischarged()) + var lifetime = new Lifetime(operation, null, operations, capturedSymbols, contracts, inference, block.CancellationToken); + if (lifetime.Analyze() == LifetimeOutcome.Abandoned) { var (location, name) = GetDiagnosticTarget(operation); block.ReportDiagnostic(Diagnostic.Create(Rule, location, name, operation.Type!.Name)); @@ -136,28 +140,39 @@ private static void ReportBorrowedUses(ImmutableArray operations, Ow OwnershipInference inference, OperationBlockAnalysisContext context) { var bindings = new Dictionary(SymbolEqualityComparer.Default); + var carrierBindings = new Dictionary(SymbolEqualityComparer.Default); var reported = new HashSet(); foreach (var operation in operations) { context.CancellationToken.ThrowIfCancellationRequested(); + if (GetPatternAlias(operation) is { } patternAlias) + { + // A newly declared pattern local has no pre-existing non-borrowed binding to merge. + TrackBinding(patternAlias.Local, patternAlias.Value, conditional: false); + } + + foreach (var input in OwnershipInference.GetOperatorInputs(operation)) + { + if ((IsBorrowedValue(input.Value) || IsBorrowedCarrierValue(input.Value)) + && inference.AcquiresOwnership(input.Parameter, ImmutableArray.Empty, context.CancellationToken)) + { + Report(input.Value); + } + } if (operation is IInvocationOperation invocation) { - if (OwnershipInference.IsDisposeCall(invocation) && IsBorrowedValue(invocation.Instance)) + if (inference.IsDisposeCall(invocation) && IsBorrowedValue(invocation.Instance)) { Report(invocation.Instance!); } CheckArguments(invocation.Arguments); - if (inference.GetInterlockedOwningValue(invocation) is { } value && IsBorrowedValue(value)) + if (inference.GetInterlockedOwningValue(invocation) is { } value + && (IsBorrowedValue(value) || IsBorrowedCarrierValue(value))) { Report(value); } } - else if (operation is IConversionOperation conversion && IsBorrowedValue(conversion.Operand) - && inference.AcquiresConversionInput(conversion, context.CancellationToken)) - { - Report(conversion.Operand); - } else if (operation is IObjectCreationOperation creation) { CheckArguments(creation.Arguments); @@ -176,7 +191,8 @@ private static void ReportBorrowedUses(ImmutableArray operations, Ow } else if (operation is ISimpleAssignmentOperation assignment) { - if (IsBorrowedValue(assignment.Value) && IsOwningDestination(Unwrap(assignment.Target), contracts)) + if ((IsBorrowedValue(assignment.Value) || IsBorrowedCarrierValue(assignment.Value)) + && IsOwningDestination(Unwrap(assignment.Target), contracts)) { Report(assignment.Value); } @@ -187,7 +203,7 @@ private static void ReportBorrowedUses(ImmutableArray operations, Ow } } else if (operation is IReturnOperation { ReturnedValue: { } value } - && IsBorrowedValue(value) + && (IsBorrowedValue(value) || IsBorrowedCarrierValue(value)) && context.OwningSymbol is IMethodSymbol method && (method.AssociatedSymbol is IPropertySymbol property ? contracts.GetReturnOwnership(property) == OwnershipKind.Owned @@ -203,17 +219,24 @@ private static void ReportBorrowedUses(ImmutableArray operations, Ow } } - bool IsBorrowedValue(IOperation? value) => IsBorrowed(value, contracts, inference, bindings); + bool IsBorrowedValue(IOperation? value) => IsBorrowed(value, contracts, inference, bindings, carrierBindings); + bool IsBorrowedCarrierValue(IOperation? value) => IsBorrowedCarrier(value, contracts, inference, bindings, carrierBindings); void TrackBinding(ISymbol symbol, IOperation value, bool conditional) { var borrowed = IsBorrowedValue(value); + var carriedBorrowing = IsBorrowedCarrierValue(value); if (conditional) { borrowed &= bindings.TryGetValue(symbol, out var previous) ? previous : contracts.IsBorrowed(symbol); + carriedBorrowing &= carrierBindings.TryGetValue(symbol, out var previousCarrier) + ? previousCarrier + : symbol is IParameterSymbol parameter && inference.IsTaskResultCarrier(parameter.Type) + && contracts.IsBorrowed(parameter); } bindings[symbol] = borrowed; + carrierBindings[symbol] = carriedBorrowing; } void CheckUsingResources(IOperation resources) @@ -238,7 +261,8 @@ void CheckArguments(ImmutableArray arguments) { foreach (var argument in arguments) { - if (argument.Parameter != null && IsBorrowedValue(argument.Value) + if (argument.Parameter != null + && (IsBorrowedValue(argument.Value) || IsBorrowedCarrierValue(argument.Value)) && inference.AcquiresOwnership(argument.Parameter, arguments, context.CancellationToken)) { Report(argument.Value); @@ -268,7 +292,8 @@ private static bool IsOwningDestination(IOperation operation, OwnershipContracts } private static bool IsBorrowed(IOperation? operation, OwnershipContracts contracts, - OwnershipInference inference, IReadOnlyDictionary bindings) + OwnershipInference inference, IReadOnlyDictionary bindings, + IReadOnlyDictionary carrierBindings) { if (operation == null) { @@ -287,17 +312,62 @@ private static bool IsBorrowed(IOperation? operation, OwnershipContracts contrac IMemberReferenceOperation member => contracts.IsBorrowed(member.Member) || contracts.GetReturnOwnership(member.Member) == OwnershipKind.Borrowed, IInvocationOperation invocation when inference.GetConfiguredDisposableResource(invocation) is { } resource => - IsBorrowed(resource, contracts, inference, bindings), + IsBorrowed(resource, contracts, inference, bindings, carrierBindings), IInvocationOperation invocation => contracts.GetReturnOwnership(invocation.TargetMethod) == OwnershipKind.Borrowed, IConversionOperation { OperatorMethod: { } method } => contracts.GetReturnOwnership(method) == OwnershipKind.Borrowed, - IConditionalAccessInstanceOperation => IsBorrowed(GetConditionalReceiver(operation), contracts, inference, bindings), - IConditionalOperation conditional => IsBorrowed(conditional.WhenTrue, contracts, inference, bindings) - && IsBorrowed(conditional.WhenFalse, contracts, inference, bindings), - ICoalesceOperation coalesce => IsBorrowed(coalesce.Value, contracts, inference, bindings) - && IsBorrowed(coalesce.WhenNull, contracts, inference, bindings), - ISimpleAssignmentOperation assignment => IsBorrowed(assignment.Value, contracts, inference, bindings), + IConditionalAccessInstanceOperation => IsBorrowed(GetConditionalReceiver(operation), contracts, inference, bindings, carrierBindings), + IConditionalOperation conditional => IsBorrowed(conditional.WhenTrue, contracts, inference, bindings, carrierBindings) + && IsBorrowed(conditional.WhenFalse, contracts, inference, bindings, carrierBindings), + ICoalesceOperation coalesce => IsBorrowed(coalesce.Value, contracts, inference, bindings, carrierBindings) + && IsBorrowed(coalesce.WhenNull, contracts, inference, bindings, carrierBindings), + ISimpleAssignmentOperation assignment => IsBorrowed(assignment.Value, contracts, inference, bindings, carrierBindings), IAwaitOperation awaited => inference.GetAwaitedMember(awaited.Operation) is { } member - && contracts.GetReturnOwnership(member) == OwnershipKind.Borrowed, + && contracts.GetReturnOwnership(member) != OwnershipKind.Unspecified + ? contracts.GetReturnOwnership(member) == OwnershipKind.Borrowed + : IsBorrowedCarrier(awaited.Operation, contracts, inference, bindings, carrierBindings), + _ => false, + }; + } + + private static bool IsBorrowedCarrier(IOperation? operation, OwnershipContracts contracts, + OwnershipInference inference, IReadOnlyDictionary bindings, + IReadOnlyDictionary carrierBindings) + { + if (operation == null) + { + return false; + } + + operation = Unwrap(operation); + if (GetAliasSymbol(operation) is { } symbol && carrierBindings.TryGetValue(symbol, out var borrowed)) + { + return borrowed; + } + + if (inference.GetCompletedTaskValue(operation) is { } value) + { + return IsBorrowed(value, contracts, inference, bindings, carrierBindings); + } + + if (inference.GetConfiguredTask(operation) is { } task) + { + return IsBorrowedCarrier(task, contracts, inference, bindings, carrierBindings); + } + + if (operation is IInvocationOperation invocation && inference.GetFluentReceiver(invocation) is { } receiver) + { + return IsBorrowedCarrier(receiver, contracts, inference, bindings, carrierBindings); + } + + return operation switch + { + IParameterReferenceOperation parameter => inference.IsTaskResultCarrier(parameter.Type) + && contracts.IsBorrowed(parameter.Parameter), + ISimpleAssignmentOperation assignment => IsBorrowedCarrier(assignment.Value, contracts, inference, bindings, carrierBindings), + IConditionalOperation conditional => IsBorrowedCarrier(conditional.WhenTrue, contracts, inference, bindings, carrierBindings) + && IsBorrowedCarrier(conditional.WhenFalse, contracts, inference, bindings, carrierBindings), + ICoalesceOperation coalesce => IsBorrowedCarrier(coalesce.Value, contracts, inference, bindings, carrierBindings) + && IsBorrowedCarrier(coalesce.WhenNull, contracts, inference, bindings, carrierBindings), _ => false, }; } @@ -345,11 +415,18 @@ internal static IEnumerable EnumerateOperations(IOperation root) // Postorder follows evaluation order for arguments, initializers and their containing call. foreach (var child in root.ChildOperations) { - if (child is IAnonymousFunctionOperation or ILocalFunctionOperation or INameOfOperation) + if (child is INameOfOperation) { continue; } + if (child is IAnonymousFunctionOperation or ILocalFunctionOperation) + { + // Captures can end local certainty, but nested bodies are separate lifetimes. + yield return child; + continue; + } + foreach (var operation in EnumerateOperations(child)) { yield return operation; @@ -359,6 +436,26 @@ internal static IEnumerable EnumerateOperations(IOperation root) yield return root; } + private static void CollectCapturedSymbols(IOperation operation, HashSet symbols, CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + if (operation is INameOfOperation) + { + return; + } + + if (operation is ILocalReferenceOperation or IParameterReferenceOperation + && GetAliasSymbol(operation) is { } symbol) + { + symbols.Add(symbol); + } + + foreach (var child in operation.ChildOperations) + { + CollectCapturedSymbols(child, symbols, cancellationToken); + } + } + internal static IOperation Unwrap(IOperation operation) { while (true) @@ -396,6 +493,24 @@ internal static IOperation Unwrap(IOperation operation) }; } + internal static (IOperation Value, ILocalSymbol Local)? GetPatternAlias(IOperation operation) + { + if (operation is not IIsPatternOperation isPattern) + { + return null; + } + + var pattern = isPattern.Pattern; + while (pattern is INegatedPatternOperation negated) + { + pattern = negated.Pattern; + } + + // Only a direct type-pattern binding aliases the input, not nested property patterns. + return pattern is IDeclarationPatternOperation { DeclaredSymbol: ILocalSymbol local } + ? (isPattern.Value, local) : null; + } + internal static bool IsConditional(IOperation operation) { for (var parent = operation.Parent; parent != null; parent = parent.Parent) @@ -411,35 +526,49 @@ or ISwitchOperation or ILoopOperation or ICatchClauseOperation return false; } + private enum LifetimeOutcome + { + Abandoned, + Discharged, + Unknown, + } + private sealed class Lifetime { private readonly IOperation? _creation; private readonly ImmutableArray _operations; + private readonly HashSet _capturedSymbols; private readonly OwnershipContracts _contracts; private readonly OwnershipInference _inference; private readonly CancellationToken _cancellationToken; private readonly HashSet _aliases = new(SymbolEqualityComparer.Default); private readonly HashSet _uncertainAliases = new(SymbolEqualityComparer.Default); + private readonly HashSet _carriers = new(SymbolEqualityComparer.Default); + private readonly HashSet _uncertainCarriers = new(SymbolEqualityComparer.Default); public Diagnostic? InvalidUse { get; private set; } public Lifetime(IOperation? creation, IParameterSymbol? parameter, ImmutableArray operations, + HashSet capturedSymbols, OwnershipContracts contracts, OwnershipInference inference, CancellationToken cancellationToken) { _creation = creation; _operations = operations; + _capturedSymbols = capturedSymbols; _contracts = contracts; _inference = inference; _cancellationToken = cancellationToken; if (parameter != null) { - _aliases.Add(parameter); + // An acquiring Task/ValueTask parameter owns its result, not wrapper cleanup. + (inference.IsTaskResultCarrier(parameter.Type) ? _carriers : _aliases).Add(parameter); } } - public bool IsDischarged() + public LifetimeOutcome Analyze() { var started = _creation == null; + var ownershipUnknown = _capturedSymbols.Overlaps(_aliases) || _capturedSymbols.Overlaps(_carriers); foreach (var operation in _operations) { _cancellationToken.ThrowIfCancellationRequested(); @@ -452,13 +581,32 @@ public bool IsDischarged() } } + if (GetPatternAlias(operation) is { } patternAlias) + { + AssignAlias(patternAlias.Local, patternAlias.Value, operation); + } + + foreach (var input in OwnershipInference.GetOperatorInputs(operation)) + { + if ((Matches(input.Value) || Carries(input.Value)) + && _inference.AcquiresOwnership(input.Parameter, ImmutableArray.Empty, _cancellationToken)) + { + return Discharge(operation, Matches(input.Value, requireDefinite: true) + || Carries(input.Value, requireDefinite: true)); + } + if ((Matches(input.Value) || Carries(input.Value)) && !_contracts.IsBorrowed(input.Parameter)) + { + ownershipUnknown = true; + } + } + switch (operation) { case IVariableDeclaratorOperation { Initializer: { } initializer } declarator: AssignAlias(declarator.Symbol, initializer.Value, operation); break; case ISimpleAssignmentOperation assignment: - if (Matches(assignment.Value)) + if (Matches(assignment.Value) || Carries(assignment.Value)) { switch (Unwrap(assignment.Target)) { @@ -466,67 +614,86 @@ public bool IsDischarged() AssignAlias(local.Local, assignment.Value, operation); break; case IParameterReferenceOperation parameter when IsOwningDestination(parameter, _contracts): - return Discharge(operation, Matches(assignment.Value, requireDefinite: true)); + return Discharge(operation, Matches(assignment.Value, requireDefinite: true) + || Carries(assignment.Value, requireDefinite: true)); case IParameterReferenceOperation parameter: AssignAlias(parameter.Parameter, assignment.Value, operation); break; case IMemberReferenceOperation member when !_contracts.IsBorrowed(member.Member): - return Discharge(operation, Matches(assignment.Value, requireDefinite: true)); + return Discharge(operation, Matches(assignment.Value, requireDefinite: true) + || Carries(assignment.Value, requireDefinite: true)); } } else if (GetAliasSymbol(assignment.Target) is { } alias) { - if (IsConditional(operation)) - { - if (_aliases.Contains(alias)) - { - _uncertainAliases.Add(alias); - } - } - else - { - _aliases.Remove(alias); - _uncertainAliases.Remove(alias); - } + AssignAlias(alias, assignment.Value, operation); } break; case IInvocationOperation invocation: - if (OwnershipInference.IsDisposeCall(invocation) && Matches(invocation.Instance)) + if (_inference.IsDisposeCall(invocation) && Matches(invocation.Instance)) { return Discharge(operation, Matches(invocation.Instance, requireDefinite: true)); } - if (MovesArgument(invocation.Arguments) || Matches(_inference.GetInterlockedOwningValue(invocation))) + var published = _inference.GetInterlockedOwningValue(invocation); + if (MovesArgument(invocation.Arguments) || Matches(published) || Carries(published)) { return Discharge(operation, MovesArgument(invocation.Arguments, requireDefinite: true) - || Matches(_inference.GetInterlockedOwningValue(invocation), requireDefinite: true)); + || Matches(published, requireDefinite: true) || Carries(published, requireDefinite: true)); + } + if (_inference.GetCompletedTaskValue(invocation) == null + && Escapes(invocation.Arguments, _inference.GetFluentReceiver(invocation, _cancellationToken))) + { + ownershipUnknown = true; } break; - case IConversionOperation conversion when Matches(conversion.Operand) - && _inference.AcquiresConversionInput(conversion, _cancellationToken): - return Discharge(operation, Matches(conversion.Operand, requireDefinite: true)); case IObjectCreationOperation creation when MovesArgument(creation.Arguments): return Discharge(operation, MovesArgument(creation.Arguments, requireDefinite: true)); + case IObjectCreationOperation creation when _inference.GetCompletedTaskValue(creation) == null + && Escapes(creation.Arguments): + ownershipUnknown = true; + break; case IPropertyReferenceOperation property when MovesArgument(property.Arguments): return Discharge(operation, MovesArgument(property.Arguments, requireDefinite: true)); - case IReturnOperation returned when Matches(returned.ReturnedValue): - return true; + case IPropertyReferenceOperation property when Escapes(property.Arguments): + ownershipUnknown = true; + break; + case IReturnOperation returned when Matches(returned.ReturnedValue) || Carries(returned.ReturnedValue): + return LifetimeOutcome.Discharged; + case IDelegateCreationOperation { Target: IMethodReferenceOperation method } when Matches(method.Instance): + ownershipUnknown = true; + break; + case IDynamicInvocationOperation dynamicInvocation + when dynamicInvocation.Arguments.Any(a => Matches(a) || Carries(a)): + ownershipUnknown = true; + break; + case IDynamicObjectCreationOperation dynamicCreation + when dynamicCreation.Arguments.Any(a => Matches(a) || Carries(a)): + ownershipUnknown = true; + break; + case IDynamicIndexerAccessOperation dynamicIndexer + when dynamicIndexer.Arguments.Any(a => Matches(a) || Carries(a)): + ownershipUnknown = true; + break; case IUsingDeclarationOperation declaration when declaration.DeclarationGroup.Declarations .SelectMany(d => d.Declarators).Any(d => _aliases.Contains(d.Symbol)): - return true; + return LifetimeOutcome.Discharged; } + // Closures capture variables, including resources assigned after closure creation. + ownershipUnknown |= _capturedSymbols.Overlaps(_aliases) || _capturedSymbols.Overlaps(_carriers); + if (GetUsingCapture(operation) is { } captured && (Matches(operation) || captured.Locals.Any(l => _aliases.Contains(l)))) { - return true; + return LifetimeOutcome.Discharged; } } - return false; + return ownershipUnknown ? LifetimeOutcome.Unknown : LifetimeOutcome.Abandoned; } - private bool Discharge(IOperation terminal, bool definite) + private LifetimeOutcome Discharge(IOperation terminal, bool definite) { // Restrict invalid-use reports to later statements in the very same lexical block. // Conditional cleanup, finally blocks and implicit using cleanup remain best effort. @@ -535,7 +702,7 @@ private bool Discharge(IOperation terminal, bool definite) || IsConditional(terminal) || EnumerateOperations(terminal).Any(o => o is IConditionalOperation or ICoalesceOperation)) { - return true; + return LifetimeOutcome.Discharged; } var passedTerminal = false; @@ -559,7 +726,7 @@ private bool Discharge(IOperation terminal, bool definite) } if (operation.Syntax.SpanStart < statement.Span.End - || operation is not (ILocalReferenceOperation or IParameterReferenceOperation) + || operation is not (ILocalReferenceOperation or IParameterReferenceOperation or IAwaitOperation) || !Matches(operation, requireDefinite: true) || IsConditional(operation) || operation.Syntax.FirstAncestorOrSelf()?.Parent != block) @@ -578,37 +745,99 @@ private bool Discharge(IOperation terminal, bool definite) break; } - return true; + return LifetimeOutcome.Discharged; } private void AssignAlias(ISymbol symbol, IOperation value, IOperation assignment) { - if (Matches(value)) + var matches = Matches(value); + var carries = Carries(value); + var definiteMatch = Matches(value, requireDefinite: true); + var definiteCarrier = Carries(value, requireDefinite: true); + Track(_aliases, _uncertainAliases, matches, definiteMatch); + Track(_carriers, _uncertainCarriers, carries, definiteCarrier); + + void Track(HashSet aliases, HashSet uncertain, bool matchesValue, bool definite) { - // May-alias tracking is enough for best-effort cleanup, but not to prove misuse. - var definite = Matches(value, requireDefinite: true) && !IsConditional(assignment); - _aliases.Add(symbol); - if (definite) + if (matchesValue) { - _uncertainAliases.Remove(symbol); + aliases.Add(symbol); + if (definite && !IsConditional(assignment)) + { + uncertain.Remove(symbol); + } + else + { + uncertain.Add(symbol); + } + } + else if (IsConditional(assignment) && aliases.Contains(symbol)) + { + uncertain.Add(symbol); } else { - _uncertainAliases.Add(symbol); + aliases.Remove(symbol); + uncertain.Remove(symbol); } } - else + } + + private bool Escapes(ImmutableArray arguments, IOperation? preservedReceiver = null) + { + return arguments.Any(argument => (Matches(argument.Value) || Carries(argument.Value)) + && !ReferenceEquals(argument.Value, preservedReceiver) + && _inference.IsUnknownArgument(argument, arguments)); + } + + private bool Carries(IOperation? value, bool requireDefinite = false) + { + if (value == null) + { + return false; + } + + value = Unwrap(value); + if (GetAliasSymbol(value) is { } symbol) + { + return _carriers.Contains(symbol) && (!requireDefinite || !_uncertainCarriers.Contains(symbol)); + } + + if (_inference.GetCompletedTaskValue(value) is { } result) { - _aliases.Remove(symbol); - _uncertainAliases.Remove(symbol); + return Matches(result, requireDefinite); } + + if (_inference.GetConfiguredTask(value) is { } task) + { + return Carries(task, requireDefinite); + } + + if (value is IInvocationOperation invocation + && _inference.GetFluentReceiver(invocation, _cancellationToken) is { } receiver) + { + return Carries(receiver, requireDefinite); + } + + return value switch + { + ISimpleAssignmentOperation assignment => Carries(assignment.Value, requireDefinite), + IConditionalOperation conditional => requireDefinite + ? Carries(conditional.WhenTrue, true) && Carries(conditional.WhenFalse, true) + : Carries(conditional.WhenTrue) || Carries(conditional.WhenFalse), + ICoalesceOperation coalesce => requireDefinite + ? Carries(coalesce.Value, true) && Carries(coalesce.WhenNull, true) + : Carries(coalesce.Value) || Carries(coalesce.WhenNull), + _ => false, + }; } private bool MovesArgument(ImmutableArray arguments, bool requireDefinite = false) { foreach (var argument in arguments) { - if (argument.Parameter != null && Matches(argument.Value, requireDefinite) + if (argument.Parameter != null + && (Matches(argument.Value, requireDefinite) || Carries(argument.Value, requireDefinite)) && _inference.AcquiresOwnership(argument.Parameter, arguments, _cancellationToken)) { return true; @@ -649,10 +878,12 @@ private bool Matches(IOperation? value, bool requireDefinite = false) : Matches(coalesce.Value) || Matches(coalesce.WhenNull); case ISimpleAssignmentOperation assignment: return Matches(assignment.Value, requireDefinite); + case IAwaitOperation awaited: + return Carries(awaited.Operation, requireDefinite); case IInvocationOperation invocation when _inference.GetConfiguredDisposableResource(invocation) is { } resource: return Matches(resource, requireDefinite); - case IInvocationOperation invocation when _inference.IsFluentAlias(invocation.TargetMethod, _cancellationToken): - return Matches(invocation.Instance ?? invocation.Arguments.FirstOrDefault()?.Value, requireDefinite); + case IInvocationOperation invocation when _inference.GetFluentReceiver(invocation, _cancellationToken) is { } receiver: + return Matches(receiver, requireDefinite); } return false; diff --git a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/OwnershipInference.cs b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/OwnershipInference.cs index 2ae1166..fcb0a30 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/OwnershipInference.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/OwnershipInference.cs @@ -15,7 +15,11 @@ internal sealed class OwnershipInference private readonly OwnershipContracts _contracts; private readonly DisposeAnalysisHelper _helper; private readonly IMethodSymbol? _configureAsyncDisposable; + private readonly IMethodSymbol? _disposeAsync; + private readonly IMethodSymbol? _configuredDisposeAsync; private readonly HashSet _configureAwaitMethods; + private readonly HashSet _completedTaskFactories; + private readonly INamedTypeSymbol? _valueTaskOfT; private readonly ConcurrentDictionary _acquires = new(SymbolEqualityComparer.Default); private readonly ConcurrentDictionary _freshReturns = new(SymbolEqualityComparer.Default); @@ -24,6 +28,10 @@ public OwnershipInference(Compilation compilation, OwnershipContracts contracts, _compilation = compilation; _contracts = contracts; _helper = helper; + _disposeAsync = helper.IAsyncDisposable?.GetMembers("DisposeAsync").OfType() + .FirstOrDefault(method => !method.IsStatic && method.Parameters.IsEmpty); + _configuredDisposeAsync = helper.IConfigureAsyncDisposable?.GetMembers("DisposeAsync").OfType() + .FirstOrDefault(method => !method.IsStatic && method.Parameters.IsEmpty); _configureAsyncDisposable = compilation.GetTypeByMetadataName("System.Threading.Tasks.TaskAsyncEnumerableExtensions") ?.GetMembers("ConfigureAwait").OfType().FirstOrDefault(method => SymbolEqualityComparer.Default.Equals(method.ReturnType, helper.IConfigureAsyncDisposable)); @@ -33,6 +41,39 @@ public OwnershipInference(Compilation compilation, OwnershipContracts contracts, "System.Threading.Tasks.ValueTask", "System.Threading.Tasks.ValueTask`1", }.SelectMany(name => compilation.GetTypeByMetadataName(name)?.GetMembers("ConfigureAwait") .OfType() ?? Enumerable.Empty()), SymbolEqualityComparer.Default); + _completedTaskFactories = new HashSet(new[] + { + "System.Threading.Tasks.Task", "System.Threading.Tasks.ValueTask", + }.SelectMany(name => compilation.GetTypeByMetadataName(name)?.GetMembers("FromResult") + .OfType() ?? Enumerable.Empty()), SymbolEqualityComparer.Default); + _valueTaskOfT = compilation.GetTypeByMetadataName("System.Threading.Tasks.ValueTask`1"); + } + + public IOperation? GetCompletedTaskValue(IOperation operation) + { + operation = DisposeBeforeLosingScopeAnalyzer.Unwrap(operation); + return operation switch + { + IInvocationOperation invocation when _completedTaskFactories.Contains(invocation.TargetMethod.OriginalDefinition) + && _contracts.GetReturnOwnership(invocation.TargetMethod) == OwnershipKind.Unspecified + => invocation.Arguments.FirstOrDefault(a => a.Parameter?.Ordinal == 0)?.Value, + IObjectCreationOperation { Constructor: { Parameters.Length: 1 } constructor } creation + when SymbolEqualityComparer.Default.Equals(constructor.ContainingType.OriginalDefinition, _valueTaskOfT) + && constructor.OriginalDefinition.Parameters[0].Type is ITypeParameterSymbol + => creation.Arguments.FirstOrDefault(a => a.Parameter?.Ordinal == 0)?.Value, + _ => null, + }; + } + + internal bool IsTaskResultCarrier(ITypeSymbol? type) => + type.IsTaskLike(_compilation, TaskLikeTypes.TaskOfT | TaskLikeTypes.ValueTaskOfT); + + public IOperation? GetConfiguredTask(IOperation operation) + { + return operation is IInvocationOperation { Instance: { } instance } invocation + && _configureAwaitMethods.Contains(invocation.TargetMethod.OriginalDefinition) + && _contracts.GetReturnOwnership(invocation.TargetMethod) == OwnershipKind.Unspecified + ? instance : null; } public IOperation? GetConfiguredDisposableResource(IInvocationOperation invocation) @@ -51,9 +92,7 @@ public OwnershipInference(Compilation compilation, OwnershipContracts contracts, public ISymbol? GetAwaitedMember(IOperation operation) { operation = DisposeBeforeLosingScopeAnalyzer.Unwrap(operation); - if (operation is IInvocationOperation { Instance: { } instance } invocation - && _configureAwaitMethods.Contains(invocation.TargetMethod.OriginalDefinition) - && _contracts.GetReturnOwnership(invocation.TargetMethod) == OwnershipKind.Unspecified) + if (GetConfiguredTask(operation) is { } instance) { return GetAwaitedMember(instance); } @@ -82,10 +121,28 @@ public OwnershipInference(Compilation compilation, OwnershipContracts contracts, ? invocation.Arguments.FirstOrDefault(a => a.Parameter?.Ordinal == 1)?.Value : null; } - public bool AcquiresConversionInput(IConversionOperation conversion, CancellationToken cancellationToken) + internal static IEnumerable<(IParameterSymbol Parameter, IOperation Value)> GetOperatorInputs(IOperation operation) { - return conversion.OperatorMethod is { Parameters.Length: 1 } method - && AcquiresOwnership(method.Parameters[0], ImmutableArray.Empty, cancellationToken); + switch (operation) + { + case IConversionOperation { OperatorMethod: { Parameters.Length: 1 } method } conversion: + yield return (method.Parameters[0], conversion.Operand); + break; + case IUnaryOperation { OperatorMethod: { Parameters.Length: 1 } method } unary: + yield return (method.Parameters[0], unary.Operand); + break; + case IBinaryOperation { OperatorMethod: { Parameters.Length: 2 } method } binary: + yield return (method.Parameters[0], binary.LeftOperand); + yield return (method.Parameters[1], binary.RightOperand); + break; + case IIncrementOrDecrementOperation { OperatorMethod: { Parameters.Length: 1 } method } increment: + yield return (method.Parameters[0], increment.Target); + break; + case ICompoundAssignmentOperation { OperatorMethod: { Parameters.Length: 2 } method } assignment: + yield return (method.Parameters[0], assignment.Target); + yield return (method.Parameters[1], assignment.Value); + break; + } } public bool ReturnsOwnership(IMethodSymbol method, CancellationToken cancellationToken) @@ -113,6 +170,13 @@ public bool IsFluentAlias(IMethodSymbol method, CancellationToken cancellationTo return candidate && !HasFreshReturn(method, cancellationToken); } + public IOperation? GetFluentReceiver(IInvocationOperation invocation, CancellationToken cancellationToken = default) + { + return IsFluentAlias(invocation.TargetMethod, cancellationToken) + ? invocation.Instance ?? invocation.Arguments.FirstOrDefault(a => a.Parameter?.Ordinal == 0)?.Value + : null; + } + private bool HasFreshReturn(IMethodSymbol method, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); @@ -197,6 +261,12 @@ public bool AcquiresOwnership(IParameterSymbol parameter, ImmutableArray InferAcquisition(p, new HashSet(SymbolEqualityComparer.Default), cancellationToken)); } + public bool IsUnknownArgument(IArgumentOperation argument, ImmutableArray arguments) + { + return argument.Parameter is not { } parameter + || KnownAcquisition(parameter, arguments) == null; + } + private bool? KnownAcquisition(IParameterSymbol parameter, ImmutableArray arguments) { if (_contracts.AcquiresOwnership(parameter)) @@ -212,7 +282,9 @@ public bool AcquiresOwnership(IParameterSymbol parameter, ImmutableArray a.Parameter?.Name == "leaveOpen"); - return leaveOpen == null || leaveOpen.Value.ConstantValue is { HasValue: true, Value: false }; + return leaveOpen == null ? true + : leaveOpen.Value.ConstantValue is { HasValue: true, Value: bool value } ? !value + : null; } return null; @@ -235,36 +307,46 @@ private bool InferAcquisition(IParameterSymbol parameter, HashSet(SymbolEqualityComparer.Default) { parameter }; + var aliases = new HashSet(SymbolEqualityComparer.Default); + var carriers = new HashSet(SymbolEqualityComparer.Default); + (IsTaskResultCarrier(parameter.Type) ? carriers : aliases).Add(parameter); foreach (var operation in DisposeBeforeLosingScopeAnalyzer.EnumerateOperations(root)) { + cancellationToken.ThrowIfCancellationRequested(); + if (DisposeBeforeLosingScopeAnalyzer.GetPatternAlias(operation) is { } patternAlias) + { + TrackAlias(patternAlias.Local, patternAlias.Value, conditional: true); + } + + if (GetOperatorInputs(operation).Any(input => + (ReferencesResource(input.Value, aliases, carriers) || ReferencesCarrier(input.Value, aliases, carriers)) + && (KnownAcquisition(input.Parameter, ImmutableArray.Empty) + ?? InferAcquisition(input.Parameter.OriginalDefinition, visiting, cancellationToken)))) + { + return true; + } + if (operation is IInvocationOperation invocation) { - if ((IsDisposeCall(invocation) && ReferencesResource(invocation.Instance, aliases)) - || ReferencesResource(GetInterlockedOwningValue(invocation), aliases)) + var published = GetInterlockedOwningValue(invocation); + if ((IsDisposeCall(invocation) && ReferencesResource(invocation.Instance, aliases, carriers)) + || ReferencesResource(published, aliases, carriers) || ReferencesCarrier(published, aliases, carriers)) { return true; } - if (ForwardsOwnership(invocation.Arguments, aliases, visiting, cancellationToken)) + if (ForwardsOwnership(invocation.Arguments, aliases, carriers, visiting, cancellationToken)) { return true; } } else if (operation is IObjectCreationOperation creation - && ForwardsOwnership(creation.Arguments, aliases, visiting, cancellationToken)) + && ForwardsOwnership(creation.Arguments, aliases, carriers, visiting, cancellationToken)) { return true; } else if (operation is IPropertyReferenceOperation property - && ForwardsOwnership(property.Arguments, aliases, visiting, cancellationToken)) - { - return true; - } - else if (operation is IConversionOperation { OperatorMethod: { Parameters.Length: 1 } method } conversion - && ReferencesResource(conversion.Operand, aliases) - && (KnownAcquisition(method.Parameters[0], ImmutableArray.Empty) - ?? InferAcquisition(method.Parameters[0].OriginalDefinition, visiting, cancellationToken))) + && ForwardsOwnership(property.Arguments, aliases, carriers, visiting, cancellationToken)) { return true; } @@ -273,38 +355,55 @@ private bool InferAcquisition(IParameterSymbol parameter, HashSet arguments, HashSet aliases, + private bool ForwardsOwnership(ImmutableArray arguments, HashSet aliases, HashSet carriers, HashSet visiting, CancellationToken cancellationToken) { - return arguments.Any(a => a.Parameter != null && ReferencesResource(a.Value, aliases) + return arguments.Any(a => a.Parameter != null + && (ReferencesResource(a.Value, aliases, carriers) || ReferencesCarrier(a.Value, aliases, carriers)) && (KnownAcquisition(a.Parameter, arguments) ?? InferAcquisition(a.Parameter.OriginalDefinition, visiting, cancellationToken))); } @@ -331,14 +431,44 @@ private static bool IsStreamWrapperParameter(IParameterSymbol parameter) "System.IO.StreamReader" or "System.IO.StreamWriter" or "System.IO.BinaryReader" or "System.IO.BinaryWriter"; } - internal static bool IsDisposeCall(IInvocationOperation invocation) + internal bool IsDisposeCall(IInvocationOperation invocation) { - return !invocation.TargetMethod.IsStatic && invocation.Arguments.IsEmpty - && ((invocation.TargetMethod.Name is "Dispose" or "Close" && invocation.TargetMethod.ReturnsVoid) - || invocation.TargetMethod.Name == "DisposeAsync"); + var method = invocation.TargetMethod; + if (method.IsStatic || !invocation.Arguments.IsEmpty) + { + return false; + } + + if (method.Name is "Dispose" or "Close" && method.ReturnsVoid) + { + return true; + } + + if (method.Name != "DisposeAsync") + { + return false; + } + + if (SymbolEqualityComparer.Default.Equals(method.OriginalDefinition, _disposeAsync) + || SymbolEqualityComparer.Default.Equals(method.OriginalDefinition, _configuredDisposeAsync)) + { + return true; + } + + var receiver = invocation.Instance?.Type as INamedTypeSymbol ?? method.ContainingType; + var implementation = _disposeAsync == null ? null : receiver.FindImplementationForInterfaceMember(_disposeAsync); + for (IMethodSymbol? candidate = method; candidate != null; candidate = candidate.OverriddenMethod) + { + if (SymbolEqualityComparer.Default.Equals(candidate.OriginalDefinition, implementation?.OriginalDefinition)) + { + return true; + } + } + + return false; } - private bool ReferencesResource(IOperation? operation, HashSet aliases) + private bool ReferencesResource(IOperation? operation, HashSet aliases, HashSet carriers) { if (operation == null) { @@ -353,19 +483,51 @@ private bool ReferencesResource(IOperation? operation, HashSet aliases) if (operation is IConditionalAccessInstanceOperation) { - return ReferencesResource(DisposeBeforeLosingScopeAnalyzer.GetConditionalReceiver(operation), aliases); + return ReferencesResource(DisposeBeforeLosingScopeAnalyzer.GetConditionalReceiver(operation), aliases, carriers); } if (operation is IInvocationOperation invocation && GetConfiguredDisposableResource(invocation) is { } resource) { - return ReferencesResource(resource, aliases); + return ReferencesResource(resource, aliases, carriers); } if (operation is ISimpleAssignmentOperation assignment) { - return ReferencesResource(assignment.Value, aliases); + return ReferencesResource(assignment.Value, aliases, carriers); } - return false; + return operation is IAwaitOperation awaited && ReferencesCarrier(awaited.Operation, aliases, carriers); + } + + private bool ReferencesCarrier(IOperation? operation, HashSet aliases, HashSet carriers) + { + if (operation == null) + { + return false; + } + + operation = DisposeBeforeLosingScopeAnalyzer.Unwrap(operation); + if (DisposeBeforeLosingScopeAnalyzer.GetAliasSymbol(operation) is { } symbol) + { + return carriers.Contains(symbol); + } + + if (GetCompletedTaskValue(operation) is { } result) + { + return ReferencesResource(result, aliases, carriers); + } + + if (GetConfiguredTask(operation) is { } task) + { + return ReferencesCarrier(task, aliases, carriers); + } + + if (operation is IInvocationOperation invocation && GetFluentReceiver(invocation) is { } receiver) + { + return ReferencesCarrier(receiver, aliases, carriers); + } + + return operation is ISimpleAssignmentOperation assignment + && ReferencesCarrier(assignment.Value, aliases, carriers); } }