Support unsafe evolution in ComInterface, ComClass, JSImport, JSExport and VtableIndexStub generators - #131701
Support unsafe evolution in ComInterface, ComClass, JSImport, JSExport and VtableIndexStub generators#131701EgorBo wants to merge 2 commits into
Conversation
Every generated body relied on an 'unsafe' modifier on the generated type for its unsafe context. Under the updated memory safety rules a type modifier opens no context for the members inside it, so those bodies stop compiling as soon as they dereference a pointer, take an address, or call a caller-unsafe API. Give each body a context of its own instead. A shadowing member that forwards to a caller-unsafe base member additionally has to declare itself unsafe and take a block body, since its implementation is unsafe and an expression body has nowhere to put the context. The type-level modifier stays. Under the legacy rules it is what makes a pointer legal to name in a stub that copies its modifiers from a user declaration carrying 'unsafe' on the type rather than on the member, and the stub cannot take the modifier itself without failing to implement the member it implements. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/interop-contrib |
There was a problem hiding this comment.
Pull request overview
This PR updates several interop source generators so their emitted code compiles when the C# “updated memory safety rules” (unsafe evolution / unsafe-v2) are enabled, by ensuring generated member bodies establish their own unsafe { } context instead of relying on an unsafe modifier on the containing type. It also adds/adjusts unit tests to compile generator output with the feature flag enabled and assert no compilation errors.
Changes:
- Add a shared
SyntaxExtensions.WrapInUnsafeBlockhelper and use it in multiple generators to wrap generated bodies inunsafe { }. - Update COM and JS interop generator output to open explicit unsafe contexts for pointer/address-taking/caller-unsafe operations (including shadowing member forwarders).
- Add new unit tests (COM + JS) that compile generated output with
updated-memory-safety-rulesenabled and assert the compilation is clean.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/UnsafeCodeGeneration.cs | New tests compiling ComInterface/ComClass/VtableIndexStub generator output under updated rules. |
| src/libraries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/ComInterfaceGeneratorOutputShape.cs | Narrow accessor-attribute assertions to the intended generated type to avoid unrelated accessors. |
| src/libraries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/ComInterfaceGenerator.Unit.Tests.csproj | Link in shared MemorySafetyRules feature-flag constant for updated-rules opt-in. |
| src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/SyntaxExtensions.cs | Add WrapInUnsafeBlock helper for consistent unsafe-block wrapping. |
| src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs | Switch to WrapInUnsafeBlock for generated stub bodies. |
| src/libraries/System.Runtime.InteropServices/gen/DownlevelLibraryImportGenerator/DownlevelLibraryImportGenerator.cs | Same WrapInUnsafeBlock adoption for downlevel generator. |
| src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/VtableIndexStubGenerator.cs | Wrap vtable population body in an explicit unsafe { } block (string-emitted code). |
| src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/VirtualMethodPointerStubGenerator.cs | Wrap generated managed/unmanaged stub bodies and accessor bodies in unsafe { }; mark ABI stubs unsafe. |
| src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/ComInterfaceGenerator.cs | Add explicit unsafe contexts for generated static ctor and pointer-returning accessor; adjust shadow emission to host unsafe contexts. |
| src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/ComClassGenerator.cs | Wrap GetComInterfaceEntries body in an explicit unsafe { } block (string-emitted code). |
| src/libraries/System.Runtime.InteropServices.JavaScript/tests/JSImportGenerator.UnitTest/UnsafeCodeGeneration.cs | New test compiling JSImport/JSExport generator output under updated rules and asserting no errors. |
| src/libraries/System.Runtime.InteropServices.JavaScript/tests/JSImportGenerator.UnitTest/JSImportGenerator.Unit.Tests.csproj | Include new JS unsafe-evolution compilation test. |
| src/libraries/System.Runtime.InteropServices.JavaScript/tests/JSImportGenerator.UnitTest/Compiles.cs | Update expected generated output to match new unsafe-block wrapping and initializer class modifier change. |
| src/libraries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSImportGenerator.cs | Wrap generated JSImport method bodies in unsafe { } using the shared helper. |
| src/libraries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSExportGenerator.cs | Wrap wrapper body in unsafe { }; remove unnecessary unsafe modifier from generated initializer class. |
The generated types carried an 'unsafe' modifier unconditionally. Under the updated memory safety rules that modifier has no effect and warns (CS9377), which is not suppressed in generated files and is on at the SDK's default warning level, so it would reach every user as a warning from code they cannot edit. Emit it only when the updated rules are off, and where the generator controls the members put the modifier on them instead, which is meaningful under both sets of rules. The one type that still needs it under the legacy rules is InterfaceImplementation, whose stubs explicitly implement user interface members and so cannot carry a modifier of their own. The flag travels on EnvironmentFlags, which already reaches the generators. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/ComClassInfo.cs:73
ComClassInfo.Equalsnow includesUseUpdatedMemorySafetyRules, butGetHashCode()does not. While this doesn't break correctness, it increases hash collisions and is inconsistent with the equality definition (and can reduce incremental generator caching effectiveness). IncludeUseUpdatedMemorySafetyRulesin the hash computation.
public bool Equals(ComClassInfo? other)
{
return other is not null
&& ClassName == other.ClassName
&& ContainingSyntaxContext.Equals(other.ContainingSyntaxContext)
&& UseUpdatedMemorySafetyRules == other.UseUpdatedMemorySafetyRules
&& ImplementedInterfacesNames.SequenceEqual(other.ImplementedInterfacesNames);
}
public override bool Equals(object obj)
{
return Equals(obj as ComClassInfo);
}
public override int GetHashCode()
{
return HashCode.Combine(ClassName, ContainingSyntaxContext, ImplementedInterfacesNames);
}
Tracking issue: #131451 (section 6, "Source generator output")
This PR makes the remaining interop source generators emit code that compiles under the new unsafe-v2 rules (unsafe evolution), following #131245 which did the same for
LibraryImportGenerator. Nothing changes under unsafe-v1.[GeneratedComInterface],[GeneratedComClass],[JSImport]and[JSExport]are public API shipping since .NET 7/8, so this is real user impact: today their generated output does not compile once the switch is flipped.Background
Every generated body relied on an
unsafemodifier stamped onto the generated type for its unsafe context. Under the updated rules a type modifier opens no context for the members inside it, so those bodies stop compiling as soon as they dereference a pointer, take an address, or call a caller-unsafe API.The fix is to give each body a context of its own:
The type-level modifier also has to go, but only under the updated rules: there it has no effect and warns with
CS9377, which is not suppressed in generated files and is on at the SDK's default warning level. Left in, it would reach every user as a warning from code they cannot edit, and break any build usingTreatWarningsAsErrors. Under the legacy rules it is load-bearing, so it is emitted conditionally.Changes in this PR
[codegen]
ComInterfaceGenerator: the static constructor, the managed-to-unmanaged stubs, the property/indexer accessor stubs and theABI_*stubs all wrap their bodies.InterfaceInformation.ManagedVirtualMethodTablebecomes a block-bodied getter so itsUnsafe.AsPointercall has somewhere to put the context.ABI_*also gains anunsafemodifier, matching how Support unsafe evolution in LibraryImportGenerator #131245 keeps the inner__PInvokecaller-unsafe.[codegen]
ComInterfaceGeneratorshadowing members: a shadow that forwards to a caller-unsafe base member is now emittedunsafewith a block body, instead of as a plain expression-bodied forwarder. Without this the derived interface hitsCS9366(the implementation isunsafebecause it copies the user's modifier, while the shadow it implements is not) andCS9362(the forwarding call itself). An expression body cannot host the context, becauseunsafe(...)is not one of the forms a statement may consist of.[codegen]
ComClassGenerator:GetComInterfaceEntrieswraps its body; it readsdetails.ManagedVirtualMethodTable, avoid**property.[codegen]
JSImportGenerator/JSExportGenerator: bodies wrapped, and the generated__GeneratedInitializerclass loses itsunsafemodifier, since none of its members names a pointer type.[codegen]
VtableIndexStubGenerator:PopulateUnmanagedVirtualMethodTablewraps its body.[cleanup]
SyntaxExtensions.WrapInUnsafeBlockreplaces theBlock(UnsafeStatement(body))pattern, which this PR would otherwise have spelled out in five more places. The two pre-existing occurrences inLibraryImportGeneratorandDownlevelLibraryImportGeneratorare moved over to it as well.[tests]
UnsafeCodeGeneration.csin both generator test projects runs each generator withupdated-memory-safety-rulesenabled and asserts the output has no errors. The JS one also asserts noCS9377, which is otherwise suppressed in// <auto-generated/>files.Why the type-level
unsafemodifier is conditional rather than goneUnder the legacy rules, naming a pointer type in a member signature requires an enclosing unsafe context, and a generated stub copies its modifiers from the user's declaration. A user is free to write
in which case the generated stub has no modifier of its own and needs the type to supply the context. Removing the type-level modifier unconditionally produces
CS0214on exactly this shape.It cannot be fixed by marking the generated stub
unsafeinstead: the stub explicitly implements the user's interface member, that member is not caller-unsafe under the updated rules, and an unsafe member cannot implement a safe one (CS9366).So the modifier is emitted only when the updated rules are off. Everywhere the generator controls the members itself - the vtable struct's fields,
ManagedVirtualMethodTable,GetComInterfaceEntries,ABI_*- the modifier moved onto the member, which is meaningful under both sets of rules and needs no condition. That leavesInterfaceImplementationand the user's own containing types as the only conditional cases.The flag travels on
EnvironmentFlags, which already reaches every generator, sourced fromParseOptionsProvider.A note on testing this
The generator test harness compiles with
LanguageVersion.Preview, and preview already relaxes the pointer-declaration rule even without the feature flag. A generator change that breaks unsafe-v1 therefore still passes the entire existing suite.CS9377has the same problem for a different reason: it sits above the test framework's default warning level, so an assertion on it silently never fires. The new tests raise the warning level to make it observable, which is whatILLink.RoslynAnalyzer.Testsalready does for the same diagnostic.Both gaps were hit while writing this PR. On top of the in-harness tests, the generated output was verified out-of-band by compiling it in all three configurations that matter:
CS9377preview+ feature)LangVersion=latestLangVersion=13.0Known limitations / follow ups
[GeneratedComInterface]methods should require an explicitsafe/unsafecontract the way[LibraryImport]methods now do (SYSLIB1064). That is an API decision rather than a codegen one.ComClassGeneratorandComInterfaceGeneratorwrite their output as strings rather than syntax nodes, so the added nesting is hand-indented.Note
This description and the changes in this PR were generated with GitHub Copilot.