Deduplicate warnings for attribute on property dataflow#127252
Deduplicate warnings for attribute on property dataflow#127252eduardo-vp wants to merge 1 commit intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses NativeAOT/ILC emitting duplicate trim-analysis warnings (e.g., IL2026) when analyzing custom attributes associated with properties/events, aligning ILC behavior with ILLink/Analyzer for the reported scenario in #120004.
Changes:
- Track whether property/event custom attributes have already been analyzed to avoid producing duplicate diagnostics.
- Add registration helpers on
UsageBasedMetadataManagerand consult them when processing property/event accessors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs |
Adds state to record which property/event custom attributes have been processed. |
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/CustomAttributeBasedDependencyAlgorithm.cs |
Uses the new tracking to skip re-analyzing custom attributes for the same property/event when multiple accessors are visited. |
| @@ -82,7 +87,10 @@ public static void AddDependenciesDueToCustomAttributes(ref CombinedDependencyLi | |||
| EventAccessors accessors = @event.GetAccessors(); | |||
|
|
|||
| if (accessors.Adder == methodHandle || accessors.Remover == methodHandle || accessors.Raiser == methodHandle) | |||
| AddDependenciesDueToCustomAttributes(ref dependencies, eventCondition, factory, method.Module, @event.GetCustomAttributes(), new EventPseudoDesc(method.OwningType, eventHandle)); | |||
| { | |||
| if (mdManager.TryRegisterEventCustomAttributesProcessed(method.Module, eventHandle)) | |||
| AddDependenciesDueToCustomAttributes(ref dependencies, eventCondition, factory, method.Module, @event.GetCustomAttributes(), new EventPseudoDesc(method.OwningType, eventHandle)); | |||
There was a problem hiding this comment.
This change introduces global “already processed” state in UsageBasedMetadataManager to deduplicate property/event attribute analysis. Besides the thread-safety concern, this state will grow with every property/event encountered and is never cleared, which is unnecessary if the duplication is only between accessors.
Consider a stateless approach: only analyze property attributes when visiting a single canonical accessor (e.g., analyze on getter, or on setter only if there is no getter; similarly analyze event attributes on adder/remover/raiser in a preferred order). That avoids extra allocations and shared mutable state entirely.
| // Avoid analyzing the same custom attributes (and producing the same diagnostics) twice. | ||
| if (mdManager.TryRegisterPropertyCustomAttributesProcessed(method.Module, propertyHandle)) | ||
| AddDependenciesDueToCustomAttributes(ref dependencies, propertyCondition, factory, method.Module, property.GetCustomAttributes(), new PropertyPseudoDesc(method.OwningType, propertyHandle)); | ||
| } |
There was a problem hiding this comment.
There are existing linker/NativeAOT trimming tests explicitly tracking the duplicate-warning behavior for this issue (e.g., src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AttributePropertyDataflow.cs and .../RequiresCapability/RequiresOnAttribute.cs have UnexpectedWarning(..., Tool.NativeAot, "https://github.com/dotnet/runtime/issues/120004")). With this fix, those UnexpectedWarning expectations likely need to be removed/updated, otherwise the test suite will start failing once the extra warning is gone.
Please update/add a regression test expectation alongside this change to ensure the warning is produced exactly once going forward.
| private readonly HashSet<(EcmaModule, PropertyDefinitionHandle)> _propertyCustomAttributesProcessed = new(); | ||
| private readonly HashSet<(EcmaModule, EventDefinitionHandle)> _eventCustomAttributesProcessed = new(); |
There was a problem hiding this comment.
The new _propertyCustomAttributesProcessed / _eventCustomAttributesProcessed fields are HashSet<> instances, but custom-attribute dependency analysis can run concurrently during IL scanning (e.g., ILScanner uses Parallel.ForEach). Mutating these sets from multiple threads can corrupt the set or throw.
Make this thread-safe (e.g., guard Add with a lock, or use a ConcurrentDictionary<(EcmaModule, Handle), byte> as a set), or avoid shared mutable state by only analyzing attributes from a single chosen accessor (e.g., prefer getter/adder) instead of tracking “already processed”.
Fixes #120004.