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

Make the STJ source generator cache friendly #86121

Merged
merged 10 commits into from
May 12, 2023
Merged

Make the STJ source generator cache friendly #86121

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
71012d8
Make the STJ source generator cache friendly.
eiriktsarpalis May 11, 2023
ea903bd
Address feedback
eiriktsarpalis May 12, 2023
23a31dd
Fix GetHashCode implementation.
eiriktsarpalis May 12, 2023
ffdd06d
Update src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
eiriktsarpalis May 12, 2023
a15e8d9
Remove TODO comment.
eiriktsarpalis May 12, 2023
ca9416c
Remove TODO comment.
eiriktsarpalis May 12, 2023
6006e74
Update src/libraries/System.Text.Json/gen/Model/ImmutableEquatableArr…
eiriktsarpalis May 12, 2023
a446e9a
Address feedback.
eiriktsarpalis May 12, 2023
52c392c
Address feedback
eiriktsarpalis May 12, 2023
3ae7cc8
Expand incremental compilation unit test coverage to include Roslyn t…
eiriktsarpalis May 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions src/libraries/System.Text.Json/Common/JsonIgnoreCondition.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,7 @@ namespace System.Text.Json.Serialization
/// a property or field is ignored during serialization and deserialization. This option
/// overrides the setting on <see cref="JsonSerializerOptions.DefaultIgnoreCondition"/>.
/// </summary>
#if BUILDING_SOURCE_GENERATOR
internal
#else
public
#endif
enum JsonIgnoreCondition
Copy link
Member

Choose a reason for hiding this comment

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

Why is this changing?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are part of the SG model which I made public for unit testing.

Copy link
Member

Choose a reason for hiding this comment

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

If it's necessary for unit testing could we leave them as internal and use InternalsVisibleTo from the unit tests? It's probably not a huge deal, but exposing public surface area from the source generator does create an opportunity for someone to reference it and depend on these. We intend to not support that, but... yeah.

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 discussed this with @ericstj and @layomia and we figured that these wouldn't be public in practice since there's no easy way for users to reference them directly. The source generator classes themselves are public already but we don't particularly care about keeping them backward compatible. The SDK assemblies appear to also have public types for the same reason. I'd rather we fall back to IVT if there's strong pushback to the idea of making the model types public.

cc @ViktorHofer

Copy link
Member

@ViktorHofer ViktorHofer May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't have a strong preference here just wanted to share that I noticed some issues with IVT in source generators. I.e. winforms heavily depends on IVT for their testing and when adding polyfills to netstandard2.0 like NullableAttributes.cs (which runtime does implicitly you get compiler errors. That's because the internal types are visible by the consumer which clash with the ones provided via the framework, i.e. when targeting .NETCoreApp.

Unsure if our source generator tests reference the source generator output assemblies directly though.

public enum JsonIgnoreCondition
{
/// <summary>
/// Property is never ignored during serialization or deserialization.
Expand Down
7 changes: 1 addition & 6 deletions src/libraries/System.Text.Json/Common/JsonKnownNamingPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,7 @@ namespace System.Text.Json.Serialization
/// <summary>
/// The <see cref="Json.JsonNamingPolicy"/> to be used at run time.
/// </summary>
#if BUILDING_SOURCE_GENERATOR
internal
#else
public
#endif
enum JsonKnownNamingPolicy
public enum JsonKnownNamingPolicy
{
/// <summary>
/// Specifies that JSON property names should not be converted.
Expand Down
7 changes: 1 addition & 6 deletions src/libraries/System.Text.Json/Common/JsonNumberHandling.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,7 @@ namespace System.Text.Json.Serialization
/// </remarks>
/// </summary>
[Flags]
#if BUILDING_SOURCE_GENERATOR
internal
#else
public
#endif
enum JsonNumberHandling
public enum JsonNumberHandling
{
/// <summary>
/// Numbers will only be read from <see cref="JsonTokenType.Number"/> tokens and will only be written as JSON numbers (without quotes).
Expand Down
7 changes: 1 addition & 6 deletions src/libraries/System.Text.Json/Common/JsonObjectCreationHandling.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,7 @@ namespace System.Text.Json.Serialization
/// <summary>
/// Determines how deserialization will handle object creation for fields or properties.
/// </summary>
#if BUILDING_SOURCE_GENERATOR
internal
#else
public
#endif
enum JsonObjectCreationHandling
public enum JsonObjectCreationHandling
{
/// <summary>
/// A new instance will always be created when deserializing a field or property.
Expand Down
7 changes: 1 addition & 6 deletions src/libraries/System.Text.Json/Common/JsonSourceGenerationMode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,7 @@ namespace System.Text.Json.Serialization
/// The generation mode for the System.Text.Json source generator.
/// </summary>
[Flags]
#if BUILDING_SOURCE_GENERATOR
internal
#else
public
#endif
enum JsonSourceGenerationMode
public enum JsonSourceGenerationMode
{
/// <summary>
/// When specified on <see cref="JsonSourceGenerationOptionsAttribute.GenerationMode"/>, indicates that both type-metadata initialization logic
Expand Down
7 changes: 1 addition & 6 deletions src/libraries/System.Text.Json/Common/JsonUnmappedMemberHandling.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,7 @@ namespace System.Text.Json.Serialization
/// Determines how <see cref="JsonSerializer"/> handles JSON properties that
/// cannot be mapped to a specific .NET member when deserializing object types.
/// </summary>
#if BUILDING_SOURCE_GENERATOR
internal
#else
public
#endif
enum JsonUnmappedMemberHandling
public enum JsonUnmappedMemberHandling
{
/// <summary>
/// Silently skips any unmapped properties. This is the default behavior.
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.Text.Json/Common/ReflectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,10 +231,10 @@ private static string GetBaseNameFromGenericTypeDef(Type genericTypeDef)
return fullName.Substring(0, length);
}

public static bool IsVirtual(this PropertyInfo? propertyInfo)
public static bool IsVirtual(this MemberInfo memberInfo)
{
Debug.Assert(propertyInfo != null);
return propertyInfo != null && (propertyInfo.GetMethod?.IsVirtual == true || propertyInfo.SetMethod?.IsVirtual == true);
Debug.Assert(memberInfo is FieldInfo or PropertyInfo);
return memberInfo is PropertyInfo p && (p.GetMethod?.IsVirtual == true || p.SetMethod?.IsVirtual == true);
}

public static bool IsKeyValuePair(this Type type, Type? keyValuePairType = null)
Expand Down
46 changes: 0 additions & 46 deletions src/libraries/System.Text.Json/gen/ContextGenerationSpec.cs
View file
Open in desktop

This file was deleted.

Loading