Skip to content

Add lightweight disposable ownership analysis - #288

Open
SergeyTeplyakov wants to merge 5 commits into
masterfrom
dev/Disposable
Open

SergeyTeplyakov wants to merge 5 commits into
masterfrom
dev/Disposable

Conversation

@SergeyTeplyakov

@SergeyTeplyakov SergeyTeplyakov commented Dec 19, 2024

Copy link
Copy Markdown
Owner

Summary

Add lightweight, attribute-driven disposable ownership with gradual adoption and no extra runtime annotation DLL.

  • ERP044 reports abandoned modeled cleanup obligations. It remains enabled by default.
  • ERP045 reports invalid external ownership XML; ERP046 reports obvious borrowed-resource disposal/transfer and use after a definite move or cleanup.
  • ErrorProne.Net.Annotations embeds nine internal annotations, including 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.
  • Source attributes and external contracts describe owned/borrowed results and acquiring/borrowing inputs independently. Bounded inference covers direct fresh returns, source consumption, aliases, owning storage, output/return transfers, and synchronous/asynchronous cleanup.
  • Add four reusable adoption skills: inventory disposables, review contracts, run an isolated pilot with deliberate diagnostic controls, and coordinate evidence-based adoption.

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-taking ValueTask<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

  • Match asynchronous cleanup to the actual IAsyncDisposable implementation or framework configured-disposal wrapper, including inherited/overridden implementations. Same-name lookalikes do not silence ERP044 or invent ERP046.
  • Enforce explicit acquiring parameters even when declared as object or unconstrained T; casts and direct type-pattern bindings preserve the relevant ownership/borrowing identity.
  • Reuse canonical generator candidates only when they derive from System.Attribute. Referenced non-attributes no longer suppress generation; unshadowable local collisions report EPANN004 rather than emitting duplicate declarations.

Review and evidence

  • The affected ownership cohort passes 449 cases; the generator cohort passes 38 cases (487 total).
  • Regression controls reproduced the latest analyzer comments before correction and cover valid cleanup, lookalikes, erased-type source/XML contracts, and generator collision behavior.
  • Local immutable preview packages support opt-in consuming-project experiments. Annotation-driven intentional omissions produce compiler failures, while corrected cleanup passes; these compile controls are not runtime leak measurements.
  • Restore used cached dependencies while NuGet.org was unavailable; Microsoft.CodeAnalysis.Analyzers 4.14.0 resolved for the declared 4.13.0 dependency. Dependency manifests were not changed.

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.

SergeyTeplyakov and others added 3 commits September 13, 2023 14:20
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 SergeyTeplyakov changed the title [WIP] Dev/disposable Add lightweight disposable ownership analysis Sep 6, 2026
@SergeyTeplyakov
SergeyTeplyakov requested a lite review from Copilot September 6, 2026 18:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.Annotations as 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 thread src/ErrorProne.NET.CoreAnalyzers/TypeExtensions.cs
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>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

  • InferAcquisition does not redirect a partial method definition to its PartialImplementationPart (unlike InferFreshReturn). Calls bind to the defining symbol, so GetSourceOperations(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

Comment thread src/ErrorProne.NET.CoreAnalyzers/DisposableAnalyzers/OwnershipInference.cs Outdated
Comment thread src/ErrorProne.NET.Annotations/AnnotationsGenerator.cs
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants