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"]
File Walkthrough
| Relevant files |
|---|
| Enhancement | 2 files
ArgumentNullExceptionExtensions.csAdd ThrowIfNull extension method polyfill |
+38/-0 |
CallerArgumentExpressionAttribute.csAdd CallerArgumentExpression attribute polyfill |
+29/-0 |
|
| Cleanup | 26 files
PermissionsBiDiExtensions.csReplace manual null check with ThrowIfNull |
+1/-4 |
WebDriver.Extensions.csReplace manual null check with ThrowIfNull |
+1/-1 |
ChromiumDriver.csReplace multiple manual null checks with ThrowIfNull |
+2/-8 |
Cookie.csReplace manual null check with ThrowIfNull |
+1/-4 |
CookieJar.csReplace manual null checks with ThrowIfNull |
+2/-8 |
CommandResponseTypeMap.csReplace multiple manual null checks with ThrowIfNull |
+2/-8 |
WebSocketConnection.csReplace manual null checks with ThrowIfNull |
+2/-8 |
V141Network.csReplace multiple manual null checks with ThrowIfNull |
+7/-28 |
V142Network.csReplace multiple manual null checks with ThrowIfNull |
+7/-28 |
V143Network.csReplace multiple manual null checks with ThrowIfNull |
+7/-28 |
DriverOptions.csReplace manual null check with ThrowIfNull |
+1/-4 |
DriverProcessStartedEventArgs.csReplace manual null check with ThrowIfNull |
+1/-4 |
FirefoxDriver.csReplace multiple manual null checks with ThrowIfNull |
+2/-8 |
FirefoxProfile.csReplace manual null check with ThrowIfNull |
+1/-4 |
Preferences.csReplace multiple manual null checks with ThrowIfNull |
+4/-16 |
InternetExplorerDriver.csReplace multiple manual null checks with ThrowIfNull |
+2/-8 |
Actions.csReplace manual null check with ThrowIfNull |
+1/-4 |
WheelInputDevice.csReplace manual null check with ThrowIfNull |
+1/-4 |
LogContext.csReplace manual null check with ThrowIfNull |
+1/-4 |
JavaScriptEngine.csReplace multiple manual null checks with ThrowIfNull |
+7/-28 |
Logs.csReplace manual null check with ThrowIfNull |
+1/-4 |
HttpCommandExecutor.csReplace manual null checks with ThrowIfNull |
+2/-8 |
RemoteWebDriver.csReplace manual null check with ThrowIfNull |
+1/-4 |
SafariDriver.csReplace multiple manual null checks with ThrowIfNull |
+2/-8 |
TargetLocator.csReplace manual null check with ThrowIfNull |
+1/-4 |
WebDriver.csReplace multiple manual null checks with ThrowIfNull |
+7/-28 |
|
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 --update_compliance=true -->
|
Compliance status legend
🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label
PR Code Suggestions ✨
Explore these optional code suggestions:
| Category | Suggestion | 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 <!-- /improve --apply_suggestion=1 -->
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 <!-- /improve_multi --more_suggestions=true -->
| |
Seems we need to explicitly specify:
- lang version 14 in csproj
- lang version in bazel build
- upgrade bazel rules for dotnet (requires bazel 8+)
upgrade bazel rules for dotnet (requires bazel 8+)
Is that so? :(
I eagerly await it, for now this can sit in the drafts.