-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ReadOnlyDictionary and improve DebuggerAttributes #94581
Fix ReadOnlyDictionary and improve DebuggerAttributes #94581
Conversation
* Fix debugger type proxy attributes for Key and Value collections of ReadOnlyDictionary * Separate methods to test validity of the DebuggerTypeProxy attribute * ValidateDebuggerTypeProxyProperties - to verify debugger type proxy in most cases * CreateDebuggerTypeProxyWithNullArgument - to verify that the constructor of the debugger type proxy throws when its argument is null. * Simplify and make more uniform debugger type proxy tests A prerequisite for dotnet#94289
Tagging subscribers to this area: @dotnet/area-system-collections Issue Details
This is the first part of work on #94289
|
} | ||
|
||
Assert.True(threwNull); | ||
TargetInvocationException tie = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(typeof(ArrayList))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just trying to undertand how non-generic collections are related to this change. Is it part of an infrastructural refactoring? Is ReadOnlyDictionary used under wraps somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While working on code that handles the creation of some debugger type proxies, I had to deal that the ValidateDebuggerTypeProxyProperties
method, which accepted an instance of the class under test and its type. The parameters could conflict with each other. Usually this method was used in one of two ways: to validate an instance of the class under test, or to check if the constructor of a debug type proxy fails when executed with a null argument. This means that only one of these arguments (instance or type) should be used. I extracted the this second use case into the CreateDebuggerTypeProxyWithNullArgument
method.
I replaced all uses of ValidateDebuggerTypeProxyProperties
with a null instance argument with CreateDebuggerTypeProxyWithNullArgument
(as seen above). In other references, I removed type arguments, so now the type is correctly determined based on the supplied instance. Both methods can be used to handle generic and non-generic classes.
This led to the Keys and Values classes of ReadOnlyDictionary
, which were attributed with mismatched debugger type proxy class (Visual Studio silently ignored their type proxy attributes).
src/libraries/System.Private.CoreLib/src/System/Collections/ObjectModel/ReadOnlyDictionary.cs
Show resolved
Hide resolved
Hi @eiriktsarpalis, I have updated the code to fix an issue with the tests that I somehow overlooked earlier. The only failing test seems unrelated. I hope the PR can be committed now, so I can complete another one for immutable collections. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This is the first part of work on #94289