Mark descriptor-preserved members as reflection-visible#128272
Open
sbomer wants to merge 12 commits into
Open
Conversation
…t impls Add a regression test for dotnet#128120. When a type is preserved via an XML descriptor, ILLink should keep its static abstract interface implementations if those methods are called through constrained generics elsewhere in the program. Currently the test fails because ILLink strips the interface and override from the descriptor-preserved type. Co-authored-by: Copilot <[email protected]> Assisted-by: Claude:claude-opus-4.6-1m
…ant casting A method preserved via XML descriptor is reflection-visible, which means its declaring type is also accessible (e.g., via MethodBase.GetCurrentMethod().DeclaringType) and could be used as a generic argument in constrained calls (e.g., via MakeGenericMethod). Mark the declaring type as relevant to variant casting so that its static abstract interface implementations are preserved by the linker. Fixes dotnet#128120 Co-authored-by: Copilot <[email protected]> Assisted-by: Claude:claude-opus-4.6-1m
…t be kept Test that a reference type with explicit layout preserved via XML descriptor retains its fields. This verifies the type is treated as fully reflection-visible (MarkImplicitlyUsedFields), not just relevant to variant casting. Co-authored-by: Copilot <[email protected]> Assisted-by: Claude:claude-opus-4.6-1m
…tion-visible Descriptor-preserved methods and fields are reflection-visible. Their declaring types are also accessible (e.g., via MethodBase.DeclaringType or FieldInfo.DeclaringType) and could be used as generic arguments in constrained calls via MakeGenericMethod. Defer full reflection-visible treatment from DescriptorMarker to MarkStep via pending sets in Annotations, so that MarkStep can call MarkMethodVisibleToReflection, MarkFieldVisibleToReflection, and MarkTypeVisibleToReflection on the declaring type. This ensures: - Static abstract interface implementations are preserved - Implicit fields for explicit-layout types are preserved - Metadata names are not swept - TypeMap entries are processed Co-authored-by: Copilot <[email protected]> Assisted-by: Claude:claude-opus-4.6-1m
Clarify that GetCurrentMethod produces IL2026 but the suppression is justified because the descriptor preserves the method. Add ExpectedNoWarnings to the explicit layout test. Co-authored-by: Copilot <[email protected]> Assisted-by: Claude:claude-opus-4.6-1m
- Remove unnecessary #pragma warning disable IL2060 from tests - Add ExpectedNoWarnings to explicit layout test - Remove issue references from test comments - Clarify comment about IL2026 suppression justification - Expose pending sets as properties, inline clear in ProcessMarkedPending - Separate doc comments for method and field pending sets Co-authored-by: Copilot <[email protected]> Assisted-by: Claude:claude-opus-4.6-1m
…entations (dotnet#128020)" This reverts commit c196cc0.
Co-authored-by: Copilot <[email protected]> Assisted-by: Claude:claude-opus-4.6-1m
bf799c8 to
323fb3e
Compare
Contributor
|
Tagging subscribers to this area: @agocke, @dotnet/illink |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the ILLink marking pipeline so that members preserved via XML descriptors are treated as reflection-visible, and it additionally marks their declaring types as reflection-visible to preserve reflection-dependent semantics (e.g., variant casting behaviors and explicit-layout field preservation).
Changes:
- Add pending-tracking for descriptor-preserved methods/fields and process them in
MarkStepviaMark*VisibleToReflection, also marking declaring types reflection-visible. - Extend
DescriptorMarkerto schedule descriptor-preserved fields/methods for reflection-visible processing. - Add new linker test cases + descriptors validating static abstract interface implementations and explicit-layout fields are preserved when reachability is via descriptor-preserved members.
Show a summary per file
| File | Description |
|---|---|
| src/tools/illink/src/linker/Linker/Annotations.cs | Adds pending collections/APIs to schedule descriptor-preserved members for reflection-visible processing. |
| src/tools/illink/src/linker/Linker.Steps/MarkStep.cs | Drains pending reflection-visible members and marks members + declaring types as reflection-visible. |
| src/tools/illink/src/linker/Linker.Steps/DescriptorMarker.cs | Schedules descriptor-preserved fields/methods for reflection-visible handling. |
| src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/StaticAbstractMethodsPreservedViaDescriptor.cs | New regression test for static abstract interface implementations preserved via descriptor reachability. |
| src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/StaticAbstractMethodsPreservedViaDescriptor.xml | Descriptor roots methods used by the static-abstract preservation test. |
| src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/DescriptorPreservedTypeIsReflectionVisible.cs | New test validating descriptor-preserved type is fully treated as reflection-visible (variant casting + explicit layout fields). |
| src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/DescriptorPreservedTypeIsReflectionVisible.xml | Descriptor roots methods used by the reflection-visible type behavior test. |
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 3
ILTrim does not support static abstract interface methods or descriptor-based reflection-visible marking. Co-authored-by: Copilot <[email protected]> Assisted-by: Claude:claude-opus-4.6-1m
- Convert tabs to spaces in test files to match .editorconfig - Remove spaces before parentheses to match neighbor test style - Make pending reflection-visible APIs internal Co-authored-by: Copilot <[email protected]> Assisted-by: Claude:claude-opus-4.6-1m
Pass DependencyInfo and MessageOrigin from DescriptorMarker through to MarkStep so warnings and dependency tracking attribute correctly to the descriptor XML location. Encapsulate the pending data behind a DrainPendingReflectionVisibleMembers method. Co-authored-by: Copilot <[email protected]> Assisted-by: Claude:claude-opus-4.6-1m
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Comment on lines
+260
to
+270
| internal List<(MethodDefinition Method, DependencyInfo Reason, MessageOrigin Origin)> DrainPendingReflectionVisibleMethods() | ||
| { | ||
| var result = pending_reflection_visible_methods; | ||
| pending_reflection_visible_methods = new List<(MethodDefinition, DependencyInfo, MessageOrigin)>(); | ||
| return result; | ||
| } | ||
|
|
||
| internal List<(FieldDefinition Field, DependencyInfo Reason, MessageOrigin Origin)> DrainPendingReflectionVisibleFields() | ||
| { | ||
| var result = pending_reflection_visible_fields; | ||
| pending_reflection_visible_fields = new List<(FieldDefinition, DependencyInfo, MessageOrigin)>(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
And mark their declaring types as reflection-visible.
When a member is marked via descriptor it should be considered visible to reflection. And when that is the case, its declaring type is accessible via reflection, so the requirements for reflection-visible types should be preserved too.
Fixes #128120