Skip to content

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Oct 12, 2025

User description

🔗 Related Issues

Related issue: #14291
Original PR (auto-closed): #14421

💥 What does this PR do?

Introduce the NullAway nullness analyzer to Selenium’s Java builds to simplify the process of adding JSpecify annotations and easily reveal inconsistencies.
This is a recreation of the original PR #14421 which was automatically closed.
I intentionally omitted the GitHub CI configuration to run NullAway analysis locally to simplify addition of the JSpecify annotations.
After applying all JSpecify annotations, we can enable verification in GitHub CI - it's too early for that right now, because it will only cause build failures.

By default, the NullAway plugin is disabled, to enable it, set the //java:nullaway_level flag:

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

For example:

bazel build //java/... --//java:nullaway_level=WARN

🔧 Implementation Notes

Used NullAway parameters:

  • -Xep:NullAway:WARN - report problems as warnings
  • -Xep:NullAway:ERROR - report problems as errors and stop compilation when an error occurs - good for CI/CD
  • -XepOpt:NullAway:AnnotatedPackages=org.openqa.selenium - analyze this package (including subpackages)

I also updated org.checkerframework:checker-qual to version 3.49.2 because there was a conflict with version 3.21.2.

💡 Additional Considerations

Once all classes are properly and consistently annotated with JSpecify, we can add the configuration to GitHub CI to ensure nullness correctness for newly added code.

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Integrates NullAway static analyzer for Java nullness checking

  • Adds configurable build flag (--//java:nullaway_level) with WARN/ERROR levels

  • Updates checker-qual dependency from 3.21.2 to 3.49.2

  • Creates custom java_library wrapper with conditional NullAway plugin injection


Diagram Walkthrough

flowchart LR
  A["Build flag configuration"] --> B["java_library wrapper"]
  B --> C["Conditional NullAway plugin"]
  C --> D["Nullness analysis"]
  E["Dependency updates"] --> D
Loading

File Walkthrough

Relevant files
Enhancement
java_library.bzl
Add custom java_library wrapper with NullAway support       

java/private/java_library.bzl

  • Creates new java_library wrapper function
  • Implements conditional NullAway plugin injection based on build flags
  • Configures NullAway javacopts for WARN and ERROR levels
  • Sets annotated packages to org.openqa.selenium
+49/-0   
export.bzl
Switch to custom java_library wrapper                                       

java/private/export.bzl

  • Removes direct import of java_library from contrib_rules_jvm
  • Imports custom java_library from local wrapper module
+1/-1     
library.bzl
Update library imports to use wrapper                                       

java/private/library.bzl

  • Removes java_library import from contrib_rules_jvm
  • Imports custom java_library from wrapper module
+1/-1     
Configuration changes
BUILD.bazel
Configure NullAway plugin and build flags                               

java/BUILD.bazel

  • Defines nullaway java_plugin with uber.nullaway dependency
  • Creates nullaway_level string flag with NONE/WARN/ERROR values
  • Adds config_setting rules for WARN and ERROR levels
+35/-0   
Dependencies
MODULE.bazel
Add NullAway and update checker-qual dependencies               

MODULE.bazel

  • Adds com.uber.nullaway:nullaway:0.12.10 dependency
  • Updates org.checkerframework:checker-qual from 3.21.2 to 3.49.2
+2/-0     
maven_install.json
Regenerate Maven dependency lockfile                                         

java/maven_install.json

  • Updates artifact hashes for new dependencies
  • Adds NullAway artifact with transitive dependencies
  • Updates checker-qual to 3.49.2 with new packages
  • Adds dataflow-nullaway dependency
+100/-6 

@selenium-ci selenium-ci added C-java Java Bindings B-build Includes scripting, bazel and CI integrations labels Oct 12, 2025
Copy link
Contributor

qodo-merge-pro bot commented Oct 12, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #14291
🔴 Add JSpecify Nullness annotations across Selenium’s Java codebase so IDEs and static
analyzers understand nullability of parameters and return values.
Ensure tooling/CI can surface nullness issues, improving developer feedback and avoiding
NullPointerExceptions.
Improve Kotlin interoperability by providing explicit nullability information.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link
Contributor

qodo-merge-pro bot commented Oct 12, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix incorrect Bazel select concatenation

Reverse the order of concatenation for plugins and javacopts to be select({...})
+ list. The current order list + select({...}) is invalid in Bazel and will
cause build failures.

java/private/java_library.bzl [38-49]

 # global place for NullAway plugin use
 _java_library(
     name = name,
     deps = deps,
     srcs = srcs,
     exports = exports,
     tags = tags,
     visibility = visibility,
-    plugins = plugins + nullaway_plugins,
-    javacopts = javacopts + nullaway_javacopts,
+    plugins = nullaway_plugins + plugins,
+    javacopts = nullaway_javacopts + javacopts,
     **kwargs
 )
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical issue in Bazel's handling of select concatenation; list + select({...}) is not a valid operation for rule attributes and will cause the build to fail, whereas select({...}) + list is the correct pattern. This fix is essential for the functionality introduced in the PR to work correctly.

High
High-level
Consider a toolchain-based implementation

Instead of wrapping the java_library rule to add the NullAway plugin, consider
using a custom java_toolchain. This approach better separates tooling
configuration from build definitions, aligning with Bazel best practices and
improving maintainability.

Examples:

java/private/java_library.bzl [6-49]
def java_library(
        name,
        deps = [],
        srcs = [],
        exports = [],
        tags = [],
        visibility = None,
        javacopts = [],
        plugins = [],
        **kwargs):

 ... (clipped 34 lines)

Solution Walkthrough:

Before:

# In java/private/java_library.bzl
load("@contrib_rules_jvm//java:defs.bzl", _java_library = "java_library")

def java_library(name, plugins=[], javacopts=[], **kwargs):
    nullaway_plugins = select({
        "//java:use_nullaway_level_warn": ["//java:nullaway"],
        "//java:use_nullaway_level_error": ["//java:nullaway"],
        ...
    })
    nullaway_javacopts = select({
        "//java:use_nullaway_level_warn": ["-Xep:NullAway:WARN", ...],
        "//java:use_nullaway_level_error": ["-Xep:NullAway:ERROR", ...],
        ...
    })

    _java_library(
        name = name,
        plugins = plugins + nullaway_plugins,
        javacopts = javacopts + nullaway_javacopts,
        **kwargs
    )

After:

# In a new file, e.g., java/toolchains/BUILD.bazel
java_toolchain(
    name = "selenium_java_toolchain_impl",
    # ... standard toolchain attributes
    plugins = ["//java:nullaway"],
)

# In WORKSPACE or MODULE.bazel
register_toolchains("//java/toolchains:selenium_java_toolchain")

# The java_library wrapper is removed, and build files use the
# standard java_library rule. The toolchain is selected via
# Bazel flags, not config_setting in every library.
# The --//java:nullaway_level flag would now control which toolchain is active.
Suggestion importance[1-10]: 7

__

Why: This is a valid and significant architectural suggestion that proposes a more idiomatic Bazel approach, improving long-term maintainability by separating tooling from build logic.

Medium
Learned
best practice
Make annotated packages configurable

Avoid hard-coding AnnotatedPackages; expose it as a build setting so different
modules can specify their own annotated packages, keeping configuration accurate
and reusable.

java/private/java_library.bzl [26-36]

+# In java/BUILD.bazel
+string_flag(
+    name = "nullaway_annotated_packages",
+    build_setting_default = "org.openqa.selenium",
+)
+config_setting(
+    name = "use_nullaway_annotated_packages_default",
+    flag_values = {":nullaway_annotated_packages": "org.openqa.selenium"},
+)
+
+# In java/private/java_library.bzl
+load("//java:defs.bzl", "nullaway_annotated_packages")  # or read via select on flag
 nullaway_javacopts = select({
     "//java:use_nullaway_level_warn": [
         "-Xep:NullAway:WARN",
-        "-XepOpt:NullAway:AnnotatedPackages=org.openqa.selenium",
+        "-XepOpt:NullAway:AnnotatedPackages=$(location //java:nullaway_annotated_packages)",
     ],
     "//java:use_nullaway_level_error": [
         "-Xep:NullAway:ERROR",
-        "-XepOpt:NullAway:AnnotatedPackages=org.openqa.selenium",
+        "-XepOpt:NullAway:AnnotatedPackages=$(location //java:nullaway_annotated_packages)",
     ],
     "//conditions:default": [],
 })
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Make concurrency and lifecycle code robust by correctly computing timeouts and ensuring deterministic configuration; avoid hard-coding scope-specific settings and prefer configurable parameters to keep API/documentation accurate and consistent.

Low
  • Update

Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Let's get this in and enable a GH workflow when more coverage has been put in place.

@diemol diemol merged commit b0dade7 into SeleniumHQ:trunk Oct 14, 2025
10 checks passed
@mk868 mk868 deleted the nullaway-build branch October 14, 2025 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations C-java Java Bindings Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants