Skip to content

[java] Add JSpecify nullable annotations to exception classes pt3 #16026

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

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Jul 8, 2025

User description

🔗 Related Issues

partially fixes #14291

💥 What does this PR do?

same as
#16024
#16025

🔧 Implementation Notes

This pull request introduces nullness annotations from the jspecify library to improve type safety in exception classes within the Selenium Java codebase. The changes ensure that method parameters and classes explicitly handle nullable values, reducing ambiguity and potential runtime errors.

Addition of jspecify nullness annotations:

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Add JSpecify nullable annotations to exception classes

  • Improve type safety with @NullMarked and @nullable

  • Update build dependencies for jspecify library


Changes diagram

flowchart LR
  A["Exception Classes"] --> B["Add @NullMarked"]
  A --> C["Add @Nullable to parameters"]
  B --> D["Type Safety Improvement"]
  C --> D
  E["Build Files"] --> F["Add jspecify dependency"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
5 files
InvalidCookieDomainException.java
Add JSpecify annotations to cookie exception                         
+7/-3     
UnableToSetCookieException.java
Add JSpecify annotations to cookie exception                         
+7/-3     
DevToolsException.java
Add JSpecify annotations to DevTools exception                     
+6/-3     
JsonException.java
Add JSpecify annotations to JSON exception                             
+6/-3     
ScreenshotException.java
Add JSpecify annotations to screenshot exception                 
+6/-3     
Dependencies
2 files
BUILD.bazel
Add jspecify dependency to devtools build                               
+1/-0     
BUILD.bazel
Add jspecify dependency to json build                                       
+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-devtools Includes everything BiDi or Chrome DevTools related labels Jul 8, 2025
    @iampopovich iampopovich marked this pull request as ready for review July 8, 2025 20:01
    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 8, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 3b4cc24)

    Here are some key observations to aid the review process:

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

    Null Dereference

    The constructor accepting a Throwable cause calls cause.getMessage() without null checking, which could cause a NullPointerException if a null cause is passed despite the @nullable annotation.

    public DevToolsException(@Nullable Throwable cause) {
      this(cause.getMessage(), cause);
    }

    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 8, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 3b4cc24
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add null check for cause parameter

    The constructor calls cause.getMessage() without null checking, which will throw
    a NullPointerException if cause is null. Add a null check before accessing the
    message.

    java/src/org/openqa/selenium/devtools/DevToolsException.java [27-29]

     public DevToolsException(@Nullable Throwable cause) {
    -  this(cause.getMessage(), cause);
    +  this(cause != null ? cause.getMessage() : null, cause);
     }
    • Apply / Chat
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies that calling cause.getMessage() can result in a NullPointerException since the cause parameter is annotated as @Nullable, and provides a valid fix.

    High
    • More

    Previous suggestions

    Suggestions up to commit 4cdd18a
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add null check before method call

    The code assumes cause is non-null when calling cause.getMessage(), but the
    parameter is marked as @Nullable. This will throw a NullPointerException if
    cause is null.

    java/src/org/openqa/selenium/devtools/DevToolsException.java [27-29]

     public DevToolsException(@Nullable Throwable cause) {
    -  this(cause.getMessage(), cause);
    +  this(cause != null ? cause.getMessage() : null, cause);
     }
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies a potential NullPointerException because the cause parameter is annotated as @Nullable but is dereferenced without a null check.

    High

    @iampopovich iampopovich marked this pull request as draft July 9, 2025 08:50
    @iampopovich iampopovich marked this pull request as ready for review July 9, 2025 10:27
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    B-devtools Includes everything BiDi or Chrome DevTools related 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
    2 participants