[java] JSpecify annotations for By locators

[mk868]

User description

Description

In this PR I'm adding nullness annotations for classes

• By
• ByIdOrName
• ByAll
• ByChained

Common

• boolean equals(Object o) - accepts null -> an argument marked with @Nullable
• Map<String, Object> toJson() - the value in map can be null (basis on the By.Remotable.Parameters#toJson() -> value type marked with @Nullable

By

• Remotable.Parameters can contain null value - based on the comment "There may be subclasses where the value is optional. Allow for this." in the Parameters constructor -> value field marked with @Nullable

ByIdOrName

No null values

ByAll

No null values

ByChained

No null values

Motivation and Context

The JSpecify nullness annotations will give developers better exposure to potential problems with their code to avoid NullPointerExceptions. Related issue: https://github.com/SeleniumHQ/selenium/issues/14291

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• [x] 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

• Added JSpecify nullness annotations (@NullMarked and @Nullable) to By, ByIdOrName, ByAll, and ByChained classes to improve null safety.
• Updated toJson methods to handle nullable values.
• Added org.jspecify:jspecify dependency to the Bazel build file.

---

Changes walkthrough 📝

	Relevant files
Enhancement	By.java Add JSpecify nullness annotations to `By` class                    java/src/org/openqa/selenium/By.java Added @NullMarked annotation to the By class. Marked Object parameters and return types with @Nullable where applicable. Updated toJson method to handle nullable values. +12/-9    ByIdOrName.java Add JSpecify nullness annotations to `ByIdOrName` class    java/src/org/openqa/selenium/support/ByIdOrName.java Added @NullMarked annotation to the ByIdOrName class. +2/-0      ByAll.java Add JSpecify nullness annotations to `ByAll` class              java/src/org/openqa/selenium/support/pagefactory/ByAll.java Added @NullMarked annotation to the ByAll class. +2/-0      ByChained.java Add JSpecify nullness annotations to `ByChained` class      java/src/org/openqa/selenium/support/pagefactory/ByChained.java Added @NullMarked annotation to the ByChained class. +2/-0
Dependencies	BUILD.bazel Add JSpecify dependency to Bazel build file                            java/src/org/openqa/selenium/support/BUILD.bazel Added org.jspecify:jspecify artifact dependency. +2/-1

---

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[qodo-code-review[bot]]

PR Reviewer Guide 🔍

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review Annotation Consistency The @Nullable annotation is used inconsistently across similar methods. For instance, equals method in Parameters class and By class both use @Nullable, but similar methods in other classes might not. It's important to ensure that nullability annotations are consistently applied across all similar methods to avoid confusion and potential bugs. Map Value Annotation The toJson method's return type in Parameters class is annotated with @Nullable for the map values. This change should be clearly documented, especially if the API behavior changes, such as returning null values where it previously did not.

[qodo-code-review[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add a null check in the equals method to prevent NullPointerException Consider checking for null in the equals method before casting to By to prevent potential NullPointerException. java/src/org/openqa/selenium/By.java [164] public boolean equals(@Nullable Object o) { - if (!(o instanceof By)) { + if (o == null || !(o instanceof By)) { return false; } [ ] Apply this suggestion <!-- /improve --apply_suggestion=0 --> Suggestion importance[1-10]: 9 Why: Adding a null check in the equals method is a good practice to prevent potential NullPointerExceptions, which enhances the robustness of the code.	9
Enhancement	Use Optional for nullable fields to provide a clearer API Consider using Optional for value in the Parameters class to better handle cases where value might be null, providing a clearer API. java/src/org/openqa/selenium/By.java [347-349] -private final @Nullable Object value; -public Parameters(String using, @Nullable Object value) { +private final Optional<Object> value; +public Parameters(String using, Object value) { this.using = Require.nonNull("Search mechanism", using); - this.value = value; + this.value = Optional.ofNullable(value); [ ] Apply this suggestion <!-- /improve --apply_suggestion=1 --> Suggestion importance[1-10]: 8 Why: Using Optional for nullable fields is a modern and clear approach to handle optional values, improving the API's clarity and reducing the risk of null-related issues.	8
Possible issue	Add null check for value parameter in constructor if it should not be nullable Ensure that the value parameter in the Parameters constructor is checked for null if it is not supposed to be nullable, as the @Nullable annotation allows null values which might lead to runtime exceptions if not handled. java/src/org/openqa/selenium/By.java [349] public Parameters(String using, @Nullable Object value) { this.using = Require.nonNull("Search mechanism", using); - this.value = value; + this.value = Require.nonNull("Value", value); // Add null check if value should not be null [ ] Apply this suggestion <!-- /improve --apply_suggestion=2 --> Suggestion importance[1-10]: 7 Why: This suggestion is contextually accurate and improves the code's robustness by ensuring that null values are handled appropriately if they are not expected.	7
	Change the map to not explicitly allow nullable values to avoid handling issues Use HashMap<String, Object> instead of HashMap<String, @Nullable Object> for params to avoid potential issues with handling null values in the map. java/src/org/openqa/selenium/By.java [382] -private Map<String, @Nullable Object> toJson() { - Map<String, @Nullable Object> params = new HashMap<>(); +private Map<String, Object> toJson() { + Map<String, Object> params = new HashMap<>(); params.put("using", using); params.put("value", value); return Collections.unmodifiableMap(params); [ ] Apply this suggestion <!-- /improve --apply_suggestion=3 --> Suggestion importance[1-10]: 5 Why: While this suggestion could simplify handling null values, it contradicts the purpose of using @Nullable annotations and might not align with the intended design.	5

[mk868]

/help

[qodo-code-review[bot]]

PR Agent Walkthrough 🤖

Welcome to the PR Agent, an AI-powered tool for automated pull request analysis, feedback, suggestions and more.

Here is a list of tools you can use to interact with the PR Agent:

Tool	Description	Trigger Interactively :gem:
DESCRIBE	Generates PR description - title, type, summary, code walkthrough and labels	[ ] Run
REVIEW	Adjustable feedback about the PR, possible issues, security concerns, review effort and more	[ ] Run
IMPROVE	Code suggestions for improving the PR	[ ] Run
UPDATE CHANGELOG	Automatically updates the changelog	[ ] Run
ADD DOCS 💎	Generates documentation to methods/functions/classes that changed in the PR	[ ] Run
TEST 💎	Generates unit tests for a specific component, based on the PR code change	[ ] Run
IMPROVE COMPONENT 💎	Code suggestions for a specific component that changed in the PR	[ ] Run
ANALYZE 💎	Identifies code components that changed in the PR, and enables to interactively generate tests, docs, and code suggestions for each component	[ ] Run
ASK	Answering free-text questions about the PR	[*]
GENERATE CUSTOM LABELS 💎	Generates custom labels for the PR, based on specific guidelines defined by the user	[*]
CI FEEDBACK 💎	Generates feedback and analysis for a failed CI job	[*]
CUSTOM PROMPT 💎	Generates custom suggestions for improving the PR code, derived only from a specific guidelines prompt defined by the user	[*]
SIMILAR ISSUE	Automatically retrieves and presents similar issues	[*]

(1) Note that each tool be triggered automatically when a new PR is opened, or called manually by commenting on a PR.

(2) Tools marked with [*] require additional parameters to be passed. For example, to invoke the /ask tool, you need to comment on a PR: /ask "<question content>". See the relevant documentation for each tool for more details.

[mk868]

/describe

[qodo-code-review[bot]]

PR Description updated to latest commit (https://github.com/SeleniumHQ/selenium/commit/10a72199c915a3acc2123678ded139d1e5763cfd)

• [ ] Copy walkthrough table to "Files Changed" Tab

[diemol]

Is there a way to check in the CI that the annotations are correct?

[mk868]

Yes, adding a null checker plugin to the CI building is a natural follow-up for nullness annotations. Currently, I've started trying to use NullAway with Bazel, for this moment I don't see any blockers, but I don't have anything ready to present yet

[diemol]

Could we add a check in the CI to verify the annotations are correct before merging more PRs?

[mk868]

Sure! I'll play with it and share the results, I think I'll have something to show and merge next week

[VietND96]

@mk868, will you add unit test for this?

[mk868]

@VietND96 the plan is to test nullness annotations using the NullAway. This tool will highlight where null values are passed to NonNull fields. PR with NullAway is here: https://github.com/SeleniumHQ/selenium/pull/14421, but it's blocked by:

• https://github.com/SeleniumHQ/selenium/pull/14768
• PR with the inclusion of SpotBugs analysis in all locations (I'll create PR when PR#14768 merged)

[VietND96]

@mk868, the dependent PR is merged, you can go ahead with others

[mk868]

@VietND96 PR created: https://github.com/SeleniumHQ/selenium/pull/14882

[VietND96]

@diemol, do you think this is good to merge?