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.

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

Refactor DesignerSerializationManager to replace HashTable and enable Nullability #8353

Merged
merged 17 commits into from
Feb 9, 2023

Conversation

elachlan
Copy link
Contributor

@elachlan elachlan commented Dec 9, 2022

@elachlan elachlan requested a review from a team as a code owner December 9, 2022 04:00
@ghost ghost assigned elachlan Dec 9, 2022
@elachlan
Copy link
Contributor Author

elachlan commented Dec 9, 2022

@RussKie, should IDesignerSerializationManager.CreateInstance return object??
runtime are taking a rightfully cautious approach with this, but I can't see how the code works without it.

dotnet/runtime#79429

Copy link
Member

@lonitra lonitra left a comment

Choose a reason for hiding this comment

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

There seems to be a bug in the original code. Otherwise, mostly about cleaning up some old comments

@ghost ghost added waiting-author-feedback The team requires more information from the author and removed waiting-author-feedback The team requires more information from the author labels Dec 9, 2022
@elachlan
Copy link
Contributor Author

I think that TypeDescriptor.CreateInstance should change the args parameter from object[]? to object?[]?.
https://learn.microsoft.com/en-us/dotnet/api/system.componentmodel.typedescriptor.createinstance?view=net-7.0

Copy link
Member

@lonitra lonitra left a comment

Choose a reason for hiding this comment

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

Small comment on whether a null check can be removed, otherwise LGTM!

@lonitra
Copy link
Member

lonitra commented Dec 16, 2022

I think that TypeDescriptor.CreateInstance should change the args parameter from object[]? to object?[]?. https://learn.microsoft.com/en-us/dotnet/api/system.componentmodel.typedescriptor.createinstance?view=net-7.0

Looking at source code it may need to be addressed in TypeDescriptorProvider. I believe runtime owns TypeDescriptor. Feel free to raise in issue.

@lonitra lonitra added the waiting-author-feedback The team requires more information from the author label Dec 16, 2022
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Dec 17, 2022
@elachlan elachlan requested a review from lonitra December 17, 2022 01:08
@elachlan
Copy link
Contributor Author

@lonitra I'm digging through runtime now. Activator.CreateInstance() has object?[]? for args. So just need to update all the annotations up through the stack. I'll put together a PR.

@lonitra
Copy link
Member

lonitra commented Dec 19, 2022

@lonitra I'm digging through runtime now. Activator.CreateInstance() has object?[]? for args. So just need to update all the annotations up through the stack. I'll put together a PR.

Thanks for doing that!

Copy link
Member

@lonitra lonitra left a comment

Choose a reason for hiding this comment

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

This LGTM :) but looks like we have some build errors. If you build on command line (cd to winforms repo on command line and type 'build') these errors should show up

@lonitra lonitra added waiting-author-feedback The team requires more information from the author ready-to-merge PRs that are ready to merge but worth notifying the internal team. labels Dec 19, 2022
@elachlan
Copy link
Contributor Author

Thanks, some of the build errors are related to the winforms PRs. I'll check over it again.

@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Dec 19, 2022
@elachlan elachlan force-pushed the DesignerSerializationManager-HashTable branch from f6e5d1c to 23570a8 Compare December 19, 2022 23:22
@elachlan elachlan requested a review from lonitra December 19, 2022 23:24
@lonitra lonitra added the waiting-author-feedback The team requires more information from the author label Dec 20, 2022
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Dec 20, 2022
@elachlan
Copy link
Contributor Author

Do we add exception information to XML comments?

@lonitra
Copy link
Member

lonitra commented Dec 21, 2022

Do we add exception information to XML comments?

Sorry, I'm not sure I follow. Which XML comments?

@lonitra lonitra added the waiting-author-feedback The team requires more information from the author label Dec 21, 2022
@elachlan elachlan force-pushed the DesignerSerializationManager-HashTable branch from deeae48 to 3df6086 Compare February 6, 2023 23:03
@elachlan
Copy link
Contributor Author

elachlan commented Feb 6, 2023

I apologize for the rebase/force push. Test should be fixed now.

Copy link
Member

@lonitra lonitra left a comment

Choose a reason for hiding this comment

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

Thank you for your persistence on this!

@lonitra lonitra added the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Feb 8, 2023
@elachlan
Copy link
Contributor Author

elachlan commented Feb 9, 2023

Thanks, I hope that the rest of the code base doesn't take me 2 months per code file.

@lonitra lonitra merged commit 2681167 into dotnet:main Feb 9, 2023
@ghost ghost added this to the 8.0 Preview2 milestone Feb 9, 2023
@ghost ghost removed the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Feb 9, 2023
@elachlan elachlan deleted the DesignerSerializationManager-HashTable branch February 9, 2023 20:05
@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Feb 24, 2023

I was looking at the source code and I think this PR caused a bug here:

if (serializers is not null)
{
// I don't double hash here. It will be a very rare day where multiple types of serializers will be used in the same scheme. We still handle it, but we just don't cache.
serializer = serializers[objectType];
if (serializer is not null && !serializerType.IsAssignableFrom(serializer.GetType()))
{
serializer = null;
}
}

When serializers was a Hashtable, serializers[objectType] returned null if there was no entry for objectType. Now that serializers is a Dictionary<Type, object>?, it's going to throw KeyNotFoundException instead. So this should call serializers.TryGetValue(objectType, out serializer) instead.

@elachlan
Copy link
Contributor Author

I'll try and put in a pr to fix it soon.

@KalleOlaviNiemitalo
Copy link

The same may apply to other hashtables such as namesByInstance.

@dreddy-work
Copy link
Member

The same may apply to other hashtables such as namesByInstance.

Thank you @KalleOlaviNiemitalo for pointing this out. @elachlan, lets take a look at these on case by case.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants