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

Json serializer type discovery #18

Merged
merged 15 commits into from
Jul 23, 2020

Conversation

kevinwkt
Copy link

@kevinwkt kevinwkt commented Jul 20, 2020

Initial type discovery approach with temporary tests

A few changes in this PR:

  1. JsonSerializableAttribute creation in System/Text/Json/Serialization/Attributes/JsonSerializableAttribute.cs
  2. Updated Source generator for type discovery along with basic temporary tests.
  3. Added ReflectionUtils by David Fowler for type wrappers from roslyn syntax tokens to System.Type.

@layomia layomia requested a review from chsienki July 21, 2020 18:44
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.

LGTM - let's file relevant issues and continue documenting our decisions.

{
GeneratorInputTypes.Add(tds);
// Find JsonSerializable Attributes.
Copy link

Choose a reason for hiding this comment

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

Let's create an issue to consider changing this to an assembly level attribute since we are not generating source on the actual types (both owned and not owned) for now, as discussed offline.

If we enable that mode in the future, we'd just need to check that the calling assembly equals the type's assembly, and the type is partial before we generate source on the type.

Copy link
Member

Choose a reason for hiding this comment

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

Was this issue created? I had the similar feedback here before I found this comment :)

@layomia layomia merged commit 816409c into dotnet:JsonCodeGen Jul 23, 2020
AttributeListSyntax attributeList = ((TypeDeclarationSyntax)syntaxNode).AttributeLists.SingleOrDefault();
if (attributeList != null)
{
serializableAttributes = attributeList.Attributes.Where(node => (node is AttributeSyntax attr && attr.Name.ToString() == "JsonSerializable")).Cast<AttributeSyntax>();

Choose a reason for hiding this comment

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

The trouble with this approach is that you can't guarantee that the attribute name will actually be called "JsonSerializable"

For example, here are a bunch of ways I can rename the attribute https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzhgHwAEAmARgFgAoIgBgAIBBAOwgwAsYoA5bAWxi4ADtjAx6AXnpEyAbmp16AZQgCOAS2YBzAKIAbXDB3MM62HoCek6WQB0Sruux71AL2zA9MRhgxR1wACuGDDyVNQA2g7+zm4eXgC6CgDM0iRM9ADeAL7UkTL2jrHunt6+/kEhSTSppPQAQlm54VQRLGycPPyCImKFMS4liSlp9ADCTXmtKmrsmroGRiZmMJbVRLXpACJNQA===

Basically when dealing with syntax, you can only make guesses at things, not full decisions. You won't be able to actually know if the same attribute without doing binding. That's what the compiler does when you ask for a semantic model (it's not a trivial operation ;))

The way the other 'attribute based' generators solve this is by just selecting candidates for inclusion (basically anything that has an attribute):
https://github.com/dotnet/roslyn-sdk/blob/94087f6f1d5414ca187d8836caf7c8a114f22978/samples/CSharp/SourceGenerators/SourceGeneratorSamples/AutoNotifyGenerator.cs#L176

Then doing the actual selection during generation. https://github.com/dotnet/roslyn-sdk/blob/94087f6f1d5414ca187d8836caf7c8a114f22978/samples/CSharp/SourceGenerators/SourceGeneratorSamples/AutoNotifyGenerator.cs#L50

As your attribute is provided via references (not injected directly via the generator) you won't need to do the 'create a new compilation' step you'll see above.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@jkotas Not really. I handle all those cases. I ensure the name ends with GeneratedDllImportAttribute or GeneratedDllImport. This handles all the cases in that example see

private static bool IsGeneratedDllImportAttribute(AttributeSyntax attrSyntaxMaybe)
{
var attrName = attrSyntaxMaybe.Name.ToString();
return attrName.EndsWith(GeneratedDllImport)
|| attrName.EndsWith(GeneratedDllImportAttribute);
}
.

This doesn't handle the other case of TrickGeneratedDllImportAttribute though. Still on the fence whether it is worth addressing based on the cost of building up the semantic model where it is being done.

runtimelab-bot pushed a commit that referenced this pull request May 18, 2021
…2769)

Transition to GC Unsafe mode on every MONO_RT_EXTERNAL_ONLY function in
reflection.c

In particular, fix mono_reflection_type_from_name which is used in
https://github.com/xamarin/xamarin-android/blob/681887ebdbd192ce7ce1cd02221d4939599ba762/src/monodroid/jni/embedded-assemblies.cc#L350

Fixes stack traces like

```
05-14 08:06:12.848 31274 31274 F DEBUG   :       #00 pc 00000b99  [vdso] (__kernel_vsyscall+9)
05-14 08:06:12.848 31274 31274 F DEBUG   :       #1 pc 0005ad68  /apex/com.android.runtime/lib/bionic/libc.so (syscall+40) (BuildId: 6e3a0180fa6637b68c0d181c343e6806)
05-14 08:06:12.848 31274 31274 F DEBUG   :       #2 pc 00076511  /apex/com.android.runtime/lib/bionic/libc.so (abort+209) (BuildId: 6e3a0180fa6637b68c0d181c343e6806)
05-14 08:06:12.848 31274 31274 F DEBUG   :       #3 pc 0002afcd  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonodroid.so (xamarin::android::internal::MonodroidRuntime::mono_log_handler(char const*, char const*, char const*, int, void*)+141) (BuildId: 9726f32ad5f8fa5e7c5762baf2f6e3294da41cc1)
05-14 08:06:12.848 31274 31274 F DEBUG   :       #4 pc 00112c5d  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (eglib_log_adapter+141) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #5 pc 00020fdf  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (monoeg_g_logv+175) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #6 pc 0002113a  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (monoeg_g_log+42) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #7 pc 00128892  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_threads_transition_do_blocking+258) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #8 pc 0012a406  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_threads_enter_gc_safe_region_unbalanced_with_info+134) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #9 pc 0012a27e  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_threads_enter_gc_safe_region_internal+46) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #10 pc 000799a7  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_loader_lock+71) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #11 pc 000447a1  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_class_create_from_typedef+129) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #12 pc 0003c073  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_class_get_checked+99) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #13 pc 0003cc0f  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_class_from_name_checked_aux+735) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #14 pc 00037989  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_class_from_name_checked+73) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #15 pc 000cc5f4  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_reflection_get_type_internal+132) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #16 pc 000c9bce  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_reflection_get_type_with_rootimage+126) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #17 pc 000ca204  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (_mono_reflection_get_type_from_info+292) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #18 pc 000ca06e  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_reflection_type_from_name_checked+334) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #19 pc 000c9f01  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_reflection_type_from_name+49) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #20 pc 0001b40b  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonodroid.so (xamarin::android::internal::EmbeddedAssemblies::typemap_java_to_managed(char const*)+427) (BuildId: 9726f32ad5f8fa5e7c5762baf2f6e3294da41cc1)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #21 pc 0001b551  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonodroid.so (xamarin::android::internal::EmbeddedAssemblies::typemap_java_to_managed(_MonoString*)+113) (BuildId: 9726f32ad5f8fa5e7c5762baf2f6e3294da41cc1)
05-14 08:06:12.849 31274 31274 F DEBUG   :       #22 pc 000211a7  /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonodroid.so (xamarin::android::internal::MonodroidRuntime::typemap_java_to_managed(_MonoString*)+39) (BuildId: 9726f32ad5f8fa5e7c5762baf2f6e3294da41cc1)
```
runtimelab-bot pushed a commit that referenced this pull request Jun 22, 2022
* Initial implementation for contract customization

fix build errors

Move converter rooting to DefaultJsonTypeInfoResolver so that it can be used standalone

Fix ConfigurationList.IsReadOnly

Minor refactorings (#1)

* Makes the following changes:

* Move singleton initialization for DefaultTypeInfoResolver behind a static property.
* Consolidate JsonSerializerContext & IJsonTypeInfoResolver values to a single field.
* Move reflection fallback logic away from JsonSerializerContext and into JsonSerializerOptions

* Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

* remove testing of removed field

Simplify the JsonTypeInfo.CreateObject implemenetation (#2)

* Simplify the JsonTypeInfo.CreateObject implemenetation

* Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs

* Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs

Co-authored-by: Krzysztof Wicher <[email protected]>

Co-authored-by: Krzysztof Wicher <[email protected]>

Tests and fixes for JsonTypeInfoKind.None

TypeInfo type mismatch tests

Allow setting NumberHandling on JsonTypeInfoKind.None

test resolver returning wrong type of options

JsonTypeInfo/JsonPropertyInfo mutability tests

rename test file

Move default converter rooting responsibility behind DefaultJsonTypeInfoResolver (#3)

* Move default converter rooting responsibility behind DefaultJsonTypeInfoResolver

* address feedback

Add simple test for using JsonTypeInfo<T> with APIs directly taking it

fix and tests for untyped/typed CreateObject

uncomment test cases, remove todo

More tests and tiny fixes

Add a JsonTypeInfoResolver.Combine test for JsonSerializerContext (#4)

* Fix JsonTypeInfoResolver.Combine for JsonSerializerContext

* Break up failing test

Fix simple scenarios for combining contexts (#6)

* Fix simple scenarios for combining contexts

* feedback

JsonSerializerContext combine test with different camel casing

Remove unneeded virtual calls & branching when accessing Get & Set delegates (#7)

JsonPropertyInfo tests everything minus ShouldSerialize & NumberHandling

Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

throw InvalidOperationException rather than ArgumentNullException for source gen when PropertyInfo.Name is assigned through JsonPropertyInfoValues

tests for duplicated property names and JsonPropertyInfo.NumberHandling

Add tests for NumberHandling and failing tests for ShouldSerialize

disable the failing test and add extra checks

disable remainder of the failing ShouldSerialize tests, fix working one

Fix ShouldSerialize and IgnoreCondition interop

Add failing tests for CreateObject + parametrized constructors

Fix CreateObject support for JsonConstructor types (#10)

* Fix CreateObject support for JsonConstructor types

* address feedback

Make contexts more combinator friendly (#9)

* Make contexts more combinator friendly

* remove converter cache

* redesign test to account for JsonConstructorAttribute

* Combine unit tests

* address feedback

* Add acceptance tests for DataContract attributes & Specified pattern (#11)

* Add private field serialization acceptance test (#13)

* tests, PR feedback (#14)

* PR feedback and extra tests

* Shorten class name, remove incorrect check (not true for polimorphic cases)

* Make parameter matching for custom properties map property Name with parameter (#16)

* Test static initialization with JsonTypeInfo (#17)

* Fix test failures and proper fix this time (#18)

* Fix test failures and proper fix this time

* reinstate ActiveIssueAttribute

* PR feedback and adjust couple of tests which don't set TypeInfoResolver

* fix IAsyncEnumerable tests

* Lock JsonSerializerOptions in JsonTypeInfo.EnsureConfigured()

Co-authored-by: Eirik Tsarpalis <[email protected]>

Co-authored-by: Eirik Tsarpalis <[email protected]>
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.

6 participants