Skip to content
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

[java] Add nullness for interactions #15118

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Jan 20, 2025

User description

Description

In this PR I'm adding nullness annotations for classes:

  • org.openqa.selenium.interactions.Coordinates
  • org.openqa.selenium.interactions.Interactive
  • org.openqa.selenium.interactions.SourceType - I also changed null to "none" as descried in the w3.org, this change should be safe to apply because SourceType.NONE has no uses in the code.

NullAway analysis: #14421

Motivation and Context

The JSpecify nullness annotations will give developers better exposure to potential problems with their code to avoid NullPointerExceptions.
Related issue: #14291

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


Description

  • Added JSpecify @NullMarked annotations to improve nullness handling.

  • Updated SourceType.NONE value from null to "none" for compliance.

  • Enhanced IDE and static analysis support for nullability.


Changes walkthrough 📝

Relevant files
Enhancement
Coordinates.java
Add `@NullMarked` annotation to `Coordinates`                       

java/src/org/openqa/selenium/interactions/Coordinates.java

  • Added @NullMarked annotation to the Coordinates interface.
  • Improved nullability handling for methods in this interface.
  • +2/-0     
    Interactive.java
    Add `@NullMarked` annotation to `Interactive`                       

    java/src/org/openqa/selenium/interactions/Interactive.java

  • Added @NullMarked annotation to the Interactive interface.
  • Enhanced nullability handling for methods in this interface.
  • +2/-0     
    SourceType.java
    Add `@NullMarked` annotation and update `SourceType.NONE`

    java/src/org/openqa/selenium/interactions/SourceType.java

  • Added @NullMarked annotation to the SourceType enum.
  • Changed SourceType.NONE value from null to "none".
  • +4/-1     

    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    14291 - Partially compliant

    Compliant requirements:

    • Add JSpecify Nullness annotations to Selenium framework code
    • Improve IDE and static code analyzer support for null checks

    Non-compliant requirements:

    • Annotations should specify which parameters and return values can be null (only class-level @NullMarked added, no specific parameter/return annotations)

    Requires further human verification:

    • Enhance interoperability with Kotlin (needs testing with Kotlin codebase)
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Breaking Change

    Changing NONE from null to "none" string value might affect existing code that relies on the null value. Verify this change aligns with w3.org spec and won't break existing implementations.

    NONE("none"),

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add null validation for type

    Consider adding a null check in the constructor since type is now used for all enum
    values including NONE. This prevents potential NullPointerException when the enum is
    used.

    java/src/org/openqa/selenium/interactions/SourceType.java [30]

     private final String type;
     
    +private SourceType(String type) {
    +    if (type == null) {
    +        throw new IllegalArgumentException("Type cannot be null");
    +    }
    +    this.type = type;
    +}
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion is highly relevant as it adds crucial null validation to prevent potential NullPointerException, especially important since the PR changes NONE's value from null to "none" and adds @NullMarked annotation.

    8

    @mk868
    Copy link
    Contributor Author

    mk868 commented Jan 20, 2025

    Fixed NullAway error:

    java/src/org/openqa/selenium/interactions/SourceType.java:23: error: [NullAway] passing @Nullable parameter 'null' where @NonNull is required
      NONE(null),
           ^
        (see http://t.uber.com/nullaway )
    

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant