Support unsafe evolution in LibraryImportGenerator - #131245
Conversation
|
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. |
There was a problem hiding this comment.
Pull request overview
Updates LibraryImportGenerator/Analyzers to participate in the “updated memory safety rules” evolution by (1) requiring [LibraryImport] methods to opt in with an explicit safe/unsafe modifier when the feature is enabled and (2) mirroring the selected modifier onto generated extern signatures (including the wrapper-path inner local DllImport).
Changes:
- Add a new diagnostic (SYSLIB1050) to fail
[LibraryImport]declarations missing an explicitsafe/unsafemodifier when updated memory-safety rules are enabled. - Flow the user’s safety modifier through to generated wrapper local
externsignatures (LibraryImportGenerator + Downlevel variant). - Add shared helpers + update unit tests and localized resource strings.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/UnsafeCodeGeneration.cs | Expands shape tests to cover safe/unsafe mirroring and missing-modifier diagnostics under the feature flag. |
| src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.csproj | Links in the new shared extensions helper. |
| src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs | Passes the extracted safety modifier into wrapper-path inner DllImport local function generation. |
| src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/GeneratorDiagnostics.cs | Adds the new diagnostic descriptor for “missing safety modifier”. |
| src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/LibraryImportDiagnosticsAnalyzer.cs | Includes + emits the new missing-safety-modifier diagnostic when the feature is enabled. |
| src/libraries/System.Runtime.InteropServices/gen/DownlevelLibraryImportGenerator/GeneratorDiagnostics.cs | Adds downlevel counterpart diagnostic descriptor. |
| src/libraries/System.Runtime.InteropServices/gen/DownlevelLibraryImportGenerator/DownlevelLibraryImportGenerator.csproj | Links in the new shared extensions helper. |
| src/libraries/System.Runtime.InteropServices/gen/DownlevelLibraryImportGenerator/DownlevelLibraryImportGenerator.cs | Mirrors safety modifier onto wrapper-path inner DllImport local function (downlevel). |
| src/libraries/System.Runtime.InteropServices/gen/DownlevelLibraryImportGenerator/DownlevelLibraryImportDiagnosticsAnalyzer.cs | Includes + emits missing-safety-modifier diagnostic (downlevel). |
| src/libraries/System.Runtime.InteropServices/gen/Common/LibraryImportGeneratorExtensions.cs | Adds shared helpers to detect safety modifiers and the updated-rules feature flag. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/Strings.resx | Adds strings for the new diagnostic message/description. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.de.xlf | Adds localization entries for the new diagnostic. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.es.xlf | Adds localization entries for the new diagnostic. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.fr.xlf | Adds localization entries for the new diagnostic. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.it.xlf | Adds localization entries for the new diagnostic. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.ja.xlf | Adds localization entries for the new diagnostic. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.ko.xlf | Adds localization entries for the new diagnostic. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.pl.xlf | Adds localization entries for the new diagnostic. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.pt-BR.xlf | Adds localization entries for the new diagnostic. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.ru.xlf | Adds localization entries for the new diagnostic. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.tr.xlf | Adds localization entries for the new diagnostic. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.zh-Hans.xlf | Adds localization entries for the new diagnostic. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.zh-Hant.xlf | Adds localization entries for the new diagnostic. |
| src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.cs.xlf | Adds localization entries for the new diagnostic. |
This does not sound right. If I define a PInvoke like: The actual internal PInvoke is going to be |
@jkotas do you mean that in this case the private |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/UnsafeCodeGeneration.cs:149
- Same concern here: including a downlevel=true test case likely routes through
TestTargetFramework.Standard2_0ref packs, which are commonly handled as outer-loop due to restore/network requirements. Consider moving the downlevel coverage to a dedicated[OuterLoop]test and keeping this theory non-downlevel for regular runs.
[Theory]
[InlineData(false)]
[InlineData(true)]
public Task UpdatedMemorySafetyRulesRequireExplicitSafetyModifier(bool downlevel)
|
The outer signature should be safe. The private extern hidden in the generated code should be caller-unsafe. Consider my example - the raw PInvoke that takes ´char*´ is obviously unsafe. |
|
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. |
Under the updated memory safety rules the compiler requires an explicit 'safe' or 'unsafe' modifier on every 'extern' member, but a method with 'LibraryImportAttribute' is only implemented by an 'extern' forwarder when its signature needs no marshalling. Require the modifier for every shape so the contract does not depend on that generator implementation detail, and add the migration analyzer and code fixer that default unannotated methods to 'unsafe'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5
0a48d9a to
8600571
Compare
This PR makes
[LibraryImport]participate in the new unsafe-v2 rules (unsafe evolution), plus tooling to migrate existing code. Nothing changes under unsafe-v1:unsafeon all[LibraryImport]if they had no safety keyword on them (migration).LibraryImportDiagnosticsAnalyzerandDownlevelLibraryImportDiagnosticsAnalyzer) to explicitly requiresafeorunsafeunder the new rules with human-readable message.Background
The compiler requires an explicit
safe/unsafemodifier onexternmembers (CS9389). But a[LibraryImport]method is only implemented by anexternforwarder when its signature needs no marshalling - otherwise the generator emits a managed wrapper around a privateexternlocal function. So today the requirement fires for some P/Invokes and not others, based on marshalling alone. The speclet calls this out (safeon non-externmembers):Changes in this PR
[analyzer]
SYSLIB1064(see 'Diagnostics IDs' below) requires an explicitsafe/unsafemodifier on every method withLibraryImportAttributewhen the updated rules are enabled - for every shape, so adding astringparameter never silently changes a P/Invoke's safety obligations. Reported by bothLibraryImportDiagnosticsAnalyzerand its downlevel counterpart.[tests] The private
externstays caller-unsafe. No codegen change here:mainalready emits the inner__PInvokelocal function asstatic extern unsafeand wraps stub bodies in an explicitunsafeblock. An earlier revision of this PR mirrored the user-facing modifier onto that local function, which @jkotas pointed out violates the model - the raw P/Invoke takingchar*is obviously unsafe, while the wrapper is what discharges the obligation - so it was dropped and replaced with tests that lock the behavior in. The user's modifier is still mirrored onto the generated wrapper, so both halves of the partial agree.[analyzer]
IL5007reports methods withLibraryImportAttributethat declare no safety contract. UnlikeSYSLIB1064it fires regardless of the opt-in, so a code base can be annotated before the switch is flipped.[fixer]
AddUnsafeToLibraryImportCodeFixProviderfixesIL5007by marking the methodunsafeby default; developers can replace it withsafeafter auditing the boundary. This is the[LibraryImport]counterpart ofAddUnsafeToExternCodeFixProviderfrom Add unsafe modifier migration code fixer #131002 and, like the rest of that tooling, is not shipping (#if DEBUG) and off by default.Since
CSharpCompilationOptions.MemorySafetyRulesis stillinternal(dotnet/roslyn#82546), the opt-in is detected via theupdated-memory-safety-rulesfeature flag - the same fallback Roslyn's ownSourceModuleSymbol.UseUpdatedMemorySafetyRulesuses.Diagnostics IDs
Just for reference
CS9389[Roslyn] - Anexternmember must be explicitly markedunsafeorsafe.CS9388[Roslyn] - Thesafemodifier is only valid on non-unsafeexternmembers or field-like members of explicit or extended-layout types.CS0764[Roslyn] - Both partial member declarations must beunsafe, or neither may beunsafe.CS9390[Roslyn] - Both partial member declarations must be markedsafe, or neither may be markedsafe.SYSLIB1064[This PR] - A method withLibraryImportAttributemust be markedsafeorunsafeunder the updated rules.IL5007[This PR] - A method withLibraryImportAttributehas no explicit safety contract (migration only, needed for the code-fixer).Alternative design
Instead of a new analyzer, the generator could emit its own part as
unsafeand let the language enforce the rest: the user getsCS0764until they writeunsafetoo, or they writesafeand we regenerate to match. Tempting - no new diagnostic ID, and noCS9389+SYSLIB1064doubling up in the forwarder shape. It was rejected because:SourceOrdinaryMethodSymbol.PartialMethodChecksreports bothCS0764andCS9390atimplementation.GetFirstLocation(), i.e. insideLibraryImports.g.cs. No code fix can be offered there, and the message never mentions P/Invoke.CS9389by contrast does land on the user's declaration, so the two shapes would report in different files with different wording.CS9389.CS0764.CS0764is suppressed whenAllowUnsafeBlocksis off (&& definition.CompilationAllowsUnsafe()), so enforcement would silently disappear in that configuration.The speclet asks the same question ("should the language provide a narrower rule for partial members implemented by source generators?") and the working group answered: use an analyzer.
Known limitations / follow ups
safeon non-externmembers yet (CS9388); Unsafe evolution: relaxsafemodifier placement restrictions roslyn#84602 lifts this and is motivated by exactly this scenario. Until thensafecan only be spelled on P/Invokes whose generated implementation is a forwarder, so tests only exercisesafein that shape.ConvertToLibraryImportFixerpreservesunsafewhen rewriting a[DllImport](new test) but dropssafe, sinceSyntaxGeneratordoes not model the modifier and carrying it over today would produce code hittingCS9388.