Fix AOT warnings by not using MakeGenericType (refactor) #5718
Conversation
WalkthroughThe changes refactor the conversion of string inputs to collection objects in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TH as TryFlatListConversion
participant TC as TryCreateCollectionObject
participant LHelper as TryCreateListCollection<T>
participant HHelper as TryCreateHashSetCollection<T>
Caller->>TH: Call TryFlatListConversion(value)
alt Not a generic collection
TH-->>Caller: return false (newValue = null)
else Is a generic collection
TH->>TC: Call TryCreateCollectionObject(propertyType)
alt Collection is List<T>
TC->>LHelper: Attempt List creation
else Collection is HashSet<T>
TC->>HHelper: Attempt HashSet creation
end
alt Creation fails
TC-->>TH: Error -> throw NLogConfigurationException
TH-->>Caller: Exception propagates
else Creation succeeds
TC-->>TH: Return collection object, add method, item type
TH-->>Caller: return true with newValue
end
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (17)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/NLog/Internal/PropertyHelper.cs (3)
403-419: Consider partial parsing vs. all-or-nothing approach.
Currently, the code attempts to parse all items in a single pass, throwing an exception if any item fails. Depending on your use case, consider whether partial success (some items parsed correctly) is preferable; if so, you could gather errors and proceed with partially valid results. Otherwise, this all-or-nothing approach is perfectly acceptable.
434-461: Potential code duplication for handling string and int collections.
This method checks forList<string>,List<int>, and then falls back to more generalized solutions. While it works, consider consolidating these special cases into a single dynamic approach if you anticipate future expansions (e.g.,List<bool>,List<double>).
535-548: Parallel approach for HashSet creation.
Similar to the list logic above, this is a straightforward approach, but there is some repeated code. Consider unifying the logic if you need to branch out to more type-specific overrides in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/NLog/Internal/PropertyHelper.cs(1 hunks)
🔇 Additional comments (8)
src/NLog/Internal/PropertyHelper.cs (8)
393-398: Well-structured check for generic collection.
The preliminary check for genericIEnumerabletypes ensures we only proceed with valid collection targets. This helps avoid erroneous parsing attempts and simplifies subsequent logic.
423-424: Clearer exception handling.
Throwing anNLogConfigurationExceptionclarifies the source of the error and provides more explicit feedback than logging. This is a beneficial improvement for diagnosing configuration issues.
427-428: Fallback approach is consistent.
ReturningfalsewithnewValue = nullensures the caller understands that the flat list conversion was not applied. This aligns with typical “TryXyz” patterns and is easy to follow.
431-433: Suppression attributes are appropriate for AOT/trimming scenarios.
Using[UnconditionalSuppressMessage]avoids undesired warnings when trimming is involved. This is consistent with many .NET AOT patterns.
462-498: Good reuse of existing instance for list/hashset properties.
Reusing an existing collection instance or replicating its comparer ensures consistent behavior and user expectations. The reflection logic is a bit verbose, but it effectively handles specialized comparers.
501-517: Structured fallback path to hash-set creation.
The fallback logic for creating or re-creating aHashSet<>object with the correct comparer is well handled. It promotes consistency when dealing with various property value states.
519-533: Generically creating List is concise and robust.
UtilizingTryCreateListCollection<T>for strongly typed lists is a neat approach. It reads clearly and makes the intended behavior obvious.
552-552: Fallback to generic type creation is smooth.
The final fallback that dynamically constructs aList<>orHashSet<>ensures a last-resort resolution with descriptive debug info. This is a well-structured solution for advanced collection conversions.Also applies to: 559-562, 566-570
|
Trimming doesn't like unknown existing types, but AOT doesn't like trying to make new generic types. Trying to find a middle road.