Conversation
…er and README conventions
…ct references - Expand frontmatter description with trigger info (when to use / not use) - Remove redundant When to Use / When Not to Use body sections - Trim type mapping to dangerous-only entries inline; full table in references/type-mapping.md - Move blittable structs, common pitfalls, failure modes, resources to references/ - Reduce SKILL.md from ~510 to 285 lines
… portability notes
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new dotnet-pinvoke skill with supporting reference documentation and a basic evaluation test suite to validate expected P/Invoke declarations across TFMs.
Changes:
- Introduces
dotnet-pinvokeskill guide (SKILL.md) covering signatures, marshalling, lifetime, SafeHandle, and diagnostics. - Adds reference docs for native-to-.NET type mapping and common diagnostics/pitfalls.
- Adds an eval scenario file to test
LibraryImportvsDllImportexpectations.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/dotnet/tests/dotnet-pinvoke/eval.yaml |
Adds eval scenarios for generating P/Invoke declarations for .NET 8 and .NET Framework |
src/dotnet/skills/dotnet-pinvoke/SKILL.md |
New skill documentation for P/Invoke/LibraryImport workflow and best practices |
src/dotnet/skills/dotnet-pinvoke/references/type-mapping.md |
New reference table for native-to-.NET type mappings and struct layout guidance |
src/dotnet/skills/dotnet-pinvoke/references/diagnostics.md |
New reference for common P/Invoke pitfalls and debugging approach |
Comments suppressed due to low confidence (1)
src/dotnet/skills/dotnet-pinvoke/references/type-mapping.md:26
- This reference recommends
CLong/CULongfor Clong, but those types are .NET 7+ and won’t be available when targeting .NET Framework. Since this doc/skill explicitly covers .NET Framework, please add a framework-specific note (e.g., conditional compilation usinginton Windows andlongon Unix, or a custom typedef/struct approach) so readers don’t end up with uncompilable signatures.
| C / Win32 Type | .NET Type | Why It's Dangerous |
|----------------|-----------|-------------------|
| `long` | **`CLong`** | C `long` is 32-bit on Windows, 64-bit on 64-bit Unix — never use `int` or `long`. With `LibraryImport`, requires `[assembly: DisableRuntimeMarshalling]` or you get SYSLIB1051. With `DllImport`, works without it |
| `size_t` | `nuint` | Pointer-sized. Never use `ulong` — causes stack corruption on 32-bit |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ```csharp | ||
| [DllImport("mylib")] | ||
| private static extern int ProcessRecords( | ||
| [In] Record[] records, nuint count, out uint outProcessed); | ||
| ``` |
There was a problem hiding this comment.
Several DllImport examples use nuint for size_t (e.g., ProcessRecords). That won’t compile for .NET Framework consumers of this guidance; nuint is .NET 5+/C# 9+. Consider documenting UIntPtr as the .NET Framework/legacy equivalent (and nuint for modern TFMs), or show framework-specific variants so the examples are accurate for both targets.
| | `long` | **`CLong`** | C `long` is 32-bit on Windows, 64-bit on 64-bit Unix — never use `int` or `long`. With `LibraryImport`, requires `[assembly: DisableRuntimeMarshalling]` or you get SYSLIB1051. With `DllImport`, works without it | | ||
| | `size_t` | `nuint` | Pointer-sized. Never use `ulong` — causes stack corruption on 32-bit | | ||
| | `intptr_t` | `nint` | Pointer-sized | | ||
| | `BOOL` (Win32) | `int` | Not `bool` — Win32 `BOOL` is 4 bytes | |
There was a problem hiding this comment.
The type mapping table unconditionally maps size_t to nuint and intptr_t to nint, but those types don’t exist on .NET Framework. Add a note/row for legacy TFMs (e.g., map to UIntPtr/IntPtr) so the reference remains correct when the target framework is .NET Framework.
This issue also appears on line 23 of the same file.
|
|
||
| | Pitfall | Impact | Solution | | ||
| |---------|--------|----------| | ||
| | `int` for `size_t` | Stack corruption on 64-bit | Use `nuint` | |
There was a problem hiding this comment.
Diagnostics table recommends nuint as the fix for size_t misuse, but for .NET Framework (and other pre-.NET 5 TFMs) the appropriate fix is UIntPtr. Consider updating this row to mention both (nuint on .NET 5+; UIntPtr on .NET Framework) to avoid generating uncompilable guidance.
| | `int` for `size_t` | Stack corruption on 64-bit | Use `nuint` | | |
| | `int` for `size_t` | Stack corruption on 64-bit | Use `nuint` (.NET 5+) or `UIntPtr` (pre-.NET 5, including .NET Framework) | |
| @@ -0,0 +1,55 @@ | |||
| scenarios: | |||
| - name: "Generate LibraryImport declaration from C header (.NET Core 5+)" | |||
There was a problem hiding this comment.
Scenario name says ".NET Core 5+" but the prompt targets .NET 8 and the assertions require LibraryImport, which is only available on .NET 7+. This mismatch makes the scenario requirements unclear; update the scenario name (and/or prompt) to align with the minimum TFM for LibraryImport.
| - name: "Generate LibraryImport declaration from C header (.NET Core 5+)" | |
| - name: "Generate LibraryImport declaration from C header (.NET 8+)" |
| value: "nuint" | ||
| - type: "output_contains" | ||
| value: "static extern" | ||
| rubric: | ||
| - "Uses DllImport instead of LibraryImport since the target is .NET Framework" | ||
| - "Maps size_t to nuint, not ulong" |
There was a problem hiding this comment.
The .NET Framework (x86) scenario asserts and rubrics for nuint, but nuint/nint are not available in .NET Framework/C# versions typically used there. For Framework guidance/tests, size_t should be represented as UIntPtr (or uint if you truly want x86-only). Update the assertions/rubric to match the correct type for .NET Framework.
| value: "nuint" | |
| - type: "output_contains" | |
| value: "static extern" | |
| rubric: | |
| - "Uses DllImport instead of LibraryImport since the target is .NET Framework" | |
| - "Maps size_t to nuint, not ulong" | |
| value: "UIntPtr" | |
| - type: "output_contains" | |
| value: "static extern" | |
| rubric: | |
| - "Uses DllImport instead of LibraryImport since the target is .NET Framework" | |
| - "Maps size_t to UIntPtr, not ulong" |
| | **Mechanism** | Runtime marshalling | Source generator (compile-time) | | ||
| | **AOT / Trim safe** | No | Yes | | ||
| | **String marshalling** | `CharSet` enum | `StringMarshalling` enum | | ||
| | **Error handling** | `SetLastError` | `SetLastPInvokeError` | |
There was a problem hiding this comment.
LibraryImportAttribute uses SetLastError = true (same property name as DllImport); SetLastPInvokeError is not a valid attribute argument and will not compile in the examples. Replace SetLastPInvokeError with SetLastError throughout, while keeping Marshal.GetLastPInvokeError() for retrieval on .NET 7+.
| | **Error handling** | `SetLastError` | `SetLastPInvokeError` | | |
| | **Error handling** | `SetLastError` | `SetLastError` | |
Skill Validation Results
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
No description provided.