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 net #15083

Merged
merged 2 commits into from
Jan 15, 2025
Merged

[java] Add nullness for net #15083

merged 2 commits into from
Jan 15, 2025

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.net.DefaultNetworkInterfaceProvider
  • org.openqa.selenium.net.HostIdentifier
  • org.openqa.selenium.net.NetworkInterface
  • org.openqa.selenium.net.NetworkInterfaceProvider

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 Nullness annotations to improve null safety.

  • Marked classes and methods with @NullMarked and @Nullable.

  • Enhanced IDE/static analyzer interoperability for nullability checks.

  • Improved code clarity and reduced potential NullPointerExceptions.


Changes walkthrough 📝

Relevant files
Enhancement
DefaultNetworkInterfaceProvider.java
Add nullness annotations to DefaultNetworkInterfaceProvider

java/src/org/openqa/selenium/net/DefaultNetworkInterfaceProvider.java

  • Added @NullMarked annotation to the class.
  • Marked getLoInterface method with @Nullable.
  • Imported JSpecify annotations.
  • +4/-1     
    HostIdentifier.java
    Add nullness annotations to HostIdentifier                             

    java/src/org/openqa/selenium/net/HostIdentifier.java

  • Added @NullMarked annotation to the class.
  • Marked hostName and hostAddress as @Nullable.
  • Imported JSpecify annotations.
  • +5/-2     
    NetworkInterface.java
    Add nullness annotations to NetworkInterface                         

    java/src/org/openqa/selenium/net/NetworkInterface.java

  • Added @NullMarked annotation to the class.
  • Marked several fields and methods with @Nullable.
  • Enhanced null safety for IP address methods.
  • +8/-5     
    NetworkInterfaceProvider.java
    Add nullness annotations to NetworkInterfaceProvider         

    java/src/org/openqa/selenium/net/NetworkInterfaceProvider.java

  • Added @NullMarked annotation to the interface.
  • Marked getLoInterface method with @Nullable.
  • Imported JSpecify annotations.
  • +5/-1     

    💡 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:

    • Add JSpecify Nullness annotations to Selenium framework code
    • Specify which parameters and return values can be null
    • Make nullability information transparent to IDEs and static code analyzers

    Requires further human verification:

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

    Null Safety

    Verify that the nullable return values in getIp4LoopbackOnly, getIp4NonLoopBackOnly, and getIp6Address methods are handled properly by all callers

    @Nullable InetAddress getIp4LoopbackOnly() {
      // Goes by the wildly unscientific assumption that if there are more than one set of
      // loopback addresses, firefox will bind to the last one we get.
      // An alternate theory if this fails is that firefox prefers 127.0.0.1
      // Most "normal" boxes don't have multiple addresses so we'll just refine this
      // algorithm until it works.
      // See NetworkUtilsTest#testOpenSuseBoxIssue1181
      // Issue 1181
      if (!isLoopBack()) {
        return null;
      }
      InetAddress lastFound = null;
      for (InetAddress inetAddress : inetAddresses) {
        if (inetAddress.isLoopbackAddress() && !isIpv6(inetAddress)) {
          lastFound = inetAddress;
        }
      }
      return lastFound;
    }
    
    static boolean isIpv6(InetAddress address) {
      return address instanceof Inet6Address;
    }
    
    public @Nullable InetAddress getIp4NonLoopBackOnly() {
      for (InetAddress inetAddress : inetAddresses) {
        if (!inetAddress.isLoopbackAddress() && !isIpv6(inetAddress)) {
          return inetAddress;
        }
      }
      return null;
    }
    
    public @Nullable InetAddress getIp6Address() {
      for (InetAddress inetAddress : inetAddresses) {
        if (isIpv6(inetAddress)) {
          return inetAddress;
        }

    @mk868
    Copy link
    Contributor Author

    mk868 commented Jan 14, 2025

    Fixed NullAway errors:

    java/src/org/openqa/selenium/net/DefaultNetworkInterfaceProvider.java:111: error: [NullAway] returning @Nullable expression from method with @NonNull return type
          return (byName != null) ? new NetworkInterface(byName) : null;
          ^
        (see http://t.uber.com/nullaway )
    java/src/org/openqa/selenium/net/NetworkInterface.java:46: error: [NullAway] initializer method does not guarantee @NonNull fields networkInterface (line 37), isLoopback (line 39) are initialized along all control-flow paths (remember to check for exceptions or early returns).
      NetworkInterface(String name, Iterable<InetAddress> inetAddresses) {
      ^
        (see http://t.uber.com/nullaway )
    java/src/org/openqa/selenium/net/NetworkInterface.java:94: error: [NullAway] returning @Nullable expression from method with @NonNull return type
          return null;
          ^
        (see http://t.uber.com/nullaway )
    java/src/org/openqa/selenium/net/NetworkInterface.java:102: error: [NullAway] returning @Nullable expression from method with @NonNull return type
        return lastFound;
        ^
        (see http://t.uber.com/nullaway )
    java/src/org/openqa/selenium/net/NetworkInterface.java:115: error: [NullAway] returning @Nullable expression from method with @NonNull return type
        return null;
        ^
        (see http://t.uber.com/nullaway )
    java/src/org/openqa/selenium/net/NetworkInterface.java:124: error: [NullAway] returning @Nullable expression from method with @NonNull return type
        return null;
        ^
        (see http://t.uber.com/nullaway )
    java/src/org/openqa/selenium/net/HostIdentifier.java:35: error: [NullAway] @NonNull static field hostName not initialized
      private static volatile String hostName;
                                     ^
        (see http://t.uber.com/nullaway )
    java/src/org/openqa/selenium/net/HostIdentifier.java:36: error: [NullAway] @NonNull static field hostAddress not initialized
      private static volatile String hostAddress;
                                     ^
        (see http://t.uber.com/nullaway )
    

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Initialize nullable fields in constructor to make null state handling explicit

    Initialize the isLoopback field in the constructor to avoid potential null pointer
    issues when accessing it later. Since it's marked as @nullable Boolean, any code
    using this field should handle the null case explicitly.

    java/src/org/openqa/selenium/net/NetworkInterface.java [42-46]

     private @Nullable Boolean isLoopback;
     
     public NetworkInterface(java.net.NetworkInterface networkInterface) {
       this(networkInterface.getName(), list(networkInterface.getInetAddresses()));
       this.networkInterface = networkInterface;
    +  this.isLoopback = null;  // Explicitly initialize
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: The suggestion to explicitly initialize the nullable field improves code clarity and makes null handling more intentional, though the field was already implicitly initialized to null.

    4

    @diemol diemol merged commit 34540ec into SeleniumHQ:trunk Jan 15, 2025
    34 checks passed
    @mk868 mk868 deleted the jspecify-net branch January 15, 2025 16:26
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    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