Skip to content

Parse custom attribute value blobs for dependencies and rewrite type name strings in ILTrim#127596

Draft
Copilot wants to merge 7 commits intomainfrom
copilot/fix-custom-attribute-todos
Draft

Parse custom attribute value blobs for dependencies and rewrite type name strings in ILTrim#127596
Copilot wants to merge 7 commits intomainfrom
copilot/fix-custom-attribute-todos

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 30, 2026

Description

CustomAttributeNode had two TODOs: it wasn't parsing the custom attribute value blob for dependencies, and it wasn't rewriting type name strings that could become stale after trimming removes type forwarders.

Dependency analysis (GetStaticDependencies)

Decodes the custom attribute blob via CustomAttributeTypeProvider and reports dependencies for:

  • typeof() arguments
  • Enum types (needed for boxing)
  • Property setters and fields referenced in named arguments
  • Non-primitive array element types

Follows the same pattern as CustomAttributeBasedDependencyAlgorithm in ILCompiler. All dependency helper methods accept a DependencyList parameter and use .Add() directly, avoiding yield return and IEnumerable overhead. GetStaticDependencies creates and returns the populated DependencyList. The unnecessary customAttribute.Constructor.IsNil guard has been removed since custom attributes always have a constructor. Dependency blob parsing and named argument handling are inlined directly into GetStaticDependencies rather than delegated to standalone methods. Constructor resolution uses TryGetMethod (backed by GetObject(handle, NotFoundBehavior.ReturnNull)) instead of try/catch.

Blob rewriting (WriteInternal)

Always re-encodes the blob using BlobEncoder.CustomAttributeSignature with FixedArgumentsEncoder and CustomAttributeNamedArgumentsEncoder APIs to resolve type name strings to their definitions. Uses writeContext.GetSharedBlobBuilder() following the established pattern of other callers — the rewriter writes directly to the provided BlobBuilder without materializing an intermediate array. The blob is unconditionally re-encoded (no early-out optimization) to ensure the encoding logic is always exercised and bugs are not hidden. The encoding logic is inlined directly into RewriteCustomAttributeBlob rather than delegated to a standalone method. Constructor resolution uses TryGetMethod instead of try/catch.

This handles the case where trimming drops type forwarders — e.g. typeof(object) stored as System.Object, System.Runtime gets re-encoded as System.Object, System.Private.CoreLib via CustomAttributeTypeNameFormatter.

Code style

  • Methods use cascading if/else if/else instead of early return statements.
  • EncodeElementType switches on type.Category and handles IsEnum and System.Type in the default case, rather than checking them before the switch.
  • Exception handling uses catch (Exception ex) when (ex is TypeSystemException or BadImageFormatException) to collapse multiple catch blocks.

Test results

24 previously-expected-failing tests now pass and have been removed from ILTrimExpectedFailures.txt (382 passed, 0 failed).

Copilot AI and others added 2 commits April 30, 2026 04:17
…ame strings

Implement custom attribute blob parsing in CustomAttributeNode:
1. GetStaticDependencies now decodes the custom attribute value blob and
   reports dependencies for typeof() arguments, enum types, property
   setters, and fields referenced in the blob.
2. WriteInternal now re-encodes the blob with type name strings resolved
   to their definitions, handling cases where trimming drops type
   forwarders (e.g. re-encoding "System.Object, System.Runtime" to
   "System.Object, System.Private.CoreLib").

Remove 24 tests from ILTrimExpectedFailures.txt that now pass.

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f4173cc7-fd45-4a62-9a1c-1a31c06cc85b

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
… remove unused parameter

- Catch TypeSystemException and BadImageFormatException instead of base Exception
- Add DependencyNode type alias to reduce verbose fully-qualified type names
- Remove unused isFixedArg parameter from WriteArgumentValue
- Hoist constructor resolution out of named argument loop
- Fix duplicate CustomAttributeTypeNameFormatter allocations

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f4173cc7-fd45-4a62-9a1c-1a31c06cc85b

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
@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.

… BlobBuilder

Use BlobEncoder.CustomAttributeSignature with FixedArgumentsEncoder and
CustomAttributeNamedArgumentsEncoder instead of manual byte writing.
Use writeContext.GetSharedBlobBuilder() following established patterns.
Removed byte[] materialization - writes directly to shared BlobBuilder.

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d078b767-9d76-41e5-9168-8f6651786108

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot April 30, 2026 09:04
Copilot AI requested a review from MichalStrehovsky April 30, 2026 09:05
…bute blobs

Remove CheckArgumentsForRewrite/CheckArgumentValueForRewrite that skipped
rewriting when no typeof() references were found. Always rewrite to exercise
the encoding logic and surface bugs.

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/46e2e54d-5313-4538-8274-c5cfe241e547

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
…pe.Category

- Replace early returns with cascading if/else if/else in EncodeNamedArgumentType,
  WriteLiteralValue, and WriteScalarValue.
- Switch on type.Category instead of type.UnderlyingType.Category in
  EncodeElementType, handling IsEnum and System.Type in the default case.

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/683a21bb-4e07-4219-81e8-b0a320967bd3

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 1, 2026 11:23
@@ -61,7 +68,154 @@ public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFacto
if (!customAttribute.Constructor.IsNil)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot this customAttribute.Constructor.IsNil is never true, we can remove that. Custom attributes always depend on the attribute constructor at minimum.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the Constructor.IsNil guard in 48062a1.

foreach (CustomAttributeTypedArgument<TypeDesc> fixedArg in value.FixedArguments)
{
foreach (var entry in GetDependenciesFromCustomAttributeArgument(factory, fixedArg.Type, fixedArg.Value))
yield return entry;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot this still has too many yield return iterating values obtained from elsewhere. Rewrite all the methods here to accept a DependencyList parameter and simply .Add the results to the DependencyList. Change IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFactory factory) to simply return the DepednecyList that we populated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rewritten in 48062a1. All dependency helper methods now accept a DependencyList parameter and use .Add() directly. GetStaticDependencies creates and returns the DependencyList.

…endencyList with .Add

- Remove unnecessary `customAttribute.Constructor.IsNil` guard since custom
  attributes always have a constructor.
- Rewrite GetStaticDependencies to return a DependencyList populated via .Add.
- Convert all helper methods (GetDependenciesFromCustomAttributeBlob,
  GetDependenciesFromCustomAttributeArgument, GetDependenciesFromPropertySetter,
  GetDependenciesFromField) to accept a DependencyList parameter and .Add
  directly, eliminating yield return and IEnumerable overhead.

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f59c4055-f47c-4e90-9a3e-b04855a33de1

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot May 4, 2026 01:04
Copilot AI requested a review from MichalStrehovsky May 4, 2026 01:06
{
decodedValue = customAttribute.DecodeValue(new CustomAttributeTypeProvider(_module));
}
catch (TypeSystemException)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot collapse this into catch (Exception ex) when (ex is TypeSystemException or BadImageFormatException) and do the same for the catch below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in a102e20. Both catch blocks (in GetStaticDependencies and RewriteCustomAttributeBlob) now use catch (Exception ex) when (ex is TypeSystemException or BadImageFormatException).

return dependencies;
}

GetDependenciesFromCustomAttributeBlob(dependencies, factory, decodedValue);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot inline the method into here, we don't need a standalone method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Inlined in a102e20.

return;
}

EncodeCustomAttributeBlob(customAttribute, decodedValue, blobBuilder);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot inline the method into here, we don't need a standalone method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Inlined in a102e20.

MethodDesc constructor;
try
{
constructor = _module.GetMethod(customAttribute.Constructor);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot use GetObject(handle, NotFoundBehavior.ReturnNull) as MethodDesc so that we don't need a try/catch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in a102e20. Using _module.TryGetMethod(customAttribute.Constructor) which calls GetObject(handle, NotFoundBehavior.ReturnNull) as MethodDesc under the hood.

- Collapse separate TypeSystemException/BadImageFormatException catch blocks
  into single `catch (Exception ex) when (ex is ... or ...)` in both
  GetStaticDependencies and RewriteCustomAttributeBlob.
- Inline GetDependenciesFromCustomAttributeBlob into GetStaticDependencies.
- Inline EncodeCustomAttributeBlob into RewriteCustomAttributeBlob.
- Use _module.TryGetMethod (GetObject with NotFoundBehavior.ReturnNull)
  instead of try/catch for constructor resolution.

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/93abc623-385e-4d42-a955-f61dd8eab979

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot May 4, 2026 02:46
Copilot AI requested a review from MichalStrehovsky May 4, 2026 02:47
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.

2 participants