Fix AOT warnings by not using MakeGenericType #5715
Conversation
WalkthroughThe changes refactor multiple components in the NLog library. Several methods now obtain assembly locations using a helper function to simplify logic and remove redundant error handling. Reflection caching and property conversion routines have been streamlined and updated with new supportive methods and attributes. Tests have been modified to capture stack traces without specifying a starting frame. Additionally, minor documentation corrections, conditional compilation adjustments, and project file updates for AOT publishing have been applied. Changes
Sequence Diagram(s)sequenceDiagram
participant L as LoggingConfigurationFileLoader
participant AH as AssemblyHelpers
participant P as Path (System.IO.Path)
L->>AH: GetAssemblyFileLocation(nlogAssembly)
AH-->>L: Return assembly location
L->>P: GetDirectoryName(assemblyLocation)
P-->>L: Return directory name
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (17)
🚧 Files skipped from review as they are similar to previous changes (16)
🔇 Additional comments (7)
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: 1
🧹 Nitpick comments (3)
src/NLog/Layouts/LayoutParser.cs (1)
517-517: Swapping MakeGenericType with direct property instantiation improves AOT compatibility.Replacing
MakeGenericTypewithActivator.CreateInstance(propertyInfo.PropertyType, ...)cleanly bypasses potential ahead-of-time reflection warnings.Consider verifying that the target type includes an appropriate constructor receiving a
Layoutparameter, preventing reflection issues in dynamically hosted scenarios.src/NLog/Internal/PropertyHelper.cs (1)
417-446: Centralized collection creation logic for property assignment.The
TryCreateCollectionObjectmethod now captures the reflection logic for creating or retrieving a suitable collection instance and itsAddmethod. Logging an error when reflection fails provides a clear indicator of misconfigurations.Consider expanding the error message with additional context (e.g., property name, expected collection type) to assist in troubleshooting.
src/NLog/Internal/ObjectReflectionCache.cs (1)
572-584: Clarify or rename the customIDictionaryEnumeratorinterface.
This code introduces an internal interface namedIDictionaryEnumerator, which differs from the built-in .NETSystem.Collections.IDictionaryEnumerator. Consider renaming it to reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/NLog/Config/LoggingConfigurationFileLoader.cs(1 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/AssemblyHelpers.cs(1 hunks)src/NLog/Internal/ObjectReflectionCache.cs(6 hunks)src/NLog/Internal/PlatformDetector.cs(1 hunks)src/NLog/Internal/PropertyHelper.cs(4 hunks)src/NLog/LayoutRenderers/NLogDirLayoutRenderer.cs(1 hunks)src/NLog/Layouts/LayoutParser.cs(2 hunks)src/NLog/SetupInternalLoggerBuilderExtensions.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs(1 hunks)tests/TestTrimPublish/TestTrimPublish.csproj(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/NLog/SetupInternalLoggerBuilderExtensions.cs
🔇 Additional comments (22)
tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs (1)
114-114: Parameter removed from SetStackTrace method call for AOT compatibility.The second parameter has been removed from the SetStackTrace method call, simplifying the API surface and improving compatibility with AOT compilation by avoiding potential MakeGenericType usage.
tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs (1)
130-130: Parameter removed from SetStackTrace method call for AOT compatibility.The second parameter (frame index) has been removed from the SetStackTrace method call, maintaining consistency with the other test files and supporting the AOT compatibility changes.
tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs (1)
108-108: Parameter removed from SetStackTrace method call for AOT compatibility.The second parameter has been removed from the SetStackTrace method call, aligning with the pattern of changes across all test files to avoid using MakeGenericType in AOT scenarios.
tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs (1)
256-256: Parameter removed from SetStackTrace method call for AOT compatibility.The second parameter has been removed from the SetStackTrace method call. This consistent change across all test files suggests a deliberate effort to improve AOT compatibility by avoiding reflection mechanisms like MakeGenericType.
src/NLog/Internal/PlatformDetector.cs (1)
82-85: Improved platform detection with macOS and Android support.The addition of checks for macOS (
OSPlatform.OSX) and Android operating systems helps ensure that NLog correctly identifies these platforms. This is particularly important for cross-platform applications that need proper runtime behavior on these systems.tests/TestTrimPublish/TestTrimPublish.csproj (2)
13-15: Commented out PublishSingleFile configuration.Commenting out the PublishSingleFile option might be intentional as part of the AOT optimization strategy, but it's worth verifying this change won't impact existing functionality.
21-22: AOT compilation and trimmer warning settings added.These new configuration options:
<PublishAot>true</PublishAot>- Enables ahead-of-time compilation<TrimmerSingleWarn>false</TrimmerSingleWarn>- Disables consolidation of trimmer warningsThese changes align perfectly with the PR objective of addressing AOT warnings.
src/NLog/Config/LoggingConfigurationFileLoader.cs (1)
262-263: Assembly location retrieval improved with helper method.Replaced direct assembly location access with
AssemblyHelpers.GetAssemblyFileLocation(). The added comment explains a key issue with NLog.dll loaded from NuGet cache via NTFS hard links that could return unexpected file locations.This change improves robustness when resolving assembly locations, particularly in AOT compilation scenarios.
src/NLog/Internal/AppEnvironmentWrapper.cs (2)
249-257: Refactored entry assembly location lookup with consistent helper method.The implementation now uses
AssemblyHelpers.GetAssemblyFileLocation()for retrieving the assembly location, which aligns with the PR objective of standardizing assembly location access. The control flow has also been simplified.
264-265: Consistent use of assembly location helper method.This change ensures that assembly locations are retrieved in a consistent manner across the codebase, which is important for AOT compilation compatibility.
src/NLog/LayoutRenderers/NLogDirLayoutRenderer.cs (1)
94-101: Use of AssemblyHelpers.GetAssemblyFileLocation ensures consistent assembly path retrieval.The updated logic simplifies the retrieval of the assembly location and cleanly handles cases where the location is null or empty by returning an empty string. This approach appears consistent with the broader refactoring for AOT compatibility.
src/NLog/Layouts/LayoutParser.cs (1)
506-507: Additional trimming suppression helps maintain backward compatibility.By adding
[System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessage(...)], the method safely retains required code paths during linking or trimming, ensuring configuration value conversions remain intact.src/NLog/Internal/AssemblyHelpers.cs (1)
44-49: Simplified method to retrieve assembly file location.All extended logging and error handling have been removed, leaving a concise approach that checks
assembly?.Locationand returns an empty string if not found. This drastically reduces complexity but no longer logs fallback conditions.src/NLog/Internal/PropertyHelper.cs (3)
302-320: Trimming suppression and reflection-based layout instantiation.Applying
[UnconditionalSuppressMessage]ensures the trimming process does not remove important reflection code. The updated approach to createLayout<>instances withActivator.CreateInstancealigns with the refactor away fromMakeGenericType.
393-415: Enhanced list and array parameter handling in TryFlatListConversion.Splitting the string according to quoted commas and then converting each item eliminates confusion around list property assignments. Dynamically creating a new collection object is a clear improvement for flexible list-based configurations.
449-559: Refined approach to identify and instantiate collections (List, HashSet, or custom types).This section systematically checks for common collection interfaces, resolves existing instances, and gracefully instantiates standard collection types when needed. Including hashset comparers furthers configurability.
Please confirm that the
IsAssignableFromchecks produce the correct behavior in edge cases (e.g., unusual derived collection types). If in doubt, additional unit tests can help ensure no unintended matching occurs.src/NLog/Internal/ObjectReflectionCache.cs (6)
144-144: Consider verifying broader platform coverage.
This conditional now restricts certain dynamic features to .NET Framework only. If .NET Standard or .NET (Core) also support the relevant APIs, you may want to revisit the condition to ensure consistency across modern runtimes.
166-172: Implementation appears consistent with the caching design.
Storing a singlestring.Emptyproperty for dictionary enumeration is an interesting approach and likely intentional for the schema. No functional concerns noticed.
234-234: Direct binding flag usage is straightforward.
Usingtype.GetProperties(BindingFlags.Instance | BindingFlags.Public)is clear and should suffice for retrieving public instance properties.
539-539: Platform define check parallels the earlier condition.
As with line 144, verify that excluding other .NET runtimes is indeed desired.
591-603: Reflecting into generic dictionary interfaces can be costly.
Although this logic is sound, reflection or runtime interface checks may be expensive in high-throughput scenarios. If performance demands grow, consider caching interface lookups or employing a more direct approach.
664-683: Empty enumerator logic looks fine.
The implementation correctly returns false forMoveNext()and yields no items, aligning with typical “do-nothing” enumerator patterns.
| private sealed class DictionaryEnumerator : IDictionaryEnumerator | ||
| { | ||
| IEnumerator<KeyValuePair<string, object>> IDictionaryEnumerator.GetEnumerator(object value) | ||
| { | ||
| if (value is ICollection dictionary) | ||
| { | ||
| if (dictionary.Count > 0) | ||
| return YieldEnumerator(dictionary); | ||
| } | ||
| return EmptyDictionaryEnumerator.Default; | ||
| } | ||
|
|
||
| private static IEnumerator<KeyValuePair<string, object>> YieldEnumerator(ICollection dictionary) | ||
| { | ||
| foreach (DictionaryEntry item in dictionary) | ||
| yield return new KeyValuePair<string, object>(item.Key.ToString(), item.Value); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for null dictionary keys.
When enumerating a non-generic ICollection as a dictionary, calling item.Key.ToString() may throw if item.Key is null. A safe approach could convert the key to "null" rather than crash.
- yield return new KeyValuePair<string, object>(item.Key.ToString(), item.Value);
+ yield return new KeyValuePair<string, object>(item.Key?.ToString() ?? "null", item.Value);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private sealed class DictionaryEnumerator : IDictionaryEnumerator | |
| { | |
| IEnumerator<KeyValuePair<string, object>> IDictionaryEnumerator.GetEnumerator(object value) | |
| { | |
| if (value is ICollection dictionary) | |
| { | |
| if (dictionary.Count > 0) | |
| return YieldEnumerator(dictionary); | |
| } | |
| return EmptyDictionaryEnumerator.Default; | |
| } | |
| private static IEnumerator<KeyValuePair<string, object>> YieldEnumerator(ICollection dictionary) | |
| { | |
| foreach (DictionaryEntry item in dictionary) | |
| yield return new KeyValuePair<string, object>(item.Key.ToString(), item.Value); | |
| } | |
| } | |
| private sealed class DictionaryEnumerator : IDictionaryEnumerator | |
| { | |
| IEnumerator<KeyValuePair<string, object>> IDictionaryEnumerator.GetEnumerator(object value) | |
| { | |
| if (value is ICollection dictionary) | |
| { | |
| if (dictionary.Count > 0) | |
| return YieldEnumerator(dictionary); | |
| } | |
| return EmptyDictionaryEnumerator.Default; | |
| } | |
| private static IEnumerator<KeyValuePair<string, object>> YieldEnumerator(ICollection dictionary) | |
| { | |
| foreach (DictionaryEntry item in dictionary) | |
| yield return new KeyValuePair<string, object>(item.Key?.ToString() ?? "null", item.Value); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog/Internal/ObjectReflectionCache.cs (1)
166-173: Use a more descriptive property label for dictionary enumerators.
Using an empty string ("") as the property name could be confusing to consumers. Consider naming this property something like"DictionaryItems"for better readability.- propertyInfos = new ObjectPropertyInfos(null, new[] { new FastPropertyLookup(string.Empty, TypeCode.Object, (o, p) => dictionaryEnumerator.GetEnumerator(o)) }); + propertyInfos = new ObjectPropertyInfos(null, new[] { new FastPropertyLookup("DictionaryItems", TypeCode.Object, (o, p) => dictionaryEnumerator.GetEnumerator(o)) });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/NLog/Config/LoggingConfigurationFileLoader.cs(1 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/AssemblyHelpers.cs(1 hunks)src/NLog/Internal/ObjectReflectionCache.cs(6 hunks)src/NLog/Internal/PlatformDetector.cs(1 hunks)src/NLog/Internal/PropertyHelper.cs(4 hunks)src/NLog/LayoutRenderers/NLogDirLayoutRenderer.cs(1 hunks)src/NLog/Layouts/LayoutParser.cs(2 hunks)src/NLog/SetupInternalLoggerBuilderExtensions.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs(1 hunks)tests/TestTrimPublish/TestTrimPublish.csproj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs
- src/NLog/SetupInternalLoggerBuilderExtensions.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs
- src/NLog/Internal/PlatformDetector.cs
- src/NLog/Internal/AppEnvironmentWrapper.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs
- tests/TestTrimPublish/TestTrimPublish.csproj
- src/NLog/Config/LoggingConfigurationFileLoader.cs
- src/NLog/LayoutRenderers/NLogDirLayoutRenderer.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs
- src/NLog/Layouts/LayoutParser.cs
- src/NLog/Internal/PropertyHelper.cs
🔇 Additional comments (6)
src/NLog/Internal/AssemblyHelpers.cs (1)
44-49: Consider handling empty Location for single-file scenarios.
By always returning an empty string when the assembly location is null, any code depending on a valid file path may misbehave in single-file deployments or certain NuGet-cache scenarios. Consider logging and/or providing a fallback for enhanced transparency.src/NLog/Internal/ObjectReflectionCache.cs (5)
144-151: Verify the conditional compilation for DynamicObject usage.
The#if !NET35 && !NET40 && NETFRAMEWORKblock restrictsDynamicObjectsupport to .NET Framework builds only. If you intend to support .NET Core or .NET 5+ with dynamic objects, consider adjusting the conditional.
234-234: Confirm that inherited public properties are desired.
By usingBindingFlags.Instance | BindingFlags.Public, inherited public properties are included. This works well if you want to reflect all public instance properties from the entire inheritance chain. If you only want those declared directly on the type, considerBindingFlags.DeclaredOnly.
572-604: Potential AOT concerns with runtime-generated generic type.
The call toMakeGenericType(interfaceType.GetGenericArguments())may generate a runtime type, which might trigger AOT warnings in certain environments. If true AOT support is needed, consider providing a statically known fallback to avoid runtime code generation.
611-628: Check for null dictionary keys.
When enumerating a non-genericICollectionviaDictionaryEntry, callingitem.Key.ToString()will throw ifitem.Keyis null. Safely handle a null key to prevent exceptions.- yield return new KeyValuePair<string, object>(item.Key.ToString(), item.Value); + yield return new KeyValuePair<string, object>(item.Key?.ToString() ?? "null", item.Value);
662-683: Empty enumerator implementation looks correct.
This lightweight enumerator properly returns no items and avoids unnecessary overhead.
8763ab7 to
806be5f
Compare
|
Create new pull-request with Android-detection: |
974ec32 to
3f0beec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog/Internal/AssemblyHelpers.cs (1)
47-48: Good simplification of assembly location retrieval.The simplified approach effectively handles null assemblies and provides a consistent pattern for assembly location retrieval. The comment about NTFS hard-links adds important context about potential unexpected file locations.
However, consider adding XML documentation to explain the method's purpose and return value behavior:
/// <summary> /// Helpers for <see cref="Assembly"/>. /// </summary> internal static class AssemblyHelpers { [System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessage("Returns empty string for assemblies embedded in a single-file app", "IL3000")] + /// <summary> + /// Gets the file location of the specified assembly, returning empty string if location is unavailable. + /// </summary> + /// <param name="assembly">The assembly to get the location for. Can be null.</param> + /// <returns>The assembly's file location or empty string if assembly is null or location is unavailable.</returns> public static string GetAssemblyFileLocation(Assembly assembly) { // Notice assembly can be loaded from nuget-cache using NTFS-hard-link, and return unexpected file-location. return assembly?.Location ?? string.Empty; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/NLog/Config/AssemblyExtensionLoader.cs(1 hunks)src/NLog/Config/LoggingConfigurationFileLoader.cs(1 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/AssemblyHelpers.cs(1 hunks)src/NLog/Internal/ObjectReflectionCache.cs(7 hunks)src/NLog/Internal/PlatformDetector.cs(1 hunks)src/NLog/Internal/PropertyHelper.cs(4 hunks)src/NLog/LayoutRenderers/NLogDirLayoutRenderer.cs(1 hunks)src/NLog/Layouts/LayoutParser.cs(2 hunks)src/NLog/SetupInternalLoggerBuilderExtensions.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs(1 hunks)tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs(1 hunks)tests/TestTrimPublish/TestTrimPublish.csproj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs
- tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs
- src/NLog/Internal/PlatformDetector.cs
- src/NLog/SetupInternalLoggerBuilderExtensions.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs
- src/NLog/Config/AssemblyExtensionLoader.cs
- src/NLog/Config/LoggingConfigurationFileLoader.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs
- src/NLog/Internal/AppEnvironmentWrapper.cs
- src/NLog/Layouts/LayoutParser.cs
- src/NLog/Internal/ObjectReflectionCache.cs
- src/NLog/LayoutRenderers/NLogDirLayoutRenderer.cs
- tests/TestTrimPublish/TestTrimPublish.csproj
- src/NLog/Internal/PropertyHelper.cs
🔇 Additional comments (2)
tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs (1)
566-566: Preprocessor directive updated to limit dynamic tests to .NET Framework only.This change restricts the
SerializeExpandoObject_TestandSerializeDynamicObject_Testto run only on .NET Framework environments (4.5+) and excludes them from .NET Core/.NET 5+. This aligns with the PR's objective of fixing AOT warnings, as dynamic object serialization might encounter issues with Ahead-of-Time compilation used in modern .NET platforms.src/NLog/Internal/AssemblyHelpers.cs (1)
44-44: Appropriate AOT warning suppression.The added attribute properly suppresses IL3000 warnings for assemblies embedded in single-file applications, aligning with the PR objective to fix AOT warnings.
05b6d9b to
3559347
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/NLog/Internal/AssemblyHelpers.cs (2)
47-50: Handle unexpected empty or in-memory assembly locations.
Whenassembly?.Locationis empty, the method silently returns an empty string. Logging or additional handling might be useful to diagnose unexpected behavior such as memory-loaded assemblies.
65-80: Observe usage ofAssembly.CodeBasein .NET Framework.
Assembly.CodeBaseis deprecated in .NET 5+ and may cause AOT warnings. Given the continuing use here, ensure it’s strictly required for .NET Framework compatibility only.src/NLog/Internal/PropertyHelper.cs (3)
314-314: Catch potential exceptions fromActivator.CreateInstance.
Although it’s rare, reflection-based instantiation can fail, especially for unusual constructors. Consider adding logging or checks for missing constructors.
417-446: Ensure concurrency safety and robust exception handling.
TryCreateCollectionObjectuses reflection to build collections on-the-fly. Confirm no concurrency issues when multiple threads call this concurrently, and verify that exception paths are well tested.
542-559: Leverage unified approach to generic collection creation.
TryMakeGenericCollectionTypeis succinct. If additional collection types arise, consider a centralized table or extension method for easier maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/NLog/Config/AssemblyExtensionLoader.cs(1 hunks)src/NLog/Config/LoggingConfigurationFileLoader.cs(1 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/AssemblyHelpers.cs(2 hunks)src/NLog/Internal/ObjectReflectionCache.cs(7 hunks)src/NLog/Internal/PropertyHelper.cs(4 hunks)src/NLog/LayoutRenderers/NLogDirLayoutRenderer.cs(1 hunks)src/NLog/Layouts/LayoutParser.cs(2 hunks)src/NLog/SetupInternalLoggerBuilderExtensions.cs(1 hunks)src/NLog/Targets/DefaultJsonSerializer.cs(2 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs(1 hunks)tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs(1 hunks)tests/TestTrimPublish/TestTrimPublish.csproj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- src/NLog/SetupInternalLoggerBuilderExtensions.cs
- tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs
- tests/TestTrimPublish/TestTrimPublish.csproj
- tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs
- src/NLog/Internal/AppEnvironmentWrapper.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs
- src/NLog/Targets/DefaultJsonSerializer.cs
- src/NLog/Config/AssemblyExtensionLoader.cs
- src/NLog/Config/LoggingConfigurationFileLoader.cs
- src/NLog/LayoutRenderers/NLogDirLayoutRenderer.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs
- src/NLog/Layouts/LayoutParser.cs
- src/NLog/Internal/ObjectReflectionCache.cs
🔇 Additional comments (5)
src/NLog/Internal/AssemblyHelpers.cs (2)
44-44: Consider ensuring single-file scenarios are fully addressed.
TheUnconditionalSuppressMessageattribute signals potential concerns about single-file application embedding. Verify that returning an empty string for embedded assemblies is the desired outcome.
109-109: Conditional compilation block is consistent.
No further issues.src/NLog/Internal/PropertyHelper.cs (3)
302-303: Confirm reflection usage under trimming.
Applying[UnconditionalSuppressMessage]here indicates acceptance of reflection in a trimmed environment. Ensure all relevant constructors exist and won't break under AOT.
393-393: Validate parsing for multi-value lists.
Splitting comma-separated values is straightforward, but ensure corner cases (e.g. empty values, escaped commas) are handled as intended.
466-540: Testing edge cases for collection type resolution.
TryResolveCollectionTypecovers many specific types (List, HashSet, etc.). Confirm coverage for complex generic types or unexpected collection subtypes that may require fallback.
99a161c to
a7667ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/NLog/Internal/ObjectReflectionCache.cs (3)
687-690: Protect against null keys in reflected enumeration.Similar to the previous comment, the code should handle possible null keys when using reflection to enumerate dictionaries.
- var keyString = getKeyProperty.GetValue(item, null); - var valueObject = getValueProperty.GetValue(item, null); - yield return new KeyValuePair<string, object>(keyString.ToString(), valueObject); + var keyString = getKeyProperty.GetValue(item, null); + var valueObject = getValueProperty.GetValue(item, null); + yield return new KeyValuePair<string, object>(keyString?.ToString() ?? "null", valueObject);
716-716: Protect against null keys in generic dictionary handling.The same null key issue exists in the generic dictionary enumerator.
- yield return new KeyValuePair<string, object>(item.Key.ToString(), item.Value); + yield return new KeyValuePair<string, object>(item.Key?.ToString() ?? "null", item.Value);
723-723: Protect against null keys in read-only dictionary handling.The same null key issue exists in the read-only dictionary enumerator.
- yield return new KeyValuePair<string, object>(item.Key.ToString(), item.Value); + yield return new KeyValuePair<string, object>(item.Key?.ToString() ?? "null", item.Value);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/NLog/Config/AssemblyExtensionLoader.cs(1 hunks)src/NLog/Config/LoggingConfigurationFileLoader.cs(1 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/AssemblyHelpers.cs(2 hunks)src/NLog/Internal/ObjectReflectionCache.cs(7 hunks)src/NLog/Internal/PropertyHelper.cs(4 hunks)src/NLog/LayoutRenderers/NLogDirLayoutRenderer.cs(1 hunks)src/NLog/Layouts/LayoutParser.cs(2 hunks)src/NLog/SetupInternalLoggerBuilderExtensions.cs(1 hunks)src/NLog/Targets/DefaultJsonSerializer.cs(2 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs(1 hunks)tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs(1 hunks)tests/TestTrimPublish/TestTrimPublish.csproj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- src/NLog/SetupInternalLoggerBuilderExtensions.cs
- src/NLog/Config/AssemblyExtensionLoader.cs
- tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs
- src/NLog/LayoutRenderers/NLogDirLayoutRenderer.cs
- tests/TestTrimPublish/TestTrimPublish.csproj
- src/NLog/Config/LoggingConfigurationFileLoader.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs
- tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs
- src/NLog/Layouts/LayoutParser.cs
- src/NLog/Targets/DefaultJsonSerializer.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs
- src/NLog/Internal/AppEnvironmentWrapper.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs
- src/NLog/Internal/PropertyHelper.cs
🔇 Additional comments (12)
src/NLog/Internal/AssemblyHelpers.cs (4)
44-44: Good addition of suppression attribute for AOT compatibility.The attribute correctly suppresses the IL3000 warning which would be triggered in single-file applications where assemblies are embedded. This aligns with the PR objective of fixing AOT warnings.
47-49: Simplified implementation for modern platforms.The code now directly returns the assembly location with a null check for non-NETFRAMEWORK platforms, which is cleaner and more maintainable. The comment accurately explains why the location might be unexpected (NTFS hard-link to NuGet cache).
50-81: Preserved compatibility path for NETFRAMEWORK with improved location resolution.The NETFRAMEWORK path retains the necessary CodeBase logic which is specifically needed for older frameworks, while making a small improvement on line 80 to better handle assembly file name resolution.
47-110:Details
✅ Verification successful
Verify that this change doesn't affect logging behavior across platforms.
The simplification for non-NETFRAMEWORK platforms removes extensive error handling and logging that was present in the original code. Should confirm this doesn't impact diagnostics capabilities when location resolution fails.
🏁 Script executed:
#!/bin/bash # Check if there are any other callers that might depend on detailed error logging rg -A 3 "GetAssemblyFileLocation" --type csLength of output: 2628
Action: Confirming that the simplified non‑NETFRAMEWORK code meets diagnostic needs
After reviewing the call sites (e.g., in LoggingConfigurationFileLoader.cs, AssemblyExtensionLoader.cs, and related files), it appears that all usages of
GetAssemblyFileLocationsimply check for a non-empty string rather than relying on the detailed logging from the original error handling. This suggests that for non‑NETFRAMEWORK targets, the intentional simplification (i.e., returningassembly?.Location ?? string.Empty) does not adversely affect diagnostic capabilities—even if it omits extensive logging.The change is acceptable provided that any critical errors in assembly location resolution in non‑NETFRAMEWORK scenarios can be detected by the non-empty-check performed by the callers.
src/NLog/Internal/ObjectReflectionCache.cs (8)
40-42: Updated preprocessor directive to properly target platforms.The preprocessor directive now includes
NETFRAMEWORKalong with!NET35 && !NET40, ensuring DynamicObject support is only included where appropriate.
166-173: Improved dictionary handling with new helper method.The updated implementation simplifies dictionary detection by using the new
TryGetDictionaryEnumeratormethod, which centralizes the logic for detecting and handling different dictionary types.
234-234: Simplified binding flags usage in GetPublicProperties.The code now directly uses binding flags instead of a constant, making the code more readable and maintainable.
571-596: Well-structured helper for dictionary enumeration.The new
TryGetDictionaryEnumeratormethod provides a clean way to handle various dictionary types, with appropriate null checks and type validation. The implementation supports both generic and non-generic dictionaries, as well as read-only dictionaries.
628-692: Complete DictionaryEnumerator implementation for AOT compatibility.The new
DictionaryEnumeratorclass provides a non-generic alternative that can handle both generic and non-generic dictionaries through reflection. The implementation includes effective fallbacks and handles edge cases.
649-649: Protect against null dictionary keys.When enumerating a dictionary, calling
item.Key.ToString()may throw an exception ifitem.Keyis null. Consider handling this case to prevent runtime exceptions.- yield return new KeyValuePair<string, object>(item.Key.ToString(), item.Value); + yield return new KeyValuePair<string, object>(item.Key?.ToString() ?? "null", item.Value);
728-747: Well-structured empty enumerator implementation.The
EmptyDictionaryEnumeratorclass provides a clean singleton pattern for representing an empty dictionary enumeration, ensuring consistent behavior across the codebase.
571-747:Details
❓ Verification inconclusive
Verify that this dictionary handling works with real-world dictionaries.
The changes to dictionary handling are significant. It would be valuable to verify that they work correctly with various dictionary types in real applications.
🏁 Script executed:
#!/bin/bash # Check for tests that might verify dictionary handling rg -A 5 "TryLookupExpandoObject|TryGetDictionaryEnumerator" --type cs --glob "**/*Test*.cs"Length of output: 93
Action: Manually Verify Real-World Dictionary Handling
The changes to the dictionary handling logic are substantial. Our automated search did not turn up any tests that specifically exercise this functionality. Please manually verify that the new implementation works correctly with typical dictionary types (e.g., IDictionary<string, object>, IDictionary<string, string>, and on non‑NET35 targets, IReadOnlyDictionary variants). It would also be beneficial to add dedicated tests to cover these use cases in real-world scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/NLog/Internal/ObjectReflectionCache.cs (4)
647-650: Ensure null dictionary keys are handled safely.When enumerating a dictionary and calling
ToString()on keys, ensure that null keys are handled appropriately.- yield return new KeyValuePair<string, object>(item.Key.ToString(), item.Value); + yield return new KeyValuePair<string, object>(item.Key?.ToString() ?? "null", item.Value);
686-690: Add null check for key value.Similar to the earlier suggestion, add a null check when converting key values to strings to avoid potential NullReferenceExceptions.
- yield return new KeyValuePair<string, object>(keyString.ToString(), valueObject); + yield return new KeyValuePair<string, object>(keyString?.ToString() ?? "null", valueObject);
715-717: Consider null-safe key conversion.In the YieldEnumerator method, add null checking for keys to prevent NullReferenceException.
- yield return new KeyValuePair<string, object>(item.Key.ToString(), item.Value); + yield return new KeyValuePair<string, object>(item.Key?.ToString() ?? "null", item.Value);
722-724: Add null check for dictionary keys.Similar to previous suggestions, add null checking for keys in the IReadOnlyDictionary enumerator.
- yield return new KeyValuePair<string, object>(item.Key.ToString(), item.Value); + yield return new KeyValuePair<string, object>(item.Key?.ToString() ?? "null", item.Value);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/NLog/Config/AssemblyExtensionLoader.cs(1 hunks)src/NLog/Config/LoggingConfigurationFileLoader.cs(1 hunks)src/NLog/Internal/AppEnvironmentWrapper.cs(1 hunks)src/NLog/Internal/AssemblyHelpers.cs(2 hunks)src/NLog/Internal/ObjectReflectionCache.cs(7 hunks)src/NLog/Internal/PropertyHelper.cs(4 hunks)src/NLog/LayoutRenderers/NLogDirLayoutRenderer.cs(1 hunks)src/NLog/Layouts/LayoutParser.cs(2 hunks)src/NLog/SetupInternalLoggerBuilderExtensions.cs(1 hunks)src/NLog/Targets/DefaultJsonSerializer.cs(2 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs(1 hunks)tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs(1 hunks)tests/NLog.UnitTests/Targets/TargetTests.cs(3 hunks)tests/TestTrimPublish/TestTrimPublish.csproj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- tests/NLog.UnitTests/LayoutRenderers/StackTraceRendererTests.cs
- src/NLog/SetupInternalLoggerBuilderExtensions.cs
- tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs
- src/NLog/Internal/AppEnvironmentWrapper.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteLineNumberTests.cs
- src/NLog/Targets/DefaultJsonSerializer.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteFileNameLayoutTests.cs
- tests/TestTrimPublish/TestTrimPublish.csproj
- src/NLog/LayoutRenderers/NLogDirLayoutRenderer.cs
- src/NLog/Config/AssemblyExtensionLoader.cs
- src/NLog/Layouts/LayoutParser.cs
- tests/NLog.UnitTests/LayoutRenderers/CallSiteTests.cs
- src/NLog/Config/LoggingConfigurationFileLoader.cs
- src/NLog/Internal/PropertyHelper.cs
🔇 Additional comments (12)
src/NLog/Internal/AssemblyHelpers.cs (4)
44-44: Good use of suppression attribute for AOT compatibility.The
UnconditionalSuppressMessageattribute correctly addresses IL3000 warnings when assemblies are embedded in single-file apps. This is important for AOT scenarios.
47-49: Excellent AOT compatibility improvement.This simplification for non-NETFRAMEWORK platforms eliminates the use of
CodeBasewhich is problematic in AOT scenarios. Usingassembly?.Locationdirectly is cleaner and more compatible with modern deployment models.
51-82: Good legacy support for NETFRAMEWORK.Maintaining the more complex implementation for NETFRAMEWORK is appropriate since older applications might rely on this behavior. The code properly handles null assemblies by returning an empty string.
80-81: Good fallback mechanism for location determination.The code intelligently chooses between assembly location sources, using either
CodeBaseorLocationbased on availability. This provides reliable location detection across different runtime environments.tests/NLog.UnitTests/Targets/TargetTests.cs (2)
1035-1036: Well-structured property additions for list types.The new properties are properly defined with appropriate types. Note that
ShortListPropertydoesn't have an initializer whileLongListPropertyis initialized with a new List, which is a good practice to avoid null reference exceptions.
816-823: Comprehensive test coverage for new list properties.The tests thoroughly verify the list properties, checking both the count and the specific values contained in each list. This ensures the properties work correctly with the XML configuration system.
src/NLog/Internal/ObjectReflectionCache.cs (6)
40-40: Updated preprocessor directive to be more specific.The preprocessing directive has been improved to target NETFRAMEWORK specifically in combination with !NET35 && !NET40. This ensures that the DynamicObject handling code is only included where appropriate.
166-173: Simplified dictionary handling with new helper method.The implementation now uses the new
TryGetDictionaryEnumeratormethod to check if a value is a dictionary, which makes the code more maintainable and easier to read. The property info creation is also streamlined.
234-234: Simplified property retrieval.Removed the use of intermediate constant and directly specified binding flags, making the code cleaner and more straightforward.
571-596: Well-structured dictionary type detection.The new method provides a comprehensive check for various dictionary types with string keys. The early return for non-enumerable types or strings improves performance.
611-616: Critical AOT compatibility fix.The code now properly handles AOT compilation by avoiding
MakeGenericTypefor non-NETFRAMEWORK platforms. This is essential for AOT scenarios as indicated by the comment "AOT does not support creating new types at runtime".
628-692: Improved dictionary enumeration with enhanced flexibility.The refactored
DictionaryEnumeratorclass now handles both generic and non-generic dictionaries more effectively. The implementation uses reflection intelligently to handle various collection types, improving compatibility.
|
Stop using obsolete Assembly-CodeBase (original source-location), but instead always using Assembly-Location (can be nuget-cache-folder, because of NTFS-links)