[src] Transient strings must always be disposed.#24894
Conversation
Fix a couple of places where transient strings weren't disposed. And to make sure this doesn't happen again, add a new Roslyn analyzer (RBI0042) that reports an error when transient strings are declared as local variables without the 'using' keyword. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes missing disposal of transient interop string wrappers in the runtime, and adds a new Roslyn analyzer (RBI0042) in rgen to prevent future occurrences by enforcing using on local declarations of transient disposable types.
Changes:
- Fix undisposed
TransientString/TransientCFStringlocals by converting them tousing vardeclarations. - Add analyzer
TransientDisposableAnalyzerthat reports RBI0042 for transient locals missingusing. - Add tests + diagnostic resources/release notes for RBI0042.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/TransientDisposableAnalyzerTests.cs | Adds analyzer test coverage for RBI0042 scenarios. |
| src/rgen/Microsoft.Macios.Bindings.Analyzer/TransientDisposableAnalyzer.cs | Introduces the new RBI0042 analyzer enforcing using for transient locals. |
| src/rgen/Microsoft.Macios.Bindings.Analyzer/RgenDiagnostics.cs | Registers the RBI0042 DiagnosticDescriptor. |
| src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.resx | Adds localized strings for RBI0042 title/message/description. |
| src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.Designer.cs | Updates generated resource accessors for RBI0042. |
| src/rgen/Microsoft.Macios.Bindings.Analyzer/AnalyzerReleases.Unshipped.md | Documents the newly added analyzer rule RBI0042. |
| src/ObjCRuntime/Protocol.cs | Disposes TransientString via using var before calling objc_getProtocol. |
| src/CoreGraphics/CGFont.cs | Disposes TransientCFString via using var before calling CGFontCreateWithFontName. |
Files not reviewed (1)
- src/rgen/Microsoft.Macios.Bindings.Analyzer/Resources.Designer.cs: Language not supported
You can also share your feedback on Copilot code review. Take the survey.
| <value>Transient types (TransientString, TransientCFString, TransientCFObject) allocate native memory and must be disposed. Use 'using var' to ensure proper cleanup.</value> | ||
| </data> | ||
| <data name="RBI0042MessageFormat" xml:space="preserve"> | ||
| <value>Variable '{0}' of type '{1}' must be declared with 'using var' to ensure proper disposal of native resources</value> |
| static readonly ImmutableHashSet<string> transientTypeNames = ImmutableHashSet.Create ( | ||
| "TransientString", | ||
| "TransientCFString", | ||
| "TransientCFObject" | ||
| ); | ||
|
|
||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create (RBI0042); | ||
|
|
||
| public override void Initialize (AnalysisContext context) | ||
| { | ||
| context.ConfigureGeneratedCodeAnalysis (GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); | ||
| context.EnableConcurrentExecution (); | ||
| context.RegisterSyntaxNodeAction (AnalyzeNode, SyntaxKind.LocalDeclarationStatement); | ||
| } | ||
|
|
||
| void AnalyzeNode (SyntaxNodeAnalysisContext context) | ||
| { | ||
| if (context.Node is not LocalDeclarationStatementSyntax localDeclaration) | ||
| return; | ||
|
|
||
| // If the declaration already has 'using', it's fine. | ||
| if (localDeclaration.UsingKeyword != default) | ||
| return; | ||
|
|
||
| foreach (var variable in localDeclaration.Declaration.Variables) { | ||
| var symbol = context.SemanticModel.GetDeclaredSymbol (variable); | ||
| if (symbol is not ILocalSymbol localSymbol) | ||
| continue; | ||
|
|
||
| var typeName = localSymbol.Type.Name; | ||
| if (!transientTypeNames.Contains (typeName)) | ||
| continue; | ||
|
|
||
| var diagnostic = Diagnostic.Create (RBI0042, variable.GetLocation (), variable.Identifier.Text, typeName); | ||
| context.ReportDiagnostic (diagnostic); |
| // TransientString without using | ||
| yield return [ | ||
| """ | ||
| using System; | ||
|
|
||
| struct TransientString : IDisposable { | ||
| public void Dispose () { } | ||
| } | ||
|
|
||
| class Test | ||
| { | ||
| void Method () | ||
| { | ||
| var s = new TransientString (); | ||
| } | ||
| } | ||
| """]; | ||
|
|
||
| // TransientCFString without using | ||
| yield return [ | ||
| """ | ||
| using System; | ||
|
|
||
| ref struct TransientCFString { | ||
| public void Dispose () { } | ||
| } | ||
|
|
||
| class Test | ||
| { | ||
| void Method () | ||
| { | ||
| TransientCFString s = new TransientCFString (); | ||
| } | ||
| } | ||
| """]; | ||
|
|
||
| // TransientCFObject without using | ||
| yield return [ | ||
| """ | ||
| using System; | ||
|
|
||
| ref struct TransientCFObject { | ||
| public void Dispose () { } | ||
| } | ||
|
|
||
| class Test | ||
| { | ||
| void Method () | ||
| { | ||
| var obj = new TransientCFObject (); | ||
| } | ||
| } | ||
| """]; |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Fix diagnostic text to say 'using' keyword instead of 'using var', since using declarations and using statements are both acceptable. - Match transient types by fully-qualified name (ObjCRuntime.TransientString etc.) instead of simple name to avoid false positives on user-defined types with the same name. - Move test type definitions into ObjCRuntime namespace to validate fully-qualified matching. - Add negative test for user-defined TransientString outside ObjCRuntime namespace to ensure no false positives. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ [CI Build #8fae065] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #8fae065] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [CI Build #8fae065] Build passed (Build macOS tests) ✅Pipeline on Agent |
🚀 [CI Build #8fae065] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 156 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
Fix a couple of places where transient strings weren't disposed.
And to make sure this doesn't happen again, add a new Roslyn analyzer (RBI0042) that
reports an error when transient strings are declared as local variables without the
'using' keyword.