Skip to content

Conversation

@RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Dec 7, 2025

User description

This includes an internal language version bump, as well as some polyfill files. In exchange, we can use modern C# methods across all TFMs.

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Replace manual null-check patterns with ArgumentNullException.ThrowIfNull()

  • Add C# 14 extension method polyfill for modern null validation

  • Reduce boilerplate code across 30+ files in WebDriver codebase

  • Maintain backward compatibility with older .NET target frameworks


Diagram Walkthrough

flowchart LR
  A["Manual null checks<br/>if x is null throw"] -->|"Refactor to"| B["ArgumentNullException<br/>.ThrowIfNull()"]
  C["Polyfill files<br/>for older TFMs"] -->|"Enable"| B
  B -->|"Applied to"| D["30+ WebDriver files<br/>BiDi, DevTools, Drivers"]
Loading

File Walkthrough

Relevant files
Enhancement
2 files
ArgumentNullExceptionExtensions.cs
Add ThrowIfNull extension method polyfill                               
+38/-0   
CallerArgumentExpressionAttribute.cs
Add CallerArgumentExpression attribute polyfill                   
+29/-0   
Cleanup
26 files
PermissionsBiDiExtensions.cs
Replace manual null check with ThrowIfNull                             
+1/-4     
WebDriver.Extensions.cs
Replace manual null check with ThrowIfNull                             
+1/-1     
ChromiumDriver.cs
Replace multiple manual null checks with ThrowIfNull         
+2/-8     
Cookie.cs
Replace manual null check with ThrowIfNull                             
+1/-4     
CookieJar.cs
Replace manual null checks with ThrowIfNull                           
+2/-8     
CommandResponseTypeMap.cs
Replace multiple manual null checks with ThrowIfNull         
+2/-8     
WebSocketConnection.cs
Replace manual null checks with ThrowIfNull                           
+2/-8     
V141Network.cs
Replace multiple manual null checks with ThrowIfNull         
+7/-28   
V142Network.cs
Replace multiple manual null checks with ThrowIfNull         
+7/-28   
V143Network.cs
Replace multiple manual null checks with ThrowIfNull         
+7/-28   
DriverOptions.cs
Replace manual null check with ThrowIfNull                             
+1/-4     
DriverProcessStartedEventArgs.cs
Replace manual null check with ThrowIfNull                             
+1/-4     
FirefoxDriver.cs
Replace multiple manual null checks with ThrowIfNull         
+2/-8     
FirefoxProfile.cs
Replace manual null check with ThrowIfNull                             
+1/-4     
Preferences.cs
Replace multiple manual null checks with ThrowIfNull         
+4/-16   
InternetExplorerDriver.cs
Replace multiple manual null checks with ThrowIfNull         
+2/-8     
Actions.cs
Replace manual null check with ThrowIfNull                             
+1/-4     
WheelInputDevice.cs
Replace manual null check with ThrowIfNull                             
+1/-4     
LogContext.cs
Replace manual null check with ThrowIfNull                             
+1/-4     
JavaScriptEngine.cs
Replace multiple manual null checks with ThrowIfNull         
+7/-28   
Logs.cs
Replace manual null check with ThrowIfNull                             
+1/-4     
HttpCommandExecutor.cs
Replace manual null checks with ThrowIfNull                           
+2/-8     
RemoteWebDriver.cs
Replace manual null check with ThrowIfNull                             
+1/-4     
SafariDriver.cs
Replace multiple manual null checks with ThrowIfNull         
+2/-8     
TargetLocator.cs
Replace manual null check with ThrowIfNull                             
+1/-4     
WebDriver.cs
Replace multiple manual null checks with ThrowIfNull         
+7/-28   

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Dec 7, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 7, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Type extension conflict

Description: The polyfill declares 'namespace System;' and uses a C# 14
'extension(ArgumentNullException)' type extension inside the System namespace, which risks
type/namespace conflicts or ambiguous resolution with BCL types on supported TFMs and
could lead to undefined behavior or runtime/type loading issues.
ArgumentNullExceptionExtensions.cs [23-38]

Referred Code
namespace System;

internal static class ArgumentNullExceptionExtensions
{
    extension(ArgumentNullException)
    {
        public static void ThrowIfNull([NotNull] object? arg, [CallerArgumentExpression(nameof(arg))] string paramName = "")
        {
            if (arg is null)
            {
                throw new ArgumentNullException(paramName);
            }
        }
    }
}
Ticket Compliance
🟡
🎫 #1234
🔴 Investigate and fix regression where clicking a link with JavaScript in href no longer
triggers in Selenium 2.48.x on Firefox 42 (worked in 2.47.1).
Provide verification that alerts are triggered as in prior version.
🟡
🎫 #5678
🔴 Diagnose and resolve "Error: ConnectFailure (Connection refused)" occurring when
instantiating additional ChromeDriver instances on Ubuntu 16.04 with Chrome 65 and
ChromeDriver 2.35 using Selenium 3.9.0.
Ensure stable creation of multiple ChromeDriver instances without connection errors.
Provide steps or fixes to avoid the error in subsequent instantiations.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Logging Detail: The new changes focus on null checks and a polyfill without introducing or modifying audit
logging for critical actions, which may be outside this PR's scope but cannot be
confirmed from the diff alone.

Referred Code
ArgumentNullException.ThrowIfNull(url);

this.Log($"Opening connection to URL {url}", DevToolsSessionLogLevel.Trace);
bool connected = false;
DateTime timeout = DateTime.Now.Add(this.startupTimeout);

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 7, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Conditionally compile the ThrowIfNull polyfill
Suggestion Impact:The polyfill files were effectively removed from compilation content; additionally, CallerArgumentExpressionAttribute.cs already includes a #if !NET8_0_OR_GREATER guard, aligning with the suggestion to conditionally compile polyfills for newer frameworks.

code diff:

# File: dotnet/src/webdriver/Internal/Polyfills/CallerArgumentExpressionAttribute.cs
@@ -1,29 +1 @@
-// <copyright file="CallerArgumentExpressionAttribute.cs" company="Selenium Committers">
-// Licensed to the Software Freedom Conservancy (SFC) under one
-// or more contributor license agreements.  See the NOTICE file
-// distributed with this work for additional information
-// regarding copyright ownership.  The SFC licenses this file
-// to you under the Apache License, Version 2.0 (the
-// "License"); you may not use this file except in compliance
-// with the License.  You may obtain a copy of the License at
-//
-//   http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing,
-// software distributed under the License is distributed on an
-// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-// KIND, either express or implied.  See the License for the
-// specific language governing permissions and limitations
-// under the License.
-// </copyright>
 
-namespace System.Runtime.CompilerServices;
-
-#if !NET8_0_OR_GREATER
-
-internal class CallerArgumentExpressionAttribute(string paramName) : Attribute
-{
-    public string ParamName = paramName;
-}
-
-#endif

The polyfill for ArgumentNullException.ThrowIfNull should be conditionally
compiled using a preprocessor directive (e.g., #if !NET8_0_OR_GREATER). This
prevents compilation errors on newer .NET frameworks that have a native
implementation of this method.

Examples:

dotnet/src/webdriver/Internal/Polyfills/ArgumentNullExceptionExtensions.cs [1-38]
// <copyright file="ArgumentNullExceptionExtensions.cs" company="Selenium Committers">
// Licensed to the Software Freedom Conservancy (SFC) under one
// or more contributor license agreements.  See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership.  The SFC licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License.  You may obtain a copy of the License at
//
//   http://www.apache.org/licenses/LICENSE-2.0

 ... (clipped 28 lines)
dotnet/src/webdriver/Internal/Polyfills/CallerArgumentExpressionAttribute.cs [22-29]
#if !NET8_0_OR_GREATER

internal class CallerArgumentExpressionAttribute(string paramName) : Attribute
{
    public string ParamName = paramName;
}

#endif

Solution Walkthrough:

Before:

// File: dotnet/src/webdriver/Internal/Polyfills/ArgumentNullExceptionExtensions.cs

namespace System;

internal static class ArgumentNullExceptionExtensions
{
    extension(ArgumentNullException)
    {
        public static void ThrowIfNull([NotNull] object? arg, ...)
        {
            if (arg is null)
            {
                throw new ArgumentNullException(paramName);
            }
        }
    }
}

After:

// File: dotnet/src/webdriver/Internal/Polyfills/ArgumentNullExceptionExtensions.cs

#if !NET6_0_OR_GREATER // Or a similar TFM check like !NET8_0_OR_GREATER

namespace System;

internal static class ArgumentNullExceptionExtensions
{
    extension(ArgumentNullException)
    {
        public static void ThrowIfNull([NotNull] object? arg, ...)
        {
            if (arg is null)
            {
                throw new ArgumentNullException(paramName);
            }
        }
    }
}

#endif
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a critical issue where the ThrowIfNull polyfill would conflict with the native implementation in newer .NET frameworks, likely causing build failures in a multi-targeted project.

High
Possible issue
Correct the polyfill's conditional compilation

Correct the preprocessor directive for the CallerArgumentExpressionAttribute
polyfill from #if !NET8_0_OR_GREATER to #if !NETCOREAPP3_0_OR_GREATER to avoid
compilation errors on supported frameworks where the attribute is already
defined.

dotnet/src/webdriver/Internal/Polyfills/CallerArgumentExpressionAttribute.cs [22-29]

-#if !NET8_0_OR_GREATER
+#if !NETCOREAPP3_0_OR_GREATER
 
-internal class CallerArgumentExpressionAttribute(string paramName) : Attribute
+internal sealed class CallerArgumentExpressionAttribute(string paramName) : Attribute
 {
     public string ParamName = paramName;
 }
 
 #endif
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a bug where the polyfill for CallerArgumentExpressionAttribute would cause compilation errors on .NET versions between .NET Core 3.0 and .NET 8, and provides the correct preprocessor directive to fix it.

High
  • Update

@nvborisenko
Copy link
Member

Seems we need to explicitly specify:

  • lang version 14 in csproj
  • lang version in bazel build
  • upgrade bazel rules for dotnet (requires bazel 8+)

@RenderMichael
Copy link
Contributor Author

upgrade bazel rules for dotnet (requires bazel 8+)

Is that so? :(

I eagerly await it, for now this can sit in the drafts.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants