Support nullable where TKey : notnull for dictionary#6068
Conversation
WalkthroughAdds conditional non-null generic constraints on several internal collections, tightens null handling for layout and internal logging, and updates some build-condition symbols from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/NLog/Logger-V1Compat.cs (1)
2043-2055: Fix typo in preprocessor symbol:NETNET9_0_OR_GREATERshould beNET9_0_OR_GREATERAt the
Info(IFormatProvider? formatProvider, string message, string? argument)overload, the#ifdirective contains a typo:#if NETSTANDARD2_1_OR_GREATER || NETNET9_0_OR_GREATER
NETNET9_0_OR_GREATERis not a valid SDK-defined symbol and should beNET9_0_OR_GREATERto match all other occurrences in the codebase.-#if NETSTANDARD2_1_OR_GREATER || NETNET9_0_OR_GREATER +#if NETSTANDARD2_1_OR_GREATER || NET9_0_OR_GREATER [OverloadResolutionPriority(-1)] #endifsrc/NLog/Logger.cs (1)
571-604: Removeparamsfrom ReadOnlySpan overloads – type mismatch with WriteToTargetsWithSpanBoth new overloads declare
argsasparams ReadOnlySpan<object?>, which makes the parameter typeReadOnlySpan<object?>[](array of spans). However, they passargsdirectly toWriteToTargetsWithSpan, which expects a singlein ReadOnlySpan<object?>:private void WriteToTargetsWithSpan(..., string message, in ReadOnlySpan<object?> args)This is a compile-time type mismatch. Remove
paramsfrom both overloads (lines 579 and 596) to make the parameter a single span:- public void Log(LogLevel level, [Localizable(false)][StructuredMessageTemplate] string message, params ReadOnlySpan<object?> args) + public void Log(LogLevel level, [Localizable(false)][StructuredMessageTemplate] string message, ReadOnlySpan<object?> args)and
- public void Log(LogLevel level, Exception? exception, [Localizable(false)][StructuredMessageTemplate] string message, params ReadOnlySpan<object?> args) + public void Log(LogLevel level, Exception? exception, [Localizable(false)][StructuredMessageTemplate] string message, ReadOnlySpan<object?> args)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/NLog/Common/InternalLogger.cssrc/NLog/Internal/MruCache.cssrc/NLog/Internal/SortHelpers.cssrc/NLog/Internal/StringBuilderExt.cssrc/NLog/Internal/ThreadSafeDictionary.cssrc/NLog/Layouts/Typed/Layout.cssrc/NLog/Logger-V1Compat.cssrc/NLog/Logger.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T01:22:52.437Z
Learnt from: thatrajeevkr
Repo: NLog/NLog PR: 5999
File: src/NLog/Layouts/JSON/JsonLayout.cs:162-166
Timestamp: 2025-09-12T01:22:52.437Z
Learning: When suggesting StringBuilder optimizations for NLog layouts marked as [ThreadAgnostic], avoid shared StringBuilder fields as they create thread safety issues. Prefer simple string operations or ThreadLocal<StringBuilder> if allocation profiling shows a real performance impact.
Applied to files:
src/NLog/Internal/StringBuilderExt.cs
🧬 Code graph analysis (1)
src/NLog/Layouts/Typed/Layout.cs (2)
src/NLog/Layouts/Typed/ValueTypeLayoutInfo.cs (2)
ToString(250-253)ToString(272-275)src/NLog/Targets/TargetWithContext.cs (1)
ToString(727-730)
🔇 Additional comments (7)
src/NLog/Common/InternalLogger.cs (1)
495-496: LGTM! Good defensive guard.This early return prevents file operations when
_logFileis null or empty, avoiding unnecessary work (string construction, lock acquisition) and potential exceptions. While the subscription logic (line 133-134) already guards against null/empty paths, this additional check protects against edge cases such as race conditions or path expansion returning an invalid result.src/NLog/Internal/SortHelpers.cs (1)
145-161: Private helper constraint aligns correctly with dictionary usageConditionally adding
where TKey : notnullonCreateBucketDictionaryWithValueunderNETSTANDARD2_1_OR_GREATER || NETis consistent with the rest of the file’s bucket APIs and BCLDictionary<TKey, TValue>expectations; no issues here.src/NLog/Internal/StringBuilderExt.cs (1)
415-441: NET9_0_OR_GREATER extension of the optimized path looks correctSwitching the 4-digit zero-padding optimization to
#if NETSTANDARD2_1_OR_GREATER || NET9_0_OR_GREATERpreserves existing behavior on older TFMs and simply enables the singleAppendpath on NET 9+; the conditional structure stays well-formed.src/NLog/Logger-V1Compat.cs (1)
2747-2752: Warn(float, IFormatProvider) overload attribute condition is consistentThe added
OverloadResolutionPriority(-1)behind#if NETSTANDARD2_1_OR_GREATER || NET9_0_OR_GREATERforWarn(IFormatProvider? formatProvider, string message, float argument)matches the pattern used for other numeric overloads and looks correct.src/NLog/Internal/ThreadSafeDictionary.cs (1)
41-45: Internal dictionary constraint tightening is reasonableAdding
where TKey : notnullforThreadSafeDictionary<TKey, TValue>underNETSTANDARD2_1_OR_GREATER || NETbrings its key constraint in line withDictionary<TKey, TValue>on newer TFMs and matches how keys are already used internally; looks good as long as call sites are updated to use non-nullable key types where necessary.src/NLog/Layouts/Typed/Layout.cs (1)
376-376: LGTM! Defensive null-safety improvement consistent with codebase patterns.The addition of null-coalescing operators ensures
ToString()never returns null, aligning with similar patterns inValueTypeLayoutInfoandTargetWithContext. While the underlying implementations of_layoutValuetypes (LayoutTypeValueandFuncMethodValue) already include fallbacks toGetType().ToString(), this explicit guard is good defensive programming.src/NLog/Internal/MruCache.cs (1)
42-44: LGTM! Appropriate compile-time constraint for dictionary key safety.The conditional
notnullconstraint onTKeyimproves type safety by enforcing at compile-time that dictionary keys cannot be nullable reference types. The conditionNETSTANDARD2_1_OR_GREATER || NETis correct, as thenotnullconstraint is available in C# 8.0+ (supported by .NET Standard 2.1 and all .NET Core 3.0+ / .NET 5+ versions).Since this is an internal class, there's no public API impact, and the constraint aligns with dictionary best practices (null keys would fail at runtime anyway).
snakefoot
force-pushed
the
net8_fixes
branch
from
0dd369a to
8fa8885
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog/Common/InternalLogger.cs (1)
493-507: Make_logFilecheck and usage consistent under concurrency by using a localThe new null/empty guard is good, but
_logFilecan still change between the check andFile.AppendText(_logFile)ifLogFileis modified concurrently. Capturing it once avoids a stale read and makes the guard fully effective.Proposed refinement
- if (string.IsNullOrEmpty(_logFile)) - return; + var logFile = _logFile; + if (string.IsNullOrEmpty(logFile)) + return; @@ - using (var textWriter = File.AppendText(_logFile)) + using (var textWriter = File.AppendText(logFile))
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/NLog/Common/InternalLogger.cssrc/NLog/Internal/MruCache.cssrc/NLog/Internal/SortHelpers.cssrc/NLog/Internal/StringBuilderExt.cssrc/NLog/Internal/ThreadSafeDictionary.cssrc/NLog/Layouts/Typed/Layout.cssrc/NLog/Logger-V1Compat.cssrc/NLog/Logger.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/NLog/Internal/SortHelpers.cs
- src/NLog/Internal/ThreadSafeDictionary.cs
- src/NLog/Logger.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T01:22:52.437Z
Learnt from: thatrajeevkr
Repo: NLog/NLog PR: 5999
File: src/NLog/Layouts/JSON/JsonLayout.cs:162-166
Timestamp: 2025-09-12T01:22:52.437Z
Learning: When suggesting StringBuilder optimizations for NLog layouts marked as [ThreadAgnostic], avoid shared StringBuilder fields as they create thread safety issues. Prefer simple string operations or ThreadLocal<StringBuilder> if allocation profiling shows a real performance impact.
Applied to files:
src/NLog/Internal/StringBuilderExt.cssrc/NLog/Layouts/Typed/Layout.cs
🧬 Code graph analysis (1)
src/NLog/Layouts/Typed/Layout.cs (1)
src/NLog/Layouts/Typed/ValueTypeLayoutInfo.cs (2)
ToString(250-253)ToString(272-275)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (5)
src/NLog/Layouts/Typed/Layout.cs (1)
373-377: Non-nullToString()result for both fixed and dynamic layoutsReturning
"null"whenFixedObjectValueor_layoutValue.ToString()is null makesLayout<T>.ToString()consistently non-null and aligns with similar patterns (e.g.,ValueTypeLayoutInfo). Looks good.src/NLog/Internal/StringBuilderExt.cs (1)
415-441: Preprocessor guard for 4-digit append optimization looks consistentThe
Append4DigitsZeroPaddedconditional now enables the single-Append path only forNETSTANDARD2_1_OR_GREATERandNET9_0_OR_GREATER, falling back to the per-digit appends otherwise. This is behaviorally safe and consistent with the PR’s target-conditional pattern.src/NLog/Logger-V1Compat.cs (2)
2041-2056: OverloadResolutionPriority condition aligned forInfo(IFormatProvider, string, string?)The conditional around
[OverloadResolutionPriority(-1)]now matches the other Info overloads, ensuring consistent overload resolution behavior on the intended targets. No further changes needed.
2747-2758: OverloadResolutionPriority condition aligned forWarn(IFormatProvider, string, float)Same here: the preprocessor condition for applying
[OverloadResolutionPriority(-1)]is now consistent with neighboring Warn overloads, fixing the prior symbol typo.src/NLog/Internal/MruCache.cs (1)
41-58: Conditionalwhere TKey : notnullonMruCacheis appropriateAdding
where TKey : notnullfor newer targets tightens key nullability in line withDictionary<TKey, TValue>expectations, without affecting runtime behavior on older TFMs. Looks good.
|
1 participant
No description provided.