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] JSpecify annotations for immutable models and enums #14395

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

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Aug 14, 2024

User description

Description

In this PR I'm adding nullness annotations for classes

  • Dimension
  • Keys
  • Point
  • Rectangle
  • UsernameAndPassword

Dimension

  • equals accepts null argument -> marked with @Nullable

Keys

  • getKeyFromUnicode can return null value -> return type marked with @Nullable

Point

  • equals accepts null argument -> marked with @Nullable

Rectangle

  • equals accepts null argument -> marked with @Nullable

UsernameAndPassword

No null values

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 @NullMarked annotations to several classes and enums to improve nullness handling.
  • Updated equals methods in Dimension, Point, and Rectangle classes to accept @Nullable parameters.
  • Modified getKeyFromUnicode method in Keys enum to return @Nullable.
  • These changes aim to enhance code safety by preventing potential NullPointerExceptions.

Changes walkthrough 📝

Relevant files
Enhancement
Dimension.java
Add nullness annotations to Dimension class                           

java/src/org/openqa/selenium/Dimension.java

  • Added @NullMarked annotation to the Dimension class.
  • Updated equals method to accept @Nullable parameter.
  • +4/-1     
    Keys.java
    Add nullness annotations to Keys enum                                       

    java/src/org/openqa/selenium/Keys.java

  • Added @NullMarked annotation to the Keys enum.
  • Updated getKeyFromUnicode method to return @Nullable.
  • +4/-1     
    Point.java
    Add nullness annotations to Point class                                   

    java/src/org/openqa/selenium/Point.java

  • Added @NullMarked annotation to the Point class.
  • Updated equals method to accept @Nullable parameter.
  • +4/-1     
    Rectangle.java
    Add nullness annotations to Rectangle class                           

    java/src/org/openqa/selenium/Rectangle.java

  • Added @NullMarked annotation to the Rectangle class.
  • Updated equals method to accept @Nullable parameter.
  • +4/-1     
    UsernameAndPassword.java
    Add nullness annotations to UsernameAndPassword class       

    java/src/org/openqa/selenium/UsernameAndPassword.java

    • Added @NullMarked annotation to the UsernameAndPassword class.
    +2/-0     

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

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Inconsistency
    The getKeyFromUnicode method is marked as returning @Nullable Keys, but the method implementation doesn't explicitly return null. It might be worth adding an explicit null return at the end of the method to match the annotation.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    Improve performance of key lookup by using a Map instead of iterating through all values

    Consider using a more efficient data structure, such as a Map, to store and lookup
    Keys by their Unicode values.

    java/src/org/openqa/selenium/Keys.java [186-190]

    +private static final Map<Character, Keys> unicodeToKeyMap = new HashMap<>();
    +
    +static {
    +  for (Keys key : values()) {
    +    unicodeToKeyMap.put(key.charAt(0), key);
    +  }
    +}
    +
     public static @Nullable Keys getKeyFromUnicode(char key) {
    -  for (Keys unicodeKey : values()) {
    -    if (unicodeKey.charAt(0) == key) {
    -      return unicodeKey;
    -    }
    +  return unicodeToKeyMap.get(key);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion improves performance by replacing iteration with a Map for constant-time lookups, which is a significant enhancement for efficiency.

    9
    Enhancement
    Simplify the equals method using instanceof pattern matching

    Consider using instanceof pattern matching to simplify the equals method and avoid
    explicit casting.

    java/src/org/openqa/selenium/Dimension.java [44-47]

     @Override
     public boolean equals(@Nullable Object o) {
    -  if (!(o instanceof Dimension)) {
    -    return false;
    +  if (o instanceof Dimension other) {
    +    return width == other.width && height == other.height;
       }
    +  return false;
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly simplifies the equals method by using instanceof pattern matching, which improves code readability and eliminates the need for explicit casting.

    8
    Convert the class to a record for a more concise and immutable representation

    Consider using a record instead of a class for Rectangle, as it's an immutable data
    class with public final fields.

    java/src/org/openqa/selenium/Rectangle.java [25-30]

     @NullMarked
    -public class Rectangle {
    +public record Rectangle(int x, int y, int width, int height) {
     
    -  public final int x;
    -  public final int y;
    -
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Converting the class to a record provides a more concise and immutable representation, aligning with modern Java practices for data classes.

    8
    Best practice
    Make fields final to ensure immutability of the class

    Consider making the x and y fields final to ensure immutability of the Point class.

    java/src/org/openqa/selenium/Point.java [26-28]

     public class Point {
    -  public int x;
    -  public int y;
    +  public final int x;
    +  public final int y;
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Making fields final enhances the immutability of the class, which is a good practice for ensuring thread safety and reducing potential bugs.

    7

    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.

    2 participants