Use Dictionary for underlying cache of ResourceSet

src/libraries/System.Private.CoreLib/src/System/Resources/ResourceSet.cs

@@ -1,18 +1,8 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-/*============================================================
-**
-**
-**
-**
-**
-** Purpose: Culture-specific collection of resources.
-**
-**
-===========================================================*/
-
 using System.Collections;
+using System.Collections.Generic;
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.IO;
@@ -29,15 +19,15 @@ namespace System.Resources
     public class ResourceSet : IDisposable, IEnumerable
     {
         protected IResourceReader Reader = null!;
-        internal Hashtable? Table;
 
-        private Hashtable? _caseInsensitiveTable;  // For case-insensitive lookups.
+        private Dictionary<string, object?>? _table;
+        private Dictionary<string, object?>? _caseInsensitiveTable;  // For case-insensitive lookups.
 
         protected ResourceSet()
         {
             // To not inconvenience people subclassing us, we should allocate a new
             // hashtable here just so that Table is set to something.
-            Table = new Hashtable();
+            _table = new Dictionary<string, object?>();
         }
 
         // For RuntimeResourceSet, ignore the Table parameter - it's a wasted
@@ -96,7 +86,7 @@ protected virtual void Dispose(bool disposing)
             }
             Reader = null!;
             _caseInsensitiveTable = null;
-            Table = null;
+            _table = null;
         }
 
         public void Dispose()
@@ -134,59 +124,46 @@ IEnumerator IEnumerable.GetEnumerator()
 
         private IDictionaryEnumerator GetEnumeratorHelper()
         {
-            Hashtable? copyOfTable = Table;  // Avoid a race with Dispose
-            if (copyOfTable == null)
-                throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet);
-            return copyOfTable.GetEnumerator();
+            // Avoid a race with Dispose
+            return _table?.GetEnumerator() ?? throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet);
         }
 
         // Look up a string value for a resource given its name.
         //
         public virtual string? GetString(string name)
         {
             object? obj = GetObjectInternal(name);
-            try
-            {
-                return (string?)obj;
-            }
-            catch (InvalidCastException)
-            {
-                throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name));
-            }
+            if (obj is string s)
+                return s;
+
+            if (obj is null)
+                return null;
+
+            throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name));
         }
 
         public virtual string? GetString(string name, bool ignoreCase)
         {
-            object? obj;
-            string? s;
-
             // Case-sensitive lookup
-            obj = GetObjectInternal(name);
-            try
-            {
-                s = (string?)obj;
-            }
-            catch (InvalidCastException)
-            {
+            object? obj = GetObjectInternal(name);
+            if (obj is string s)
+                return s;
+
+            if (obj is not null)
                 throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name));
-            }
 
-            // case-sensitive lookup succeeded
-            if (s != null || !ignoreCase)
-            {
-                return s;
-            }
+            if (!ignoreCase)
+                return null;
 
             // Try doing a case-insensitive lookup
             obj = GetCaseInsensitiveObjectInternal(name);
-            try
-            {
-                return (string?)obj;
-            }
-            catch (InvalidCastException)
-            {
-                throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name));
-            }
+            if (obj is string si)
+                return si;
+
+            if (obj is null)
+                return null;
+
+            throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Name, name));
         }
 
         // Look up an object value for a resource given its name.
@@ -208,13 +185,15 @@ private IDictionaryEnumerator GetEnumeratorHelper()
 
         protected virtual void ReadResources()
         {
-            Debug.Assert(Table != null);
+            Debug.Assert(_table != null);
             Debug.Assert(Reader != null);
             IDictionaryEnumerator en = Reader.GetEnumerator();
             while (en.MoveNext())
             {
-                object? value = en.Value;
-                Table.Add(en.Key, value);
+                if (en.Key is not string key)
+                    continue;
+
+                _table.Add(key, en.Value);
             }
             // While technically possible to close the Reader here, don't close it
             // to help with some WinRes lifetime issues.
@@ -225,35 +204,31 @@ protected virtual void ReadResources()
             if (name == null)
                 throw new ArgumentNullException(nameof(name));
 
-            Hashtable? copyOfTable = Table;  // Avoid a race with Dispose
+            Dictionary<string, object?>? copyOfTable = _table;  // Avoid a race with Dispose
 
             if (copyOfTable == null)
                 throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet);
 
-            return copyOfTable[name];
+            copyOfTable.TryGetValue(name, out object? value);
+            return value;
         }
 
         private object? GetCaseInsensitiveObjectInternal(string name)
         {
-            Hashtable? copyOfTable = Table;  // Avoid a race with Dispose
+            Dictionary<string, object?>? copyOfTable = _table;  // Avoid a race with Dispose
 
             if (copyOfTable == null)
                 throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet);
 
-            Hashtable? caseTable = _caseInsensitiveTable;  // Avoid a race condition with Close
+            Dictionary<string, object?>? caseTable = _caseInsensitiveTable;  // Avoid a race condition with Close
             if (caseTable == null)
             {
-                caseTable = new Hashtable(StringComparer.OrdinalIgnoreCase);
-
-                IDictionaryEnumerator en = copyOfTable.GetEnumerator();
-                while (en.MoveNext())
-                {
-                    caseTable.Add(en.Key, en.Value);
-                }
+                caseTable = new Dictionary<string, object?>(copyOfTable, StringComparer.OrdinalIgnoreCase);
                 _caseInsensitiveTable = caseTable;
             }
 
-            return caseTable[name];
+            caseTable.TryGetValue(name, out object? value);
+            return value;
         }
     }
 }

src/libraries/System.Resources.ResourceManager/tests/ResourceSetTests.cs

@@ -5,6 +5,7 @@
 using System.IO;
 using System.Reflection;
 using System.Globalization;
+using System.Collections;
 using System.Collections.Generic;
 
 namespace System.Resources.Tests
@@ -101,6 +102,69 @@ public void GetString()
         }
     }
 
+    public class ResourceSetTests_IResourceReader
+    {
+        class SimpleResourceReader : IResourceReader
+        {
+            Hashtable data;
+            public SimpleResourceReader()
+            {
+                data = new Hashtable();
+                data.Add(1, "invalid");
+                data.Add("String", "message");
+                data.Add("Int32", 5);
+            }
+
+            IEnumerator IEnumerable.GetEnumerator()
+            {
+                throw new NotSupportedException();
+            }
+
+            public IDictionaryEnumerator GetEnumerator()
+            {
+                return data.GetEnumerator();
+            }
+
+            public void Close()
+            {
+            }
+
+            public void Dispose()
+            {
+            }
+        }
+
+        [Fact]
+        public void Empty_Ctor()
+        {
+            Assert.Throws<ArgumentNullException>(() => new ResourceSet(null as IResourceReader));
+        }
+
+        [Fact]
+        public void GetObject()
+        {
+            var rs = new ResourceSet(new SimpleResourceReader());
+
+            Assert.Null(rs.GetObject("DoesNotExist"));
+            Assert.Null(rs.GetObject("1"));
+
+            Assert.Equal(5, rs.GetObject("Int32"));
+            Assert.Equal(5, rs.GetObject("int32", true));
+        }
+
+        [Fact]
+        public void GetString()
+        {
+            var rs = new ResourceSet(new SimpleResourceReader());
+
+            Assert.Null(rs.GetString("DoesNotExist"));
+            Assert.Null(rs.GetString("1"));
+
+            Assert.Equal("message", rs.GetString("String"));
+            Assert.Equal("message", rs.GetString("string", true));
+        }
+    }
+
     public class ResourceSetTests_StreamCtor : ResourceSetTests
     {
         public override ResourceSet GetSet(string base64Data)