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

NAOT compatibility #572

Open
euju-ms opened this issue Oct 29, 2024 · 5 comments
Open

NAOT compatibility #572

euju-ms opened this issue Oct 29, 2024 · 5 comments
Assignees

Comments

@euju-ms
Copy link
Contributor

euju-ms commented Oct 29, 2024

Here is what to include in your request to make sure we implement a solution as quickly as possible.

1. Description

Native AOT incompatibility with Utilities.To

When publishing an App with AOT enabled, following warnings appear indicating the AOT incompatibility:

HtmlAgilityPack.Utilities.To(Object,Type): Using member 'System.ComponentModel.TypeDescriptor.GetConverter(Object)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Generic TypeConverters may require the generic types to be annotated. For example, NullableConverter requires the underlying type to be DynamicallyAccessedMembers All. The Type of component cannot be statically discovered.

This can be easily fixed by making GetAttributeValue method generic and only do the conversion when a custom converter is provided. However, this does change the behavior of the original GetAttributeValue method so it's probably better to just modify the internal usages of the Utilities.To to use explicit converters and keep the original API the same in terms of behavior.

E.g.

Method in HtmlNode:

public T GetAttributeValueAot<T>(string name, T defaultValue, Func<string, T>? converter = null)
{
    if (name == null)
        throw new ArgumentNullException(nameof(name));

    if (!HasAttributes)
        return defaultValue;

    HtmlAttribute att = Attributes[name];
    if (att == null)
        return defaultValue;

    return att.Value is T value
        ? value
        : att.Value != null && converter != null
            ? converter(att.Value)
            : defaultValue;
}

Note that I can actually work around the AOT issues by not using the provided HtmlNode.GetAttributeValue method and using my own implementation since HtmlNode.Attributes and HtmlAttribute.Value are public.

2. Exception

Not applicable

3. Fiddle or Project

App must be published with AOT enabled for the warning to appear.
E.g.

<Project Sdk="Microsoft.NET.Sdk">
    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net8.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
	<RuntimeIdentifier>win-x64</RuntimeIdentifier>
    </PropertyGroup>

    <PropertyGroup>
        <PublishAot>true</PublishAot>
        <TrimmerSingleWarn>false</TrimmerSingleWarn>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="HtmlAgilityPack" Version="1.11.70" />
    </ItemGroup>
</Project>

And the code that triggers the warning will look like this:

using System;
using HtmlAgilityPack;
					
public class Program
{
	public static void Main()
	{
		Console.WriteLine("Hello World");
		HtmlNode node = HtmlNode.CreateNode("<div foo=\"hello\">test</div>");

		// Call to GetAttributeValue produces AOT in-compatibility warning because it calls into Utilities.To
		string value = node.GetAttributeValue("foo", "world");
	}
}

4. Any further technical details

Native AOT

@JonathanMagnan JonathanMagnan self-assigned this Oct 29, 2024
@JonathanMagnan
Copy link
Member

Hello @euju-ms ,

Thank you for reporting.

Do you think you could provide a PR? I will be happy to look at it.

I never did any AOT library, so you surely have more knowledge than me about what else should be modified.

Best Regards,

Jon

@euju-ms
Copy link
Contributor Author

euju-ms commented Oct 30, 2024

I can create the PR, but how do you want to deal with the old API?

I think the right way to move forward is to remove the support for HtmlNode.GetAttributeValue<T>(string name, T def), and only support the new API where it takes in an explicit converter as an argument. But maybe we can make the old API obsolete first?

Keeping the old API is fine as far as AOT goes because the warning will only appear when the old API is in use.
I'll create a draft PR then you can help me get it to be in the right state :)

As far as other AOT issues go, I'm not totally sure how many exists. I thought I could just set IsAotCompatible to true, and it would show the warnings for the project, but I wasn't able to get it working (NAOT deployment).

From a quick look, here are some other methods that are not AOT compatible:

  • HtmlNode.GetEncapsulatedData
  • Tools.GetPropertiesDefinedXPath
  • Tools.CreateIListOfType
  • Tools.IsInstantiable

All these warnings came up when I published an app that calls into GetEncapsulatedData.

Though, I want to focus on GetAttributeValue for the time being as this is mainly the problem I'm hitting :P.

By the way, if I were using HtmlAgilityPackage directly, I can actually avoid the issue by avoiding GetAttributeValue usages and getting the attribute value myself from HtmlNode.Attributes. However, I'm consuming a package that references HtmlAgilityPackage, so I wanted to fix the problem in the root.

@euju-ms
Copy link
Contributor Author

euju-ms commented Oct 30, 2024

Here's the PR: #573

@JonathanMagnan
Copy link
Member

Hello @euju-ms ,

The PR has been merged and released in v1.11.71

The only thing I removed is the Obsolete part: 77ae081

I don't feel the method should be obsolete and is only a problem for people using AOT which is a big minority of HAP user.

Let me know if everything work as expected

And thank you for the PR ;)

Best Regards,

Jon

@euju-ms
Copy link
Contributor Author

euju-ms commented Nov 11, 2024

@JonathanMagnan
Makes sense to me. Thank you!!

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

No branches or pull requests

2 participants