[dotnet] Enable Nullable Reference Types #15354

RenderMichael · 2025-03-01T07:46:02Z

User description

Description

Enable Nullable Reference Types

Motivation and Context

Fixes #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

Description

Enabled nullable reference types in the .NET WebDriver project.
Added nullable attribute to Bazel build rules for C# projects.
Updated WebDriver.csproj to configure nullable contexts for different target frameworks.

Changes walkthrough 📝

Relevant files

Configuration changes

3 files

executable_assembly.bzl Added `nullable` attribute to C# executable assembly rule.	+4/-0
BUILD.bazel `Configured nullable context in Bazel build for WebDriver.`	+4/-0
WebDriver.csproj `Enabled nullable reference types in project file.`	+10/-4

Additional files

101 files

Alert.cs	+0/-2
BiDi.cs	+0/-2
BiDiException.cs	+0/-2
Broker.cs	+0/-2
Command.cs	+0/-2
CommandOptions.cs	+0/-2
EventHandler.cs	+0/-2
BrowserClientWindowConverter.cs	+0/-2
BrowserUserContextConverter.cs	+0/-2
BrowsingContextConverter.cs	+0/-2
ChannelConverter.cs	+0/-2
DateTimeOffsetConverter.cs	+0/-2
GetClientWindowsResultConverter.cs	+0/-2
GetCookiesResultConverter.cs	+0/-2
GetRealmsResultConverter.cs	+0/-2
GetUserContextsResultConverter.cs	+0/-2
InputSourceActionsConverter.cs	+0/-2
LocateNodesResultConverter.cs	+0/-2
HandleConverter.cs	+0/-2
InputOriginConverter.cs	+0/-2
InterceptConverter.cs	+0/-2
InternalIdConverter.cs	+0/-2
NavigationConverter.cs	+0/-2
EvaluateResultConverter.cs	+0/-2
LogEntryConverter.cs	+0/-2
MessageConverter.cs	+0/-2
RealmInfoConverter.cs	+0/-2
RemoteValueConverter.cs	+0/-2
PreloadScriptConverter.cs	+0/-2
PrintPageRangeConverter.cs	+0/-2
RealmConverter.cs	+0/-2
RealmTypeConverter.cs	+0/-2
RequestConverter.cs	+0/-2
SubscriptionConverter.cs	+0/-2
Message.cs	+0/-2
ITransport.cs	+0/-2
WebSocketTransport.cs	+0/-2
EventArgs.cs	+0/-2
BrowserModule.cs	+0/-2
ClientWindow.cs	+0/-2
ClientWindowInfo.cs	+0/-2
CloseCommand.cs	+0/-2
CreateUserContextCommand.cs	+0/-2
GetClientWindowsCommand.cs	+0/-2
GetUserContextsCommand.cs	+0/-2
RemoveUserContextCommand.cs	+0/-2
UserContext.cs	+0/-2
UserContextInfo.cs	+0/-2
ActivateCommand.cs	+0/-2
BrowsingContext.cs	+0/-2
BrowsingContextInfo.cs	+0/-2
BrowsingContextInputModule.cs	+0/-2
BrowsingContextLogModule.cs	+0/-2
BrowsingContextModule.cs	+0/-2
BrowsingContextNetworkModule.cs	+0/-2
BrowsingContextScriptModule.cs	+0/-2
BrowsingContextStorageModule.cs	+0/-2
CaptureScreenshotCommand.cs	+0/-2
CloseCommand.cs	+0/-2
CreateCommand.cs	+0/-2
GetTreeCommand.cs	+0/-2
HandleUserPromptCommand.cs	+0/-2
LocateNodesCommand.cs	+0/-2
Locator.cs	+0/-2
NavigateCommand.cs	+0/-2
Navigation.cs	+0/-2
NavigationInfo.cs	+0/-2
PrintCommand.cs	+0/-2
ReloadCommand.cs	+0/-2
SetViewportCommand.cs	+0/-2
TraverseHistoryCommand.cs	+0/-2
UserPromptClosedEventArgs.cs	+0/-2
UserPromptOpenedEventArgs.cs	+0/-2
InputModule.cs	+0/-2
Origin.cs	+0/-2
PerformActionsCommand.cs	+0/-2
ReleaseActionsCommand.cs	+0/-2
SourceActions.cs	+0/-2
Entry.cs	+0/-2
LogModule.cs	+0/-2
Module.cs	+0/-2
AddInterceptCommand.cs	+0/-2
AuthChallenge.cs	+0/-2
AuthCredentials.cs	+0/-2
AuthRequiredEventArgs.cs	+0/-2
BaseParametersEventArgs.cs	+0/-2
BeforeRequestSentEventArgs.cs	+0/-2
BytesValue.cs	+0/-2
ContinueRequestCommand.cs	+0/-2
ContinueResponseCommand.cs	+0/-2
ContinueWithAuthCommand.cs	+0/-2
Cookie.cs	+0/-2
CookieHeader.cs	+0/-2
FailRequestCommand.cs	+0/-2
FetchErrorEventArgs.cs	+0/-2
FetchTimingInfo.cs	+0/-2
Header.cs	+0/-2
Initiator.cs	+0/-2
Intercept.cs	+0/-2
NetworkModule.cs	+0/-2
Additional files not shown

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-01T07:46:17Z

PR Reviewer Guide 🔍

(Review updated until commit `9a1f270`)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Configuration Validation The nullable configuration differs between build systems - Bazel uses 'annotations' and 'enable' while MSBuild uses conditional properties. This could lead to inconsistent null checking behavior. <PropertyGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))"> <Nullable>enable</Nullable> <!--TODO when AOT is ready https://github.com/SeleniumHQ/selenium/issues/14480--> <!--<IsAotCompatible>true</IsAotCompatible>--> </PropertyGroup> <PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard2.0'"> <Nullable>annotations</Nullable> </PropertyGroup>

qodo-merge-pro · 2025-03-01T07:46:28Z

PR Code Suggestions ✨

Latest suggestions up to 9a1f270
Explore these optional code suggestions:

Category Suggestion Impact

General

✅ ~~Unify nullable reference type settings~~

Suggestion Impact:

The commit implemented the suggestion by moving the Nullable=enable setting to the top-level PropertyGroup and removing the framework-specific PropertyGroup conditions, effectively unifying the nullable reference type settings across all target frameworks.

code diff:

+    <Nullable>enable</Nullable>
   </PropertyGroup>
 
   <PropertyGroup>
@@ -41,18 +42,12 @@
 
     <GenerateDocumentationFile>true</GenerateDocumentationFile>
   </PropertyGroup>
-
-  <PropertyGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))">
-    <Nullable>enable</Nullable>
-    
-    <!--TODO when AOT is ready https://github.com/SeleniumHQ/selenium/issues/14480-->
-    <!--<IsAotCompatible>true</IsAotCompatible>-->
-  </PropertyGroup>
-
-  <PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard2.0'">
-    <Nullable>annotations</Nullable>
-  </PropertyGroup>
-
+  <!--TODO when AOT is ready https://github.com/SeleniumHQ/selenium/issues/14480-->
+  <!--<PropertyGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))">
+    -->
+  <!--<IsAotCompatible>true</IsAotCompatible>-->
+  <!--
+  </PropertyGroup>-->

Consider using a single Nullable property setting for all target frameworks to
maintain consistent null-checking behavior across the codebase.

dotnet/src/webdriver/WebDriver.csproj [45-54]

-<PropertyGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))">
+<PropertyGroup>
   <Nullable>enable</Nullable>
   
   <!--TODO when AOT is ready https://github.com/SeleniumHQ/selenium/issues/14480-->
   <!--<IsAotCompatible>true</IsAotCompatible>-->
 </PropertyGroup>
 
-<PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard2.0'">
-  <Nullable>annotations</Nullable>
-</PropertyGroup>
-

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 2

__

Why: While the suggestion to unify nullable settings is technically valid, having different nullable settings for different target frameworks is intentional here, as netstandard2.0 may have compatibility constraints that make full nullable enforcement impractical. The current approach is more nuanced and appropriate.

Low

Update

Previous suggestions

✅ Suggestions up to commit 74a78fc

Category	Suggestion	Impact
General	✅ ~~Standardize nullable settings across targets~~ Suggestion Impact: The commit implemented the suggestion by changing all nullable settings from 'annotations' to 'enable' across all library targets, standardizing the nullable behavior as suggested. code diff: - nullable="annotations" + nullable = "enable", resources = [ "//javascript/atoms/fragments:find-elements.js", "//javascript/atoms/fragments:is-displayed.js", @@ -77,7 +77,7 @@ "WebDriver.Common.Tests", ], langversion = "12.0", - nullable="enable" + nullable = "enable", resources = [ "//javascript/atoms/fragments:find-elements.js", "//javascript/atoms/fragments:is-displayed.js", @@ -105,7 +105,7 @@ out = "WebDriver.StrongNamed", keyfile = "//dotnet:WebDriver.snk", langversion = "12.0", - nullable="annotations" + nullable = "enable", resources = [ "//javascript/atoms/fragments:find-elements.js", "//javascript/atoms/fragments:is-displayed.js", @@ -143,7 +143,7 @@ ], keyfile = "//dotnet:WebDriver.snk", langversion = "12.0", - nullable="enable" + nullable = "enable", Consider using consistent nullable settings across all library targets. Currently mixing "enable" and "annotations" which could lead to inconsistent null checking behavior. dotnet/src/webdriver/BUILD.bazel [40-146] -+ nullable="annotations" ++ nullable="enable" ... + nullable="enable" Suggestion importance[1-10]: 2 __ Why: The suggestion to standardize nullable settings to "enable" across all targets is incorrect. The PR intentionally uses different nullable settings ("annotations" for netstandard2.0 and "enable" for net8.0) to match the configuration in WebDriver.csproj, which shows this is a deliberate design choice.	Low

RenderMichael · 2025-03-03T04:45:52Z

@nvborisenko As part of finishing the nullable reference type work, this PR removes #nullable enable from every file.

Could this interfere with any changes you may have open? Could you also review the bazel work? (only two bazel files changed)

dotnet/private/executable_assembly.bzl

dotnet/src/webdriver/WebDriver.csproj

RenderMichael · 2025-03-05T16:39:00Z

Failures are unrelated:

//java/test/org/openqa/selenium/bidi/browsingcontext:BrowsingContextInspectorTest
//java/test/org/openqa/selenium/bidi/browsingcontext:BrowsingContextInspectorTest-chrome
//java/test/org/openqa/selenium/bidi/browsingcontext:BrowsingContextInspectorTest-chrome-remote
//java/test/org/openqa/selenium/bidi/browsingcontext:BrowsingContextInspectorTest-remote
//javascript/node/selenium-webdriver:test-bidi-browsingcontext-inspector-test.js-chrome
//javascript/node/selenium-webdriver:test-bidi-browsingcontext-inspector-test.js-firefox
//py:common-chrome-bidi-test/selenium/webdriver/common/bidi_tests.py
//rb/spec/integration/selenium/webdriver:driver-chrome
//rb/spec/integration/selenium/webdriver:driver-chrome-remote
//rb/spec/integration/selenium/webdriver:driver-firefox-beta
//rb/spec/integration/selenium/webdriver:select-chrome-remote

RenderMichael · 2025-03-05T16:45:36Z

For simplicity of review, the only changes outside of removing #nullable enable are:

dotnet/private/executable_assembly.bzl
dotnet/src/webdriver/BUILD.bazel
dotnet/src/webdriver/WebDriver.csproj

nvborisenko

Finally! Great job, @RenderMichael . There are some warnings, will review it later. Let's land it!

* [dotnet] Enable Nullable Reference Types * Remove nullable enable * fix build * Add nullable handling to `executable_assembly.bzl` * Fix format build * Enable nullable on .NET Standard 2.0 * fix comment formatting * whitespace

[dotnet] Enable Nullable Reference Types

74a78fc

RenderMichael marked this pull request as draft March 1, 2025 07:46

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

RenderMichael added 5 commits March 1, 2025 03:10

Merge branch 'trunk' into dotnet-nullable

d28a683

Remove nullable enable

c081391

fix build

026328e

Add nullable handling to executable_assembly.bzl

c6add61

Fix format build

9a1f270

RenderMichael marked this pull request as ready for review March 1, 2025 16:48

RenderMichael requested a review from nvborisenko March 1, 2025 18:04

Merge branch 'trunk' into dotnet-nullable

3ae348f

RenderMichael commented Mar 3, 2025

View reviewed changes

dotnet/private/executable_assembly.bzl Show resolved Hide resolved

RenderMichael commented Mar 3, 2025

View reviewed changes

dotnet/src/webdriver/WebDriver.csproj Outdated Show resolved Hide resolved

RenderMichael added 3 commits March 5, 2025 10:21

Enable nullable on .NET Standard 2.0

43c24cb

fix comment formatting

8244e8c

whitespace

4305958

nvborisenko approved these changes Mar 5, 2025

View reviewed changes

RenderMichael merged commit ef4b82b into SeleniumHQ:trunk Mar 5, 2025
9 of 10 checks passed

RenderMichael deleted the dotnet-nullable branch March 5, 2025 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[dotnet] Enable Nullable Reference Types #15354

[dotnet] Enable Nullable Reference Types #15354

Uh oh!

RenderMichael commented Mar 1, 2025 •

edited by qodo-merge-pro bot

Loading

Uh oh!

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

edited

Loading

Uh oh!

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

edited

Loading

Uh oh!

RenderMichael commented Mar 3, 2025

Uh oh!

Uh oh!

Uh oh!

RenderMichael commented Mar 5, 2025 •

edited

Loading

Uh oh!

RenderMichael commented Mar 5, 2025 •

edited

Loading

Uh oh!

nvborisenko left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[dotnet] Enable Nullable Reference Types #15354

[dotnet] Enable Nullable Reference Types #15354

Uh oh!

Conversation

RenderMichael commented Mar 1, 2025 • edited by qodo-merge-pro bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Motivation and Context

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

Uh oh!

qodo-merge-pro bot commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 9a1f270)

Uh oh!

qodo-merge-pro bot commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

RenderMichael commented Mar 3, 2025

Uh oh!

Uh oh!

Uh oh!

RenderMichael commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RenderMichael commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvborisenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RenderMichael commented Mar 1, 2025 •

edited by qodo-merge-pro bot

Loading

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

edited

Loading

(Review updated until commit `9a1f270`)

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

edited

Loading

RenderMichael commented Mar 5, 2025 •

edited

Loading

RenderMichael commented Mar 5, 2025 •

edited

Loading