[Foundation] Fix nullability in NSDictionary<TKey,TValue>. #24457
Conversation
This is file 41 of 47 files with nullability disabled in Foundation. * Removes SupportedOSPlatform attributes without version numbers. * Enables nullability and adds nullability attributes where appropriate. * Replaces 'To be added' XML comments with proper documentation. * Adds missing XML documentation for all public members. * Uses ArgumentNullException.ThrowIfNull consistently. * Uses modern C# array syntax ([]) for empty arrays. * Improves IDictionary interface implementation to properly handle missing keys. * Adds some tests. Contributes towards #17285.
There was a problem hiding this comment.
Pull request overview
This PR enables nullable reference types for NSDictionary<TKey, TValue> in the Foundation namespace, part of an ongoing effort to enable nullability across 47 files (this is file 41). The changes improve code safety through nullability annotations, modernize the code with C# best practices, and enhance documentation quality by replacing placeholder XML comments with detailed descriptions.
Key changes:
- Enables
#nullableand adds appropriate nullability annotations throughout the class - Replaces
ArgumentNullExceptionnull checks withArgumentNullException.ThrowIfNullfor consistency - Uses modern C# empty array syntax
[]instead ofnew TValue[] {} - Adds comprehensive XML documentation for all public members
- Adds tests for missing key access scenarios to verify IDictionary interface behavior
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Foundation/NSDictionary_2.cs |
Enables nullability, adds proper XML documentation, uses modern C# syntax, and improves IDictionary implementation with correct nullability annotations |
tests/monotouch-test/Foundation/NSDictionary2Test.cs |
Adds comprehensive tests for missing key scenarios, validating that both class and IDictionary interface return null instead of throwing exceptions |
tests/cecil-tests/Documentation.KnownFailures.txt |
Removes entries for methods and properties that now have proper documentation |
src/NaturalLanguage/NLLanguage.cs |
Refactors enumeration pattern to use KeyValuePair iteration for improved efficiency |
| public void IndexerGetterThrowsKeyNotFoundTest () | ||
| { | ||
| var value1 = NSDate.FromTimeIntervalSinceNow (1); | ||
| var key1 = new NSString ("key1"); | ||
| var keyMissing = new NSString ("missing"); | ||
|
|
||
| var dict = new NSDictionary<NSString, NSDate> (key1, value1); | ||
|
|
||
| // Accessing via the property indexer should return null for missing keys | ||
| Assert.IsNull (dict [keyMissing], "missing key"); | ||
|
|
||
| // Accessing via IDictionary interface should also return null (NSDictionary behavior) | ||
| IDictionary<NSString, NSDate> idict = dict; | ||
| Assert.IsNull (idict [keyMissing], "missing key via interface"); | ||
| } |
There was a problem hiding this comment.
The test name IndexerGetterThrowsKeyNotFoundTest is misleading. The test actually verifies that the indexer returns null for missing keys, not that it throws KeyNotFoundException. Consider renaming to IndexerGetterReturnsNullForMissingKeyTest to accurately reflect the behavior being tested.
| // Indexer getter should return null for missing keys | ||
| Assert.IsNull (dict [keyMissing], "Indexer missing"); | ||
|
|
||
| // IDictionary indexer should also return null (NSDictionary can have null values) |
There was a problem hiding this comment.
The comment states "NSDictionary can have null values" but this contradicts the implementation comment on line 378 of NSDictionary_2.cs which states "NSDictionary can not contain NULLs, if you want a NULL, it exists as an NSNull". The comment should be corrected to clarify that NSDictionary returns null for missing keys, not that it stores null values.
| // IDictionary indexer should also return null (NSDictionary can have null values) | |
| // IDictionary indexer should also return null for missing keys (NSDictionary returns null for missing keys, not because it stores null values) |
✅ [CI Build #8eda040] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #8eda040] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ [CI Build #8eda040] Build passed (Build macOS tests) ✅Pipeline on Agent |
💻 [CI Build #8eda040] Tests on macOS arm64 - Mac Sequoia (15) passed 💻✅ All tests on macOS arm64 - Mac Sequoia (15) passed. Pipeline on Agent |
💻 [CI Build #8eda040] Tests on macOS M1 - Mac Monterey (12) passed 💻✅ All tests on macOS M1 - Mac Monterey (12) passed. Pipeline on Agent |
💻 [CI Build #8eda040] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅ All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
💻 [CI Build #8eda040] Tests on macOS arm64 - Mac Tahoe (26) passed 💻✅ All tests on macOS arm64 - Mac Tahoe (26) passed. Pipeline on Agent |
💻 [CI Build #8eda040] Tests on macOS M1 - Mac Ventura (13) passed 💻✅ All tests on macOS M1 - Mac Ventura (13) passed. Pipeline on Agent |
🔥 [CI Build #8eda040] Test results 🔥Test results❌ Tests failed on VSTS: test results 1 tests crashed, 2 tests failed, 106 tests passed. Failures❌ cecil tests1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ introspection tests1 tests failed, 3 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (iOS)🔥 Failed catastrophically on VSTS: test results - monotouch_ios (no summary found). Html Report (VSDrops) Download Successes✅ dotnettests (iOS): All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
This is file 41 of 47 files with nullability disabled in Foundation.
Contributes towards #17285.