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

Add new APIs for code gen #50

Merged
merged 5 commits into from
Aug 11, 2020
Merged

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Aug 7, 2020

Per offline reviews, this is a first pass at adding new public APIs to support the code-gen effort.

Primarily this exposes the existing internal metadata classes (JsonPropertyInfo\JsonClassInfo) as public and also extends these existing classes by adding derived classes (for value-based, object-based and collection-based types) along with helper methods to initialize the metadata and (de)serialize.

There is still work remaining including:

  • API review \ usability.
  • Renaming of existing classes including ReadStack, WriteStack and JsonClassInfo.
  • Add additional helper method that will be called by code-gen if there are left-over JSON that was not deserialized.
  • Fallback to reflection to obtain metadata for unsupported cases (fields, ctor parameters)
  • Support for circular Type references (a lazy-loaded approach for metadata).
  • Additional tests: circular references, preserve references, custom converters, benchmarks (especially startup time).
  • Design overlap with custom converters and to support the linker for code removal.
    • May add a new constructor to JsonSerializerOptions that will not initialize the built-in converters and prevent the Emit- and Reflection-based MemberAccessor strategies from being used.

Copy link

@layomia layomia left a comment

Choose a reason for hiding this comment

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

looks exciting :shipit:


namespace System.Text.Json.Serialization.Converters
{
/// <summary>
/// Default base class implementation of <cref>JsonObjectConverter{T}</cref>.
/// </summary>
internal class ObjectDefaultConverter<T> : JsonObjectConverter<T> where T : notnull
internal class ObjectDefaultConverter<T> : JsonObjectConverter<T>
Copy link

Choose a reason for hiding this comment

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

why the change to notnull?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why this was originally added; affects new usage of generics added elsewhere. T can be null...

@@ -68,6 +70,8 @@ internal override sealed JsonParameterInfo CreateJsonParameterInfo()
/// </summary>
internal bool CanBeNull { get; }

internal readonly EqualityComparer<T> _defaultComparer = EqualityComparer<T>.Default;
Copy link

Choose a reason for hiding this comment

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

is this used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes in JsonPropertyInfo WriteValue and Write. I believe you refactored this earlier in master; not sure if this branch is up-to-date with master to have that change yet.

{
_isInitialized = true;

//todo: should we not add?
Copy link

Choose a reason for hiding this comment

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

What is the benefit of adding given all the generated TypeInfos have references to each other via their JsonPropertyInfos?

Copy link
Member Author

Choose a reason for hiding this comment

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

It allows direct usage of the type in the existing (De)Serialize methods without having to fall back to reflection\IL Emit, and also allows options.GetConverter(type) to return the type without reflection\IL Emit.

}
}

// todo: add SerializeAsync method
Copy link

Choose a reason for hiding this comment

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

👀

Copy link
Member Author

Choose a reason for hiding this comment

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

As a proof-of-concept, just the DeserializeAsync was added (to ensure callers can still use that, even though the generated (de)serialize methods won't get called. The SerializeAsync will be added next pass.

@steveharter
Copy link
Member Author

Test failure is not related: System.Net.Http.Functional.Tests

@steveharter steveharter merged commit 9e5cf8b into dotnet:JsonCodeGen Aug 11, 2020
@steveharter steveharter deleted the NewApis branch August 11, 2020 02:57
MichalStrehovsky pushed a commit to MichalStrehovsky/runtimelab that referenced this pull request Oct 15, 2021
* Add support for properties

* PR feedback

- Avoid creating PropertyMap rows for types without properties
- Use GetAccessors to get property accessors

* PR feedback

- Don't forget to map tokens!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants