Skip to content

Refactor DownlevelLibraryImportGenerator to inject types via RegisterPostInitializationOutput#126056

Open
Copilot wants to merge 6 commits intomainfrom
copilot/adjust-downlevel-library-import-generator
Open

Refactor DownlevelLibraryImportGenerator to inject types via RegisterPostInitializationOutput#126056
Copilot wants to merge 6 commits intomainfrom
copilot/adjust-downlevel-library-import-generator

Conversation

Copy link
Contributor

Copilot AI commented Mar 24, 2026

Refactors the DownlevelLibraryImportGenerator to inject internal definitions of LibraryImportAttribute and StringMarshalling directly into the user's compilation using RegisterPostInitializationOutput, instead of shipping them as separate .cs source files via eng/generators.targets.

Changes Made

  • Switched to RegisterPostInitializationOutput — the downlevel LibraryImportAttribute and StringMarshalling types are now injected directly into the compilation rather than distributed as source files, removing the need for the corresponding entries in eng/generators.targets.
  • Trimmed injected types — the injected LibraryImportAttribute definition only exposes properties supported on downlevel frameworks; StringMarshalling only exposes the Utf16 value.
  • Added [Microsoft.CodeAnalysis.Embedded] — both injected types are annotated with this attribute so Roslyn treats them as compiler-internal, automatically suppressing CS0436 ("type conflicts with imported type") without needing explicit #pragma warning directives.
  • Removed visibility #if ifdefs — the PUBLIC_CONTRACT / internal visibility ifdefs in the BCL source files for LibraryImportAttribute and StringMarshalling are no longer needed and have been removed.
  • Removed #pragma warning disable/restore CS0436 — redundant now that [Embedded] suppresses CS0436 automatically.
  • Removed #nullable restore — a nullable context restore at the very end of a generated file has no effect and has been removed.
  • Updated unit tests — removed tests that relied on the old source-file injection path; added a test that verifies the downlevel attribute is injected and compiles correctly when no using for LibraryImportAttribute exists in the user's code.
  • Fixed brittle tree-count assertionOnlyTypeDefinitionsOutputTest now asserts that no stub output trees (with "LibraryImports" in their path) are added, rather than asserting an exact count of +2 injected trees, making the test resilient to future changes in Roslyn's embedded attribute handling.
  • Restored CannotForwardToDllImport downlevel test — re-added StringMarshallingForwardingNotSupported_ReportsDiagnostic as a downlevel test that uses (StringMarshalling)1 (numeric cast for Utf8) instead of StringMarshalling.Utf8, since the injected downlevel enum only exposes Utf16 = 2, keeping the forwarder-stub diagnostic path covered.

⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.

Copilot AI and others added 5 commits March 19, 2026 22:20
…or to inject internal LibraryImportAttribute and StringMarshalling definitions

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
…port downlevel test

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
…Attribute, remove visibility ifdefs

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/7e3db6e3-c81a-48ae-a946-ed5279403813
…eeded with [Embedded])

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/e54fed70-d31f-4d62-bc70-d698d655e325
… of file)

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ef2d7d3b-0418-42e0-9dfd-5c4def8bd451
@jkoritzinsky
Copy link
Member

@copilot please revise the PR title and description to cover all of the work that you've done in this PR, not just the last few changes.

@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor Author

Copilot AI commented Mar 24, 2026

@copilot please revise the PR title and description to cover all of the work that you've done in this PR, not just the last few changes.

Updated the PR description to cover the full set of changes across all commits: switching to RegisterPostInitializationOutput, trimming the injected types to downlevel-only members, adding [Embedded] to suppress CS0436, removing the now-redundant #pragma warning disable CS0436, removing the visibility #if ifdefs from BCL source, removing #nullable restore from the generated string, and updating the unit tests.

Copilot AI changed the title Remove redundant CS0436 suppression and #nullable restore from downlevel LibraryImport generator output Refactor DownlevelLibraryImportGenerator to inject types via RegisterPostInitializationOutput Mar 24, 2026
Copilot AI requested a review from jkoritzinsky March 24, 2026 20:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the downlevel LibraryImport source generator flow so required interop types are injected directly into the user compilation (via RegisterPostInitializationOutput) instead of being compiled in from shared .cs files via eng/generators.targets.

Changes:

  • Inject downlevel System.Runtime.InteropServices.LibraryImportAttribute + StringMarshalling via RegisterPostInitializationOutput (and mark them [Microsoft.CodeAnalysis.Embedded]) to avoid CS0436 without pragmas.
  • Remove the now-unneeded compile-time inclusion of LibraryImportAttribute.cs and StringMarshalling.cs from eng/generators.targets, and simplify the BCL sources accordingly.
  • Update unit tests to stop relying on the old “include a source snippet for the attribute/enum” approach and adjust expectations for generator outputs.

Reviewed changes

Copilot reviewed 7 out of 7 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/Diagnostics.cs Removes an OuterLoop downlevel diagnostic test tied to the old source-injection mechanism.
src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/Compiles.cs Updates downlevel compile snippets/tests to rely on post-init injected types; adds a new test class to assert only type-definition output when no [LibraryImport] is present.
src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/AdditionalAttributesOnStub.cs Drops manual injection of LibraryImportAttribute snippet for downlevel tests.
src/libraries/System.Runtime.InteropServices/gen/DownlevelLibraryImportGenerator/DownlevelLibraryImportGenerator.cs Adds post-initialization injection of embedded interop types into the compilation.
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/StringMarshalling.cs Removes visibility #if and makes the enum unconditionally public in CoreLib source.
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/LibraryImportAttribute.cs Removes downlevel-only internal/CS0436 pragmas and makes the attribute unconditionally public in CoreLib source.
eng/generators.targets Removes compile inclusion of CoreLib shared LibraryImportAttribute.cs and StringMarshalling.cs for downlevel generator enablement.

Comment on lines +795 to +798
// The generator should inject exactly two extra syntax trees:
// one for the EmbeddedAttribute definition and one for the LibraryImport interop types,
// but no method stubs.
Assert.Equal(originalCompilation!.SyntaxTrees.Count() + 2, newCompilation.SyntaxTrees.Count());
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new OnlyTypeDefinitionsOutputTest asserts the generator always adds exactly 2 syntax trees. This is brittle: any future post-initialization sources (or changes in Roslyn’s embedded attribute handling) will break the test even if the behavior is still correct. Consider asserting on the absence of stub output (e.g., no generated "LibraryImports" stub tree) or validating the specific generated source names instead of an exact +2 count.

Suggested change
// The generator should inject exactly two extra syntax trees:
// one for the EmbeddedAttribute definition and one for the LibraryImport interop types,
// but no method stubs.
Assert.Equal(originalCompilation!.SyntaxTrees.Count() + 2, newCompilation.SyntaxTrees.Count());
// The DownlevelLibraryImportGenerator should inject type definitions (for example, via
// RegisterPostInitializationOutput) but, when there are no [LibraryImport] usages, it
// must not emit any stub output such as the generated "LibraryImports" stub tree.
var originalTrees = originalCompilation!.SyntaxTrees.ToImmutableArray();
var newTrees = newCompilation.SyntaxTrees.ToImmutableArray();
var addedTrees = newTrees.Except(originalTrees).ToImmutableArray();
Assert.DoesNotContain(addedTrees, tree => tree.FilePath.Contains("LibraryImports", StringComparison.Ordinal));

Copilot uses AI. Check for mistakes.
Comment on lines 240 to 242
[Fact]
public async Task InvalidStringMarshallingConfiguration_ReportsDiagnostic()
{
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the downlevel test for CannotForwardToDllImport leaves the forwarder-specific StringMarshalling validation path untested (the analyzer still reports this diagnostic when a forwarder stub is produced and StringMarshalling != Utf16). Consider adding a replacement downlevel test that works with the injected trimmed enum (e.g., using an explicit numeric cast to a non-Utf16 value) so this diagnostic behavior stays covered.

Copilot uses AI. Check for mistakes.
@jkoritzinsky
Copy link
Member

@copilot apply changes based on the comments in this thread

…rwardToDllImport downlevel test

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/e82b595f-cd92-40d5-a099-e993fc9a1a12
Copy link
Contributor Author

Copilot AI commented Mar 24, 2026

@copilot apply changes based on the comments in this thread

Applied both review suggestions in commit d446c83:

  1. Compiles.cs — Replaced the brittle Assert.Equal(count + 2, ...) assertion in OnlyTypeDefinitionsOutputTest with Assert.DoesNotContain(addedTrees, tree => tree.FilePath.Contains("LibraryImports", StringComparison.Ordinal)), which checks the meaningful invariant (no stub output) without coupling to an exact tree count.

  2. Diagnostics.cs — Added back StringMarshallingForwardingNotSupported_ReportsDiagnostic as a downlevel test. Since the injected StringMarshalling enum only exposes Utf16 = 2, the test uses (StringMarshalling)1 (numeric cast for Utf8) to trigger the CannotForwardToDllImport diagnostic, keeping the forwarder-stub path covered.

Copilot AI requested a review from jkoritzinsky March 24, 2026 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants