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

[dotnet] Add trimming attributes, address some trim warnings #14637

Open
wants to merge 10 commits into
base: trunk
Choose a base branch
from

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Oct 23, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

This improves the story for trimming/AOT using Selenium, specifically addressing non-JSON related warnings.

Motivation and Context

In furtherance of #14480

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • x] I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Other


Description

  • Enhanced the DevToolsDomains class to improve AOT compatibility by introducing a DomainType struct and using DynamicallyAccessedMembers attributes.
  • Improved the JsonEnumMemberConverter class by adding attributes for public fields and updating enum value retrieval for .NET 5.0 or greater.
  • Added a new file TrimmingAttributes.cs with custom attributes to support trimming and AOT compatibility, including conditional compilation for different .NET versions.
  • Updated the WebDriver.csproj file to include a conditional AOT compatibility property for future .NET versions.

Changes walkthrough 📝

Relevant files
Enhancement
DevToolsDomains.cs
Enhance DevToolsDomains for AOT compatibility                       

dotnet/src/webdriver/DevTools/DevToolsDomains.cs

  • Introduced DomainType struct to handle trimming.
  • Added DynamicallyAccessedMembers attribute for public constructors.
  • Updated dictionary to use DomainType instead of Type.
  • +18/-8   
    JsonEnumMemberConverter.cs
    Improve JsonEnumMemberConverter for AOT compatibility       

    dotnet/src/webdriver/DevTools/Json/JsonEnumMemberConverter.cs

  • Added DynamicallyAccessedMembers attribute for public fields.
  • Updated enum value retrieval for .NET 5.0 or greater.
  • +9/-4     
    TrimmingAttributes.cs
    Add custom trimming attributes for AOT support                     

    dotnet/src/webdriver/Internal/TrimmingAttributes.cs

  • Added custom attributes for trimming support.
  • Defined DynamicallyAccessedMembers and related attributes.
  • Implemented conditional compilation for different .NET versions.
  • +420/-0 
    Configuration changes
    WebDriver.csproj
    Update project file for potential AOT compatibility           

    dotnet/src/webdriver/WebDriver.csproj

  • Added conditional AOT compatibility property for future .NET versions.

  • +5/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    14480 - Partially compliant

    Fully compliant requirements:

    • Partially addresses AOT compatibility by adding trimming attributes and addressing some trim warnings

    Not compliant requirements:

    • Full AOT compatibility is not yet achieved
    • Specific components (W3C WebDriver, CDP, W3C BiDi) are not explicitly addressed in this PR
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Performance Impact
    The introduction of the DomainType struct and the use of DynamicallyAccessedMembers attributes may have an impact on runtime performance. This should be validated to ensure it doesn't introduce significant overhead.

    Conditional Compilation
    The use of conditional compilation (#if NET5_0_OR_GREATER) may lead to different behavior across .NET versions. Ensure that this doesn't introduce unexpected issues in older .NET versions.

    Incomplete Implementation
    The AOT compatibility property is commented out. This suggests that the AOT compatibility work is not complete and may need further attention in future PRs.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Oct 23, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use a read-only dictionary to prevent unintended modifications

    Consider using a readonly dictionary for SupportedDevToolsVersions to prevent
    accidental modifications during runtime.

    dotnet/src/webdriver/DevTools/DevToolsDomains.cs [37-43]

    -private static readonly Dictionary<int, DomainType> SupportedDevToolsVersions = new Dictionary<int, DomainType>()
    +private static readonly IReadOnlyDictionary<int, DomainType> SupportedDevToolsVersions = new Dictionary<int, DomainType>()
     {
         { 127, new DomainType(typeof(V127.V127Domains)) },
         { 129, new DomainType(typeof(V129.V129Domains)) },
         { 128, new DomainType(typeof(V128.V128Domains)) },
         { 85, new DomainType(typeof(V85.V85Domains)) }
     };
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Changing the dictionary to a read-only dictionary enhances code safety by preventing accidental modifications at runtime. This is a good practice for maintaining the integrity of static data structures.

    7
    Enhancement
    Use a more concise conditional compilation structure for better readability

    Consider using a switch expression for better readability and maintainability when
    handling different .NET versions.

    dotnet/src/webdriver/DevTools/Json/JsonEnumMemberConverter.cs [20-24]

    +var values = 
     #if NET5_0_OR_GREATER
    -TEnum[] values = Enum.GetValues<TEnum>();
    +    Enum.GetValues<TEnum>();
     #else
    -Array values = Enum.GetValues(type);
    +    Enum.GetValues(type);
     #endif
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion improves readability by simplifying the conditional compilation structure. It makes the code cleaner and easier to maintain without changing its functionality.

    6
    Use a conditional property group for future AOT compatibility

    Consider using a conditional property group for future AOT compatibility instead of
    commenting it out.

    dotnet/src/webdriver/WebDriver.csproj [45-48]

    -<!--TODO when AOT is ready https://github.com/SeleniumHQ/selenium/issues/14480-->
    -<!--<PropertyGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))">
    +<PropertyGroup Condition="'$(EnableAOTCompilation)' == 'true' AND $([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))">
       <IsAotCompatible>true</IsAotCompatible>
    -</PropertyGroup>-->
    +</PropertyGroup>
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion provides a more structured approach to handle future AOT compatibility, which could be beneficial when the feature becomes available. However, it is speculative and depends on future developments, which slightly reduces its immediate relevance.

    5

    💡 Need additional feedback ? start a PR chat

    };

    /// <summary>Workaround for trimming.</summary>
    Copy link
    Contributor Author

    @RenderMichael RenderMichael Oct 23, 2024

    Choose a reason for hiding this comment

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

    This is the kind of workaround necessary when using a Type as a generic argument that requires trimming annotations. Hope this is acceptable.

    TEnum[] values = Enum.GetValues<TEnum>();
    #else
    Array values = Enum.GetValues(type);
    #endif
    foreach (var value in values)
    Copy link
    Contributor Author

    @RenderMichael RenderMichael Oct 23, 2024

    Choose a reason for hiding this comment

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

    var here is actually TEnum in .NET, but object in .NET Standard. All use of value is either cast or are ToString()-ed, so there's no runtime difference.

    @nvborisenko
    Copy link
    Member

    I will start to review after basic #14574 is merged. I thought that CDP requires deeper knowledge, so on hold for now.

    @RenderMichael
    Copy link
    Contributor Author

    @nvborisenko Thanks!

    It looks like a lot of the work you're doing could be addressed if System.Text.Json implemented this feature request: dotnet/runtime#98038

    It doesn't have much support because this kind of conversion isn't roundtrippable. Maybe Selenium can make the case for this functionality there?

    @nvborisenko
    Copy link
    Member

    No, Source Generation in Json is very good. The problem I am trying to resolve is that many many years ago wire objects were serialized as objects. And the issue is that object is not compile time available meta type. Microsoft wrote great docs about it.

    You referenced to the issue, which is in open state...

    @RenderMichael
    Copy link
    Contributor Author

    @nvborisenko Yeah the object propagation makes things harder. I guess a deeper fix could be to propagate everything as JsonNode (or the derived JsonObject in place of Dictionary<string, object>. There's only a handful of places where the command parameters are exposed to users, like the the Command constructor.

    But, that would involve a lot of legwork. I would love to contribute any PRs if that's the direction this project wants to go down.

    @nvborisenko
    Copy link
    Member

    I am in communication with https://github.com/appium/appium team, they are testing #14574

    We should migrate "classic" before moving further.

    @nvborisenko
    Copy link
    Member

    could be to propagate everything as JsonNode

    Unfortunately it is big boom breaking change :(

    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.

    3 participants