[dotnet] Annotate nullability on most remaining types

[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.

Annotate nullability on most remaining types

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

• Annotated nullability for most remaining types across the project.

• Improved code consistency by replacing legacy patterns with modern C# features.

• Added null checks and exception handling for better error management.

• Refactored properties and methods to use concise syntax and readonly fields.

---

Changes walkthrough 📝

Relevant files

Enhancement

19 files By.cs Annotated nullability and refactored `By` class for clarity +89/-82  Cookie.cs Added nullability annotations and refactored `Cookie` class +50/-69  DevToolsSession.cs Added nullability annotations to `DevToolsSession`              +1/-1      DomMutationData.cs Added nullability annotations to `DomMutationData`              +1/-1      InternetExplorerDriverLogLevel.cs Enabled nullability in `InternetExplorerDriverLogLevel`    +2/-0      InternetExplorerOptions.cs Refactored InternetExplorerOptions with nullability and readonly fields +46/-135 INetwork.cs Added nullability annotations to `INetwork` interface        +4/-2      ISupportsLogs.cs Enabled nullability in `ISupportsLogs`                                      +2/-0      IWrapsElement.cs Enabled nullability in `IWrapsElement`                                      +2/-0      PortUtilities.cs Added nullability annotations to `PortUtilities`                  +3/-1      ReturnedCookie.cs Added nullability annotations to `ReturnedCookie`                +4/-2      LogType.cs Enabled nullability in `LogType`                                                  +2/-0      NetworkAuthenticationHandler.cs Added nullability annotations to `NetworkAuthenticationHandler` +4/-2      NetworkManager.cs Added nullability annotations and refactored `NetworkManager` +17/-11  NetworkRequestHandler.cs Added nullability annotations to `NetworkRequestHandler`  +5/-3      NetworkResponseHandler.cs Added nullability annotations to `NetworkResponseHandler` +4/-2      RelativeBy.cs Added nullability annotations and improved error handling in RelativeBy +28/-19  DesiredCapabilities.cs Refactored `DesiredCapabilities` with nullability and concise syntax +27/-74  RemoteWebDriver.cs Added nullability annotations and improved error handling in RemoteWebDriver +32/-20

---

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-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Null Safety The NetworkManager class has nullable event handlers but does not check for null before invoking them in the event raising code. This could lead to runtime NullReferenceExceptions. public event EventHandler<NetworkRequestSentEventArgs>? NetworkRequestSent; /// <summary> /// Occurs when a browser receives a network response. /// </summary> public event EventHandler<NetworkResponseReceivedEventArgs>? NetworkResponseReceived; Required Properties ResponseMatcher and ResponseTransformer are marked as required (non-null) but there is no validation in the constructor to ensure they are initialized. This could lead to NullReferenceExceptions if not set. public Func<HttpResponseData, bool> ResponseMatcher { get; set; } = null!; /// <summary> /// Gets or sets a function that accepts an <see cref="HttpResponseData"/> object describing a network /// response received by the browser, and returns a modified <see cref="HttpResponseData"/> object to used /// as the actual network response. /// </summary> public Func<HttpResponseData, HttpResponseData> ResponseTransformer { get; set; } = null!;

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Add null checks for nullable delegates Suggestion Impact: Added null checks for FindElementMethod and FindElementsMethod using pattern matching syntax, with more descriptive error messages code diff: + if (this.FindElementMethod is not { } findElementMethod) + { + throw new InvalidOperationException("FindElement method not set. Override the By.FindElement method, set the By.FindElementMethod property, or use a constructor that sets a query mechanism."); + } + + return findElementMethod(context); } /// <summary> @@ -340,7 +342,12 @@ /// matching the current criteria, or an empty list if nothing matches.</returns> public virtual ReadOnlyCollection<IWebElement> FindElements(ISearchContext context) { - return this.FindElementsMethod(context); + if (this.FindElementsMethod is not { } findElementsMethod) + { + throw new InvalidOperationException("FindElements method not set. Override the By.FindElements method, set the By.FindElementsMethod property, or use a constructor that sets a query mechanism."); + } + + return findElementsMethod(context); The FindElementMethod and FindElementsMethod properties are marked as nullable but used without null checks in FindElement and FindElements methods. Add null checks to prevent potential NullReferenceException. dotnet/src/webdriver/By.cs [332-333] public virtual IWebElement FindElement(ISearchContext context) { + if (this.FindElementMethod == null) + { + throw new InvalidOperationException("FindElementMethod not set"); + } return this.FindElementMethod(context); } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: The suggestion addresses a critical potential NullReferenceException by adding necessary null checks for nullable delegate properties that are used directly. This is especially important since the code has #nullable enable.	High
	Add defensive null checking Add null check for the namesArray value before accessing it to prevent potential NullReferenceException if the response is malformed. dotnet/src/webdriver/Remote/RemoteWebDriver.cs [527-528] -object[] namesArray = (object[])value["names"]!; -return namesArray.Select(obj => obj.ToString()!).ToList(); +object[]? namesArray = value["names"] as object[]; +if (namesArray == null) +{ + throw new WebDriverException("GetDownloadableFiles response did not contain valid 'names' array"); +} +return namesArray.Select(obj => obj?.ToString() ?? string.Empty).ToList(); Apply this suggestion Suggestion importance[1-10]: 8 __ Why: The suggestion adds important null checks to prevent potential runtime exceptions when handling malformed server responses, which could improve application stability and provide better error messages.	Medium
	✅ Avoid null reference runtime exceptions Suggestion Impact: The commit addresses the null reference concern by making RequestMatcher nullable and adding [DisallowNull] attribute instead of using null! code diff: - public Func<HttpRequestData, bool> RequestMatcher { get; set; } = null!; + [DisallowNull] + public Func<HttpRequestData, bool>? RequestMatcher { get; set; } Initialize RequestMatcher in the constructor instead of using null! to avoid potential null reference exceptions at runtime. dotnet/src/webdriver/NetworkRequestHandler.cs [35] -public Func<HttpRequestData, bool> RequestMatcher { get; set; } = null!; +public Func<HttpRequestData, bool> RequestMatcher { get; set; } = _ => false; [Suggestion has been applied] Suggestion importance[1-10]: 8 __ Why: The suggestion provides a safer default implementation that prevents null reference exceptions at runtime, which is a critical improvement over using null! that could lead to runtime crashes.	Medium
	✅ Provide safe default function implementations Suggestion Impact: The commit addresses the null safety concern by making the properties nullable and adding [DisallowNull] attribute instead of using null! or default implementations code diff: - public Func<HttpResponseData, bool> ResponseMatcher { get; set; } = null!; + [DisallowNull] + public Func<HttpResponseData, bool>? ResponseMatcher { get; set; } /// <summary> /// Gets or sets a function that accepts an <see cref="HttpResponseData"/> object describing a network /// response received by the browser, and returns a modified <see cref="HttpResponseData"/> object to used /// as the actual network response. /// </summary> - public Func<HttpResponseData, HttpResponseData> ResponseTransformer { get; set; } = null!; + [DisallowNull] + public Func<HttpResponseData, HttpResponseData>? ResponseTransformer { get; set; } Initialize ResponseMatcher and ResponseTransformer in the constructor with safe default implementations instead of using null! to avoid potential null reference exceptions. dotnet/src/webdriver/NetworkResponseHandler.cs [35-42] -public Func<HttpResponseData, bool> ResponseMatcher { get; set; } = null!; -public Func<HttpResponseData, HttpResponseData> ResponseTransformer { get; set; } = null!; +public Func<HttpResponseData, bool> ResponseMatcher { get; set; } = _ => false; +public Func<HttpResponseData, HttpResponseData> ResponseTransformer { get; set; } = response => response; [Suggestion has been applied] Suggestion importance[1-10]: 8 __ Why: The suggestion replaces potentially dangerous null! assignments with safe default implementations for both ResponseMatcher and ResponseTransformer, preventing runtime exceptions and providing sensible default behavior.	Medium
	✅ Add safe default function implementation Suggestion Impact: The commit addresses the null safety concern but uses a different approach by making UriMatcher nullable and adding [DisallowNull] attribute instead of providing a default implementation code diff: - public Func<Uri, bool> UriMatcher { get; set; } = null!; + [DisallowNull] + public Func<Uri, bool>? UriMatcher { get; set; } Initialize UriMatcher with a safe default implementation instead of using null! to avoid potential null reference exceptions. dotnet/src/webdriver/NetworkAuthenticationHandler.cs [35] -public Func<Uri, bool> UriMatcher { get; set; } = null!; +public Func<Uri, bool> UriMatcher { get; set; } = _ => false; [Suggestion has been applied] Suggestion importance[1-10]: 8 __ Why: The suggestion replaces the null! assignment with a safe default implementation for UriMatcher, preventing potential null reference exceptions and providing a reasonable default behavior.	Medium
	Validate transformed response not null Add null check for the response transformer result before using it with ContinueRequestWithResponse to prevent potential NullReferenceException. dotnet/src/webdriver/NetworkManager.cs [272] -await this.session.Value.Domains.Network.ContinueRequestWithResponse(requestData, handler.ResponseTransformer(e.ResponseData)).ConfigureAwait(false); +var transformedResponse = handler.ResponseTransformer(e.ResponseData); +if (transformedResponse == null) +{ + throw new WebDriverException("Response transformer returned null response"); +} +await this.session.Value.Domains.Network.ContinueRequestWithResponse(requestData, transformedResponse).ConfigureAwait(false); Apply this suggestion Suggestion importance[1-10]: 7 __ Why: The suggestion adds a critical null check for the transformed response before using it, which could prevent NullReferenceException and provide clearer error messaging when the transformer returns invalid data.	Medium
Learned best practice	✅ Add null validation checks for nullable delegate properties before invoking them to prevent NullReferenceExceptions Suggestion Impact: The commit implements null checks for FindElementMethod and FindElementsMethod properties, but uses pattern matching and throws InvalidOperationException with more descriptive messages instead of ArgumentNullException code diff: + if (this.FindElementMethod is not { } findElementMethod) + { + throw new InvalidOperationException("FindElement method not set. Override the By.FindElement method, set the By.FindElementMethod property, or use a constructor that sets a query mechanism."); + } + + return findElementMethod(context); } /// <summary> @@ -340,7 +342,12 @@ /// matching the current criteria, or an empty list if nothing matches.</returns> public virtual ReadOnlyCollection<IWebElement> FindElements(ISearchContext context) { - return this.FindElementsMethod(context); + if (this.FindElementsMethod is not { } findElementsMethod) + { + throw new InvalidOperationException("FindElements method not set. Override the By.FindElements method, set the By.FindElementsMethod property, or use a constructor that sets a query mechanism."); + } + + return findElementsMethod(context); The FindElement and FindElements methods should validate that their FindElementMethod and FindElementsMethod properties are not null before using them, since they are marked as nullable. Add ArgumentNullException checks at the start of these methods. dotnet/src/webdriver/By.cs [330-344] public virtual IWebElement FindElement(ISearchContext context) { + ArgumentNullException.ThrowIfNull(this.FindElementMethod, nameof(FindElementMethod)); return this.FindElementMethod(context); } -public virtual ReadOnlyCollection<IWebElement> FindElements(ISearchContext context) +public virtual ReadOnlyCollection<IWebElement> FindElements(ISearchContext context) { + ArgumentNullException.ThrowIfNull(this.FindElementsMethod, nameof(FindElementsMethod)); return this.FindElementsMethod(context); } Apply this suggestion	Low

[RenderMichael]

Sorry for the recent changes, I just wanted to remove unnecessary complexity. This PR shouldn't make unnecessary optimizations, those can be reviewed separately