Fix Invalid Trim Warning Suppression in BindConverter.cs#66997
Fix Invalid Trim Warning Suppression in BindConverter.cs#66997surya3655 wants to merge 6 commits into
Conversation
|
Thanks for your PR, @surya3655. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
@dotnet-policy-service agree company="Syncfusion, Inc." |
There was a problem hiding this comment.
Pull request overview
This PR refines trimming/AOT warning handling in the Components layer by removing overly broad reflection suppressions in BindConverter and replacing them with smaller helper methods that carry more targeted suppressions. It also adds IL3050-related annotations around persistent state JSON serialization and serializer lookup to reduce AOT analyzer noise.
Changes:
- Refactored
BindConverterdelegate caches to route reflection-based generic construction through dedicated helper methods with localized suppressions. - Added IL3050 handling around
PersistentComponentStateJSON (de)serialization methods. - Added IL3050 handling around runtime
MakeGenericTypeusage for resolvingPersistentComponentStateSerializer<T>.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/Components/Components/src/BindConverter.cs | Replaces blanket reflection suppressions with helper methods and targeted suppressions for enum/array parser/formatter creation. |
| src/Components/Components/src/PersistentComponentState.cs | Adds IL3050-related handling on JSON persistence APIs. |
| src/Components/Components/src/PersistentState/PersistentValueProviderComponentSubscription.cs | Adds IL3050-related handling for runtime generic serializer type lookup. |
| [UnconditionalSuppressMessage( | ||
| "ReflectionAnalysis", | ||
| "IL3050", | ||
| Justification = "ConvertToEnum has DynamicallyAccessedMembers(All) constraint. T is constrained to have All members, so this is safe.")] |
| [UnconditionalSuppressMessage( | ||
| "ReflectionAnalysis", | ||
| "IL2060:MakeGenericMethod", | ||
| Justification = "ConvertToNullableEnum has DynamicallyAccessedMembers(PublicParameterlessConstructor) constraint on the inner type. The innerType is verified to be an enum at runtime.")] |
| [UnconditionalSuppressMessage( | ||
| "ReflectionAnalysis", | ||
| "IL3050", | ||
| Justification = "ConvertToNullableEnum is safe for the verified enum type.")] |
| [RequiresUnreferencedCode("JSON serialization and deserialization might require types that cannot be statically analyzed.")] | ||
| [UnconditionalSuppressMessage("AOT", "IL3050", Justification = "JsonSerializer needs dynamic code generation for type-based serialization. The type is constrained by DynamicallyAccessedMembers(JsonSerialized).")] | ||
| public void PersistAsJson<[DynamicallyAccessedMembers(JsonSerialized)] TValue>(string key, TValue instance) |
There was a problem hiding this comment.
The suppression of IL3050 on this API is intentional. While [RequiresDynamicCode] would surface a blanket requirement to all callers, the actual scenario here is narrower: the JsonSerializer.Deserialize call is safe because the type argument has already been validated at the call site. Marking the method with [RequiresDynamicCode] would incorrectly imply that every use of this API requires dynamic code generation, which isn’t accurate and would unnecessarily burden consumers. By suppressing the warning and documenting why this usage is safe under AOT, we avoid misleading callers while still acknowledging the analyzer’s concern. This keeps the warning scoped to the specific case rather than overstating the requirement.
| [RequiresUnreferencedCode("JSON serialization and deserialization might require types that cannot be statically analyzed.")] | ||
| [UnconditionalSuppressMessage("AOT", "IL3050", Justification = "JsonSerializer needs dynamic code generation for type-based serialization. The type is constrained by DynamicallyAccessedMembers(JsonSerialized).")] | ||
| internal void PersistAsJson(string key, object instance, [DynamicallyAccessedMembers(JsonSerialized)] Type type) |
There was a problem hiding this comment.
Regarding [RequiresDynamicCode] suggestion:
Thank you for the suggestion. After analyzing the implications, we found that using [RequiresDynamicCode] instead of UnconditionalSuppressMessage would actually increase the maintenance burden rather than reduce it.
Problem with [RequiresDynamicCode]:
Unlike UnconditionalSuppressMessage which only affects the local method, [RequiresDynamicCode] propagates the warning to ALL callers. Adding it to PersistAsJson would warn every call site in the codebase, requiring additional suppressions at each caller.
Build Errors Encountered:
When we attempted to use [RequiresDynamicCode] on the JSON serialization methods, we encountered cascading IL3050 warnings at multiple call sites:
IL3050: 'PersistAsJson<TValue>(string, TValue)' has requirements for dynamic code.
Callers of this method require the same annotation.
[...register call site...] IL3050
IL3050: 'PersistAsJson(string, object, Type)' has requirements for dynamic code.
Callers of this method require the same annotation.
[...register call site...] IL3050
IL3050: 'TryTakeFromJson<TValue>(string, out TValue)' has requirements for dynamic code.
Callers of this method require the same annotation.
[...register call site...] IL3050
Result: Each call site would need its own [RequiresDynamicCode] or UnconditionalSuppressMessage, multiplying the suppressions instead of consolidating them.
Our Decision:
Since PersistAsJson is called from multiple internal callers (e.g., RegisterForSerialization, state persistence infrastructure), using UnconditionalSuppressMessage at the method level is the pragmatic choice - it provides the same functional safety without cascade of new warnings across the codebase.
| [RequiresUnreferencedCode("JSON serialization and deserialization might require types that cannot be statically analyzed.")] | ||
| [UnconditionalSuppressMessage("AOT", "IL3050", Justification = "JsonSerializer needs dynamic code generation for type-based deserialization. The type is constrained by DynamicallyAccessedMembers(JsonSerialized).")] | ||
| public bool TryTakeFromJson<[DynamicallyAccessedMembers(JsonSerialized)] TValue>(string key, [MaybeNullWhen(false)] out TValue? instance) |
There was a problem hiding this comment.
In this case, the API suppresses IL3050 intentionally. While [RequiresDynamicCode] would surface a general dynamic code requirement to all callers, the actual usage here (JsonSerializer.Deserialize with a verified runtime type) does not always require dynamic code generation. Marking the method with [RequiresDynamicCode] would overstate the requirement and mislead consumers into thinking every call is unsafe under AOT. The suppression, combined with a justification that explains why the call is safe, avoids false positives while still documenting the analyzer’s concern. This keeps the warning scoped to the specific scenario rather than broadly impacting all callers.
| [RequiresUnreferencedCode("JSON serialization and deserialization might require types that cannot be statically analyzed.")] | ||
| [UnconditionalSuppressMessage("AOT", "IL3050", Justification = "JsonSerializer needs dynamic code generation for type-based deserialization. The type is constrained by DynamicallyAccessedMembers(JsonSerialized).")] | ||
| internal bool TryTakeFromJson(string key, [DynamicallyAccessedMembers(JsonSerialized)] Type type, [MaybeNullWhen(false)] out object? instance) |
There was a problem hiding this comment.
The suppression of IL3050 here is intentional. While [RequiresDynamicCode] communicates a general requirement to callers, in this specific case the usage of MakeGenericType is safe because the type parameter is a runtime Type that has already been validated at the call site. Annotating with [RequiresDynamicCode] would incorrectly imply that all callers must account for dynamic code generation, which isn’t accurate for this scenario.
| [UnconditionalSuppressMessage("AOT", "IL3050", Justification = "MakeGenericType is safe here because the type parameter is a runtime Type that has already been verified at the call site.")] | ||
| private IPersistentComponentStateSerializer? SerializerFactory(Type type, IServiceProvider serviceProvider) |
There was a problem hiding this comment.
The justification for suppressing IL3050 refers to the runtime type being “verified at the call site,” but that’s not what the warning is about. IL3050 is raised because JsonSerializer.Deserialize and MakeGenericType may require dynamic code generation under AOT. Suppressing the warning hides that requirement from callers, whereas [RequiresDynamicCode] would correctly surface it. If this API truly depends on dynamic code, it should be annotated with [RequiresDynamicCode] (consistent with other Components APIs) or explicitly documented as safe under AOT. Using a trimming-oriented justification here is misleading, since the analyzer concern is not type validation but dynamic code usage.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Fix Invalid Trim Warning Suppression in BindConverter.cs
Description
Fixed invalid
UnconditionalSuppressMessageattributes onBindConverter.csthat were causing incorrect trimmer behavior. The original blanket suppressions on 130-line methods claimed "methods don't have DynamicallyAccessedMembers annotations" which was factually incorrect. Extracted reflection operations into small helper methods with targeted suppressions and accurate justifications.Problem Description
The original code had blanket suppressions on large
Get<T>()methods (130+ lines) that claimed:This was false. Methods like
MakeArrayTypeConverter<T>DO have[DynamicallyAccessedMembers(All)]annotations. The incorrect suppressions caused the trimmer to make wrong decisions about what to keep/remove, potentially breaking:string[],int[])Solution
Extracted reflection operations into small targeted helper methods with precise suppressions:
CreateArrayFormatter<T>DynamicallyAccessedMembers(All)constraint"CreateEnumParser<T>DynamicallyAccessedMembers(All)constraint"CreateNullableEnumParser<T>CreateArrayParser<T>Files Changed
src/Components/Components/src/BindConverter.cssrc/Components/Components/src/PersistentComponentState.cssrc/Components/Components/src/PersistentValueProviderComponentSubscription.csValidation
<EnableAOTAnalyzer>trueFixes : #57016