Refactor BindingContext to use Dictionary<TKey, TValue> instead of Hashtable and List<T> instead of ArrayList

[Jericho]

Related: #8143 and #8140

Microsoft Reviewers: Open in CodeFlow

[elachlan]

Looks like tests are checking for DictionaryEntry instead of KeyValuePair.

System.Windows.Forms.Tests.BindingContextTests.BindingContext_CopyTo_WithNullWeakReferenceTarget_ScrubsWeakRefs
Assert.IsType() Failure\r\nExpected: System.Collections.DictionaryEntry\r\nActual:   System.Collections.Generic.KeyValuePair`2[[System.Windows.Forms.BindingContext+HashKey, System.Windows.Forms, Version=8.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089],[System.WeakReference, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]

Can you also move HashKey to a partial class file?

@dreddy-work, this might be a public API change? The associated test is testing ICollection.CopyTo. The test code is expecting a DictionaryEntry but getting a KeyValuePair.

[dreddy-work]

@dreddy-work, this might be a public API change? The associated test is testing ICollection.CopyTo. The test code is expecting a DictionaryEntry but getting a KeyValuePair.

At first glance, i do not see any public API surface change here. Need to check tests on why specific types are checked. Will take a look later.

[elachlan]

@dreddy-work, this might be a public API change? The associated test is testing ICollection.CopyTo. The test code is expecting a DictionaryEntry but getting a KeyValuePair.

At first glance, i do not see any public API surface change here. Need to check tests on why specific types are checked. Will take a look later.

The API design doesn't guarantee a type because the collection before wasn't typed. Even though it was only storing a particular set of values. Now generics are being used underneath, so the values in the collection have changed.

So in my mind its okay. Since no gaurantees were made, but it would cause issue for people relying on code returning a dictionary entry. This will be a problem for most collections we convert I think.

[dreddy-work]

This will be a problem for most collections we convert I think.

Agree. Tests need to be updated to account this change. @Jericho , can you make necessary changes in tests?

[Jericho]

I'm trying to replace DictionaryEntry in this line:

Assert.NotNull(Assert.IsType<DictionaryEntry>(array[1]).Key);

with the following:

Assert.NotNull(Assert.IsType<KeyValuePair<HashKey, WeakReference>>(array[1]).Key);

but the HashKey type is a private class, therefore not accessible. My change results in the following exception: CS0246 The type or namespace name 'HashKey' could not be found (are you missing a using directive or an assembly reference?).

To be honest I don't understand the logic of BindingContext being a public class but the data type of its enumerator key being a private type? To be clear: this problem existed before I changed anything, but it was obscured by the fact the data type of a DictionaryEntry key is object and the fact that none of the unit tests attempt to verify the data type of the key.

To further illustrate what I'm talking about, let's add the following assertion to the BindingContext_CopyTo_Invoke_Success unit test (in a branch that does not include my PR):

Assert.NotNull(Assert.IsType<HashKey>(Assert.IsType<DictionaryEntry>(array[1]).Key));

You'll get the same CS0246 The type or namespace name 'HashKey' could not be found (are you missing a using directive or an assembly reference?) exception.

[dreddy-work]

Assert.NotNull(Assert.IsType<KeyValuePair<HashKey, WeakReference>>(array[1]).Key);

Tests seems simulating GC collection and Key was never checked for type in these tests. Did you try replacing HashKey with object in KeyValuePair as work around here? @Tanya-Solyanik / @KlausLoeffelmann do you guys have any suggestion here?

[elachlan]

Tests are supposed to have visibility via InternalsVisibleToAttribute

[dreddy-work]

Tests are supposed to have visibility via InternalsVisibleToAttribute

HashKey has been private.

[Jericho]

@dreddy-work : I replaced Hashkey with object as you suggested but unit test fails:

Message: 
Assert.IsType() Failure
Expected: System.Collections.Generic.KeyValuePair`2[[System.Object, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.WeakReference, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]
Actual:   System.Collections.Generic.KeyValuePair`2[[System.Windows.Forms.BindingContext+HashKey, System.Windows.Forms, Version=8.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089],[System.WeakReference, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]

@elachlan : The class in question is defined like this: private class HashKey and as far as I know InternalsVisibleTo only helps with internal members, not with private.

Even if we figured out a way to fix the unit test, I think the larger question remains : does it make sense for the public member of a public class to contain objects of a private type? I have to admit that I have never seen this scenario, and I fail to understand how this make sense.

[elachlan]

Its exposed either way.
In the test:

In a test app using .NET 7:

Developer experience and the API are already partially "broken", because you won't be able to use anything in Key.

Can we cast the KeyValuePair to DictionaryEntry to solve the test issue? Otherwise we'd have to expose HashKey as Internal.

[Jericho]

Can we cast the KeyValuePair to DictionaryEntry to solve the test issue? Otherwise we'd have to expose HashKey as Internal.

How about this:

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

    int i = index;
    foreach (KeyValuePair<HashKey, WeakReference> kvp in _listManagers)
    {
        ar.SetValue(new DictionaryEntry(kvp.Key, kvp.Value), i++);
    }
}

IEnumerator IEnumerable.GetEnumerator()
{
    ScrubWeakRefs();
    return _listManagers
        .Select(kvp => new DictionaryEntry(kvp.Key, kvp.Value))
        .GetEnumerator();
}

I tested it and it resolves all the failing unit tests.

[KlausLoeffelmann]

So, what's the current state here:
Are collection items exposed publicly, and would this PR change the type of one item?

[Jericho]

So, what's the current state here: Are collection items exposed publicly, and would this PR change the type of one item?

The status is that I made a suggestion that would ensure the type of the items publicly exposed would be preserved.

Having said that, I want to be clear that the situation we are trying to preserve is that instances of a private type are being publicly exposed (which is very weird to me!), but I understand that fixing this weird situation would be a breaking change and I don't know if the team has any appetite for such a change.

At this point I'm waiting for feedback on the solution I proposed and/or guidance on what the team wants to do.

[KlausLoeffelmann]

I don't know if the team has any appetite for such a change.

The team does absolutely not (me as part of the team and focusing on Data Binding both in the Designer and in the runtime, know that pretty well!). The reason is: Especially around data binding, it would be not only a breaking change, but also such a subtle one, that it would become a potential migration blocker, and it would be particularly bad for large LOB apps.

[Jericho]

The team does absolutely not

Understood!

For my own education, do you know why instances of the private HashKey type are being publicly exposed? The fact that the type is private, as far as I understand, means that these instances are unusable so this whole situation does not make a lot of sense to me. I am assuming there is some legacy reason and, as you pointed out, the cost of "fixing" it is simply too high.

[KlausLoeffelmann]

For my own education, do you know why instances of the private HashKey type are being publicly exposed? The fact that the type is private, as far as I understand, means that these instances are unusable so this whole situation does not make a lot of sense to me. I am assuming there is some legacy reason and, as you pointed out, the cost of "fixing" it is simply too high.

Welcome to the club! 😄

The "original-original" code base is over 20 years old, and Data Binding is one of the earliest features, because it was important to have it in for (at least concept-) backwards compatibility to VB6 design scenarios. So, when we have head-scratchers like this (and I agree with you here, no doubt), the first thing I do is try to put this in the context of the time: First version (maybe first beta version) of C#, lack of experience, lack of ready NetFX APIs - maybe? But, yes there might be even more to it - but we wouldn't necessarily know. Sure, we have internal design documents from the time, but they don't have the granularity needed to explain every single implementation-detail-design decision. So, on that level, some things will remain a mystery when it comes to the motivation behind a technical implementation decision.

Now, that all said, the moment an implementation status-quo could become a potential security risk (which I - on first glance here - do not think it does), our decision process certainly must completely change, and at that point that wins over avoiding breaking changes. And I am also not per se against breaking changes, as long as they can be easily discovered and fixed. But that exactly is my concern in this scenario. Would you agree on that?

[Jericho]

Yes, I completely agree. I was just trying to understand the "reasoning" (if you can call it that!) behind what appears to be a non-sensical situation. I appreciate the importance of preserving (as much as reasonably possible) the status-quo unless there's a security concern.

By the way, thank you for taking the time to explain, I really appreciate it. Many lifetimes ago I wrote VB6 LOB apps for a living and also taught VB at a local college, and I do remember how smooth the transition was when I switched to c# and winform in 2001. So, if some of the "quirks" we find in the codebase today are due to an effort to preserve backward compatibility to VB6 back then, my "20-year-younger" self is grateful!

[KlausLoeffelmann]

So, if some of the "quirks" we find...

While you mentioned quirks (even though in a different context):

When a new feature would have the potential of (somewhat noticeable at least) breaking changes but is still too useful to not make it, 'quirks' are always something, we can and should consider. Folks could then with a switch opt in or out of a new feature to preserve the old behavior. These switches we call quirks.

[elachlan]

I mean, technically its not breaking the public contract. Since ICollection has no type guarantees. But the traditionally returned types from CopyTo will change.

I wrote a test to double check, but going from a constrained kyvaluepair to DictionaryEntry is fine. Because the types are becoming more lax (unsure on formal name).

This test worked:

Dictionary<int, string> test = new();
test.Add(1, "test1");
test.Add(2, "test2");
test.Add(3, "test3");

DictionaryEntry[] items = new DictionaryEntry[test.Count];
((ICollection)test).CopyTo(items, 0);

foreach (var item in items)
{
    Console.WriteLine(item.Value);
}

The only issue is if they are checking the type. As our tests are doing for whatever reason.

[KlausLoeffelmann]

So, just to understand correctly, because I haven't really caught up with reading the whole thread, yet:
CopyTo aside, because that's not soooo much my concern:

Iterating over a BindingContext instance used to yield DictionaryEntry items before and would now yield KeyValuePair items, is that correct?

To keep the old behavior (which I think would be important), we would convert the whole dictionary in GetEnumerator to return DictionaryEntry items?

What actual benefit would this PR then leave us with?

Since ICollection has no type guarantees.

At the time this was implemented, generics did not exist. So, I would assume that type guarantees kind of existed by allowing customary rights, if you know what I mean.

[elachlan]

Just tested GetEnumerator, it will break existing code if we expose it like we are with InvalidCastException using a ForEach loop. So we can't make this change.

[Jericho]

@elachlan I'll go ahead and commit my proposed changes which resolve the problems with CopyTo and GetEnumerator.

[elachlan]

Errors in build: Converting null literal or possible null value to non-nullable type.

[dreddy-work]

You need to revert this as indexing dictionary is not straight.

[Jericho]

Not sure what you mean by "straight". This unit test completes successfully and demonstrates that HashtableCopyTo can copy the elements from the source dictionary to the target.

What is it exactly that you want me to revert?

[dreddy-work]

now, i am trying to recollect :)

[dreddy-work]

@Jericho , can you revisit this PR and finish final pending review comments?

[dreddy-work]

Merging the pull request while there is still pending feedback that consists of minor nits and can be taken care of in later PRs that touching this file.