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

Enable nullability in CodeDomSerializerBase #9099

Merged
merged 4 commits into from
Jul 12, 2023
Merged

Enable nullability in CodeDomSerializerBase #9099

merged 4 commits into from
Jul 12, 2023

Conversation

halgab
Copy link
Contributor

@halgab halgab commented May 10, 2023 •
edited
Loading

This PR is pretty big, apologies for that. It enables nullability in CodeDomSerializerBase to get one foot in the door to annotate more serializers down the road,
One issue I had doing this is the lack of annoations in System.CodeDom (see dotnet/runtime#41720, dotnet/runtime#78036)

Contributes to #8342

Proposed changes

  • Enable nullability in CodeDomSerializerBase
  • Refactor CodeDomSerializerBase and fix an apparent bug
  • Introduce new helper functions to simplify more parts of the code

Each change is its own commit.

Microsoft Reviewers: Open in CodeFlow

@ghost ghost assigned halgab May 10, 2023
@ghost ghost added area: NRT draft draft PR labels May 10, 2023
halgab
halgab commented May 10, 2023
View reviewed changes
...Windows.Forms.Design/src/System/ComponentModel/Design/Serialization/CodeDomSerializerBase.cs
{
if (statement is CodeMethodReturnStatement cmrs)
{
DeserializeExpression(manager, null, ces.Expression);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looked a lot like a bug

...Windows.Forms.Design/src/System/ComponentModel/Design/Serialization/CodeDomSerializerBase.cs
@@ -1242,11 +1165,11 @@ protected object DeserializeExpression(IDesignerSerializationManager manager, st
//
for (int i = 0; i < indexes.Length; i++)
{
IConvertible index = DeserializeExpression(manager, name, arrayIndexerEx.Indices[i]) as IConvertible;
object? index = DeserializeExpression(manager, name, arrayIndexerEx.Indices[i]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should fix the trace in the non IConvertible case

@halgab halgab marked this pull request as ready for review May 10, 2023 13:29
@halgab halgab requested a review from a team as a code owner May 10, 2023 13:29
@ghost ghost removed the draft draft PR label May 10, 2023
@elachlan
Copy link
Contributor

Thanks for working on this!

@dreddy-work dreddy-work added the waiting-on-team This work item needs to be discussed with team or is waiting on team action in order to proceed label Jun 21, 2023
JeremyKuhne
JeremyKuhne requested changes Jun 21, 2023
View reviewed changes
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

In general, looks really good! I think there is a bit more opportunity to use pattern matching. See my comments.

...Windows.Forms.Design/src/System/ComponentModel/Design/Serialization/CodeDomSerializerBase.cs
{
typename.Append(',');
typeName.Append('[');
Copy link
Member

Choose a reason for hiding this comment

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

This would be further improved by aggregating these Append calls into a single Append($',[{GetTypeNameFromCodeTypeReferenceHelper(manager, childref, typeName)}],). Same for other cases where we're making multiple calls in a row.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. In the end there weren't many opportunities to do so, as line 93 isn't a direct Append call

...Windows.Forms.Design/src/System/ComponentModel/Design/Serialization/CodeDomSerializerBase.cs Outdated
@@ -194,7 +180,7 @@ protected static Type GetReflectionTypeHelper(IDesignerSerializationManager mana
Type type = instance.GetType();
if (type.IsValueType)
{
TypeDescriptionProvider targetProvider = GetTargetFrameworkProvider(manager, instance);
TypeDescriptionProvider? targetProvider = GetTargetFrameworkProvider(manager, instance);
Copy link
Member

Choose a reason for hiding this comment

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

It's better to use pattern matching here. if (GetTargetFrameworkProvider(manager, instance) is TypeDescriptionProvider targetProvider)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the opportunity to move this logic to a helper to reduce code duplication

...Windows.Forms.Design/src/System/ComponentModel/Design/Serialization/CodeDomSerializerBase.cs Outdated Show resolved Hide resolved
...Windows.Forms.Design/src/System/ComponentModel/Design/Serialization/CodeDomSerializerBase.cs
private static object ExecuteBinaryOperator(IConvertible left, IConvertible right, CodeBinaryOperatorType op)
{
TypeCode leftType = left.GetTypeCode();
TypeCode rightType = right.GetTypeCode();

// The compatible types are listed in order from lowest bitness to highest. We must operate on the highest bitness to keep fidelity.
TypeCode[] compatibleTypes = new TypeCode[]
ReadOnlySpan<TypeCode> compatibleTypes = new TypeCode[]
Copy link
Member

Choose a reason for hiding this comment

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

Why change this to ReadOnlySpan?

Copy link
Contributor Author

@halgab halgab Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Now that dotnet/runtime#60948 is closed it should remove the array allocation

...mitives/src/System/ComponentModel/Design/Serialization/DesignerSerializationManagerHelper.cs Outdated Show resolved Hide resolved
@ghost ghost added the 📭 waiting-author-feedback The team requires more information from the author label Jun 21, 2023
@dreddy-work dreddy-work removed the waiting-on-team This work item needs to be discussed with team or is waiting on team action in order to proceed label Jun 21, 2023
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jun 22, 2023
@halgab halgab requested a review from JeremyKuhne June 24, 2023 21:28
@halgab halgab mentioned this pull request Jul 6, 2023
Merged
JeremyKuhne
JeremyKuhne approved these changes Jul 12, 2023
View reviewed changes
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for this!

@dreddy-work dreddy-work merged commit 664b9f6 into dotnet:main Jul 12, 2023
@ghost ghost added this to the 8.0 Preview7 milestone Jul 12, 2023
@halgab halgab deleted the code-dom-ser-base-null branch July 13, 2023 05:49
This was referenced Jul 16, 2023
Merged
Merged
@steveharter
Copy link
Member

What is left to fully address #8342?

@halgab
Copy link
Contributor Author

halgab commented Aug 3, 2023

There are quite a lot of files that still need annotating. I count 192 as of current main branch.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JeremyKuhne JeremyKuhne JeremyKuhne approved these changes

Assignees

@halgab halgab

Labels
area: NRT
Projects
None yet
Milestone
8.0 Preview7
Development

Successfully merging this pull request may close these issues.
5 participants
@halgab @elachlan @steveharter @JeremyKuhne @dreddy-work