Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve and unify debug views of dictionaries. #92534

Merged
merged 11 commits into from
Nov 2, 2023
Merged

Improve and unify debug views of dictionaries. #92534

merged 11 commits into from
Nov 2, 2023

Conversation

arturek
Copy link
Contributor

@arturek arturek commented Sep 23, 2023

The change allows generic and non-generic dictionaries to be displayed in the same way: with separate columns for keys and values with an ability to expand each column.

Currently non-generic dictionaries are displayed using two columns but without an option to expand keys or values:
Hashtable debug view

Generic dictionaries are displayed as ordered lists of key/value pairs with a drill-down functionality:
Dictionary debug view

The PR unifies the way both kinds of dictionaries are displayed by a debugger. Dictionaries are shown using two-column format (which IMO better represents a dictionary) and both keys and values can be expanded:
New debug view of dictionaries

Fixes #88736

@arturek
Improves and unifies debug views of dictionaries.
e7963bc 
The change alows generic and non-generic dictionaries to be displayed in the same way: with separate columns for keys and values with an ability to expand each column.

Fixes dotnet#88736
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 23, 2023
@ghost
Copy link

ghost commented Sep 23, 2023

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

The change allows generic and non-generic dictionaries to be displayed in the same way: with separate columns for keys and values with an ability to expand each column.

Currently non-generic dictionaries are displayed using two columns but without an option to expand keys or values:
Hashtable debug view

Generic dictionaries are displayed as ordered lists of key/value pairs with a drill-down functionality:
Dictionary debug view

The PR unifies the way both kinds of dictionaries are displayed by a debugger. Dictionaries are shown using two-column format (which IMO better represents a dictionary) and both keys and values can be expanded:
New debug view of dictionaries

Fixes #88736

Author: arturek
Assignees: -
Labels:

area-System.Collections
Milestone: -

@build-analysis build-analysis bot mentioned this pull request Sep 23, 2023
Closed
@layomia layomia added this to the 9.0.0 milestone Sep 25, 2023
eiriktsarpalis
eiriktsarpalis reviewed Sep 28, 2023
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks! We should investigate and check if other included dictionary types need the same treatment (including the ones in System.Collections.Immutable and System.Collections.Frozen).

@eiriktsarpalis
Copy link
Member

@arturek this seems to be causing test failures in CI, could you take a look?

@arturek
Copy link
Contributor Author

arturek commented Sep 28, 2023

@eiriktsarpalis
Immutable and frozen dictionaries use their own debugger proxy implementation to cache their enumerated items. That proxy displays items the same way as a generic dictionary does it now. I can make updates to them after this PR is completed. Other types of generic dictionaries (in System.Collections, System.Collections.Concurrent, System.Collections.NonGeneric and System.Private.CoreLib) are updated by this PR. I hope there are no dictionaries defined in other places.

I'll fix the tests. I thought they are unrelated but now I can see the reason they are failing.

@arturek arturek mentioned this pull request Oct 9, 2023
Open
@eiriktsarpalis
Copy link
Member

Any luck getting the tests fixed? Thanks.

@eiriktsarpalis eiriktsarpalis added the needs-author-action An issue or pull request that requires more info or actions from the author. label Oct 27, 2023
@arturek
Merge branch 'main' of https://github.com/dotnet/runtime into Diction…
7ced9e2 
…aryDebugView
@arturek
Fix more DebuggerView tests
fc177d5 
* Included more types that implement IDictionary in the DebuggerView tests.
* Improved the testing code to support classes with attributes declared by their base classes.
* Fixed .Net Framework 4.8 build error by removing a dependency on the record c# feature.
* Fixed tests remaining tests (outside of the System.Collections subset)
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Oct 27, 2023
This was referenced Oct 27, 2023
Closed
Closed
@eiriktsarpalis
Copy link
Member

Still seeing a number of failing tests, e.g.

  Starting:    System.Collections.Immutable.Tests (parallel test collections = on, max threads = 2)
    System.Collections.Tests.DebugView_Tests.TestDebuggerAttributes_Null(obj: []) [FAIL]
      System.InvalidOperationException : Expected one DebuggerTypeProxyAttribute on System.Collections.ListDictionaryInternal.
      Stack Trace:
        /_/src/libraries/Common/tests/System/Diagnostics/DebuggerAttributes.cs(129,0): at System.Diagnostics.DebuggerAttributes.FindAttribute(Type type, Type attributeType)
        /_/src/libraries/Common/tests/System/Diagnostics/DebuggerAttributes.cs(141,0): at System.Diagnostics.DebuggerAttributes.GetProxyType(Type type, Type[] genericTypeArguments)
        /_/src/libraries/Common/tests/System/Collections/DebugView.Tests.cs(166,0): at System.Collections.Tests.DebugView_Tests.TestDebuggerAttributes_Null(Object obj)
    System.Collections.Tests.DebugView_Tests.TestDebuggerAttributes_Null(obj: [System.Collections.DictionaryEntry, System.Collections.DictionaryEntry]) [FAIL]
      System.InvalidOperationException : Expected one DebuggerTypeProxyAttribute on System.Collections.ListDictionaryInternal.
      Stack Trace:
        /_/src/libraries/Common/tests/System/Diagnostics/DebuggerAttributes.cs(129,0): at System.Diagnostics.DebuggerAttributes.FindAttribute(Type type, Type attributeType)
        /_/src/libraries/Common/tests/System/Diagnostics/DebuggerAttributes.cs(141,0): at System.Diagnostics.DebuggerAttributes.GetProxyType(Type type, Type[] genericTypeArguments)
        /_/src/libraries/Common/tests/System/Collections/DebugView.Tests.cs(166,0): at System.Collections.Tests.DebugView_Tests.TestDebuggerAttributes_Null(Object obj)
    System.Collections.Tests.DebugView_Tests.TestDebuggerAttributes_Dictionary(obj: [], expected: []) [FAIL]
      System.InvalidOperationException : Expected one DebuggerDisplayAttribute on System.Collections.ListDictionaryInternal.
      Stack Trace:
        /_/src/libraries/Common/tests/System/Diagnostics/DebuggerAttributes.cs(129,0): at System.Diagnostics.DebuggerAttributes.FindAttribute(Type type, Type attributeType)
        /_/src/libraries/Common/tests/System/Diagnostics/DebuggerAttributes.cs(115,0): at System.Diagnostics.DebuggerAttributes.ValidateDebuggerDisplayReferences(Object obj)
        /_/src/libraries/Common/tests/System/Collections/DebugView.Tests.cs(140,0): at System.Collections.Tests.DebugView_Tests.TestDebuggerAttributes_Dictionary(IDictionary obj, KeyValuePair`2[] expected)

@arturek
Copy link
Contributor Author

arturek commented Oct 29, 2023

@eiriktsarpalis:
I'm working on it. The latest issues are quite unexpected.

The tests you mentioned failed because ListDictionaryInternal doesn't have DebuggerTypeProxyAttribute, but this attribute was added a month ago. They also fail because they use the previous definition of the IDictionaryDebugView instead of the one modified by this PR. What's even stranger, these tests pass on my machine when run in the System.Collection solution, but not in System.Collection.Immutable (and earlier I used only the former solution to verify my fixes before committing them).

I'll look at it tomorrow.

@arturek
Update DebugView tests to expect different outcomes on .Net Framework
4c7782b 
.Net Framwork does not support recent improvements in the way the debugger displays a dictionary. Depending on the framwork used, the debugger view of generic dictionaries and ListDictionaryInternal are different.
@arturek
Copy link
Contributor Author

arturek commented Oct 30, 2023

@eiriktsarpalis
I updated tests and they almost pass. The only failure reason is "System.Net.Quic.QuicException : The connection timed out from inactivity" which doesn't seem related to my changes.
I couldn't find out what exactly preprocessor symbols NETFRAMEWORK and NETCOREAPP represent, and which of them would be better in this case, but current code seems to work as expected.

@eiriktsarpalis eiriktsarpalis self-assigned this Oct 31, 2023
@arturek
Applied suggested changes and fixes
70a5d55 
* mostly code style changes
* restored a rest for an empty HashSet.
* fixed testing of the generic SortedList.
@build-analysis build-analysis bot mentioned this pull request Oct 31, 2023
Closed
eiriktsarpalis
eiriktsarpalis reviewed Nov 1, 2023
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost forgot, but would it be possible to include this change in immutable collections as well? If you prefer to do this in a future PR that's also fine.

@arturek
Minor improvents
347e36c 
Renamed an internal method to match its new behavior and removed unnecessary init accessors.
@arturek
Copy link
Contributor Author

arturek commented Nov 1, 2023

@eiriktsarpalis

Almost forgot, but would it be possible to include this change in immutable collections as well? If you prefer to do this in a future PR that's also fine.

It's on my list. I'd prefer to do it in a separate PR just to close this phase and keep scope of this PR smaller.

eiriktsarpalis
eiriktsarpalis approved these changes Nov 2, 2023
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@eiriktsarpalis eiriktsarpalis merged commit 34bf55c into dotnet:main Nov 2, 2023
173 of 175 checks passed
@eiriktsarpalis eiriktsarpalis mentioned this pull request Nov 2, 2023
Closed
@JamesNK JamesNK mentioned this pull request Nov 2, 2023
Closed
17 tasks
@radekdoulik radekdoulik mentioned this pull request Nov 8, 2023
Open
@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

Assignees

@eiriktsarpalis eiriktsarpalis

Labels
area-System.Collections community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.

Debugger display of dictionary types and DebuggerDisplayAttribute.Name
4 participants
@arturek @eiriktsarpalis @stephentoub @layomia