Fix nullability bug in BindingContext

[hughbe]

Proposed Changes

• Make ICollection.SyncRoot return non-null value

TODO

We have the following warnings

System/Windows/Forms/BindingContext.cs(377,38): warning CS8605: Unboxing a possibly null value. [/Users/hugh/Documents/GitHub/winforms/src/System.Windows.Forms/src/System.Windows.Forms.csproj]
System/Windows/Forms/BindingContext.cs(379,38): warning CS8600: Converting null literal or possible null value to non-nullable type. [/Users/hugh/Documents/GitHub/winforms/src/System.Windows.Forms/src/System.Windows.Forms.csproj]
System/Windows/Forms/BindingContext.cs(380,21): warning CS8602: Dereference of a possibly null reference. [/Users/hugh/Documents/GitHub/winforms/src/System.Windows.Forms/src/System.Windows.Forms.csproj]
3 Warning(s)

Happens in cases like below in foreach (DictionaryEntry de in _listManagers), how are we unboxing a possibly null value

private void ScrubWeakRefs()
{
    List<object>? cleanupList = null;
    foreach (DictionaryEntry de in _listManagers)
    {
        WeakReference wRef = (WeakReference)de.Value;
        if (wRef.Target == null)
        {
            cleanupList ??= new List<object>();
            cleanupList.Add(de.Key);
        }
    }

    ...
}

/cc @weltkante

Microsoft Reviewers: Open in CodeFlow

[weltkante]

how are we unboxing a possibly null value

foreach (DictionaryEntry de in _listManagers) uses the non-generic IEnumerable and thus does an implicit cast from object to the variable type you state in the foreach operation, since DictionaryEntry is a struct that would be an unboxing. If the _listManagers contained null entries this would throw. The warning would be fixed by using a generic dictionary instead of a non-generic one (however that requires reviewing/updating all callers since e.g. generic dictionaries don't use DictionaryEntry, they use KeyValuePair<K,V> and in general have a slightly different API)

[hughbe]

The warning would be fixed by using a generic dictionary instead of a non-generic one

Hmm this would definitely break ICollection.CopyTo since we would be copying KVPs rather than DictionaryEntrys:

void ICollection.CopyTo(Array ar, int index)
{
    ScrubWeakRefs();
    _listManagers.CopyTo(ar, index);
}

[weltkante]

Yes, if you don't want to take that breaking change you could rewrap the KVPs though.

I don't think that method call is common since its hidden behind an explicit interface implementation, callers would have to cast to use it. This is probably also something to discuss in context of #2644 when implementing generic collection types, because it will determine whether we use ICollection<KeyValuePair<...>> or ICollection<DictionaryEntry>. The latter might be more compatible to old code, but it'll definitely be less compatible to modern code.

[hughbe]

I don't think that method call is common since its hidden behind an explicit interface implementation, callers would have to cast to use it.

Perhaps the more common case would be the following, that would throw an InvalidCastException if we changed _listManager to be a Dictionary<HashKey, BindingManagerBase>. I don't know if its worth the hassle manually implementing compliant enumerators etc.

var context = new BindingContext();
// Add items...
foreach (DictionaryEntry in context)
{
}

[weltkante]

that would throw an InvalidCastException if we changed _listManager to be a Dictionary<HashKey, BindingManagerBase>

You are right of course. Lets not discuss changing implementation here, this belongs more into the discussion of #2644, its just a warning when doing nullability but for #2644 its a major decision what type you take as collection content (with the option to separate the decision out into its own issue). Thanks for bringing it up though!

[weltkante]

Also I think this warning is a false-positive, non-generic dictionaries won't return a null DictionaryEntry. Its probably not possible to express this with annotations though so you get a warning anyways.

[hughbe]

I tried suppressing it with foreach (DictionaryEntry! de in _listManager) but this has a syntax error. Couldn't find a way of doing this ugh

[hughbe]

Updated just to fix the nullability bug of SyncRoot since this is a behaviour change

[codecov]

Codecov Report

Merging #2646 into master will increase coverage by 0.55176%.
The diff coverage is 100%.

@@                Coverage Diff                 @@
##             master       #2646         +/-   ##
==================================================
+ Coverage   57.5673%   58.11907%   +0.55177%     
==================================================
  Files          1233        1240          +7     
  Lines        417560      423004       +5444     
  Branches      38754       38852         +98     
==================================================
+ Hits         240378      245846       +5468     
+ Misses       171808      171784         -24     
  Partials       5374        5374

Flag	Coverage Δ
#Debug	58.11907% <100%> (+0.55176%)	⬆️
#production	31.43367% <100%> (+0.10362%)	⬆️
#test	98.92968% <100%> (-0.001%)	⬇️

[hughbe]

@RussKie how does this bug fix look?

[RussKie]

Thank you all