[dotnet] Enable NRT on exceptional types

[RenderMichael]

User description

Description

Adds nullable reference type annotations to exceptional types, such as exception types.

This also includes the polyfill nullability attributes, so this PR may block some other nullability work.

Contributes to #14640

Motivation and Context

Some easy wins on types whose nullability status is fairly unambiguous.

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 across multiple files to improve null safety.
• Updated exception classes to use nullable types for constructor parameters.
• Added polyfill for nullable attributes to ensure compatibility with older .NET versions.
• Enhanced FirefoxDriver with nullable reference types and added MemberNotNullWhen attribute.
• Updated IFileDetector and DefaultFileDetector with NotNullWhen attribute for method parameters.

---

PRDescriptionHeader.CHANGES_WALKTHROUGH

Relevant files

Enhancement

15 files DefaultFileDetector.cs Enable nullable reference types and add NotNullWhen attribute dotnet/src/webdriver/DefaultFileDetector.cs Enabled nullable reference types. Added NotNullWhen attribute to IsFile method parameter. +5/-1      DetachedShadowRootException.cs Enable nullable reference types for exception class            dotnet/src/webdriver/DetachedShadowRootException.cs Enabled nullable reference types. +2/-1      CommandResponseException.cs Update exception constructors with nullable parameters      dotnet/src/webdriver/DevTools/CommandResponseException.cs Enabled nullable reference types. Updated constructor parameters to nullable types. +4/-2      DriverServiceNotFoundException.cs Update exception constructors with nullable parameters      dotnet/src/webdriver/DriverServiceNotFoundException.cs Enabled nullable reference types. Updated constructor parameters to nullable types. +5/-4      ElementClickInterceptedException.cs Update exception constructors with nullable parameters      dotnet/src/webdriver/ElementClickInterceptedException.cs Enabled nullable reference types. Updated constructor parameters to nullable types. +4/-3      ElementNotInteractableException.cs Update exception constructors with nullable parameters      dotnet/src/webdriver/ElementNotInteractableException.cs Enabled nullable reference types. Updated constructor parameters to nullable types. +5/-4      ElementNotSelectableException.cs Update exception constructors with nullable parameters      dotnet/src/webdriver/ElementNotSelectableException.cs Enabled nullable reference types. Updated constructor parameters to nullable types. +5/-4      ElementNotVisibleException.cs Update exception constructors with nullable parameters      dotnet/src/webdriver/ElementNotVisibleException.cs Enabled nullable reference types. Updated constructor parameters to nullable types. +4/-3      ErrorResponse.cs Update ErrorResponse with nullable fields and parameters  dotnet/src/webdriver/ErrorResponse.cs Enabled nullable reference types. Updated fields and constructor parameters to nullable types. +10/-8    FirefoxDriver.cs Enhance FirefoxDriver with nullable reference types            dotnet/src/webdriver/Firefox/FirefoxDriver.cs Enabled nullable reference types. Added MemberNotNullWhen attribute for HasActiveDevToolsSession. Updated method parameters to nullable types. +22/-3    IFileDetector.cs Enable nullable reference types and add NotNullWhen attribute dotnet/src/webdriver/IFileDetector.cs Enabled nullable reference types. Added NotNullWhen attribute to IsFile method parameter. +5/-1      InsecureCertificateException.cs Update exception constructors with nullable parameters      dotnet/src/webdriver/InsecureCertificateException.cs Enabled nullable reference types. Updated constructor parameters to nullable types. +5/-4      NullableAttributes.cs Add polyfill for nullable attributes                                          dotnet/src/webdriver/Internal/NullableAttributes.cs Added polyfill for nullable attributes for compatibility. +149/-0  InvalidCookieDomainException.cs Update exception constructors with nullable parameters      dotnet/src/webdriver/InvalidCookieDomainException.cs Enabled nullable reference types. Updated constructor parameters to nullable types. +4/-3      InvalidElementStateException.cs Update exception constructors with nullable parameters      dotnet/src/webdriver/InvalidElementStateException.cs Enabled nullable reference types. Updated constructor parameters to nullable types. +4/-3

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[qodo-merge-pro]

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 Possible Bug The GetContext() method may throw a NullReferenceException if the Execute method returns a null value. Code Smell The GetFullPageScreenshot() method assumes the Value property of screenshotResponse is not null, which may lead to a NullReferenceException if it is. Code Smell The constructor assumes that certain dictionary keys exist and their values are not null, which may lead to KeyNotFoundException or NullReferenceException if these assumptions are not met.

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Use null-coalescing operator to handle potential null values when converting to string Consider using a null-coalescing operator (??) instead of the null-conditional operator (?.) followed by ToString(). This will provide a default empty string if the Value is null, avoiding potential null reference exceptions. dotnet/src/webdriver/Firefox/FirefoxDriver.cs [266] -string? response = this.Execute(GetContextCommand, null).Value.ToString(); +string response = this.Execute(GetContextCommand, null).Value?.ToString() ?? string.Empty; Apply this suggestion Suggestion importance[1-10]: 9 Why: The suggestion correctly replaces the null-conditional operator followed by ToString() with a null-coalescing operator, which is a safer approach to handle potential null values and avoid null reference exceptions. This change enhances the robustness of the code.	9
Enhancement	✅ Use null-coalescing operator with null-conditional operator for safer string conversion Suggestion Impact: The commit implemented a safer string conversion by using the null-conditional operator with pattern matching, which aligns with the suggestion's intention to handle null values safely. code diff: - if (responseValue.ContainsKey("message")) + if (responseValue.TryGetValue("message", out object? messageObj) + && messageObj?.ToString() is string message) { - if (responseValue["message"] != null) - { - this.message = responseValue["message"].ToString() ?? ""; - } - else - { - this.message = "The error did not contain a message."; - } + this.message = message; Consider using the null-coalescing operator (??) instead of the null-conditional operator (?.) followed by ToString(). This will provide a default empty string if the value is null, avoiding potential null reference exceptions. dotnet/src/webdriver/ErrorResponse.cs [55] -this.message = responseValue["message"].ToString() ?? ""; +this.message = responseValue["message"]?.ToString() ?? string.Empty; Apply this suggestion Suggestion importance[1-10]: 9 Why: This suggestion enhances the code by using both the null-conditional and null-coalescing operators, ensuring that the conversion to string is safe and defaults to an empty string if null. This improves code safety and prevents potential null reference exceptions.	9
	Use null-forgiving operator to assert non-null value when converting to string Consider using the null-forgiving operator (!) instead of the null-conditional operator (?) when accessing the Value property of screenshotResponse. This assumes that the Value property will never be null in this context, which seems to be the case given the nature of the GetFullPageScreenshot method. dotnet/src/webdriver/Firefox/FirefoxDriver.cs [394] -string base64 = screenshotResponse.Value.ToString()!; +string base64 = screenshotResponse.Value!.ToString(); Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion to use the null-forgiving operator is appropriate here, as it asserts that the Value property is non-null, aligning with the method's context. This change improves code clarity and maintains functionality.	8
	Use null-conditional operator with null-coalescing operator for more robust null handling Consider using the null-coalescing operator (??) instead of the null-conditional operator (?.) followed by the null-coalescing operator (??) for a more concise expression. dotnet/src/webdriver/StackTraceElement.cs [54-73] -this.className = elementAttributes["className"].ToString() ?? ""; -this.methodName = elementAttributes["methodName"].ToString() ?? ""; -this.fileName = elementAttributes["fileName"].ToString() ?? ""; +this.className = elementAttributes["className"]?.ToString() ?? ""; +this.methodName = elementAttributes["methodName"]?.ToString() ?? ""; +this.fileName = elementAttributes["fileName"]?.ToString() ?? ""; Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion improves the robustness of null handling by using the null-conditional operator, which is more concise and prevents potential null reference exceptions. This enhances code readability and maintainability.	8
	✅ Use pattern matching for more concise type checking and casting Suggestion Impact: The commit implemented pattern matching for type checking and casting for the "lineNumber" attribute, making the code more concise. code diff: - if (elementAttributes.ContainsKey("lineNumber")) + if (elementAttributes.TryGetValue("lineNumber", out object? lineNumberObj)) { - int line = 0; - if (int.TryParse(elementAttributes["lineNumber"].ToString(), out line)) + if (int.TryParse(lineNumberObj?.ToString(), out int line)) { this.lineNumber = line; } Consider using pattern matching with 'is' keyword for type checking and casting in a single step, which can make the code more concise and readable. dotnet/src/webdriver/StackTraceElement.cs [62-69] -if (elementAttributes.ContainsKey("lineNumber")) +if (elementAttributes.TryGetValue("lineNumber", out var lineNumberObj) && lineNumberObj is int lineNumber) { - int lineNumber; - if (int.TryParse(elementAttributes["lineNumber"].ToString(), out lineNumber)) - { - this.lineNumber = lineNumber; - } + this.lineNumber = lineNumber; } Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion makes the code more concise and readable by using pattern matching for type checking and casting, which simplifies the logic and reduces the number of lines.	7
Best practice	Use a more general method for checking file existence Consider using Path.Exists instead of File.Exists to check for file existence, as it's more general and can handle both files and directories. dotnet/src/webdriver/Remote/LocalFileDetector.cs [40] -return File.Exists(keySequence); +return !string.IsNullOrEmpty(keySequence) && Path.Exists(keySequence); Apply this suggestion Suggestion importance[1-10]: 6 Why: The suggestion to use Path.Exists instead of File.Exists is more general and can handle both files and directories, enhancing the flexibility of the code.	6

💡 Need additional feedback ? start a PR chat

[RenderMichael]

@nvborisenko PR feedback addressed, PTAL

[RenderMichael]

@nvborisenko I think everything's been addressed in this PR, it's ready for another look

[nvborisenko]

Sorry for the delay, paid work. Thanks, will review it (90% is already reviewed). For me it is conceptual basis for further upcoming PRs (especially for attributes), thus thanks for understanding.

[RenderMichael]

@nvborisenko Absolutely no worries!

Yes the attributes are important here, and if we are serious about investing in AOT compatibility, those attributes are even more important (not realistic to suppress those warnings the same way as for nullability).

Here is a guide from Microsoft that I use for AOT trimming, with an example of the two options for polyfilling attributes to netstandard (same guideline applies for nullability and AOT attributes).

I think adding the attributes ourselves is fine as long as we hide behind #if !NET8_0_OR_GREATER and make them internal (can duplicate or somehow share for the support package)

[nvborisenko]

Regarding AOT compatibility, we already made a big step forward: we made classic command/responses which are objects properly serialized. Let's keep this progress: CDP and BiDi namespaces promise to be easier, at least it is strongly-typed.

I understand the importance of attributes for nullability, and thanks for sharing the link: now I can vote for Approach 2: Define the attributes internally. If you are seriously motivated, please continue - you are founder!

[RenderMichael]

Changes are now implemented! Thanks for the feedback

[nvborisenko]

I am 99% confident with the proposed changes.

Now, the question is: what if we enable nullable context globally? Given that I can disable it for "DevTools/CDP" namespace, how many warnings will we get related to nullability?

[RenderMichael]

For that, we can add

#nullable disable warnings
#nullable enable annotations

to the top of the generated CDP code. Ideally it would be nice to handle nullability on that side, as long as it’s simple enough.

[RenderMichael]

Sorry, misread your comment about nullability aside from the devtools code.

After this PR, there’s still more code to enable nullability for. Some of it may even involve changing code to handle null values. After all the changes are done, we can set <Nullable>enable<Nullable> in the project file.

[nvborisenko]

Just want to understand how far we are from nullable context enabled globally. If we enable it right now, then do you think we can achieve the goal soon?.. within your help.

[RenderMichael]

I’m not sure. It involves touching every file, but some files were trivial to enable. It all depends on the state of null-protection in the remaining code.

[nvborisenko]

OK, thank you, then let's continue enable it file by file. At the end of this story we will enable it globally, like big commit spread across all files. But please, if you feel that it would be better to enable it globally (to avoid one big boom commit), then let's do. BTW, how many warnings will we get?

[nvborisenko]

Amazing start!