Add lightweight disposable ownership analysis - #288
Open
SergeyTeplyakov wants to merge 3 commits into
Open
Conversation
Integrate master and enable gradual ownership-contract adoption through bounded analysis, external annotations, and source-embedded attributes without an additional runtime dependency. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
SergeyTeplyakov
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A correctness issue in TypeExtensions.DerivesFrom can fail to match constructed generics against generic definitions (e.g., Task<int> vs Task<T>), leading to incorrect ownership/disposal behavior and diagnostics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a first-pass disposable ownership model to ErrorProne.NET, enabling opt-in contracts (source + external XML) plus bounded inference to detect missing disposal/transfer and obvious misuse without requiring a runtime annotations assembly.
Changes:
- Add new ownership diagnostics ERP044 (undischarged owned resources), ERP045 (invalid external XML contracts), ERP046 (borrowed misuse + use-after-dispose/transfer).
- Introduce
ErrorProne.Net.Annotationsas a build-time source generator that embeds internal attribute types into the consuming project namespace. - Add extensive analyzer + generator test coverage and rule documentation for the new diagnostics.
File summaries
| File | Description |
|---|---|
| src/ErrorProne.NET.sln | Adds the new annotations generator project to the solution. |
| src/ErrorProne.NET.CoreAnalyzers/WellKnownTypesProvider.cs | Adds reusable metadata-name constants for core disposable types. |
| src/ErrorProne.NET.CoreAnalyzers/TypeExtensions.cs | Adds type-relationship + disposable detection helpers used by ownership analysis. |
| src/ErrorProne.NET.CoreAnalyzers/SymbolExtensions.cs | Adds symbol helpers used by new analysis code paths. |
| src/ErrorProne.NET.CoreAnalyzers/SymbolAnalysisContextExtensions.cs | Removes legacy file header so the file is a normal source unit. |
| src/ErrorProne.NET.CoreAnalyzers/ExceptionsAnalyzers/SwallowAllExceptionsAnalyzer.cs | Minor whitespace-only change in an existing analyzer. |
| src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/OwnershipInference.cs | Implements bounded ownership inference (fresh returns, acquisition inference, alias tracking hooks). |
| src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/OwnershipContracts.cs | Implements resolution/validation for source + external .ownership.xml contracts and emits ERP045 diagnostics. |
| src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzer.cs | Adds the new ERP044/045/046 analyzer integrating contracts + inference. |
| src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposeAnalysisHelper.cs | Centralizes disposable-type detection and policy exemptions used by ownership analysis. |
| src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/DisposableAttributes.cs | Defines the analyzer-recognized attribute short names for ownership contracts. |
| src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs | Registers ERP044/045/046 descriptors and adds a new “Reliability” category. |
| src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md | Announces ERP044/045/046 in the analyzer release tracking file. |
| src/ErrorProne.NET.CoreAnalyzers.Tests/ErrorProne.NET.CoreAnalyzers.Tests.csproj | References the new annotations generator project for tests. |
| src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/OwnershipContractsTests.cs | Adds targeted tests for contract resolution, precedence, and external XML validation. |
| src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.ReviewRegressions.cs | Adds regression coverage for known tricky ownership/disposal patterns. |
| src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.MoveSemantics.cs | Adds tests around transfer/move semantics and known exemptions. |
| src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.Inference.cs | Adds tests for bounded inference rules and ownership-oblivious behavior. |
| src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.FreshReturns.cs | Adds tests for “direct fresh return” ownership inference and its limits. |
| src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.cs | Adds baseline tests validating ERP044 behavior on common patterns. |
| src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisposeBeforeLoosingScopeAnalyzerTests.Borrowing.cs | Adds tests ensuring borrowed contracts are enforced (ERP046) without suppressing acquired obligations (ERP044). |
| src/ErrorProne.NET.CoreAnalyzers.Tests/DisposableAnalyzers/DisoseTaskAnalyzerTests.cs | Adds tests for Task-in-using diagnostics in the presence of Task-returning-disposable patterns. |
| src/ErrorProne.NET.CoreAnalyzers.Tests/Annotations/AnnotationsGeneratorTests.cs | Adds tests for the annotations generator behavior and metadata survivability across assemblies/reference assemblies. |
| src/ErrorProne.NET.CoreAnalyzers.CodeFixes/ErrorProne.NET.CoreAnalyzers.CodeFixes.csproj | Updates package release notes to mention the new ownership rules and annotation support. |
| src/ErrorProne.NET.Annotations/README.md | Documents how to consume the generator and how internal annotations work across assembly boundaries. |
| src/ErrorProne.NET.Annotations/ErrorProne.NET.Annotations.csproj | Defines the build-only generator package project and packing configuration. |
| src/ErrorProne.NET.Annotations/buildTransitive/ErrorProne.Net.Annotations.props | Ensures required MSBuild properties are compiler-visible to the generator. |
| src/ErrorProne.NET.Annotations/AnnotationsGenerator.cs | Implements the incremental generator producing internal attribute types and diagnostics EPANN001-003. |
| src/ErrorProne.NET.Annotations/AnalyzerReleases.Unshipped.md | Tracks unshipped generator diagnostics EPANN001-003. |
| ReadMe.md | Adds end-user documentation for disposable ownership rules and the annotations package. |
| docs/Rules/ERP046.md | Documents ERP046 behavior and limits (borrowed misuse + use-after-dispose/transfer). |
| docs/Rules/ERP045.md | Documents the external .ownership.xml schema, validation, and how contracts apply. |
| docs/Rules/ERP044.md | Documents ERP044, ownership-oblivious defaults, contracts, and bounded inference model. |
Review details
- Files reviewed: 33/33 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+249
to
+259
| while (symbol != null) | ||
| { | ||
| if (SymbolEqualityComparer.Default.Equals(symbol, candidateBaseType)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| symbol = symbol.BaseType; | ||
| } | ||
|
|
||
| return false; |
Comment on lines
+10
to
+11
| [TestFixture] | ||
| public partial class DisoseTaskAnalyzerTests |
Comment on lines
+1
to
+21
| namespace ErrorProne.NET.DisposableAnalyzers; | ||
|
|
||
| public static class DisposableAttributes | ||
| { | ||
| public const string DoNotDisposeAttribute = "DoNotDisposeAttribute"; | ||
|
|
||
| // 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 |
Comment on lines
+20
to
+22
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| public sealed class DisposeBeforeLoosingScopeAnalyzer : DiagnosticAnalyzerBase | ||
| { |
|
|
||
| StatementSyntax syntax = catchBlock.Block; | ||
| var controlFlow = context.SemanticModel.AnalyzeControlFlow(syntax); | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enable gradual disposable-ownership adoption without an extra runtime annotation assembly or ownership assumptions for unannotated API results.
RootNamespace, with an optionalErrorProneAnnotationsNamespaceoverride. Public API contracts remain visible through compiled and reference-assembly metadata; no annotation runtime DLL is required.master, retaining its project layout and existing task-in-using analyzer. The separate EventSource ERP043 work in Add ERP043 for empty EventSource messages #335 is not included.Intentional v1 limits
Unannotated API results are ownership-oblivious by default. This is best-effort analysis, not a full path/exception-safety proof or a Rust borrow checker. Type-wide ownership annotations and general escape diagnostics remain deferred. Existing Task, StringReader, and MemoryStream disposal exemptions are retained.
See ERP044, ERP045, ERP046, and the annotations package guide.
Review and validation
No NuGet publication, reviewers, or auto-merge are requested.