Skip to content

Add missing custom attribute marking to ILTrim TokenBased nodes#127369

Open
Copilot wants to merge 4 commits intomainfrom
copilot/fix-custom-attribute-marking
Open

Add missing custom attribute marking to ILTrim TokenBased nodes#127369
Copilot wants to merge 4 commits intomainfrom
copilot/fix-custom-attribute-marking

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 24, 2026

Description

Several TokenBasedNode subclasses in ILTrim were not calling CustomAttributeNode.AddDependenciesDueToCustomAttributes, causing custom attributes on those metadata tables to be silently dropped during trimming.

Nodes fixed

  • ParameterNode — was returning null from GetStaticDependencies; now marks parameter custom attributes
  • GenericParameterNode — converted from yield return to lazily-initialized DependencyList to integrate CA marking without unnecessary allocations
  • GenericParameterConstraintNode — same pattern
  • ManifestResourceNode — refactored to have a single AddDependenciesDueToCustomAttributes call and single return at the end; removed early return null in the IgnoreDescriptors path so CA marking is never bypassed

Intentionally skipped (problematic)

InterfaceImpl, StandAloneSig, TypeSpec — along with the already-excluded MethodSpec, TypeRef, AssemblyRef, MemberRef, ModuleRef.

Test impact

  • 20 previously-failing tests now pass (removed from ILTrimExpectedFailures.txt)
  • 5 new expected failures added — OnlyKeepUsed tests that assert unused attribute removal, which requires --used-attrs-only (not yet implemented in ILTrim)
  • Fixed spurious diffs in ILTrimExpectedFailures.txt by preserving original line ordering (undoing unintended sort)

Example of the change pattern applied to each node:

// Before (ParameterNode)
public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFactory context) => null;

// After
public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFactory context)
{
    DependencyList dependencies = null;
    Parameter parameter = _module.MetadataReader.GetParameter(Handle);
    CustomAttributeNode.AddDependenciesDueToCustomAttributes(ref dependencies, context, _module, parameter.GetCustomAttributes());
    return dependencies;
}

Copilot AI and others added 2 commits April 24, 2026 04:42
…traint, and ManifestResource nodes; update expected failures

Add AddDependenciesDueToCustomAttributes calls to ParameterNode,
GenericParameterNode, GenericParameterConstraintNode, and
ManifestResourceNode so that custom attributes on these metadata
tables are correctly preserved during ILTrim.

Skip InterfaceImpl, StandAloneSig, and TypeSpec as requested (problematic).

Update ILTrimExpectedFailures.txt: remove 20 now-passing tests,
add 5 new expected failures (OnlyKeepUsed tests that expect unused
attribute removal, not yet implemented in ILTrim).

All 915 tests pass (358 passed, 557 skipped, 0 failed).

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/aa76f39a-b849-4fbe-a977-e9c4df50939f

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
…al line ordering

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ca9868d9-9404-4b98-8ba9-a2d5e04076d7

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 24, 2026 05:08
Copilot AI review requested due to automatic review settings April 24, 2026 05:08
@MichalStrehovsky MichalStrehovsky added area-Tools-ILLink .NET linker development as well as trimming analyzers and removed area-crossgen2-coreclr labels Apr 24, 2026
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

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

…butes only once

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/61b20371-b1f0-4b46-8e29-d5211b24471a

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot April 24, 2026 06:35
Copilot AI requested a review from MichalStrehovsky April 24, 2026 06:36
@MichalStrehovsky MichalStrehovsky marked this pull request as ready for review April 27, 2026 00:23
Copilot AI review requested due to automatic review settings April 27, 2026 00:23
Copy link
Copy Markdown
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

This PR fixes ILTrim TokenBasedNode subclasses that weren’t marking dependencies from metadata custom attributes, which caused those attributes (and their referenced constructors/types) to be silently dropped during trimming.

Changes:

  • Added CustomAttributeNode.AddDependenciesDueToCustomAttributes marking to additional token-based metadata nodes (Parameter, GenericParameter, GenericParameterConstraint, ManifestResource).
  • Refactored affected nodes to return a single dependency list while integrating custom attribute marking.
  • Updated ILTrimExpectedFailures.txt to reflect newly passing tests and to add new expected failures for OnlyKeepUsed scenarios that require --used-attrs-only.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/coreclr/tools/ILTrim.Tests/ILTrimExpectedFailures.txt Updates expected-failure baselines to reflect the new custom-attribute marking behavior.
src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/ParameterNode.cs Marks dependencies introduced by parameter custom attributes.
src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/ManifestResourceNode.cs Consolidates dependency collection and adds manifest resource custom-attribute dependency marking.
src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/GenericParameterNode.cs Switches from yield to an explicit dependency list to integrate generic-parameter custom-attribute marking.
src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/GenericParameterConstraintNode.cs Switches from yield to an explicit dependency list to integrate constraint custom-attribute marking.

@MichalStrehovsky
Copy link
Copy Markdown
Member

@copilot apply the code review feedback

… return bypassing CA marking

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/25d36104-0c73-4530-85ca-cdf871c5c846

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 27, 2026

@copilot apply the code review feedback

Applied both code review items in eb50492:

  1. GenericParameterNodeDependencyList is now lazily initialized (null + ??= on first constraint) to avoid unnecessary allocations.
  2. ManifestResourceNode — Removed the early return null when IgnoreDescriptors is set. Now wraps the descriptor processing in if (!factory.Settings.IgnoreDescriptors) instead, so the AddDependenciesDueToCustomAttributes call at the end always runs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Tools-ILLink .NET linker development as well as trimming analyzers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants