[dotnet] Increment WebDriver towards nullability

[RenderMichael]

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.

Increment WebDriver towards nullability

Motivation and Context

Contributes to #14640

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, Bug fix

---

Description

• Incrementally refactored WebDriver to support nullability annotations.

• Improved error handling with null checks and exceptions.

• Enhanced code readability by simplifying property definitions.

• Updated WebElementFactory methods to handle nullable dictionaries.

---

Changes walkthrough 📝

Relevant files

Enhancement

WebDriver.cs Refactored `WebDriver` for nullability and error handling dotnet/src/webdriver/WebDriver.cs Added nullability annotations to properties and methods. Replaced manual null checks with null-coalescing operators. Simplified property definitions using expression-bodied members. Improved error handling with specific exceptions for null values. +105/-127 WebElementFactory.cs Enhanced `WebElementFactory` for nullability support          dotnet/src/webdriver/WebElementFactory.cs Updated methods to handle nullable dictionaries. Added null checks and exceptions for better error handling. Ensured compatibility with nullable reference types. +5/-5

---

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]

PR Reviewer Guide 🔍

(Review updated until commit 0226224)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Null Reference The ParseJavaScriptReturnValue method may return null without proper initialization of the returnValue variable, which could lead to NullReferenceException when the value is used. private object? ParseJavaScriptReturnValue(object? responseValue) { object? returnValue; if (responseValue is Dictionary<string, object?> resultAsDictionary) { if (this.elementFactory.ContainsElementReference(resultAsDictionary)) { returnValue = this.elementFactory.CreateElement(resultAsDictionary); } else if (ShadowRoot.TryCreate(this, resultAsDictionary, out ShadowRoot? shadowRoot)) { returnValue = shadowRoot; } Type Safety The cast to IWebElement in the foreach loop assumes the listItem is non-null and can be cast to IWebElement without checking, which could throw InvalidCastException. foreach (object? listItem in toReturn) { elementList.Add((IWebElement)listItem!); } Null Check The GetElementId method forces non-null return with ! operator without proper validation that ToString() actually returned non-null string. string? elementId = elementIdObj?.ToString(); if (string.IsNullOrEmpty(elementId)) { throw new InvalidOperationException("The specified element ID is either null or the empty string."); } return elementId!;

[qodo-merge-pro]

PR Code Suggestions ✨

Latest suggestions up to 0226224
Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	✅ Add null checks for response Suggestion Impact: The commit implements null checking for response values, but uses a different approach with EnsureValueIsNotNull() method instead of the suggested null conditional operators code diff: Response commandResponse = this.Execute(DriverCommand.GetPageSource, null); + commandResponse.EnsureValueIsNotNull(); return commandResponse.Value.ToString(); The PageSource property should handle null responses from Execute() to prevent potential NullReferenceException when accessing Value property. dotnet/src/webdriver/WebDriver.cs [133-140] public string PageSource { get { Response commandResponse = this.Execute(DriverCommand.GetPageSource, null); - - return commandResponse.Value.ToString(); + return commandResponse?.Value?.ToString() ?? string.Empty; } } Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion prevents potential NullReferenceException by adding proper null checks when accessing response values. This is an important defensive programming fix for a real potential bug.	8
	✅ Add null check for handles array Suggestion Impact: The commit implements a different approach to handle null values by adding a call to EnsureValueIsNotNull() before accessing the Value property, which achieves the same goal of preventing NullReferenceException code diff: + + commandResponse.EnsureValueIsNotNull(); object[] handles = (object[])commandResponse.Value; The WindowHandles property should validate that handles is not null before attempting to create a new List with its length to prevent potential NullReferenceException. dotnet/src/webdriver/WebDriver.cs [165-166] -object[] handles = (object[])commandResponse.Value; +object[] handles = (object[])commandResponse.Value ?? Array.Empty<object>(); List<string> handleList = new List<string>(handles.Length); Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion prevents a potential NullReferenceException by providing a fallback empty array when the response value is null. This is an important defensive programming improvement.	7
General	Improve null response error handling The GetElementFromResponse() method should handle null responses by throwing a more specific exception like NoSuchElementException with a descriptive message, rather than letting a potential NullReferenceException occur. dotnet/src/webdriver/WebDriver.cs [523-528] internal IWebElement? GetElementFromResponse(Response response) { if (response == null) { - throw new NoSuchElementException(); + throw new NoSuchElementException("Response was null when attempting to find element"); } Apply this suggestion Suggestion importance[1-10]: 6 Why: Adding a descriptive error message helps with debugging by providing more context about why the NoSuchElementException occurred. This is a moderate improvement for error handling and maintainability.	6
Learned best practice	Add null validation for required parameters to prevent NullReferenceExceptions and provide clear error messages The ExecuteScriptCommand method accepts nullable parameters but doesn't validate them properly. Add null validation for the script parameter since it's required for executing JavaScript commands. This will prevent potential NullReferenceExceptions and provide clearer error messages. dotnet/src/webdriver/WebDriver.cs [872-877] protected object? ExecuteScriptCommand(string script, string commandName, params object?[]? args) { + ArgumentNullException.ThrowIfNull(script, nameof(script)); object?[] convertedArgs = ConvertArgumentsToJavaScriptObjects(args); Dictionary<string, object> parameters = new Dictionary<string, object>(); parameters.Add("script", script); Apply this suggestion	6

---

Previous suggestions

✅ Suggestions up to commit 13d222a

Category	Suggestion	Score
Possible issue	✅ Fix incorrect null check Suggestion Impact: The commit implements the suggestion's intent by adding EnsureValueIsNotNull() method calls throughout the code to properly handle null response values code diff: + + commandResponse.EnsureValueIsNotNull(); The GetElementFromResponse method should throw NoSuchElementException when response.Value is null, not when response itself is null. The current implementation could mask underlying issues. dotnet/src/webdriver/WebDriver.cs [524-527] -if (response == null) +if (response?.Value == null) { throw new NoSuchElementException(); } Suggestion importance[1-10]: 8 Why: The suggestion fixes a potential bug where null response values would not be properly handled, which could mask underlying issues and cause incorrect error reporting.	8
	✅ Prevent potential null reference Suggestion Impact: The commit addresses null safety but uses a different approach - instead of the suggested null check, it adds EnsureValueIsNotNull() method calls to validate commandResponse.Value code diff: Response commandResponse = this.Execute(DriverCommand.GetTitle, null); - object returnedTitle = commandResponse?.Value ?? string.Empty; + object returnedTitle = commandResponse.Value ?? string.Empty; The Title property should check if commandResponse is null before using the null-coalescing operator on its Value property to avoid potential NullReferenceException. dotnet/src/webdriver/WebDriver.cs [123] -object returnedTitle = commandResponse?.Value ?? string.Empty; +object returnedTitle = commandResponse == null ? string.Empty : commandResponse.Value ?? string.Empty; Suggestion importance[1-10]: 7 Why: The suggestion adds important null safety checks to prevent NullReferenceException when commandResponse is null, improving the robustness of the code.	7
	Add null safety checks The PageSource property should handle null response and null response value to avoid potential NullReferenceException. dotnet/src/webdriver/WebDriver.cs [139] -return commandResponse.Value.ToString(); +return commandResponse?.Value?.ToString() ?? string.Empty; Suggestion importance[1-10]: 7 Why: The suggestion adds comprehensive null checks to prevent potential NullReferenceException when accessing Value property, making the code more resilient.	7

[RenderMichael]

commandResponse == null is almost certainly unreachable code, but I will not rock this boat.

[RenderMichael]

I verified that passing in null for the name will throw an exception anyway, This just clarifies the exception for any user who does this.