[dotnet] Fully annotate `Command` for AOT support #15527

RenderMichael · 2025-03-28T05:35:25Z

User description

AOT warnings are user-facing, even if those warnings are within Selenium's own code. For that reason, we should ensure we are not emitting any warnings unless such code paths are annotated as AOT-unsafe.

Motivation and Context

Contributes to #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

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

Description

Added AOT-friendly annotations to the Command class.
Refactored JsonSerializerOptions into a static nested class for safety.
Introduced helper methods for parameter handling in Command.
Updated HttpCommandInfo to use new parameter handling methods.

Changes walkthrough 📝

Relevant files

Enhancement

Command.cs Refactor `Command` class for AOT compatibility dotnet/src/webdriver/Command.cs Added AOT and trimming annotations to methods and properties. Refactored `JsonSerializerOptions` into a static nested class. Introduced `HasParameters` and `TryGetValueAndRemoveIfNotNull` helper methods. Updated parameter handling logic for better AOT support.	+50/-12
HttpCommandInfo.cs Update parameter handling in `HttpCommandInfo` dotnet/src/webdriver/HttpCommandInfo.cs Replaced direct parameter access with `HasParameters` and `TryGetValueAndRemoveIfNotNull`. Improved parameter extraction logic for AOT compatibility.	+3/-7

Need help?
Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
Check out the documentation for more information.

qodo-merge-pro · 2025-03-28T05:35:49Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 14480 - PR Code Verified Compliant requirements: Contributes to making Selenium .NET library compatible with Native AOT Deployment Addresses AOT-unfriendly code in the Command class Requires further human verification: Verify that the changes actually resolve runtime failures when applications using Selenium are published as native AOT Confirm that all necessary AOT annotations have been added to the Command class
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review AOT Annotation Completeness The PR adds AOT annotations to the Command class, but it's important to verify that all methods that could potentially use reflection or dynamic code generation are properly annotated with RequiresUnreferencedCode and RequiresDynamicCode attributes. [RequiresUnreferencedCode("Adding untyped parameter values for JSON serialization has best-effort AOT support. Ensure only Selenium types and well-known .NET types are added, or use the overload that takes pre-serialized string jsonParameters for guaranteed AOT compatibility.")] [RequiresDynamicCode("Adding untyped parameter values for JSON serialization has best-effort AOT support. Ensure only Selenium types and well-known .NET types are added, or use the overload that takes pre-serialized string jsonParameters for guaranteed AOT compatibility.")] public Command(SessionId? sessionId, string name, Dictionary<string, object?>? parameters) Parameter Handling The new TryGetValueAndRemoveIfNotNull method changes the behavior of parameter handling. Verify that this change doesn't affect existing functionality, especially in edge cases with null values. internal bool TryGetValueAndRemoveIfNotNull(string key, [NotNullWhen(true)] out object? value) { if (this._parameters.TryGetValue(key, out value)) { if (value is not null) { this._parameters.Remove(key); return true; } } return false; }

qodo-merge-pro · 2025-03-28T05:36:09Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Set output parameter consistently The method should set `value` to null when returning false to ensure consistent behavior. Currently, if the key exists but the value is null, the method returns false but `value` remains set to null from TryGetValue, which could be confusing. dotnet/src/webdriver/Command.cs [128-140] internal bool TryGetValueAndRemoveIfNotNull(string key, [NotNullWhen(true)] out object? value) { if (this._parameters.TryGetValue(key, out value)) { if (value is not null) { this._parameters.Remove(key); return true; } } + value = null; return false; } Apply this suggestion Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies an issue with the out parameter behavior. When returning false, explicitly setting value=null ensures consistent behavior regardless of whether the key exists with a null value or doesn't exist at all, which improves method reliability and prevents potential bugs.	Medium
Update

RenderMichael · 2025-03-28T05:36:16Z

dotnet/src/webdriver/Command.cs

@@ -31,24 +32,33 @@ namespace OpenQA.Selenium
    /// </summary>
    public class Command


Best viewed with whitespace off

nvborisenko · 2025-05-20T18:08:58Z

dotnet/src/webdriver/Command.cs

-    private readonly static JsonSerializerOptions s_jsonSerializerOptions = new()
+    [UnconditionalSuppressMessage("Trimming", "IL2026", Justification = $"All trimming-unsafe access points to {nameof(s_jsonSerializerOptions)} are annotated as such")]
+    [UnconditionalSuppressMessage("AOT", "IL3050", Justification = $"All AOT-unsafe access points to {nameof(s_jsonSerializerOptions)} are annotated as such")]
+    private static class JsonOptionsHolder


Any reason to introduce dummy class? Just to hold some object used only in here? I think standard field is good.

Fields cannot be AOT-annotated, only types and methods. Having a “holder” type is common when you want to annotate specific fields as AOT-unsafe but not everything in the type.

nvborisenko · 2025-05-20T18:09:38Z

dotnet/src/webdriver/WebDriver.cs

@@ -40,7 +40,7 @@ public class WebDriver : IWebDriver, ISearchContext, IJavaScriptExecutor, IFinds
    /// </summary>
    protected static readonly TimeSpan DefaultCommandTimeout = TimeSpan.FromSeconds(60);
    private IFileDetector fileDetector = new DefaultFileDetector();
-    private NetworkManager network;
+    private NetworkManager? network;


Unintentional change?

It’s intentional and correct (it is lazy-initialized) but not necessarily relevant to this PR. Just one more internal warning gone.

nvborisenko · 2025-05-20T18:11:54Z

dotnet/src/webdriver/Command.cs

        get
        {
-            if (this.Parameters != null && this.Parameters.Count > 0)


this.Parameters?.Count > 0 pretty simple to not introduce new method

this.Parameters is an AOT-unsafe member. We must handle the logic inside Command for the AOT annotations.

nvborisenko · 2025-05-20T18:14:43Z

dotnet/src/webdriver/Command.cs

+                }
+                catch (NotSupportedException ex)
+                {
+                    throw new WebDriverException("Attempted to serialize an unsupported type. Ensure you are using Selenium types, or well-known .NET types such as Dictionary<string, object> and object[]", ex);


I thought we support any type, falling back to reflection based if not in the json context.

Not if we are AOT-compiled. the reflection fallback will not work. That is the only case this NotSupportedException is thrown.

Then we are not AOT-compatible. Users will don't see warnings at compile time, but will face issues at runtime. And what we?.. just suppress it. It is bad :(

I don't see good solution as a quick win, right solution would be avoid any object type and be strongly typed?

nvborisenko · 2025-05-20T18:19:26Z

dotnet/src/webdriver/HttpCommandInfo.cs

        {
            // Extract the URL parameter, and remove it from the parameters dictionary
            // so it doesn't get transmitted as a JSON parameter.
-            if (commandToExecute.Parameters.TryGetValue(propertyName, out var propertyValueObject))
+            if (commandToExecute.TryGetValueAndRemoveIfNotNull(propertyName, out var propertyValueObject))


Hm, interesting situation. Transmitter checks each time whether some "infected" parameter should be removed. What are these cases? I hope we can just fix it on consumer side. Based on comments, somebody tries to send url, thus we should fix this somebody.

The case is for parameters in the URL. This extracts those values from the dictionary and uses them in the URL instead of the parameter body.

nvborisenko · 2025-05-21T22:00:30Z

This is show stopper: #15527 (comment)

[dotnet] Fully annotate Command for AOT support

b142f52

qodo-merge-pro bot added the Review effort 2/5 label Mar 28, 2025

RenderMichael commented Mar 28, 2025

View reviewed changes

RenderMichael requested a review from nvborisenko March 28, 2025 05:37

RenderMichael added 6 commits March 28, 2025 02:04

Avoid silent failures in AOT when type is missing

8276506

minimize diffs

5c6b6ab

Merge branch 'trunk' into aot-command

bd8ffb3

Merge branch 'trunk' into aot-command

3f66a71

merge trunk

cebf1d2

fix whitespace

0742e3e

nvborisenko reviewed May 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dotnet] Fully annotate `Command` for AOT support #15527

[dotnet] Fully annotate `Command` for AOT support #15527

RenderMichael commented Mar 28, 2025 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Mar 28, 2025

qodo-merge-pro bot commented Mar 28, 2025 •

edited

Loading

RenderMichael Mar 28, 2025

nvborisenko May 20, 2025

RenderMichael May 21, 2025

nvborisenko May 20, 2025

RenderMichael May 21, 2025

nvborisenko May 20, 2025

RenderMichael May 21, 2025

nvborisenko May 20, 2025

RenderMichael May 21, 2025

nvborisenko May 21, 2025

nvborisenko May 20, 2025

RenderMichael May 21, 2025

nvborisenko commented May 21, 2025

		@@ -31,24 +32,33 @@ namespace OpenQA.Selenium
		/// </summary>
		public class Command

[dotnet] Fully annotate Command for AOT support #15527

Are you sure you want to change the base?

[dotnet] Fully annotate Command for AOT support #15527

Conversation

RenderMichael commented Mar 28, 2025 • edited by qodo-merge-pro bot Loading

User description

Motivation and Context

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

qodo-merge-pro bot commented Mar 28, 2025

PR Reviewer Guide 🔍

qodo-merge-pro bot commented Mar 28, 2025 • edited Loading

PR Code Suggestions ✨

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvborisenko commented May 21, 2025

[dotnet] Fully annotate `Command` for AOT support #15527

[dotnet] Fully annotate `Command` for AOT support #15527

RenderMichael commented Mar 28, 2025 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Mar 28, 2025 •

edited

Loading