Skip to content

[java] Feat 14291/jspecify nullable annotation #15924

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Jun 20, 2025

User description

🔗 Related Issues

fixes #14291

💥 What does this PR do?

This pull request introduces the use of @Nullable annotations from the jspecify library to improve nullability handling across the Selenium Java codebase. It also updates the Bazel build files to include the jspecify dependency. The most important changes are grouped into updates to nullability handling and build file modifications.

Nullability Handling Enhancements:

Build File Modifications:

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Add JSpecify @Nullable annotations to improve nullability handling

  • Update InternetExplorerDriver constructor parameters with nullable annotations

  • Enhance DriverService methods with nullable return type annotations

  • Update Bazel build files to include JSpecify dependency


Changes walkthrough 📝

Relevant files
Enhancement
InternetExplorerDriver.java
Add nullable annotations to constructor parameters             

java/src/org/openqa/selenium/ie/InternetExplorerDriver.java

  • Import JSpecify @Nullable annotation
  • Add @Nullable annotations to constructor parameters (service, options,
    clientConfig)
  • +4/-3     
    DriverService.java
    Add nullable annotations to method return types                   

    java/src/org/openqa/selenium/remote/service/DriverService.java

  • Import JSpecify @Nullable annotation
  • Add @Nullable annotations to method return types (getDriverName,
    getDriverProperty, getDriverExecutable)
  • +4/-3     
    Dependencies
    BUILD.bazel
    Add JSpecify dependency to build file                                       

    java/src/org/openqa/selenium/ie/BUILD.bazel

    • Add JSpecify dependency to IE driver build configuration
    +1/-0     
    BUILD.bazel
    Add JSpecify dependency to build file                                       

    java/src/org/openqa/selenium/remote/BUILD.bazel

  • Add JSpecify artifact dependency to remote package build configuration
  • +1/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added C-java Java Bindings B-build Includes scripting, bazel and CI integrations labels Jun 20, 2025
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    1234 - Not compliant

    Non-compliant requirements:

    • Fix JavaScript execution in link's href attribute when using click() method
    • Ensure compatibility with Firefox 42.0
    • Restore functionality that worked in version 2.47.1 but broke in 2.48.0/2.48.2

    5678 - Not compliant

    Non-compliant requirements:

    • Fix ChromeDriver connection failure error on Ubuntu 16.04.4
    • Resolve "ConnectFailure (Connection refused)" error for subsequent ChromeDriver instances
    • Ensure first ChromeDriver instance works without console errors
    • Support Chrome 65.0.3325.181 with ChromeDriver version 2.35

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Annotation Inconsistency

    The constructor parameters are annotated as @nullable but the method implementation immediately checks for null and creates default instances, which suggests these parameters should not be nullable or the annotation placement is incorrect.

    @Nullable InternetExplorerDriverService service,
    @Nullable InternetExplorerOptions options,
    @Nullable ClientConfig clientConfig) {

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 20, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add missing null check validation
    Suggestion Impact:The suggestion was implemented but with a different coding style. Instead of using if-statements, the commit used ternary operators to handle null checks for all three parameters including clientConfig

    code diff:

    +    options = options == null ? new InternetExplorerOptions() : options;
    +    service = service == null ? InternetExplorerDriverService.createDefaultService() : service;
    +    clientConfig = clientConfig == null ? ClientConfig.defaultConfig() : clientConfig;

    Add null check for clientConfig parameter to maintain consistency with other
    parameter validations. The method handles null options and service but doesn't
    validate clientConfig which could cause issues downstream.

    java/src/org/openqa/selenium/ie/InternetExplorerDriver.java [111-119]

     public InternetExplorerDriver(
         @Nullable InternetExplorerDriverService service,
         @Nullable InternetExplorerOptions options,
         @Nullable ClientConfig clientConfig) {
       if (options == null) {
         options = new InternetExplorerOptions();
       }
       if (service == null) {
         service = InternetExplorerDriverService.createDefaultService();
    +  }
    +  if (clientConfig == null) {
    +    clientConfig = ClientConfig.defaultConfig();
    +  }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that while the clientConfig parameter was annotated as @Nullable in the PR, the corresponding null-handling logic was missed. Adding this check prevents a potential NullPointerException and makes the constructor's behavior consistent with the other nullable parameters.

    Medium
    • Update

    @diemol diemol merged commit 3876882 into SeleniumHQ:trunk Jun 23, 2025
    33 of 34 checks passed
    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.

    [🚀 Feature]: JSpecify Nullness annotations for Java
    3 participants