Skip to content

Commit e13ed0f

Browse files
wieslawsolteswieslawsoltes
committed
Improve disposable analyzers
1 parent cef3210 commit e13ed0f

21 files changed

Lines changed: 1721 additions & 457 deletions

‎.claude/settings.local.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,9 @@
4747
"Bash(wc:*)",
4848
"Bash(dotnet script:*)",
4949
"Read(//Users/wieslawsoltes/.nuget/packages/microsoft.codeanalysis.csharp.codefix.testing.xunit/1.1.2/**)",
50-
"Bash(awk:*)"
50+
"Bash(awk:*)",
51+
"Bash(tee:*)",
52+
"Read(//private/tmp/**)"
5153
],
5254
"deny": [],
5355
"ask": []

‎src/DisposableAnalyzer/Analyzers/DisposableBaseCallAnalyzer.cs‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,14 @@ private void AnalyzeOperationBlockStart(OperationBlockStartAnalysisContext conte
4747

4848
// Check if this type has a disposable base class
4949
var containingType = method.ContainingType;
50-
if (!DisposableHelper.HasDisposableBase(containingType))
50+
var baseType = containingType.BaseType;
51+
if (baseType == null)
52+
return;
53+
54+
var baseImplementsDisposal = DisposableHelper.IsAnyDisposableType(baseType) ||
55+
baseType.GetMembers("Dispose").OfType<IMethodSymbol>().Any();
56+
57+
if (!baseImplementsDisposal)
5158
return;
5259

5360
var hasBaseCall = false;
@@ -57,15 +64,12 @@ private void AnalyzeOperationBlockStart(OperationBlockStartAnalysisContext conte
5764
if (operationContext.Operation is IInvocationOperation invocation)
5865
{
5966
// Check for base.Dispose() or base.Dispose(disposing) call
60-
if (invocation.Instance is IInstanceReferenceOperation instanceRef &&
61-
instanceRef.ReferenceKind == InstanceReferenceKind.ContainingTypeInstance)
67+
var targetMethod = invocation.TargetMethod;
68+
if (targetMethod.Name == "Dispose" &&
69+
baseType != null &&
70+
SymbolEqualityComparer.Default.Equals(targetMethod.ContainingType, baseType))
6271
{
63-
var targetMethod = invocation.TargetMethod;
64-
if (targetMethod.Name == "Dispose" &&
65-
!SymbolEqualityComparer.Default.Equals(targetMethod.ContainingType, containingType))
66-
{
67-
hasBaseCall = true;
68-
}
72+
hasBaseCall = true;
6973
}
7074
}
7175
}, OperationKind.Invocation);

‎src/DisposableAnalyzer/Analyzers/DisposableFactoryPatternAnalyzer.cs‎

Lines changed: 41 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using System;
12
using System.Collections.Immutable;
23
using System.Linq;
34
using Microsoft.CodeAnalysis;
@@ -7,20 +8,21 @@
78
namespace DisposableAnalyzer.Analyzers;
89

910
/// <summary>
10-
/// Analyzer that validates factory method naming for disposable returns.
11-
/// DISP027: Factory method disposal responsibility
11+
/// Analyzer that ensures factory methods returning disposable instances
12+
/// clearly document the caller's disposal responsibilities.
13+
/// DISP027: Factory method disposal responsibility.
1214
/// </summary>
1315
[DiagnosticAnalyzer(LanguageNames.CSharp)]
1416
public class DisposableFactoryPatternAnalyzer : DiagnosticAnalyzer
1517
{
1618
private static readonly DiagnosticDescriptor Rule = new(
1719
id: DiagnosticIds.DisposableFactoryPattern,
18-
title: "Factory method should use clear naming for disposal ownership",
19-
messageFormat: "Method '{0}' returns IDisposable. Consider naming it 'Create{0}' to indicate caller owns disposal",
20-
category: "Design",
21-
defaultSeverity: DiagnosticSeverity.Info,
20+
title: "Factory method should document disposal ownership",
21+
messageFormat: "Method '{0}' returns a disposable type but XML documentation does not describe caller disposal responsibility",
22+
category: "Documentation",
23+
defaultSeverity: DiagnosticSeverity.Warning,
2224
isEnabledByDefault: true,
23-
description: "Factory methods returning disposables should use naming conventions (Create*, Build*, Make*) to indicate the caller is responsible for disposal.");
25+
description: "Factory methods that create disposable instances should document that the caller must dispose the returned value.");
2426

2527
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
2628

@@ -32,48 +34,52 @@ public override void Initialize(AnalysisContext context)
3234
context.RegisterSymbolAction(AnalyzeMethod, SymbolKind.Method);
3335
}
3436

35-
private void AnalyzeMethod(SymbolAnalysisContext context)
37+
private static void AnalyzeMethod(SymbolAnalysisContext context)
3638
{
37-
var method = (IMethodSymbol)context.Symbol;
39+
if (context.Symbol is not IMethodSymbol method)
40+
return;
3841

39-
// Skip special methods
4042
if (method.MethodKind != MethodKind.Ordinary)
4143
return;
4244

43-
// Skip private/internal methods
44-
if (method.DeclaredAccessibility != Accessibility.Public &&
45-
method.DeclaredAccessibility != Accessibility.Protected)
45+
if (method.ReturnsVoid)
4646
return;
4747

48-
// Check if return type is disposable
4948
if (!DisposableHelper.IsAnyDisposableType(method.ReturnType))
5049
return;
5150

52-
var methodName = method.Name;
51+
// Skip methods that return the disposable interface directly (self-descriptive)
52+
var returnTypeName = method.ReturnType.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat);
53+
if (returnTypeName is "System.IDisposable" or "System.IAsyncDisposable")
54+
return;
5355

54-
// Check if method has clear factory naming
55-
var hasFactoryNaming = methodName.StartsWith("Create") ||
56-
methodName.StartsWith("Build") ||
57-
methodName.StartsWith("Make") ||
58-
methodName.StartsWith("New") ||
59-
methodName.StartsWith("Open") ||
60-
methodName.StartsWith("Initialize");
56+
// Only flag accessible factory-like methods
57+
if (method.DeclaredAccessibility is not (Accessibility.Public or Accessibility.Protected or Accessibility.Internal or Accessibility.ProtectedOrInternal))
58+
return;
6159

62-
// Check for non-factory naming that might be confusing
63-
var hasConfusingNaming = methodName.StartsWith("Get") ||
64-
methodName.StartsWith("Find") ||
65-
methodName.StartsWith("Retrieve") ||
66-
methodName.StartsWith("Fetch");
60+
var documentation = method.GetDocumentationCommentXml(
61+
preferredCulture: null,
62+
expandIncludes: true,
63+
cancellationToken: context.CancellationToken);
6764

68-
if (hasConfusingNaming && !hasFactoryNaming)
65+
if (string.IsNullOrWhiteSpace(documentation))
6966
{
70-
// Get* methods with disposable returns are confusing
71-
// Did they already exist (Get) or create new (should be Create)?
72-
var diagnostic = Diagnostic.Create(
73-
Rule,
74-
method.Locations.FirstOrDefault(),
75-
methodName);
76-
context.ReportDiagnostic(diagnostic);
67+
Report(context, method);
68+
return;
7769
}
70+
71+
var hasReturnsElement = documentation.IndexOf("<returns", StringComparison.OrdinalIgnoreCase) >= 0;
72+
var mentionsDispose = documentation.IndexOf("dispose", StringComparison.OrdinalIgnoreCase) >= 0;
73+
74+
if (!hasReturnsElement || !mentionsDispose)
75+
{
76+
Report(context, method);
77+
}
78+
}
79+
80+
private static void Report(SymbolAnalysisContext context, IMethodSymbol method)
81+
{
82+
var location = method.Locations.FirstOrDefault();
83+
context.ReportDiagnostic(Diagnostic.Create(Rule, location, method.Name));
7884
}
7985
}

‎src/DisposableAnalyzer/Analyzers/DisposableInConstructorAnalyzer.cs‎

Lines changed: 164 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,12 @@ public class DisposableInConstructorAnalyzer : DiagnosticAnalyzer
1717
{
1818
private static readonly DiagnosticDescriptor Rule = new(
1919
id: DiagnosticIds.DisposableInConstructor,
20-
title: "Exception in constructor with disposable",
21-
messageFormat: "Constructor initializes disposable field '{0}' but doesn't handle exceptions. Resources may leak if constructor fails",
22-
category: "Reliability",
20+
title: "Disposable created in constructor is not disposed",
21+
messageFormat: "Disposable '{0}' created in constructor is not stored or disposed",
22+
category: "Resource Management",
2323
defaultSeverity: DiagnosticSeverity.Warning,
2424
isEnabledByDefault: true,
25-
description: "When initializing disposable fields in constructors, wrap initialization in try-catch to dispose resources if construction fails.");
25+
description: "Disposables created in constructors should either be stored for later disposal or wrapped in a using/try-finally to avoid leaks.");
2626

2727
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
2828

@@ -43,48 +43,182 @@ private void AnalyzeOperationBlockStart(OperationBlockStartAnalysisContext conte
4343
if (method.MethodKind != MethodKind.Constructor)
4444
return;
4545

46-
var disposableFieldAssignments = new List<IFieldSymbol>();
47-
var hasTryCatch = false;
46+
var trackedLocals = new Dictionary<ILocalSymbol, LocalInfo>(SymbolEqualityComparer.Default);
4847

4948
context.RegisterOperationAction(operationContext =>
5049
{
51-
// Track disposable field assignments
52-
if (operationContext.Operation is IAssignmentOperation assignment)
50+
var creation = (IObjectCreationOperation)operationContext.Operation;
51+
52+
if (!DisposableHelper.IsAnyDisposableType(creation.Type))
53+
return;
54+
55+
if (DisposableHelper.IsInUsingStatement(creation.Syntax))
56+
return;
57+
58+
if (IsAssignedToInstanceMember(creation))
59+
return;
60+
61+
if (IsPartOfBaseConstructorInitializer(creation))
62+
return;
63+
64+
var local = GetAssignedLocal(creation);
65+
if (local != null)
5366
{
54-
if (assignment.Target is IFieldReferenceOperation fieldRef)
67+
trackedLocals[local] = new LocalInfo(creation.Syntax.GetLocation());
68+
}
69+
}, OperationKind.ObjectCreation);
70+
71+
// Track explicit disposal calls
72+
context.RegisterOperationAction(operationContext =>
73+
{
74+
var operation = operationContext.Operation;
75+
if (DisposableHelper.IsDisposalCall(operation, out _))
76+
{
77+
var local = GetTargetLocal(operation);
78+
if (local != null && trackedLocals.TryGetValue(local, out var info))
5579
{
56-
if (DisposableHelper.IsAnyDisposableType(fieldRef.Field.Type))
57-
{
58-
// Check if RHS creates a disposable
59-
if (assignment.Value is IObjectCreationOperation)
60-
{
61-
disposableFieldAssignments.Add(fieldRef.Field);
62-
}
63-
}
80+
info.IsDisposed = true;
81+
trackedLocals[local] = info;
6482
}
6583
}
84+
}, OperationKind.Invocation, OperationKind.ConditionalAccess);
6685

67-
// Check for try-catch blocks
68-
if (operationContext.Operation is ITryOperation)
86+
// Track simple escape scenarios (assignment to field/property or return)
87+
context.RegisterOperationAction(operationContext =>
88+
{
89+
switch (operationContext.Operation)
6990
{
70-
hasTryCatch = true;
91+
case IAssignmentOperation assignment:
92+
if (assignment.Value is ILocalReferenceOperation localRef &&
93+
trackedLocals.TryGetValue(localRef.Local, out var info) &&
94+
(assignment.Target is IFieldReferenceOperation or IPropertyReferenceOperation))
95+
{
96+
info.Escapes = true;
97+
trackedLocals[localRef.Local] = info;
98+
}
99+
break;
100+
101+
case IReturnOperation returnOp:
102+
if (returnOp.ReturnedValue is ILocalReferenceOperation returnedLocal &&
103+
trackedLocals.TryGetValue(returnedLocal.Local, out var info2))
104+
{
105+
info2.Escapes = true;
106+
trackedLocals[returnedLocal.Local] = info2;
107+
}
108+
break;
71109
}
72-
}, OperationKind.SimpleAssignment, OperationKind.Try);
110+
}, OperationKind.SimpleAssignment, OperationKind.Return);
73111

74112
context.RegisterOperationBlockEndAction(blockEndContext =>
75113
{
76-
// If we have disposable field assignments and no try-catch, warn
77-
if (disposableFieldAssignments.Count > 0 && !hasTryCatch)
114+
if (trackedLocals.Count == 0)
115+
return;
116+
117+
foreach (var kvp in trackedLocals)
118+
{
119+
var local = kvp.Key;
120+
var info = kvp.Value;
121+
122+
if (info.IsDisposed || info.Escapes)
123+
continue;
124+
125+
var diagnostic = Diagnostic.Create(
126+
Rule,
127+
info.Location,
128+
local.Type.Name);
129+
blockEndContext.ReportDiagnostic(diagnostic);
130+
}
131+
});
132+
}
133+
134+
private static ILocalSymbol? GetAssignedLocal(IObjectCreationOperation creation)
135+
{
136+
var parent = creation.Parent;
137+
while (parent != null)
138+
{
139+
switch (parent)
78140
{
79-
foreach (var field in disposableFieldAssignments)
141+
case IVariableDeclaratorOperation declarator:
142+
return declarator.Symbol as ILocalSymbol;
143+
case IVariableInitializerOperation initializer when initializer.Parent is IVariableDeclaratorOperation declarator:
144+
return declarator.Symbol as ILocalSymbol;
145+
case IAssignmentOperation assignment when assignment.Target is ILocalReferenceOperation localRef:
146+
return localRef.Local;
147+
}
148+
parent = parent.Parent;
149+
}
150+
return null;
151+
}
152+
153+
private static bool IsAssignedToInstanceMember(IObjectCreationOperation creation)
154+
{
155+
var parent = creation.Parent;
156+
while (parent != null)
157+
{
158+
if (parent is IAssignmentOperation assignment)
159+
{
160+
if (assignment.Target is IFieldReferenceOperation fieldRef &&
161+
fieldRef.Instance is IInstanceReferenceOperation)
80162
{
81-
var diagnostic = Diagnostic.Create(
82-
Rule,
83-
method.Locations.FirstOrDefault(),
84-
field.Name);
85-
blockEndContext.ReportDiagnostic(diagnostic);
163+
return true;
164+
}
165+
166+
if (assignment.Target is IPropertyReferenceOperation propertyRef &&
167+
propertyRef.Instance is IInstanceReferenceOperation)
168+
{
169+
return true;
86170
}
87171
}
88-
});
172+
173+
parent = parent.Parent;
174+
}
175+
176+
return false;
177+
}
178+
179+
private static bool IsPartOfBaseConstructorInitializer(IObjectCreationOperation creation)
180+
{
181+
var parent = creation.Parent;
182+
while (parent != null)
183+
{
184+
if (parent is IArgumentOperation argument &&
185+
argument.Parent is IInvocationOperation invocation &&
186+
invocation.TargetMethod.MethodKind == MethodKind.Constructor &&
187+
invocation.Parent is IConstructorBodyOperation)
188+
{
189+
return true;
190+
}
191+
parent = parent.Parent;
192+
}
193+
194+
return false;
195+
}
196+
197+
private static ILocalSymbol? GetTargetLocal(IOperation operation)
198+
{
199+
switch (operation)
200+
{
201+
case IInvocationOperation invocation when invocation.Instance is ILocalReferenceOperation localRef:
202+
return localRef.Local;
203+
case IConditionalAccessOperation conditional when conditional.Operation is ILocalReferenceOperation localRef &&
204+
conditional.WhenNotNull is IInvocationOperation:
205+
return localRef.Local;
206+
}
207+
208+
return null;
209+
}
210+
211+
private struct LocalInfo
212+
{
213+
public LocalInfo(Location location)
214+
{
215+
Location = location;
216+
IsDisposed = false;
217+
Escapes = false;
218+
}
219+
220+
public Location Location { get; }
221+
public bool IsDisposed { get; set; }
222+
public bool Escapes { get; set; }
89223
}
90224
}

0 commit comments

Comments
 (0)