[dotnet] Finalize nullability in internal devtools generator

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

Description

Renames devtools generator project to align with its project name.

Motivation and Context

This front-loads some diffs for work currently being done making a source-generated version of the devtools generator, which will significantly improve the development experience when developing the .NET bindings of Selenium.

Future steps include extracting shareable code into a DevToolsGenerator.Core library, and making another DevToolsGenerator.SourceGenerator library which leverages the shared code and gets hooked into WebDriver.csproj to allow seamless devtools source generation.

Also 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

• Modernized and enhanced the DevTools generator codebase.

• Enabled nullability annotations for improved type safety.

• Refactored classes to use constructor-based initialization.

• Improved error handling and added null checks.

---

Changes walkthrough 📝

	Relevant files
Enhancement	18 files CodeGenerationSettings.cs Enabled nullable reference types for runtime version.        +1/-1      CodeGenerationTemplateSettings.cs Added nullability annotations and default values.                +4/-2      CodeGeneratorBase.cs Updated generic type constraint to enforce class.                +1/-1      CodeGeneratorContext.cs Refactored to use constructor-based initialization.            +3/-3      CommandInfo.cs Refactored to use constructor-based initialization.            +4/-4      EventInfo.cs Refactored to use constructor-based initialization.            +3/-3      ICodeGenerator.cs Updated generic type constraint to enforce class.                +1/-1      ProtocolGenerator.cs Improved null checks and refactored type handling.              +35/-43  TypeInfo.cs Refactored to use constructor-based initialization.            +5/-5      Utility.cs Added null checks and improved regex handling.                      +28/-23  CommandLineOptions.cs Added default values and nullability annotations.                +5/-5      CommandDefinition.cs Enabled nullable reference types for redirect property.    +1/-1      DomainDefinition.cs Enabled nullable reference types for name property.            +1/-1      ProtocolDefinition.cs Enabled nullable reference types for key properties.          +2/-2      ProtocolDefinitionItem.cs Enabled nullable reference types for description and name. +4/-4      ProtocolVersionDefinition.cs Simplified properties and enabled nullability.                      +5/-45    TypeDefinition.cs Refactored to use constructor-based initialization.            +5/-5      Version.cs Enabled nullable reference types for version properties.  +4/-4
Bug fix	2 files TemplatesManager.cs Enhanced error handling for template path resolution.        +4/-4      Program.cs Improved null checks and error handling.                                  +15/-12
Configuration changes	2 files BUILD.bazel Enabled nullability in Bazel build configuration.                +1/-1      DevToolsGenerator.csproj Enabled nullability in project file configuration.              +1/-1

---

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

[qodo-merge-pro]

PR Reviewer Guide 🔍

(Review updated until commit 999eb78)

Here are some key observations to aid the review process:

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Null Reference The code assumes Settings.TemplatesPath is not null when checking if it's empty, but then uses Path.GetDirectoryName which could return null. This could lead to a NullReferenceException. if (string.IsNullOrWhiteSpace(Settings.TemplatesPath)) { Settings.TemplatesPath = Path.GetDirectoryName(Settings.TemplatesPath)!; } Null Check The code assumes browserProtocol and jsProtocol are not null when accessing their properties, but they could be null based on the JsonNode.Parse results. This could cause runtime exceptions. if (jsProtocol!["version"]!["majorVersion"] != browserProtocol!["version"]!["majorVersion"] || jsProtocol!["version"]!["minorVersion"] != browserProtocol!["version"]!["minorVersion"]) Possible Issue The GetTypeMappingForType method assumes typeDefinition.TypeReference is not null when type is empty/whitespace, which could lead to an ArgumentException with a misleading message. if (string.IsNullOrWhiteSpace(type)) { type = typeDefinition.TypeReference ?? throw new ArgumentException("Type definition has neither Type or TypeReference", nameof(typeDefinition)); }

[qodo-merge-pro]

PR Code Suggestions ✨

Latest suggestions up to 999eb78
Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Prevent potential null reference exception when handling file paths The code attempts to get directory name from a null or empty path, which could cause a NullReferenceException. Add proper null check before accessing the path. third_party/dotnet/devtools/src/generator/CodeGen/ProtocolGenerator.cs [24-27] if (string.IsNullOrWhiteSpace(Settings.TemplatesPath)) { - Settings.TemplatesPath = Path.GetDirectoryName(Settings.TemplatesPath)!; + throw new InvalidOperationException("Templates path cannot be null or empty"); } Apply this suggestion Suggestion importance[1-10]: 9 Why: The suggestion fixes a critical bug where getting directory name from a null/empty path could cause a runtime crash. The improved code properly handles the invalid state with a clear error message.	9
	Add input validation for required constructor parameters to prevent invalid state The constructor requires an id parameter but there's no validation to ensure it's not null or empty. Add input validation to prevent potential issues with invalid IDs. third_party/dotnet/devtools/src/generator/ProtocolDefinition/TypeDefinition.cs [8-11] public sealed class TypeDefinition(string id) : ProtocolDefinitionItem { [JsonPropertyName("id")] - public string Id { get; } = id; + public string Id { get; } = !string.IsNullOrEmpty(id) ? id : throw new ArgumentException("Type definition ID cannot be null or empty", nameof(id)); Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion addresses a potential runtime issue by adding validation for a required constructor parameter, preventing the creation of invalid objects with empty IDs that could cause problems later in the code.	8
	Add proper null-conditional operators when accessing nested JSON properties The code accesses JSON properties without proper null checks, which could lead to NullReferenceException. Add null checks when accessing nested JSON properties. third_party/dotnet/devtools/src/generator/Program.cs [163-164] -if (jsProtocol!["version"]!["majorVersion"] != browserProtocol!["version"]!["majorVersion"] || - jsProtocol!["version"]!["minorVersion"] != browserProtocol!["version"]!["minorVersion"]) +if (jsProtocol?["version"]?["majorVersion"] != browserProtocol?["version"]?["majorVersion"] || + jsProtocol?["version"]?["minorVersion"] != browserProtocol?["version"]?["minorVersion"]) Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion improves code robustness by using null-conditional operators when accessing nested JSON properties, preventing potential NullReferenceExceptions during JSON parsing.	7

---

Previous suggestions

Suggestions up to commit c1c4441

Category	Suggestion	Score
Possible issue	Prevent potential null reference exception when validating template path Fix potential null reference issue when checking TemplatesPath. The current check could throw NullReferenceException if TemplatesPath is null. third_party/dotnet/devtools/src/DevToolsGenerator/CodeGen/ProtocolGenerator.cs [23-26] if (string.IsNullOrWhiteSpace(Settings.TemplatesPath)) { - Settings.TemplatesPath = Path.GetDirectoryName(Settings.TemplatesPath); + throw new InvalidOperationException("TemplatesPath cannot be null or empty"); } Suggestion importance[1-10]: 9 Why: The suggestion fixes a critical bug where calling GetDirectoryName on a null TemplatesPath would cause a NullReferenceException. The improved code properly validates the path and provides a clear error message.	9
	Add proper error handling for missing required protocol files Add null checks and error handling for missing protocol files. The code should throw a specific exception with a clear message if either protocol file is not found. third_party/dotnet/devtools/src/DevToolsGenerator/Program.cs [121-127] string browserProtocolPath = args.BrowserProtocolPath; if (!File.Exists(browserProtocolPath)) { browserProtocolPath = Path.Combine(browserProtocolPath, "browser_protocol.json"); if (!File.Exists(browserProtocolPath)) { + throw new FileNotFoundException($"Browser protocol file not found at {browserProtocolPath}"); } } Suggestion importance[1-10]: 8 Why: The suggestion addresses a critical issue where missing protocol files could cause silent failures. Adding explicit error handling improves robustness and provides clear error messages.	8
	Add null validation for required properties to prevent potential null reference exceptions The Version property is marked as required but lacks null checks in its usage, which could lead to NullReferenceException. Add validation in the property setter. third_party/dotnet/devtools/src/DevToolsGenerator/ProtocolDefinition/ProtocolDefinition.cs [13-15] [JsonPropertyName("version")] [JsonRequired] -public Version Version { get; set; } +public Version Version +{ + get => _version; + set => _version = value ?? throw new ArgumentNullException(nameof(value)); +} +private Version _version; Suggestion importance[1-10]: 7 Why: Adding null validation for the required Version property would prevent potential runtime NullReferenceExceptions and enforce the [JsonRequired] constraint at the property level.	7
Security	Improve HTML encoding security by using a comprehensive encoding library instead of manual character replacement The HTML encoding in the Description property getter is not secure against all HTML entities. Consider using a proper HTML encoding library like System.Web.HttpUtility.HtmlEncode() to handle all HTML entities correctly. third_party/dotnet/devtools/src/DevToolsGenerator/ProtocolDefinition/ProtocolDefinitionItem.cs [12-16] public string Description { - get => InitialDescription?.Replace("<", "&lt;").Replace(">", "&gt;"); + get => InitialDescription != null ? System.Web.HttpUtility.HtmlEncode(InitialDescription) : null; set => InitialDescription = value; } Suggestion importance[1-10]: 8 Why: The current manual HTML encoding approach is vulnerable to security issues as it only handles < and > characters. Using HttpUtility.HtmlEncode would provide comprehensive protection against all HTML injection vulnerabilities.	8

[nvborisenko]

Bazel supports generators, so most likely we can leave this project as single assembly, which will be consumed by bazel and dotnet sdk. I just want to avoid 2 different paths when building code. Like the situation when the issue is reproducible in IDE, but not in bazel.

I propose to build POC before moving files.

[RenderMichael]

It would be very convenient to use the generator only. I will work down that path instead (I'm 95% done with a POC on my branch).

[nvborisenko]

From my point of view it would be very convenient if input source for CDP generation would be stored in git repo. Like CDP specification (json files). But these files are generated on fly as part of entire build process, which means that in any case dotnet sdk will always delegate some work to bazel.

The same situation for atoms (js files). And Selenium Manager binaries.

Finally: right now, when dotnet sdk delegates all work to bazel, the last one is smart enough to utilize caching mechanism. AFAIK dotnet incremental source generators are always invoked when I build project....

[qodo-merge-pro]

Persistent review updated to latest commit 999eb78