Fix: Improve nullable operator handling for dictionary keys (#523)

* Improve nullable operator handling for dictionary keys

- Enhances SmartFormat's support for nullable operators ('?') in dictionary formatting.
- Refactors `DictionarySource` to detect dictionary types and handle missing keys gracefully, when the nullable operator is used in the selector of a format string.
- Adds tests to verify correct behavior for missing keys and improper nullable operator placement.
- Updates `Source.HasNullableOperator` logic: Returns true, if the current `Selector` or any `Selector` before contains the nullable operator.

Resolves #522

* Optimize IReadOnlyDictionary check by caching

- Introduced a thread-safe cache to IsIReadOnlyDictionary for improved performance and reduced allocations.
- Added tests for nullable operator with enum-keyed dictionaries to ensure missing keys return empty values.

Author: axunonb

src/SmartFormat.Tests/Extensions/DictionarySourceTests.cs

@@ -2,8 +2,8 @@
 using System.Collections;
 using System.Collections.Generic;
 using System.Dynamic;
-using System.Linq;
 using NUnit.Framework;
+using SmartFormat.Core.Formatting;
 using SmartFormat.Core.Settings;
 using SmartFormat.Extensions;
 using SmartFormat.Tests.TestUtils;
@@ -159,6 +159,98 @@ public void Dictionary_Dot_Notation_Nullable()
         Assert.That(result, Is.EqualTo(expected));
     }
 
+    [Test]
+    public void Dictionary_NullableOperator_MissingKey_ReturnsEmpty()
+    {
+        // Test similar to GitHub issue #522, but using string keys
+        var sf = Smart.CreateDefaultSmartFormat();
+
+        var obj = new {
+            Changes = new Dictionary<string, object>
+            {
+                { "Count", 100 },
+                // Volume is intentionally missing
+                // although it is accessed in the format string
+                { "Name", "ABCD" },
+            }
+        };
+
+        // Use nullable syntax to avoid exceptions for selectors
+        // that cannot be resolved with the DictionarySource:
+        // 'Volume' may be missing, but 'Changes' is expected to be present
+        var resultSyntax1 =
+            sf.Format("{Changes:{Count};{?.Volume};{Name}}", obj);
+        var resultSyntax2 =
+            sf.Format("{Changes.Count};{Changes?.Volume};{Changes.Name}", obj);
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(resultSyntax1, Is.EqualTo("100;;ABCD"));
+            Assert.That(resultSyntax2, Is.EqualTo("100;;ABCD"));
+        });
+
+        // Now add the missing key and verify that it is properly resolved
+        obj.Changes.Add("Volume", 200);
+        Assert.That(sf.Format("{Changes.Count};{Changes?.Volume};{Changes.Name}", obj),
+            Is.EqualTo("100;200;ABCD"));
+    }
+
+    private enum ChangeKey { Count, Volume, Name }
+
+    [Test]
+    public void Dictionary_NullableOperator_MissingEnumKey_ReturnsEmpty()
+    {
+        // Test for GitHub issue #522 with non-string dictionary keys
+        var sf = Smart.CreateDefaultSmartFormat();
+        var obj = new {
+            Changes = new Dictionary<ChangeKey, object>
+            {
+                { ChangeKey.Count, 100 },
+                // Volume is intentionally missing
+                // although it is accessed in the format string
+                { ChangeKey.Name, "ABCD" },
+            }
+        };
+        // Use nullable syntax to avoid exceptions for selectors
+        // that cannot be resolved with the DictionarySource:
+        // 'Volume' may be missing, but 'Changes' is expected to be present
+        var result =
+            sf.Format("{Changes.Count};{Changes?.Volume};{Changes.Name}", obj);
+        Assert.Multiple(() =>
+        {
+            Assert.That(result, Is.EqualTo("100;;ABCD"));
+        });
+
+        // Now add the missing key and verify that it is properly resolved
+        obj.Changes.Add(ChangeKey.Volume, 200);
+        Assert.That(sf.Format("{Changes.Count};{Changes?.Volume};{Changes.Name}", obj),
+            Is.EqualTo("100;200;ABCD"));
+    }
+
+    [Test]
+    public void Dictionary_NullableOperator_MissingKey_OnWrongSelector()
+    {
+        // Test for GitHub issue #522
+        var sf = Smart.CreateDefaultSmartFormat();
+
+        var obj = new { Changes = new Dictionary<string, object>
+            {
+                { "Count", 100 },
+                // Volume is intentionally missing
+                // although it is accessed in the format string
+                { "Name", "ABCD" },
+            }
+        };
+
+        // The nullable operator must only be applied
+        // to the selector that is expected to be missing or
+        // a preceding selector in the chain that is expected to be missing
+        // Here: 'Changes' is expected to be present, but 'Volume' is missing,
+        // so the nullable operator must be applied to 'Volume' instead of after 'Volume'
+        Assert.Throws<FormattingException>(() =>
+            sf.Format("{Changes.Count};{Changes.Volume?.Anything};{Changes.Name}", obj));
+    }
+
     [Test]
     public void Generic_Dictionary_String_String()
     {
@@ -195,28 +287,6 @@ public void IReadOnlyDictionary_With_String_Key()
         Assert.That(result, Is.EqualTo("12three"));
     }
 
-    [Test]
-    public void IReadOnlyDictionary_Cache_Should_Store_Types_It_Cannot_Handle()
-    {
-        var dictSource = new DictionarySource { IsIReadOnlyDictionarySupported = true };
-        var kvp = new KeyValuePair<string, object?>("One", 1);
-        var smart = new SmartFormatter()
-            .AddExtensions(new DefaultSource(), dictSource, new KeyValuePairSource())
-            .AddExtensions(new DefaultFormatter());
-        var result = smart.Format("{One}", kvp);
-
-        Assert.Multiple(() =>
-        {
-            Assert.That(result, Is.EqualTo("1"));
-            Assert.That(dictSource.RoDictionaryTypeCache.Keys, Has.Count.EqualTo(1));
-        });
-        Assert.Multiple(() =>
-        {
-            Assert.That(dictSource.RoDictionaryTypeCache.Keys.First(), Is.EqualTo(typeof(KeyValuePair<string, object?>)));
-            Assert.That(dictSource.RoDictionaryTypeCache.Values.First(), Is.Null);
-        });
-    }
-
     public class CustomReadOnlyDictionary<TKey, TValue> : IReadOnlyDictionary<TKey, TValue?>
     {
         private readonly IDictionary<TKey, TValue?> _dictionary;

src/SmartFormat/Core/Extensions/Source.cs

@@ -33,23 +33,31 @@ public virtual void Initialize(SmartFormatter smartFormatter)
     }
 
     /// <summary>
-    /// Checks if any of the <see cref="Placeholder"/>'s <see cref="Placeholder.Selectors"/> has nullable <c>?</c> as their first operator.
+    /// Checks whether any of the <see cref="Placeholder"/>'s <see cref="Placeholder.Selectors"/>,
+    /// up to and including the selector at <see cref="ISelectorInfo.SelectorIndex"/>,
+    /// has the nullable <c>?</c> as the first character of its operator.
     /// </summary>
-    /// <param name="selectorInfo"></param>
+    /// <param name="selectorInfo">The <see cref="ISelectorInfo"/> for the selector currently being evaluated.</param>
     /// <returns>
-    /// <see langword="true"/>, any of the <see cref="Placeholder"/>'s <see cref="Placeholder.Selectors"/> has nullable <c>?</c> as their first operator.
+    /// <see langword="true"/> if any <see cref="Placeholder.Selectors"/> with a
+    /// <see cref="Parsing.Selector.SelectorIndex"/> less than or equal to <see cref="ISelectorInfo.SelectorIndex"/>
+    /// has the nullable <c>?</c> as the first character of its operator; otherwise <see langword="false"/>.
     /// </returns>
     /// <remarks>
-    /// The nullable operator '?' can be followed by a dot (like '?.') or a square brace (like '.[')
+    /// Only selectors up to the current one are considered, so a nullable operator on a later selector
+    /// in the same <see cref="Placeholder"/> does not influence the evaluation of earlier selectors.
+    /// The nullable operator <c>?</c> must be followed by a dot (e.g. <c>?.</c>) or a square bracket (e.g. <c>?[</c>).
     /// </remarks>
-    private bool HasNullableOperator(ISelectorInfo selectorInfo)
+    protected virtual bool HasNullableOperator(ISelectorInfo selectorInfo)
     {
         if (_smartSettings != null && selectorInfo.Placeholder != null)
         {
 #pragma warning disable S3267 // Don't use LINQ in favor of less GC
             foreach (var s in selectorInfo.Placeholder.Selectors)
             {
-                if (s.OperatorLength > 1 && s.BaseString[s.OperatorStartIndex] == ParserSettings.NullableOperator)
+                if (s.SelectorIndex <= selectorInfo.SelectorIndex
+                    && s.OperatorLength > 1
+                    && s.BaseString[s.OperatorStartIndex] == ParserSettings.NullableOperator)
                     return true;
             }
 #pragma warning restore S3267 // Restore: Loops should be simplified with "LINQ" expressions

src/SmartFormat/Extensions/DictionarySource.cs

@@ -4,6 +4,7 @@
 
 using System;
 using System.Collections;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Reflection;
 using SmartFormat.Core.Extensions;
@@ -27,33 +28,46 @@ public override bool TryEvaluateSelector(ISelectorInfo selectorInfo)
     {
         var current = selectorInfo.CurrentValue;
         if (TrySetResultForNullableOperator(selectorInfo)) return true;
-            
-        if (current is null) return false;
+
+        if (current == null) return false;
+        var dictionaryType = GetDictionaryType(current);
+        if (dictionaryType == DictionaryType.None) return false;
 
         var selector = selectorInfo.SelectorText;
         var comparison = selectorInfo.FormatDetails.Settings.GetCaseSensitivityComparison();
 
         // Try to get the selector value for IDictionary
-        if (TryGetIDictionaryValue(current, selector, comparison, out var value))
+        if (dictionaryType == DictionaryType.NonGeneric
+            && TryGetIDictionaryValue(current, selector, comparison, out var value))
         {
             selectorInfo.Result = value;
             return true;
         }
 
         // Try to get the selector value for Dictionary<,> and dynamics (ExpandoObject)
-        if (TryGetGenericDictionaryValue(current, selector, comparison, out value))
+        if (dictionaryType == DictionaryType.Generic
+            && TryGetGenericDictionaryValue(current, selector, comparison, out value))
         {
             selectorInfo.Result = value;
             return true;
         }
 
         // Try to get the selector value for IReadOnlyDictionary<,>
-        if (IsIReadOnlyDictionarySupported && TryGetReadOnlyDictionaryValue(current, selector,
-                comparison, out value))
+        if (dictionaryType == DictionaryType.ReadOnly && IsIReadOnlyDictionarySupported
+            && TryGetReadOnlyDictionaryValue(current, selector, comparison, out value))
         {
             selectorInfo.Result = value;
             return true;
         }
+
+        // No matching key found in a dictionary, but if the selector
+        // has a nullable operator, we set the result to null
+        // instead of leaving it as not found.
+        if (HasNullableOperator(selectorInfo))
+        {
+            selectorInfo.Result = null;
+            return true;
+        }
 
         return false;
     }
@@ -144,13 +158,6 @@ private bool TryGetDictionaryProperties(Type type, out (PropertyInfo KeyProperty
         if (RoDictionaryTypeCache.TryGetValue(type, out propertyTuple))
             return propertyTuple != null;
 
-        if (!IsIReadOnlyDictionary(type))
-        {
-            // don't check the type again, although it's not a IReadOnlyDictionary
-            RoDictionaryTypeCache[type] = null;
-            return false;
-        }
-
         // get Key and Item properties of the dictionary
         propertyTuple = (type.GetProperty(nameof(IDictionary.Keys)), type.GetProperty("Item"))!;
 
@@ -160,19 +167,45 @@ private bool TryGetDictionaryProperties(Type type, out (PropertyInfo KeyProperty
         return true;
     }
 
+    private static readonly ConcurrentDictionary<Type, bool> RoDictionaryTypeBoolCache = new();
+
     private static bool IsIReadOnlyDictionary(Type type)
     {
+        if (RoDictionaryTypeBoolCache.TryGetValue(type, out var cached))
+            return cached;
+
         // No Linq for less garbage
+        var result = false;
         foreach (var typeInterface in type.GetInterfaces())
         {
             if (typeInterface == typeof(IReadOnlyDictionary<,>) ||
                 (typeInterface.IsGenericType
                  && typeInterface.GetGenericTypeDefinition() == typeof(IReadOnlyDictionary<,>)))
-                return true;
+            {
+                result = true;
+                break;
+            }
         }
 
-        return false;
+        RoDictionaryTypeBoolCache[type] = result;
+        return result;
     }
 
     #endregion
+
+    internal enum DictionaryType
+    {
+        None,
+        NonGeneric,
+        Generic,
+        ReadOnly
+    }
+
+    private DictionaryType GetDictionaryType(object current)
+    {
+        if (current is IDictionary) return DictionaryType.NonGeneric;
+        if (current is IDictionary<string, object?>) return DictionaryType.Generic;
+        if (IsIReadOnlyDictionarySupported && IsIReadOnlyDictionary(current.GetType())) return DictionaryType.ReadOnly;
+        return DictionaryType.None;
+    }
 }