Skip to content

Dictionary default value: retarget net10.0, fix the mis-wired tests, add the fallback overload - #2174

Open
vladimir-pecanac-main wants to merge 1 commit into
CodeMazeBlog:mainfrom
vladimir-pecanac-main:seo/80844-dict-default
Open

Dictionary default value: retarget net10.0, fix the mis-wired tests, add the fallback overload#2174
vladimir-pecanac-main wants to merge 1 commit into
CodeMazeBlog:mainfrom
vladimir-pecanac-main:seo/80844-dict-default

Conversation

@vladimir-pecanac-main

Copy link
Copy Markdown
Collaborator

Updates collections-dictionary/DefaultValueFromDictionaryInCSharp for the article rewrite of How to Return a Default Value From a Dictionary in C#.

What changed

  1. Retarget all three projects net7.0 -> net10.0.

  2. The test suite was broken and green at the same time. Four of the six tests in DefaultValueFromDictionaryUnitTests.cs were named GivenMethodTryGetValue_* and GivenMethodContainsKey_*, but the file's only using was DefaultValueFromDictionaryInCSharp.MethodGetValueOrDefault and all six tests called MethodGetValueOrDefault.GetValueFromDictionary. MethodContainsKey and MethodTryGetValue had zero coverage while the suite reported 6/6 passing. Each test now calls the method its name claims, with the two missing using directives added.

  3. Reversed assert arguments. Every assert was Assert.AreEqual(value, 0). MSTest's signature is AreEqual(expected, actual), so the assertions passed either way and the failure message named the wrong side. All swapped.

  4. dynamic removed. All three methods returned dynamic (or dynamic?) from a <T>-generic method over Dictionary<string, T>, which erases the type the caller already has and adds a DLR call site. They now return T?.

  5. MethodGetValueOrDefault took string? key while its two siblings took string. That nullable parameter dragged the inferred TKey to string?, which on net10.0 produced, at Methods/MethodGetValueOrDefault.cs(7,20):

    warning CS8620: Argument of type 'Dictionary<string, T>' cannot be used for parameter 'dictionary' of type 'IReadOnlyDictionary<string?, T>' ... due to differences in the nullability of reference types.
    warning CS8603: Possible null reference return.
    

    MethodContainsKey.cs(7,20) emitted CS8603 as well. Both are gone: the build is now 0 warnings, 0 errors.

  6. New Methods/MethodGetValueOrDefaultWithFallback.cs covering the two-argument GetValueOrDefault(key, fallback) overload, which the sample did not demonstrate at all. Its tests prove the fallback comes back for a missing key and that a stored value equal to the fallback is returned identically, so the two cases are genuinely indistinguishable.

  7. New FirstOrDefault test. Dictionary<TKey, TValue> is an IEnumerable<KeyValuePair<TKey, TValue>>, so FirstOrDefault() compiles on it and returns default(KeyValuePair<TKey, TValue>) for no match. That default is a struct, not null.

  8. Packages. BenchmarkDotNet 0.13.4 -> 0.15.8. Microsoft.NET.Test.Sdk 17.3.2 -> 18.9.0, MSTest.TestAdapter / MSTest.TestFramework 2.2.10 -> 4.3.3, coverlet.collector 3.1.2 -> 10.0.1. Removed the unused BenchmarkDotNet reference and the System.Collections 4.3.0 .NET Framework compatibility shim from the app project (the build itself flagged the latter: NU1510: PackageReference System.Collections will not be pruned ... likely unnecessary).

  9. Security advisory cleared, deliberately not by accident. The old MSTest 2.2.10 chain pulled Newtonsoft.Json 10.0.3 transitively, which carries the high-severity advisory NU1903 (GHSA-5crp-9r3c-p9vr). The package bumps above remove it: dotnet list package --vulnerable --include-transitive now reports no vulnerable packages in any of the three projects.

Evidence

Build and tests, SDK 10.0.302:

Build succeeded.
    0 Warning(s)
    0 Error(s)

Passed!  - Failed:     0, Passed:     9, Skipped:     0, Total:     9

The mis-wiring fix is proved by a deliberate break, not by a green run (a green run is what the bug already produced). Breaking MethodContainsKey and MethodTryGetValue on this branch fails exactly the four tests named for them; on main the same break failed nothing, because no test called either method.

Benchmarks, re-run on net10.0

The article's published figures were measured in 2023 on net7.0 with BenchmarkDotNet 0.13.4. Both runs below are on this branch, verbatim.

Run 1, key present (_key = "number_1000"):

BenchmarkDotNet v0.15.8, Windows 10 (10.0.19045.6466/22H2/2022Update)
AMD Ryzen 5 3600 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET SDK 10.0.302
  [Host]     : .NET 10.0.10 (10.0.10, 10.0.1026.32716), X64 RyuJIT x86-64-v3
  DefaultJob : .NET 10.0.10 (10.0.10, 10.0.1026.32716), X64 RyuJIT x86-64-v3


| Method            | Mean      | Error     | StdDev    | Rank | Allocated |
|------------------ |----------:|----------:|----------:|-----:|----------:|
| TryGetValue       |  9.976 ns | 0.1061 ns | 0.0993 ns |    1 |         - |
| GetValueOrDefault | 10.024 ns | 0.1307 ns | 0.1159 ns |    1 |         - |
| ContainsKey       | 18.260 ns | 0.3597 ns | 0.3364 ns |    2 |         - |

Run 2, key absent (same binaries, _key edited to "number_-1" for the run and reverted):

BenchmarkDotNet v0.15.8, Windows 10 (10.0.19045.6466/22H2/2022Update)
AMD Ryzen 5 3600 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET SDK 10.0.302
  [Host]     : .NET 10.0.10 (10.0.10, 10.0.1026.32716), X64 RyuJIT x86-64-v3
  DefaultJob : .NET 10.0.10 (10.0.10, 10.0.1026.32716), X64 RyuJIT x86-64-v3


| Method            | Mean     | Error     | StdDev    | Rank | Allocated |
|------------------ |---------:|----------:|----------:|-----:|----------:|
| TryGetValue       | 6.774 ns | 0.0580 ns | 0.0542 ns |    1 |         - |
| GetValueOrDefault | 6.980 ns | 0.1697 ns | 0.1816 ns |    1 |         - |
| ContainsKey       | 7.888 ns | 0.1543 ns | 0.1367 ns |    2 |         - |

The ordering the 2023 run reported survives, and the reason is now visible in the numbers: TryGetValue() and GetValueOrDefault() land inside each other's error bars in both runs (BenchmarkDotNet ranks them equal first), because GetValueOrDefault() is a wrapper over TryGetValue(). The ContainsKey() check pays for its second lookup, and it pays most when the key is present and the second lookup actually has to find something.

… dynamic

- All three projects retargeted net7.0 -> net10.0.
- Tests: four of six tests were named for MethodTryGetValue and MethodContainsKey
  but all six called MethodGetValueOrDefault, so two of the three methods had zero
  coverage while the suite reported 6/6 green. Each test now calls the method its
  name claims, and the reversed Assert.AreEqual(actual, expected) arguments are
  swapped to AreEqual(expected, actual) so a failure message tells the truth.
- Methods: dynamic / dynamic? return types replaced with T?; MethodGetValueOrDefault
  takes string instead of string? (clears CS8620 and CS8603 on net10.0).
- New MethodGetValueOrDefaultWithFallback covering the two-argument
  GetValueOrDefault(key, fallback) overload, with tests proving the fallback is
  returned for a missing key and that a stored value equal to the fallback is
  indistinguishable from a missing one.
- New test for FirstOrDefault on a dictionary: the default KeyValuePair is a struct,
  not null.
- Packages: BenchmarkDotNet 0.15.8; Microsoft.NET.Test.Sdk 18.9.0, MSTest 4.3.3,
  coverlet.collector 10.0.1. Removed the unused BenchmarkDotNet reference and the
  System.Collections 4.3.0 compatibility shim from the app project.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant