ILTrim: Preserve property/event metadata when accessor methods are kept#126992
Open
ILTrim: Preserve property/event metadata when accessor methods are kept#126992
Conversation
Add reverse dependency from accessor methods to their owning PropertyDefinition/EventDefinition in MethodDefinitionNode. When a method with SpecialName is kept, scan the declaring type's properties and events to find ownership and root the metadata row. For properties, accessor methods are NOT force-rooted from the property — only accessors that are independently reachable get kept. This allows unused setters to be removed while keeping the property metadata. WriteInternal only writes MethodSemantics for marked accessors. For events, all accessors ARE force-rooted when the event is kept — if you can subscribe you must be able to unsubscribe. Also fix TypeDefinitionNode.WriteInternal to call AddTypeDefinition before AddPropertyMap/AddEventMap and use the output handle, fixing wrong type associations when types are removed during trimming. Based on MichalStrehovsky/runtime@36d613c Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d2eabc84-aafb-45e6-907d-292882d9b985 Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
MichalStrehovsky
April 16, 2026 09:24
View session
MichalStrehovsky
added
the
area-Tools-ILLink
MichalStrehovsky
approved these changes
Apr 16, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Updates ILTrim’s metadata preservation to better match ILLinker behavior by retaining property/event metadata rows when accessor methods are kept, and fixing map table associations when types are removed during trimming.
Changes:
- Add reverse dependencies from kept accessor methods to their owning
PropertyDefinition/EventDefinitionmetadata rows. - Adjust property emission so
MethodSemanticsrows are only written for accessors that are actually marked (and stop rooting accessors from the property node). - Fix
PropertyMap/EventMapemission to use the outputTypeDefinitionHandle(and correct a typo), and pruneILTrimExpectedFailures.txtaccordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/tools/ILTrim.Tests/ILTrimExpectedFailures.txt | Removes expected-failure entries for tests that now pass with corrected metadata preservation. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/TypeDefinitionNode.cs | Fixes PropertyMap/EventMap association by using the output type handle; corrects a typo. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/PropertyDefinitionNode.cs | Stops rooting accessors from properties; emits MethodSemantics only for marked accessors. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/MethodDefinitionNode.cs | Adds accessor→property/event back-reference dependency logic to preserve owning metadata rows. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/EventDefinitionNode.cs | Clarifies intent: events root all accessors to preserve runtime semantics. |
Comment on lines
+36
to
40
| // We intentionally do NOT root accessor methods here. The accessor methods are kept | ||
| // independently when they are called. The property definition is pulled in by the | ||
| // accessor → property back-reference in MethodDefinitionNode. This avoids keeping | ||
| // unused accessors (e.g. an unused setter when only the getter is called). | ||
| EcmaSignatureAnalyzer.AnalyzePropertySignature( |
There was a problem hiding this comment.
PropertyDefinitionNode no longer uses System.Diagnostics (the previous Debug.Assert block was removed), but the file still imports using System.Diagnostics;. With TreatWarningsAsErrors=true, this will surface as an unnecessary-using warning and can break the build. Remove the unused using directive.
Contributor
|
Tagging subscribers to this area: @agocke, @dotnet/illink |
This was referenced Apr 16, 2026
Open
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.
Description
Ports property/event metadata preservation logic from MichalStrehovsky/runtime@36d613c to match ILLink's existing behavior in
MarkStep.ILTrim was dropping
PropertyDefinition/EventDefinitionmetadata rows even when accessor methods were reachable, andAddPropertyMap/AddEventMapused the inputTypeDefinitionHandleinstead of the output handle — producing wrong type associations when types are removed during trimming.Changes
SpecialNamemethod is kept, scan the declaring type's properties/events to add a reverse dependency on the owning metadata row (mirrors ILLink'sTryGetProperty/TryGetEvent→MarkProperty/MarkEventinMarkStep)WriteInternalonly emitsMethodSemanticsfor accessors that areMarked(matches ILLink'sMarkPropertywhich does not rootGetMethod/SetMethod)MarkEventsemantics — partial event subscriptions break runtime semanticsAddTypeDefinitionbeforeAddPropertyMap/AddEventMapand passoutputHandleinstead of the input handle; fix "tehy" typo