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

support net 7's native aot #740

Closed
ivanjx opened this issue Nov 10, 2022 · 50 comments
Closed

support net 7's native aot #740

ivanjx opened this issue Nov 10, 2022 · 50 comments

Comments

@ivanjx
Copy link

ivanjx commented Nov 10, 2022

Is your feature request related to a problem? Please describe.
i cant use this library with native aot on net 7 because this uses system.reflection.

Describe the solution you'd like
a solution similar to system.text.json is to use source generator.

Describe alternatives you've considered
excluding the types that i want to deserialize in rd.xml works.

Additional context
https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot/

@EdwardCooke
Copy link
Collaborator

When I get some time I’ll work on this. Not sure what it will take because we heavily use reflection, but supporting current frameworks and features is important. In the meantime, if you want to take a look at it we are happy to accept contributions.

@EdwardCooke
Copy link
Collaborator

I’m still working on this. I got it to instantiate a class. It’s taking a lot of attributes on the methods so the classes and other objects don’t get stripped away. It also requires an attribute added to the class that gets deserialized so all of your yaml objects will require that as well.

@EdwardCooke
Copy link
Collaborator

Well that is an interesting idea @ivanjx . When I get some more time, I'll look in to them.

@EdwardCooke
Copy link
Collaborator

Just to give you an update on this. I have a code generator/analyzer that will generate static factories for classes and property/field accessesors. It works for primitive types and anything marked with a YamlSerializable attribute. Generics do not work yet, so things like Dictionaries won't serialize/deserialize correctly.

I have not merged any of this in yet since it's not ready just yet.

I just wanted to let you know that I am making progress on getting this done. I am hoping to have this all done and ready by end of week.

@EdwardCooke
Copy link
Collaborator

If you want to check out what I've got, here's my branch: https://github.com/EdwardCooke/YamlDotNet/tree/ec-aot

The demo app that shows how to implement it is in the YamlDotNet.Core7AoTCompileTest project. It's pretty simple.

@ivanjx
Copy link
Author

ivanjx commented Dec 12, 2022

thank you for your great job!

@EdwardCooke
Copy link
Collaborator

Pending a review of the PR I just made, we'll have the code in place in the main YamlDotNet library to support the AoT/Trimmed version of the library. Once approved and approved and merged in, I will push up a NuGet package containing the analyzer that you can try out to make sure it meets your needs.

@EdwardCooke
Copy link
Collaborator

K, I just merged in my PR with necessary changes to the library for the analyzer. I'll try and push up a test instance of the analyzer, hopefully this week, But with Christmas coming up, it may not happen. In the meantime, you are more than welcome to test out the analyzer on your own, Just take a look at the Core7AoTCompileTest project to see how to reference the project.

@EdwardCooke
Copy link
Collaborator

Ok, I have the code generator pushed up. The package name is Vecc.YamlDotNet.Analyzers.StaticGenerator, use version 0.0.2.

Here is an example, working code
https://github.com/EdwardCooke/YamlDotNet.StaticSerializerExample

To run that repo example
Checkout the code
then run

dotnet publish -c release
ConsoleApp1\bin\Release\net7.0\win-x64\ConsoleApp1.exe

@ivanjx
Copy link
Author

ivanjx commented Dec 19, 2022

which part of the library that throws IL3050? is it possible to remove or isolate those?

@EdwardCooke
Copy link
Collaborator

The warnings are next on the list but won’t be until later. If you want to take a stab at it that would be great. What I was thinking was try a static builder that doesn’t reference the reflection based stuff and see if it goes away.

@EdwardCooke
Copy link
Collaborator

Ok. I got the warnings resolved. Here's what needs to be done to check it out

Update the static generator package to the latest, 0.0.3
Use the Vecc.YamlDotNet package, version 0.0.1-aot (you'll need to do the pre-release) instead of YamlDotNet (there's changes I don't want to merge in until we make sure it solves the needs)

When doing an AoT/trimmed library, instead of using SerializerBuilder and DeserializeBuilder use the StaticSerializerBuilder and StaticDeserializerBuilder. There should be a couple of warnings that will show up when using the current SerializerBuilder and DeserializerBuilder and AoT/trimming. The static ones don't use reflection anywhere, thus, no warnings. There was a bunch of areas in YamlDotNet that was generating generic classes when a custom dictionary object didn't implement IDictionary or a custom list didn't implement IList. That's probably the biggest limitation in the static version, and I couldn't find a way around it.

My testing showed that the static builders worked for the simple serialization without showing any warnings. I'll be working on setting up unit tests for the static implementation over the next bit. If you don't find any issues, I'll bring those changes in to the master branch when the unit tests are complete and push a new version.

There are some minor breaking changes in some of the underlying interfaces/classes, namely the IObjectFactory, there is a new class, ObjectFactoryBase that can be inherited from, which will cover the current implementation without needing to write any additional code or other changes. If you manually instantiated some of the node deserializers, there will need to be code changes, they now take in a ITypeFactory and/or IObjectFactory object.

Those changes were necessary to break out the rest of the reflection implementation so it could be replaced with the static implementation.

@EdwardCooke
Copy link
Collaborator

Have you had a chance to try out the versions I posted? Did they do what you needed?

@ivanjx
Copy link
Author

ivanjx commented Jan 1, 2023

hi @EdwardCooke

i will try them out once im back from vacation as soon as possible. i will try this or if you have updated steps of how to try out your changes please let me know. thanks for your great work!

@ivanjx
Copy link
Author

ivanjx commented Jan 10, 2023

sorry for the late response.

i have added these to my csproj (netstandard2.0) and removed the original ones:

<PackageReference Include="Vecc.YamlDotNet" Version="0.0.1-aot" />
<PackageReference Include="Vecc.YamlDotNet.Analyzers.StaticGenerator" Version="0.0.3" />

however i cant seem to be able to resolve StaticDeserializerBuilder from your example on my project. am i missing something?

image

image

my usings:
image

@ivanjx
Copy link
Author

ivanjx commented Jan 10, 2023

i also got these when i tried to build:

/home/user/Documents/projects/myproj/myproj.Shared/YamlDotNet.Analyzers.StaticGenerator/YamlDotNet.Analyzers.StaticGenerator.TypeFactoryGenerator/YamlDotNetAutoGraph.g.cs(18,31): error CS0115: 'StaticObjectFactory.CreateArray(Type, int)': no suitable method found to override [/home/user/Documents/projects/myproj/myproj.Shared/myproj.Shared.csproj]
/home/user/Documents/projects/myproj/myproj.Shared/YamlDotNet.Analyzers.StaticGenerator/YamlDotNet.Analyzers.StaticGenerator.TypeFactoryGenerator/YamlDotNetAutoGraph.g.cs(26,30): error CS0115: 'StaticObjectFactory.IsArray(Type)': no suitable method found to override [/home/user/Documents/projects/myproj/myproj.Shared/myproj.Shared.csproj]

@ivanjx
Copy link
Author

ivanjx commented Jan 10, 2023

this is my class:

[YamlSerializable]
public record InternalTestYamlEntry
{
    [YamlMember(Alias = "x1")]
    public string? X1
    {
        get;
        init;
    }

    [YamlMember(Alias = "x2")]
    public string? X2
    {
        get;
        init;
    }

    [YamlMember(Alias = "x3")]
    public long? X3
    {
        get;
        init;
    }

    [YamlMember(Alias = "x4")]
    public long? X4
    {
        get;
        init;
    }

    [YamlMember(Alias = "x5")]
    public string? X5
    {
        get;
        init;
    }

    [YamlMember(Alias = "X6")]
    public string? X6
    {
        get;
        init;
    }
}
InternalTestYamlEntry[] yamlEntries =
            deserializer.Deserialize<InternalTestYamlEntry[]>(yamlStr);

@EdwardCooke
Copy link
Collaborator

No worries on the late response, I hope your vacation was great!

It almost looks like your project may still be referencing the official yamldotnet library somewhere. I just pulled my nuget package manually and inspected it with ILSpy and the classes are there.

Also, records won't work with the static serializer, if I recall they have to be fully initialized when constructed, as in, we can't set the properties after the object is created. The static generator also doesn't recognize them as objects that can be deserialized. I can fix that, but, since the compiler won't let you set properties on a record, not sure if it's worth that, except for the serializing piece. The reason they work with the non-aot style of the deserializer is that it bypasses the compiler and uses reflection, which does work to set properties. Not much I can do there for deserialization, sorry.

This example did expose a bug in the code generator though, nullable properties don't get generated correctly and it throws an error when trying to compile. The error you will receive is:

The typeof operator cannot be used on a nullable reference type

I'll work on that and get a new version of the static generator out.

@ivanjx
Copy link
Author

ivanjx commented Jan 10, 2023

ok then seems you are right about the static deserializer class since the errors i see on the build are just no suitable method found to override

@EdwardCooke
Copy link
Collaborator

I just published 0.0.2-aot of Vecc.YamlDotNet and 0.0.4 of Vecc.YamlDotNet.Analyzers.StaticGenerator to NuGet. Give those a try, the nullable properties should be resolved now. I have your example class added (as a class, not a record) and it runs and works.

@ivanjx
Copy link
Author

ivanjx commented Jan 10, 2023

i get these warnings:

warning CS8669: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context. Auto-generated code requires an explicit '#nullable' directive in source.
warning CS0436: The type 'StaticTypeInspector' in 'YamlDotNet.Analyzers.StaticGenerator/YamlDotNet.Analyzers.StaticGenerator.TypeFactoryGenerator/YamlDotNetAutoGraph.g.cs' conflicts with the imported type 'StaticTypeInspector' in 'myproject,

@ivanjx
Copy link
Author

ivanjx commented Jan 11, 2023

ok it seems that the second warning is because i am using the deserializer in a shared project and i use that shared project in another project (avalonia) so that the source is generated twice on both projects. build with dotnet build /p:EmitCompilerGeneratedFiles=true and check the obj folder for each projects to see what i mean. the system.text.json does not have this behavior so i think it is possible to not emit generated files on both projects.

@ivanjx
Copy link
Author

ivanjx commented Jan 11, 2023

this is the generated source for the project that is not using the YamlDotNet. seems empty here while in the shared project it generates with the information of the types.

using System;
using System.Collections.Generic;
namespace YamlDotNet.Static
{
    public partial class StaticContext : YamlDotNet.Serialization.StaticContext
    {
        public YamlDotNet.Serialization.ObjectFactories.StaticObjectFactory ObjectFactory { get; } = new StaticObjectFactory();
        public StaticTypeInspector TypeInspector { get; } = new StaticTypeInspector();
        public override YamlDotNet.Serialization.ObjectFactories.StaticObjectFactory GetFactory() => ObjectFactory;
        public override YamlDotNet.Serialization.ITypeInspector GetTypeInspector() => TypeInspector;
    }
    public class StaticObjectFactory : YamlDotNet.Serialization.ObjectFactories.StaticObjectFactory
    {
        public override object Create(Type type)
        {
            throw new ArgumentOutOfRangeException("Unknown type: " + type.ToString());
        }
        public override Array CreateArray(Type type, int count)
        {
            throw new ArgumentOutOfRangeException("Unknown type: " + type.ToString());
        }
        public override bool IsDictionary(Type type)
        {
            return false;
        }
        public override bool IsArray(Type type)
        {
            return false;
        }
        public override bool IsList(Type type)
        {
            return false;
        }
        public override Type GetKeyType(Type type)
        {
            throw new ArgumentOutOfRangeException("Unknown type: " + type.ToString());
        }
        public override Type GetValueType(Type type)
        {
            throw new ArgumentOutOfRangeException("Unknown type: " + type.ToString());
        }
    }
    public class StaticPropertyDescriptor : YamlDotNet.Serialization.IPropertyDescriptor
    {
        private YamlDotNet.Serialization.IObjectAccessor _accessor;
        private readonly Attribute[] _attributes;
        public string Name { get; }
        public bool CanWrite { get; }
        public Type Type { get; }
        public Type TypeOverride { get; set; }
        public int Order { get; set; }
        public YamlDotNet.Core.ScalarStyle ScalarStyle { get; set; }
        public T GetCustomAttribute<T>() where T : Attribute
        {
            foreach (var attribute in this._attributes)
            {
                if (attribute.GetType() == typeof(T)) { return (T)attribute; }
            }
            return null;
        }
        public YamlDotNet.Serialization.IObjectDescriptor Read(object target)
        {
            return new YamlDotNet.Serialization.ObjectDescriptor(_accessor.Read(Name, target), Type, Type, this.ScalarStyle);
        }
        public void Write(object target, object value)
        {
            _accessor.Set(Name, target, value);
        }
        public StaticPropertyDescriptor(YamlDotNet.Serialization.IObjectAccessor accessor, string name, bool canWrite, Type type, Attribute[] attributes)
        {
            this._accessor = accessor;
            this._attributes = attributes;
            this.Name = name;
            this.CanWrite = canWrite;
            this.Type = type;
            this.ScalarStyle = YamlDotNet.Core.ScalarStyle.Any;
        }
    }
    public class StaticTypeInspector : YamlDotNet.Serialization.ITypeInspector
    {
        public IEnumerable<YamlDotNet.Serialization.IPropertyDescriptor> GetProperties(Type type, object container)
        {
            return new List<YamlDotNet.Serialization.IPropertyDescriptor>();
        }
        public YamlDotNet.Serialization.IPropertyDescriptor GetProperty(Type type, object container, string name, bool ignoreUnmatched)
        {
            if (ignoreUnmatched) return null;
            throw new ArgumentOutOfRangeException("Field or property not valid: " + name);
        }
    }
}

@ivanjx
Copy link
Author

ivanjx commented Jan 11, 2023

as for the CS8669 warning you can just emit this at the top of the generated source file:

#nullable enable
...

as shown here

@EdwardCooke
Copy link
Collaborator

I’ll fix the warnings when I get some time. Thanks for the link.

@ivanjx
Copy link
Author

ivanjx commented Jan 11, 2023

great. i will try it tomorrow if possible. also does your deserializer support array? i would be using it like deserializer.Deserialize<InternalTestYamlEntry[]>(yamlStr); instead of deserializer.Deserialize<InternalTestYamlEntry>(yamlStr); with the same data structure as before.

@EdwardCooke
Copy link
Collaborator

It should handle it without an issue. I will test that when I get a minute today to verify.

@JJ11teen
Copy link
Contributor

Hey both,

I'm also interested in aot support for YamlDotNet and have got your ec-aotwarnings branch working locally @EdwardCooke.

One issue I've found is that classes with inheritance are not able to deserialise inherited members.

I was able to fix the issue by changing the following lines:
https://github.com/EdwardCooke/YamlDotNet/blob/2b8d230a1255e53a08bda79016ed33128f2bb449/YamlDotNet.Analyzers.StaticGenerator/ClassSyntaxReceiver.cs#L62-L82

var members = classSymbol.GetMembers();
foreach (var member in members)
{
    if (member.IsStatic ||
        member.DeclaredAccessibility != Accessibility.Public ||
        member.GetAttributes().Any(x => x.AttributeClass!.ToDisplayString() == "YamlDotNet.Serialization.YamlIgnoreAttribute"))
    {
        continue;
    }


    if (member is IPropertySymbol propertySymbol)
    {
        classObject.PropertySymbols.Add(propertySymbol);
        CheckForSupportedGeneric(propertySymbol.Type);
    }
    else if (member is IFieldSymbol fieldSymbol)
    {
        classObject.FieldSymbols.Add(fieldSymbol);
        CheckForSupportedGeneric(fieldSymbol.Type);
    }
}

To include a while loop that loops down through base classes:

var currentSymbol = classSymbol;
do
{
    var members = currentSymbol.GetMembers();
    foreach (var member in members)
    {
        if (member.IsStatic ||
            member.DeclaredAccessibility != Accessibility.Public ||
            member.GetAttributes().Any(x => x.AttributeClass!.ToDisplayString() == "YamlDotNet.Serialization.YamlIgnoreAttribute"))
        {
            continue;
        }

        if (member is IPropertySymbol propertySymbol)
        {
            classObject.PropertySymbols.Add(propertySymbol);
            CheckForSupportedGeneric(propertySymbol.Type);
        }
        else if (member is IFieldSymbol fieldSymbol)
        {
            classObject.FieldSymbols.Add(fieldSymbol);
            CheckForSupportedGeneric(fieldSymbol.Type);
        }
    }

    currentSymbol = currentSymbol.BaseType;
}
while (currentSymbol.BaseType is not null);

I've not used source generators before and I'm not across YamlDotNet internals so I have no idea if this is a comprehensive solution or not, but hopefully its useful to someone 😅

Is there anything I can do to help/test aot support for YamlDotNet? It's a very useful feature for me 😊

@EdwardCooke
Copy link
Collaborator

Thanks for finding that. I’ll make the change and get an updated test library up shortly. There’s this now and another one to support list and array as base objects that I want to do. Hopefully today.

@EdwardCooke
Copy link
Collaborator

Check out 0.0.5-aot of Vecc.YamlDotNet and 0.0.7 of Vecc.YamlDotNet.Analyzers.StaticGenerator. That should contain the array fix and the inherited class fix. I'll merge this into master after I build up the test project for making sure it all works.

@EdwardCooke
Copy link
Collaborator

Got my tests done, found a bug with using List as the root instead of array, using 0.0.6-aot of Vecc.YamlDotNet and 0.0.8 of Vecc.YamlDotNet.Analyzers.StaticGenerator

@JJ11teen
Copy link
Contributor

JJ11teen commented Feb 1, 2023

I'm getting a build error with the new versions @EdwardCooke, it may be something stale in my pipeline but I think I've cleared everything and it still persists. Any help would be greatly appreciated (:

<my_project>/YamlDotNet.Analyzers.StaticGenerator.TypeFactoryGenerator/YamlDotNetAutoGraph.g.cs(53,31): error CS0115: 'StaticObjectFactory.CreateArray(Type, int)': no suitable method found to override [<my_project>]
<my_project>/YamlDotNet.Analyzers.StaticGenerator.TypeFactoryGenerator/YamlDotNetAutoGraph.g.cs(76,30): error CS0115: 'StaticObjectFactory.IsArray(Type)': no suitable method found to override [<my_project>]

When I pull your aot-warnings I can see where CreateArray & IsArray are being overloaded in the source generator, and I can also see where they are defined in the standard StaticObjectFactory. I guess I'm just double checking these latest versions were both built from that branch? If so any other ideas you have for things I might be doing wrong? I suspect the build process is picking up an old version of YamlDotNet without those methods from somewhere...

@EdwardCooke
Copy link
Collaborator

I’ll push them up again with new versions later tonight just to make sure they are latest.

@JJ11teen
Copy link
Contributor

JJ11teen commented Feb 2, 2023

Thank you! 🙏

@EdwardCooke
Copy link
Collaborator

There should be a 1.0.0-aot version of both packages up there now.

@EdwardCooke
Copy link
Collaborator

The code is merged in master, however, the nuget api key is most likely scoped to only allow the YamlDotNet library to be pushed up since it failed trying to push up the new static generator package. That will need to be allowed by @aaubry, until it is, there won't be an "official" one, but, I will keep mine updated with the official code base.

@EdwardCooke
Copy link
Collaborator

Version 13.0.0 of the static generator and yamldotnet is available.

@ivanjx
Copy link
Author

ivanjx commented Feb 13, 2023

@EdwardCooke
is it still required to use Vecc.YamlDotNet.Analyzers.StaticGenerator with version 13.0.0?

@ivanjx
Copy link
Author

ivanjx commented Feb 13, 2023

closing this as i got it to work with version 13.0.0

@ivanjx ivanjx closed this as completed Feb 13, 2023
@EdwardCooke
Copy link
Collaborator

@ivanjx I suspect you already know, yes, you still need to use the Vecc static analyzer package.

@ivanjx
Copy link
Author

ivanjx commented Feb 13, 2023

one more issue is that the static generator does not support nested class, for example:

public class A
{
    [YamlSerializable]
    public class B
    {
    }
}

currently it cannot find B inside A. sometimes i want to do this to isolate B from others. feel free to close if this is not planned. thanks.

@ivanjx ivanjx reopened this Feb 13, 2023
@EdwardCooke
Copy link
Collaborator

I’ll see what I can do about adding that. I think you could mark the class as internal which would at least hide it from other libraries.

@EdwardCooke
Copy link
Collaborator

A new version has been pushed up supporting nested classes. If theres any additional work needed, open up a new issue, this one's kind of long.

@prochnowc
Copy link
Contributor

@EdwardCooke would it be possible to add support for multiple static contexts ? With System.Text.Json it's possible to use multiple statically generated SerializerContext instances. This is really helpfull when having multiple Model assemblies where each contains it's own static context.

@EdwardCooke
Copy link
Collaborator

You want the serializer/deserializer to work with multiple of them? I think that could actually be relatively easy to do. We would need to add a method to the serializer context to return whether the type is supported, then create a combined serializer context that accepts all of the contexts and does a for each. Or something that.

Do you want to to do it? It might be a few days before I can get to it.

@prochnowc
Copy link
Contributor

Yes that was my idea - I already did my own "combined context" but I didnt like to catch the exception to move on to the next context. I will submit a PR ...

@EdwardCooke
Copy link
Collaborator

Great! If you can open a new issue and reference that it would be fantastic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants