Skip to content

Native aot logic#629

Merged
gregsdennis merged 10 commits into
native-aotfrom
native-aot-logic
Jan 29, 2024
Merged

Native aot logic#629
gregsdennis merged 10 commits into
native-aotfrom
native-aot-logic

Conversation

@gregsdennis

Copy link
Copy Markdown
Collaborator

Depends on #628

@github-actions

github-actions Bot commented Jan 28, 2024

Copy link
Copy Markdown

Test Results

    15 files  ±  0      15 suites  ±0   13s ⏱️ ±0s
20 472 tests +277  19 114 ✅ +277  1 358 💤 ± 0  0 ❌ ±0 
20 564 runs  ±  0  19 203 ✅ + 45  1 361 💤  - 45  0 ❌ ±0 

Results for commit a8965d6. ± Comparison against base commit 31a1768.

♻️ This comment has been updated with latest results.

@gregsdennis

gregsdennis commented Jan 28, 2024

Copy link
Copy Markdown
Collaborator Author

Getting a few oddities

  • can't deserialize null into a rule
  • building values using the nodes API doesn't work

var array = new JsonArray { 1, 2, 3 };

apparently throws a serialization exception that int hasn't been registered on the serialization context!

stack trace:

System.NotSupportedException : JsonTypeInfo metadata for type 'System.Int32' was not provided by TypeInfoResolver of type 'System.Text.Json.Serialization.Metadata.EmptyJsonTypeInfoResolver'. If using source generation, ensure that all root types passed to the serializer have been annotated with 'JsonSerializableAttribute', along with any types that might be serialized polymorphically.
   at System.Text.Json.ThrowHelper.ThrowNotSupportedException_NoMetadataForType(Type type, IJsonTypeInfoResolver resolver)
   at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, Boolean ensureConfigured, Nullable`1 ensureNotNull, Boolean resolveIfMutable, Boolean fallBackToNearestAncestorType)
   at System.Text.Json.JsonSerializerOptions.GetTypeInfo(Type type)
   at System.Text.Json.Nodes.JsonNode.ConvertFromValue[T](T value, Nullable`1 options)
   at System.Text.Json.Nodes.JsonArray.Add[T](T value)

@eiriktsarpalis, can you shed any light on this?

Edit: We ended up working around it by using the params constructor, but it still seems like this should work.

@gregsdennis gregsdennis mentioned this pull request Jan 28, 2024
17 tasks

@jevansaks jevansaks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also see that RuleRegistry is using self-reflection and triggering some AOT warnings. Can you update that to a hard-coded list of the rule types? The attribute fetching logic should be fine.

Comment thread JsonLogic/Rule.cs Outdated
if (value.Source != null)
{
JsonSerializer.Serialize(writer, value.Source, options);
JsonSerializer.Serialize(writer, value.Source, LogicSerializerContext.Default.JsonNode);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know the context that these JsonConverters are being used, but I think we want a pattern in the Write methods that's similar to the read -- that is, respecting the full TypeInfoResolver chain that's in the JsonSerializerOptions passed in.

By switching from "options" to the JsonSerializerContext from our module only, we prevent a caller from extending the list.

As we discussed in Slack, I think we should have a Write overload that can help validate but that still passes the options along for any nested deserialization.

Comment thread JsonLogic.Tests/GithubTests.cs Outdated
Comment thread JsonLogic/Rule.cs
else if (node is JsonArray)
{
var data = node.Deserialize<List<Rule>>(options)!;
var data = node.Deserialize(LogicSerializerContext.Default.RuleArray)!;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we switch to options.Read here too?

public override void Write(Utf8JsonWriter writer, LiteralRule value, JsonSerializerOptions options)
{
JsonSerializer.Serialize(writer, value.Value, options);
JsonSerializer.Serialize(writer, value.Value, LogicSerializerContext.Default.JsonNode!);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same comment about wanting an options.Write overload here so we get type safety but also preserving options.

Comment thread JsonLogic/Rules/LooseEqualsRule.cs Outdated
Accumulator = accumulator
};
#pragma warning disable IL2026, IL3050
var item = JsonSerializer.SerializeToNode(intermediary, _options);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would a '_options.WriteToNode' helper be useful for type safety in cases like this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Yes it would.

@gregsdennis gregsdennis marked this pull request as draft January 28, 2024 06:32
@github-actions github-actions Bot removed the pkg:more label Jan 28, 2024
@gregsdennis

Copy link
Copy Markdown
Collaborator Author

The only thing this needs fixed is the null deserialization issue that apparently STJ v8 fixes.

@jevansaks jevansaks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some comments, but non-blocking if you want to merge this in.

Comment thread JsonLogic/Rule.cs
rule = args is null
? (Rule)JsonSerializer.Deserialize("[]", ruleType, options)!
: (Rule)args.Deserialize(ruleType, options)!;
? (Rule)JsonSerializer.Deserialize("[]", ruleType, LogicSerializerContext.Default)!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is technically a behavior change because we're not passing the options along.

I would recommend passing options and suppressing the warning, as unsavory as that sounds.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I actually just probably need to use the options extension. Likely just missed this.

Comment thread JsonLogic/RuleRegistry.cs

static RuleRegistry()
{
_rules = new ConcurrentDictionary<string, Type>(new Dictionary<string, Type>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Didn't one of the rules have two operator aliases for it? Eyeballing this I don't see any double types in the list.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it's the if rule. It's there.

@jevansaks

Copy link
Copy Markdown
Contributor

Here's a workaround for the null issue. Update Read in JsonSerializerOptionsExtensions.cs to include this slightly unsavory workaround:

	public static T? Read<T>(this JsonSerializerOptions options, ref Utf8JsonReader reader, JsonTypeInfo<T>? typeInfo = null)
	{
#if !NET8_0_OR_GREATER // Workaround for System.Text.Json 6.x missing fix for https://github.com/dotnet/runtime/issues/85172
		if (reader.TokenType == JsonTokenType.Null && typeof(T) == typeof(JsonNode))
		{
			return default(T);
		}
#endif
		return options.GetConverter<T>(typeInfo).Read(ref reader, typeof(T), options);
	}

@gregsdennis gregsdennis marked this pull request as ready for review January 29, 2024 07:33
@gregsdennis gregsdennis merged commit cda63dc into native-aot Jan 29, 2024
@gregsdennis gregsdennis deleted the native-aot-logic branch January 29, 2024 08:01
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.

2 participants