[java] NullAway checks added

[mk868]

User description

Description

In this PR I'm adding NullAway nullness analyzer to the project. By default NullAway plugin is disabled, to enable them set //java:nullaway_level flag:

• --//java:nullaway_level=WARN
• --//java:nullaway_level=ERROR

NullAway Configuration details:

• -Xep:NullAway:WARN - report problems as warnings
• -Xep:NullAway:ERROR - report problems as errors - stop compilation when an error occurs
• -XepOpt:NullAway:AnnotatedPackages=org.openqa.selenium - analyze this package (including subpackages)
• -XepOpt:NullAway:JSpecifyMode=true - enable support for annotations on generic types

NOTE: NullAway has a special behavior, all classes located under AnnotatedPackages are treated as annotated with @NullMarked - source. Anyway, We need to mark these classes with @NullMarked annotation manually to remain compliant with JSpecify specification.

As suggested, I also added null checking as part of the GitHub workflow.

Motivation and Context

The JSpecify nullness annotations will give developers better exposure to potential problems with their code to avoid NullPointerExceptions.
Related issue: #14291
To verify potential problems with the Selenium source code, we can use nullness analyzers that report code issues based on JSpecify annotations.
One of plugins that is easy to configure with Bazel is NullAway, potential alternatives described here.
Context: #14372 (comment)

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, configuration changes

---

Description

• Added NullAway as a dependency to the project to enhance nullness analysis.
• Introduced a new java_plugin for NullAway in the Bazel build configuration.
• Configured NullAway for selenium-api and selenium-support modules by adding it to java_export and java_library targets.
• Updated Maven install JSON to include NullAway and its dependencies.

---

Changes walkthrough 📝

	Relevant files
Dependencies	MODULE.bazel Add NullAway dependency to Maven install                                  MODULE.bazel Added com.uber.nullaway:nullaway:0.11.2 to the list of Maven dependencies. +1/-0      maven_install.json Update Maven install with NullAway artifacts                          java/maven_install.json Updated artifact hashes. Added NullAway and its dependencies to the resolved artifacts. +116/-2
Configuration changes	BUILD.bazel Introduce NullAway Java plugin                                                      java/BUILD.bazel Added a new java_plugin for NullAway. +10/-0    BUILD.bazel Configure NullAway for selenium-api module                              java/src/org/openqa/selenium/BUILD.bazel Configured NullAway plugin for java_export. Added javacopts for NullAway configuration. +8/-0      BUILD.bazel Configure NullAway for selenium-support module                      java/src/org/openqa/selenium/support/BUILD.bazel Configured NullAway plugin for java_export and java_library. Added javacopts for NullAway configuration. +16/-0

---

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

[qodo-merge-pro]

PR Reviewer Guide 🔍

(Review updated until commit a1bc007)

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review Configuration Complexity The new java_library function introduces complex configuration for NullAway. Consider extracting the NullAway configuration into a separate function for better maintainability. Build Configuration The addition of string_flag and config_setting for NullAway levels increases build complexity. Ensure this doesn't negatively impact build times or cause confusion for developers.

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	Add an option to treat generated code as unannotated in NullAway configuration Consider adding the '-XepOpt:NullAway:TreatGeneratedAsUnannotated=true' option to the javacopts. This option tells NullAway to treat generated code as unannotated, which can help reduce false positives in generated code that may not have proper null annotations. java/src/org/openqa/selenium/BUILD.bazel [44-48] javacopts = [ '-Xep:NullAway:WARN', '-XepOpt:NullAway:AnnotatedPackages=org.openqa.selenium', - '-XepOpt:NullAway:JSpecifyMode=true' + '-XepOpt:NullAway:JSpecifyMode=true', + '-XepOpt:NullAway:TreatGeneratedAsUnannotated=true' ], Apply this suggestion Suggestion importance[1-10]: 7 Why: This suggestion is valid as it enhances the NullAway configuration by reducing false positives in generated code. It is a minor improvement but can be beneficial for code quality.	7
	Add an option to treat generated code as unannotated in NullAway configuration for both targets Consider adding the '-XepOpt:NullAway:TreatGeneratedAsUnannotated=true' option to the javacopts in both targets. This option tells NullAway to treat generated code as unannotated, which can help reduce false positives in generated code that may not have proper null annotations. java/src/org/openqa/selenium/support/BUILD.bazel [42-46] javacopts = [ '-Xep:NullAway:WARN', '-XepOpt:NullAway:AnnotatedPackages=org.openqa.selenium', - '-XepOpt:NullAway:JSpecifyMode=true' + '-XepOpt:NullAway:JSpecifyMode=true', + '-XepOpt:NullAway:TreatGeneratedAsUnannotated=true' ], Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion is appropriate for improving the NullAway configuration by addressing potential false positives in generated code. It is a minor enhancement but useful for maintaining code quality.	7
Best practice	Add a tag to the NullAway plugin to identify it as a static analysis tool Consider adding a tags attribute to the nullaway java_plugin to mark it as a custom static analysis tool. This can be useful for build system integrations and CI/CD pipelines that need to identify and potentially skip or separately run static analysis tools. java/BUILD.bazel [24-32] java_plugin( name = "nullaway", visibility = [ "//java:__subpackages__", ], deps = [ artifact("com.uber.nullaway:nullaway"), - ] + ], + tags = ["static-analysis"] ) Apply this suggestion Suggestion importance[1-10]: 6 Why: Adding a tag to identify the NullAway plugin as a static analysis tool is a good practice for build system integration. It is a minor improvement that aids in better organization and management of build processes.	6

[joerg1985]

Why not -Xep:NullAway:ERROR, otherwise the huge output of the CI might swallow the warnings.
What do others think about this?

[mk868]

@joerg1985
At the moment we have ~80 warnings.
I think setting the ERROR level will cause the compilation to fail

I have such steps in my mind:

1. For each Selenium artifact: (selenium-api, selenium-support, selenium-remote-driver, ...)
   1. Add NullAway with WARNING level
   2. Annotate all classes in the module with the proper JSpecify annotations (the number of warnings should decrease)
   3. Fix bugs in the code (the number of warnings should ideally be 0)
   4. Change NullAway level to ERROR

What do you think?

[diemol]

What is the ideal way to proceed? Should we have the build fail if this check fails, or should the CI pipeline fail?

I tend to prefer the latter. What do you think?

[mk868]

Agree,

So, I'll modify PR:

• I'll set NullAway reporting level to ERROR
• I'll make Bazel optionally run NullAway analysis via flag/tag/argument (I need to check that)
• I'll create an extra GitHub workflow job in the ci-java.yml file with NullAway analysis

Does it sound good?

[mk868]

/describe

[qodo-merge-pro]

Preparing PR description...

[mk868]

/review

[qodo-merge-pro]

Persistent review updated to latest commit a1bc007

[mk868]

Hi @joerg1985, @diemol
I updated the Pull Request.
Now nullness checks are part of the workflow (NullAway set to log issues with the error level) - I added nullaway_level flag to not destroy regular builds during development.

[mk868]

Branch rebased

[mk868]

Hello,

I just pushed two commits, with changes:

• the `trunk' branch was merged
• NullAway updated to the 0.11.3
• the code was reformatted

I'm still not sure if the changes related to NullAway in the GitHub pipeline that I proposed fit the Selenium workflow.
If I get any suggestions for changes then I will try to improve the code

[mk868]

Is there any chance to merge this?
Or should I apply some changes / split the PR into several smaller ones?

[diemol]

Are the spotbugs failures coming from the checks you added?

[mk868]

To be honest, these SpotBugs errors are confusing to me because there is no clear error message
From what I see NullAway and Spotbugs, don't have common transitive dependencies which updates could corrupt spotbugs checks.

Could you re-run GitHub Workflow to see if the problem is repeatable?

[diemol]

Yes, those errors only happen in this PR.

[mk868]

Thank you for pointing this out, I found the reason

Until my change, SpotBugs analysis has been run only for classes enclosed in the java_library.
For example: /java/src/org/openqa/selenium/cli/BUILD.bazel - SpotBugs analysis is run for *.java files.

Files that were not included in java_library but in java_export were not subject to SpotBugs analysis.
That's because java_export calls the native.java_library, which by default has not linting tests.
For example /java/src/org/openqa/selenium/edge/BUILD.bazel - no SpotBugs analysis for *.java files.

My changes have updated the implementation so that SpotBugs analysis is also run for files enclosed in the java_export.
This exposed actual flaws in the code that we were not aware of.

New bugs found by SpotBugs:

• //java/src/org/openqa/selenium/chromium:chromium-lib-spotbugs
  • H C RV: Bad attempt to compute absolute value of signed 32-bit hashcode in org.openqa.selenium.chromium.ChromiumDriver.pin(String) At ChromiumDriver.java:[line 215] - also linted in my IDE by Sonar
• //java/src/org/openqa/selenium/devtools/v85:v85-lib-spotbugs
  • D UC: Useless condition: it's known that code <= 399 (0x18f) at this point At V85Network.java:[line 140] - also linted in my IDE, condition if (code < 300 && code > 399) is never true
• //java/src/org/openqa/selenium/firefox:firefox-lib-spotbugs
  • M D NP: Possible null pointer dereference in org.openqa.selenium.firefox.AddHasExtensions$1.zipDirectory(Path) due to return value of called method Dereferenced at AddHasExtensions.java:[line 104] - probably a false flag
• //java/src/org/openqa/selenium:core-lib-spotbugs
  • M D REC: Exception is caught when Exception is not thrown in org.openqa.selenium.net.PortProber.isFree(String, int) At PortProber.java:[line 122] - rather cosmetic change (replace catch (Exception e) with catch (IOException | RuntimeException e)

I think this analysis is desired.
Maybe I should activate SpotBugs for java_export and also address these 4 errors in a dedicated PR?
What do you think?

[diemol]

I think a different PR for that makes sense, thank you.

[mk868]

I merged trunk into my branch, SpotBugs related issues should be fixed now.
Can we re-run checks?