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 Require #15084

Merged
merged 2 commits into from
Jan 15, 2025
Merged

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Jan 14, 2025

User description

Description

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

  • org.openqa.selenium.internal.Require

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

  • Introduced JSpecify Nullness annotations to Require class.

  • Added @Nullable annotations to method parameters and fields.

  • Marked the Require class with @NullMarked for null-safety enforcement.

  • Improved static analysis and IDE support for nullability checks.


Changes walkthrough 📝

Relevant files
Enhancement
Require.java
Added JSpecify Nullness annotations to `Require` class     

java/src/org/openqa/selenium/internal/Require.java

  • Added @NullMarked annotation to the Require class.
  • Annotated method parameters and fields with @Nullable.
  • Enhanced null-safety checks for method arguments.
  • Improved compatibility with static analysis tools.
  • +35/-32 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    14291 - PR Code Verified

    Compliant requirements:

    • Added JSpecify Nullness annotations to Require class
    • Specified which parameters can be null using @nullable
    • Made nullability information transparent to IDEs and static analyzers

    Requires further human verification:

    • Verify IDE integration and static analyzer behavior with the new annotations
    • Test Kotlin interoperability with the annotated code
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Annotation Coverage

    Verify that all method return types that could potentially return null are properly annotated with @nullable

    public static <T> T nonNull(String argName, @Nullable T arg) {
      if (arg == null) {
        throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
      }
      return arg;
    }
    
    public static <T> T nonNull(String argName, @Nullable T arg, String message, Object... args) {
      if (arg == null) {
        throw new IllegalArgumentException(String.join(" ", argName, String.format(message, args)));
      }
      return arg;
    }
    
    public static <T> ArgumentChecker<T> argument(String argName, @Nullable T arg) {
      return new ArgumentChecker<>(argName, arg);
    }
    
    public static Duration nonNegative(String argName, @Nullable Duration arg) {
      if (arg == null) {
        throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
      }
      if (arg.isNegative()) {
        throw new IllegalArgumentException(String.format(MUST_BE_NON_NEGATIVE, argName));
      }
      return arg;
    }
    
    public static Duration nonNegative(@Nullable Duration arg) {
      if (arg == null) {
        throw new IllegalArgumentException(String.format(MUST_BE_SET, "Duration"));
      }
      if (arg.isNegative()) {
        throw new IllegalArgumentException(String.format(MUST_BE_NON_NEGATIVE, "Duration"));
      }
      return arg;
    }
    
    public static Duration positive(String argName, @Nullable Duration arg) {
      if (arg == null) {
        throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
      }
      if (arg.isNegative() || arg.isZero()) {
        throw new IllegalArgumentException(String.format(MUST_BE_POSITIVE, argName));
      }
      return arg;
    }
    
    public static Duration positive(@Nullable Duration arg) {
      if (arg == null) {
        throw new IllegalArgumentException(String.format(MUST_BE_SET, "Duration"));
      }
      if (arg.isNegative() || arg.isZero()) {
        throw new IllegalArgumentException(String.format(MUST_BE_POSITIVE, "Duration"));
      }
      return arg;
    }
    
    public static int nonNegative(String argName, @Nullable Integer number) {
      if (number == null) {
        throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
      }
      if (number < 0) {
        throw new IllegalArgumentException(String.format(MUST_BE_NON_NEGATIVE, argName));
      }
      return number;
    }
    
    public static int positive(String argName, @Nullable Integer number, @Nullable String message) {
      if (number == null) {
        throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
      }
      if (number <= 0) {
        throw new IllegalArgumentException(
            Objects.requireNonNullElseGet(message, () -> String.format(MUST_BE_POSITIVE, argName)));
      }
      return number;
    }
    
    public static double positive(String argName, @Nullable Double number, @Nullable String message) {
      if (number == null) {
        throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
      }
      if (number <= 0) {
        throw new IllegalArgumentException(
            Objects.requireNonNullElseGet(message, () -> String.format(MUST_BE_POSITIVE, argName)));
      }
      return number;
    }
    
    public static double positive(String argName, @Nullable Double number) {
      return positive(argName, number, null);
    }
    
    public static int positive(String argName, @Nullable Integer number) {
      return positive(argName, number, null);
    }
    
    public static IntChecker argument(String argName, @Nullable Integer number) {
      return new IntChecker(argName, number);
    }
    
    @Deprecated(forRemoval = true)
    public static FileChecker argument(String argName, @Nullable File file) {
      return new FileChecker(argName, file);
    }
    
    public static PathChecker argument(String argName, @Nullable Path path) {
      return new PathChecker(argName, path);
    }
    
    public static void stateCondition(boolean state, String message, Object... args) {
      if (!state) {
        throw new IllegalStateException(String.format(message, args));
      }
    }
    
    public static <T> StateChecker<T> state(String name, @Nullable T state) {
      return new StateChecker<>(name, state);
    }
    
    @Deprecated(forRemoval = true)
    public static FileStateChecker state(String name, @Nullable File file) {
      return new FileStateChecker(name, file);
    }
    
    public static PathStateChecker state(String name, @Nullable Path path) {
      return new PathStateChecker(name, path);
    }
    
    public static class ArgumentChecker<T> {
    
      private final String argName;
      private final @Nullable T arg;
    
      ArgumentChecker(String argName, @Nullable T arg) {
        this.argName = argName;
        this.arg = arg;
      }
    
      public T nonNull() {
        if (arg == null) {
          throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
        }
        return arg;
      }
    
      public T nonNull(String message, Object... args) {
        if (arg == null) {
          throw new IllegalArgumentException(String.format(message, args));
        }
        return arg;
      }
    
      public T equalTo(Object other) {
        if (arg == null) {
          throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
        }
        if (!Objects.equals(arg, other)) {
          throw new IllegalArgumentException(String.format(MUST_BE_EQUAL, argName, other));
        }
        return arg;
      }
    
      public T instanceOf(Class<?> cls) {
        if (arg == null) {
          throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
        }
        if (!cls.isInstance(arg)) {
          throw new IllegalArgumentException(argName + " must be an instance of " + cls);
        }
        return arg;
      }
    }
    
    public static class IntChecker {
    
      private final String argName;
      private final @Nullable Integer number;
    
      IntChecker(String argName, @Nullable Integer number) {
        this.argName = argName;
        this.number = number;
      }
    
      public int greaterThan(int max, String message) {
        if (number == null) {
          throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
        }
        if (number <= max) {
          throw new IllegalArgumentException(message);
        }
        return number;
      }
    }
    
    @Deprecated(forRemoval = true)
    public static class FileChecker {
    
      private final String argName;
      private final @Nullable File file;
    
      FileChecker(String argName, @Nullable File file) {
        this.argName = argName;
        this.file = file;
      }
    
      public File isFile() {
        if (file == null) {
          throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
        }
        if (!file.exists()) {
          throw new IllegalArgumentException(
              String.format(MUST_EXIST, argName, file.getAbsolutePath()));
        }
        if (!file.isFile()) {
          throw new IllegalArgumentException(
              String.format(MUST_BE_FILE, argName, file.getAbsolutePath()));
        }
        return file;
      }
    
      public File isDirectory() {
        if (file == null) {
          throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
        }
        if (!file.exists()) {
          throw new IllegalArgumentException(
              String.format(MUST_EXIST, argName, file.getAbsolutePath()));
        }
        if (!file.isDirectory()) {
          throw new IllegalArgumentException(
              String.format(MUST_BE_DIR, argName, file.getAbsolutePath()));
        }
        return file;
      }
    }
    
    public static class PathChecker {
    
      private final String argName;
      private final @Nullable Path path;
    
      PathChecker(String argName, @Nullable Path path) {
        this.argName = argName;
        this.path = path;
      }
    
      public Path isFile() {
        if (path == null) {
          throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
        }
        if (!Files.exists(path)) {
          throw new IllegalArgumentException(
              String.format(MUST_EXIST, argName, path.toAbsolutePath()));
        }
        if (!Files.isRegularFile(path)) {
          throw new IllegalArgumentException(
              String.format(MUST_BE_FILE, argName, path.toAbsolutePath()));
        }
        return path;
      }
    
      public Path isDirectory() {
        if (path == null) {
          throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
        }
        if (!Files.exists(path)) {
          throw new IllegalArgumentException(
              String.format(MUST_EXIST, argName, path.toAbsolutePath()));
        }
        if (!Files.isDirectory(path)) {
          throw new IllegalArgumentException(
              String.format(MUST_BE_DIR, argName, path.toAbsolutePath()));
        }
        return path;
      }
    }
    
    public static class StateChecker<T> {
    
      private final String name;
      private final @Nullable T state;
    
      StateChecker(String name, @Nullable T state) {
        this.name = name;
        this.state = state;
      }
    
      public T nonNull() {
        if (state == null) {
          throw new IllegalStateException(name + " must not be null");
        }
        return state;
      }
    
      public T nonNull(String message, Object... args) {
        if (state == null) {
          throw new IllegalStateException(String.join(" ", name, String.format(message, args)));
        }
        return state;
      }
    
      public T instanceOf(Class<?> cls) {
        if (state == null) {
          throw new IllegalStateException(String.format(MUST_BE_SET, name));
        }
        if (!cls.isInstance(state)) {
          throw new IllegalStateException(name + " must be an instance of " + cls);
        }
        return state;
      }
    }
    
    @Deprecated(forRemoval = true)
    public static class FileStateChecker {
    
      private final String name;
      private final @Nullable File file;
    
      FileStateChecker(String name, @Nullable File file) {
        this.name = name;
        this.file = file;
      }
    
      public File isFile() {
        if (file == null) {
          throw new IllegalStateException(String.format(MUST_BE_SET, name));
        }
        if (!file.exists()) {
          throw new IllegalStateException(String.format(MUST_EXIST, name, file.getAbsolutePath()));
        }
        if (!file.isFile()) {
          throw new IllegalStateException(String.format(MUST_BE_FILE, name, file.getAbsolutePath()));
        }
        return file;
      }
    
      public File isDirectory() {
        if (file == null) {
          throw new IllegalStateException(String.format(MUST_BE_SET, name));
        }
        if (!file.exists()) {
          throw new IllegalStateException(String.format(MUST_EXIST, name, file.getAbsolutePath()));
        }
        if (!file.isDirectory()) {
          throw new IllegalStateException(String.format(MUST_BE_DIR, name, file.getAbsolutePath()));
        }
        return file;
      }
    
      public File isExecutable() {
        if (file == null) {
          throw new IllegalStateException(String.format(MUST_BE_SET, name));
        }
        if (!file.exists()) {
          throw new IllegalStateException(String.format(MUST_EXIST, name, file.getAbsolutePath()));
        }
        if (!file.canExecute()) {
          throw new IllegalStateException(
              String.format(MUST_BE_EXECUTABLE, name, file.getAbsolutePath()));
        }
        return file;
      }
    }
    
    public static class PathStateChecker {
    
      private final String name;
      private final @Nullable Path path;
    
      PathStateChecker(String name, @Nullable Path path) {
        this.name = name;
        this.path = path;
      }

    @mk868
    Copy link
    Contributor Author

    mk868 commented Jan 14, 2025

    Fixed NullAway errors:

    java/src/org/openqa/selenium/internal/Require.java:150: error: [NullAway] passing @Nullable parameter 'null' where @NonNull is required
        return positive(argName, number, null);
                                         ^
        (see http://t.uber.com/nullaway )
    java/src/org/openqa/selenium/internal/Require.java:154: error: [NullAway] passing @Nullable parameter 'null' where @NonNull is required
        return positive(argName, number, null);
                                         ^
        (see http://t.uber.com/nullaway )
    

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Remove nullable annotation from message parameter to prevent potential null pointer exceptions in error handling

    The @Nullable annotation on the message parameter in positive() methods could lead
    to NullPointerException when the message is used in Objects.requireNonNullElseGet().
    Consider adding a null check or using a non-nullable parameter.

    java/src/org/openqa/selenium/internal/Require.java [130-139]

    -public static int positive(String argName, @Nullable Integer number, @Nullable String message) {
    +public static int positive(String argName, @Nullable Integer number, String message) {
       if (number == null) {
         throw new IllegalArgumentException(String.format(MUST_BE_SET, argName));
       }
       if (number <= 0) {
         throw new IllegalArgumentException(
    -        Objects.requireNonNullElseGet(message, () -> String.format(MUST_BE_POSITIVE, argName)));
    +        message != null ? message : String.format(MUST_BE_POSITIVE, argName));
       }
       return number;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies a potential issue where using @nullable with Objects.requireNonNullElseGet could lead to NPE. The proposed solution using a direct null check is more straightforward and safer.

    7

    @diemol diemol merged commit a3a007b into SeleniumHQ:trunk Jan 15, 2025
    1 check passed
    @mk868 mk868 deleted the jspecify-Require branch January 15, 2025 16:26
    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