Add lightweight disposable ownership analysis - #288
SergeyTeplyakov wants to merge 5 commits into
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>
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.
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>
There was a problem hiding this comment.
🟡 Changes recommended
Four unresolved correctness issues, including one critical flaw, can cause missed or incorrect ownership diagnostics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/OwnershipInference.cs:232
InferAcquisitiondoes not redirect a partial method definition to itsPartialImplementationPart(unlikeInferFreshReturn). Calls bind to the defining symbol, soGetSourceOperations(parameter.ContainingSymbol, ...)sees the declaration without the consuming body and reports ERP044 even when the partial implementation disposes or transfers the parameter. Normalize the parameter to the implementation parameter before source traversal.
- Files reviewed: 34/34 changed files
- Comments generated: 3
- Review effort level: Balanced
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>
Summary
Add lightweight, attribute-driven disposable ownership with gradual adoption and no extra runtime annotation DLL.
MustUseResult, its compatibility name, and both ConfigureAwait policies. Namespace selection and existing-definition reuse support legacy applications; public API contracts survive compiled and reference-assembly metadata.Enabled, low-noise v1 policy
Unknown argument handoffs and callback captures make cleanup uncertain, so ERP044 stays quiet without pretending that ownership transferred. Ordinary receiver calls such as
resource.Use()do not hide an obligation.Task.FromResult,ValueTask.FromResult, and result-takingValueTask<T>constructors transport resource identity through supported aliases, awaits, configured awaits, returns and storage. Discarding or disposing a task wrapper is not cleanup of its carried resource. Explicit contracts take precedence over heuristics.Acquiring
Task<T>/ValueTask<T>inputs acquire the eventual result. Wrapped arguments honor explicit and inferred consumers, including constructor/indexer/operator boundaries; borrowed results cannot bypass the contract by being wrapped. The callee must consume the awaited result or transfer responsibility, not merely dispose its wrapper.This is not a Rust borrow checker, exhaustive escape analysis, or path/exception/lifecycle safety proof. Unknown boundaries can hide real leaks. Type-wide ownership annotations, independent nested-function analysis, and dedicated unknown-escape diagnostics are deferred. Ordinary unannotated Task, StringReader and MemoryStream disposal exemptions remain; explicit acquiring contracts establish their own obligations.
Latest review corrections
IAsyncDisposableimplementation or framework configured-disposal wrapper, including inherited/overridden implementations. Same-name lookalikes do not silence ERP044 or invent ERP046.objector unconstrainedT; casts and direct type-pattern bindings preserve the relevant ownership/borrowing identity.System.Attribute. Referenced non-attributes no longer suppress generation; unshadowable local collisions report EPANN004 rather than emitting duplicate declarations.Review and evidence
See ERP044, ERP045, ERP046, the annotations guide, and the adoption workflow.
This updates the existing PR without merging it. No public NuGet release, reviewer assignment, or auto-merge is requested.