[bgen] Remove support for ZeroCopyStrings#25515
Conversation
The zero-copy string marshaling feature was never fully implemented (it always emitted a warning and disabled itself). Remove the dead code paths and simplify the string marshaling logic. - Remove ZeroCopyStrings field and type_wants_zero_copy from Generator - Remove ZeroCopyStringMarshal from MarshalInfo - Remove CollectFastStringMarshalParameters and the fixed char* block - Simplify GenerateMarshalString/GenerateDisposeString (always copies) - Keep ZeroCopyStringsAttribute and DisableZeroCopyAttribute as no-op stubs for source compatibility - Update --use-zero-copy to emit a warning instead of setting a flag - Update documentation to mark both attributes as obsolete Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…utes - Add [Obsolete] and #if !XAMCORE_5_0 to DisableZeroCopyAttribute and ZeroCopyStringsAttribute so they're removed in the next breaking change - Guard --use-zero-copy option with #if !XAMCORE_5_0 - Mark ObjCBindings.Property.DisableZeroCopy enum value as [Obsolete] - Remove [DisableZeroCopy] usages from binding sources (glkit, corebluetooth) since the attribute is now a no-op Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Add #pragma warning disable CS0618 to suppress obsolete warnings in rgen source and test files that still reference the now-obsolete ObjCBindings.Property.DisableZeroCopy enum value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
The cecil ObsoleteTest requires newly obsoleted APIs to also have [EditorBrowsable(EditorBrowsableState.Never)] to hide them from IntelliSense. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
There was a problem hiding this comment.
Pull request overview
This PR removes the unused/defunct “zero-copy” string marshaling feature from bgen (the binding generator), while keeping ZeroCopyStringsAttribute / DisableZeroCopyAttribute as obsolete no-op stubs for source compatibility and updating related warnings/docs.
Changes:
- Remove all zero-copy string marshaling state and code paths from
bgen(including CLI/config plumbing). - Mark
DisableZeroCopyflags/attributes as obsolete/no-op and adjust rgen/analyzer/tests to tolerate the obsolete symbol. - Update BI1027 text and binding documentation to reflect that
--use-zero-copyis ignored.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/PropertyOrFieldValidatorTests.cs | Disable obsolete-warning noise in tests referencing DisableZeroCopy. |
| tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/FieldValidatorTests.cs | Disable obsolete-warning noise in tests referencing DisableZeroCopy. |
| tests/rgen/Microsoft.Macios.Bindings.Analyzer.Tests/Validators/ArrayValidatorTests.cs | Disable obsolete-warning noise in tests referencing DisableZeroCopy. |
| src/rgen/Microsoft.Macios.Transformer/AttributesNames.cs | Update attribute documentation to state DisableZeroCopy is obsolete/no-op. |
| src/rgen/Microsoft.Macios.Generator/DataModel/Property.Generator.cs | Keep DisableZeroCopy flag readable while suppressing CS0618. |
| src/rgen/Microsoft.Macios.Bindings.Analyzer/Validators/FieldValidator.cs | Suppress CS0618 where the obsolete flag is enumerated as “ignored”. |
| src/Resources.resx | Update BI1027 warning text for --use-zero-copy. |
| src/Resources.Designer.cs | Regenerate BI1027 designer comment to match updated resource. |
| src/ObjCBindings/ExportTag.cs | Mark ObjCBindings.Property.DisableZeroCopy as obsolete/no-op and hide from IntelliSense. |
| src/glkit.cs | Remove [DisableZeroCopy] usage from bindings. |
| src/corebluetooth.cs | Remove [DisableZeroCopy] usage from bindings. |
| src/bgen/Models/MarshalInfo.cs | Remove ZeroCopyStringMarshal tracking and related attribute/flag checks. |
| src/bgen/Models/BindingTouchConfig.cs | Remove UseZeroCopy configuration knob. |
| src/bgen/Generator.cs | Remove zero-copy marshaling generation and simplify to always use the copy path. |
| src/bgen/BindingTouch.cs | Keep parsing --use-zero-copy (when enabled) but only emit BI1027 and ignore it. |
| src/bgen/Attributes.cs | Keep DisableZeroCopyAttribute / ZeroCopyStringsAttribute as obsolete stubs (conditional). |
| docs/website/generator-errors.md | Update BI1027 documentation text (currently slightly inconsistent with resources). |
| docs/website/binding_types_reference_guide.md | Mark the two attributes as obsolete/no-op in the binding docs. |
Files not reviewed (1)
- src/Resources.Designer.cs: Language not supported
| ### <a name='BI1026'/>BI1026: `*`: Enums attributed with [\*] must have an underlying type of `long` or `ulong` | ||
|
|
||
| ### <a name='BI1027'/>BI1027: Support for ZeroCopy strings is not implemented. Strings will be marshalled as NSStrings. | ||
| ### <a name='BI1027'/>BI1027: Support for ZeroCopy strings is not implemented. The --use-zero-copy option is not supported and will be ignored. |
| // @probe_null: determines whether null is allowed, and | ||
| // whether we need to generate code to handle this | ||
| // | ||
| // @must_copy: determines whether to create a new NSString, necessary | ||
| // for NSString properties that are flagged with "retain" instead of "copy" | ||
| // | ||
| // @prefix: prefix to prepend on each line | ||
| // | ||
| // @property: the name of the property | ||
| // | ||
| public string GenerateMarshalString (bool probe_null, bool must_copy) | ||
| { | ||
| if (must_copy) { | ||
| return "var ns{0} = CFString.CreateNative ({1});\n"; | ||
| } | ||
| return | ||
| "ObjCRuntime.NSStringStruct _s{0}; Console.WriteLine (\"" + CurrentMethod + ": Marshalling: {{1}}\", {1}); \n" + | ||
| "_s{0}.ClassPtr = ObjCRuntime.NSStringStruct.ReferencePtr;\n" + | ||
| "_s{0}.Flags = 0x010007d1; // RefCount=1, Unicode, InlineContents = 0, DontFreeContents\n" + | ||
| "_s{0}.UnicodePtr = _p{0};\n" + | ||
| "_s{0}.Length = " + (probe_null ? "{1} is null ? 0 : {1}.Length;" : "{1}.Length;\n"); | ||
| } | ||
|
|
||
| public string GenerateDisposeString (bool probe_null, bool must_copy) | ||
| public string GenerateMarshalString (bool probe_null) | ||
| { | ||
| if (must_copy) { | ||
| return "CFString.ReleaseNative (ns{0});\n"; | ||
| } else | ||
| return "if (_s{0}.Flags != 0x010007d1) throw new Exception (\"String was retained, not copied\");"; | ||
| return "var ns{0} = CFString.CreateNative ({1});\n"; |
✅ [PR Build #c15dad3] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #c15dad3] Build passed (Build packages) ✅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 |
✅ [PR Build #c15dad3] Build passed (Build macOS tests) ✅Pipeline on Agent |
🔥 [CI Build #c15dad3] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 22 tests failed, 161 tests passed. Failures❌ fsharp tests2 tests failed, 2 tests passed.Failed tests
Html Report (VSDrops) Download ❌ interdependent-binding-projects tests2 tests failed, 2 tests passed.Failed tests
Html Report (VSDrops) Download ❌ introspection tests4 tests failed, 2 tests passed.Failed tests
Html Report (VSDrops) Download ❌ linker tests (tvOS)11 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ windows tests1 tests failed, 2 tests passed.Failed tests
Html Report (VSDrops) Download ❌ xcframework tests2 tests failed, 2 tests passed.Failed tests
Html Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. ( macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
Remove the dead zero-copy string marshaling code from the binding generator.
This feature was never fully working, because it never really worked well (it ran into a number of bugs in other places, causing crashes because APIs would retain or copy NSStrings when they shouldn't and this optimization would run head-first into those).
Note that this option never did anything in .NET, it was always forcefully disabled if someone tried to enable it.
Changes
ZeroCopyStringsfield,type_wants_zero_copy,ZeroCopyStringMarshal,CollectFastStringMarshalParameters, and all zero-copy code paths from the generatorGenerateMarshalString/GenerateDisposeString(always use the copy path)ZeroCopyStringsAttributeandDisableZeroCopyAttributeas no-op stubs wrapped in#if !XAMCORE_5_0with[Obsolete]for source compatibility--use-zero-copyCLI option with#if !XAMCORE_5_0(emits warning BI1027)ObjCBindings.Property.DisableZeroCopyenum value as[Obsolete][DisableZeroCopy]usages from binding sources (glkit, corebluetooth)🤖 Pull request created by Copilot