Add Opt.ifPresentOrElse()

[mernst]

Resolves TODO comment at line 76 in Opt.java by implementing the ifPresentOrElse method that mirrors java.util.Optional.ifPresentOrElse introduced in Java 9.

The method takes a nullable value, a Consumer to be executed if the value is non-null, and a Runnable to be executed if the value is null.

Also adds corresponding test cases to OptTest.java to verify the implementation works correctly with the nullness checker.

[claude]

PR Review: Add Opt.ifPresentOrElse()

Thank you for implementing the ifPresentOrElse method to resolve the TODO comment! The implementation is well-structured and follows the existing patterns in the Opt class. Here's my detailed review:

✅ Code Quality and Best Practices

Strengths:

• The implementation correctly mirrors java.util.Optional.ifPresentOrElse() from Java 9
• Method signature follows the existing pattern in the class with proper nullness annotations
• Javadoc is clear and includes the @see reference to the Java standard library equivalent
• Code style is consistent with the rest of the class
• Removes the TODO comment as intended

Observations:

• Parameter naming differs slightly from ifPresent method:

  • ifPresent uses consumer (line 70)
  • ifPresentOrElse uses action (line 87)

  For consistency, consider using consumer for the first parameter to match the existing ifPresent method, unless there's a specific reason to follow Java 9's parameter naming convention.

✅ Implementation Correctness

The implementation is correct and bug-free:

• Properly handles the null check on primary
• Correctly invokes action.accept(primary) when non-null
• Correctly invokes emptyAction.run() when null
• The type parameter <T> is appropriate (matches ifPresent)

✅ Test Coverage

Strengths:

• Three test methods cover different scenarios:
  • m3: Tests the same pattern as m1 (requires suppression due to flow refinement limitations)
  • m4: Verifies the parameter s is properly refined to @NonNull in the consumer
  • m5: Tests both branches with a side-effect pattern

Suggestions for Enhanced Coverage:

While the current tests verify the nullness checker works correctly, consider adding tests that mirror the comprehensive patterns in flow/TestOpt.java:

1. Test with null argument that should trigger error:

   void m6(@Nullable String o) {
     // :: error: (argument)
     Opt.ifPresentOrElse(o, s -> s.toString(), null);
   }

2. Test with null consumer that should trigger error:

   void m7(@Nullable String o) {
     // :: error: (argument)
     Opt.ifPresentOrElse(o, null, () -> System.out.println("empty"));
   }

3. Test that verifies the parameter is non-null in the action:

   void m8(@Nullable String o) {
     Opt.ifPresentOrElse(o, (@NonNull String s) -> s.toString(), () -> {});
   }

These additional tests would validate that the nullness annotations on the method signature are being enforced correctly by the checker.

✅ Performance Considerations

No performance concerns:

• Simple conditional logic with negligible overhead
• Matches the performance characteristics of java.util.Optional.ifPresentOrElse()

✅ Security Concerns

No security issues identified:

• No unsafe operations
• No potential for null pointer exceptions when used correctly
• Properly typed to prevent misuse

📊 Summary

This is a solid implementation that correctly resolves the TODO comment. The code quality is high, and the implementation is correct. The test coverage is adequate for basic verification, though it could be enhanced with negative test cases to ensure the nullness checker properly validates incorrect usage patterns.

Recommendation: ✅ Approve (with optional suggestions for enhanced test coverage)

The suggestions above are minor improvements and not blockers for merging this PR.

[coderabbitai]

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Added a new public static method ifPresentOrElse to org.checkerframework.checker.nullness.util.Opt that accepts a primary value, a Consumer action, and a Runnable emptyAction; it invokes the action when the primary is non-null and runs emptyAction otherwise. Tests were added to checker/tests/nullness/OptTest.java (m3, m4, m5) exercising the new method (one suppressing a nullable dereference warning, one unannotated, and one tracking which branch ran). docs/CHANGELOG.md was updated to list the new API.

Possibly related PRs

• Add Opt.ifPresentOrElse() #7384: Adds the same Opt.ifPresentOrElse method and corresponding OptTest tests, indicating a direct code-level match.

Pre-merge checks and finishing touches

✅ Passed checks (1 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fd25d28 and dbe3aa2.

📒 Files selected for processing (1)

• docs/CHANGELOG.md (1 hunks)

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review

Thank you for implementing the ifPresentOrElse method! This is a solid addition that resolves the TODO and maintains consistency with java.util.Optional. Here's my detailed review:

✅ Strengths

1. API Consistency: The implementation correctly mirrors java.util.Optional.ifPresentOrElse() with appropriate type signatures and behavior.

2. Type Annotations: The nullness annotations are properly applied:

   • Consumer<@NonNull ? super @NonNull T> correctly indicates the consumer receives a non-null value
   • Parameter ordering matches the Optional API

3. Documentation: The Javadoc is clear, concise, and includes the @see reference to the corresponding Optional method, following the pattern established in other methods.

4. Test Coverage: The test cases cover key scenarios:

   • m3(): Tests the same refinement limitation as ifPresent (correctly requires @SuppressWarnings)
   • m4(): Tests correct usage with the refined parameter
   • m5(): Tests the else branch execution

5. Changelog: Appropriately documented in CHANGELOG.md

🔍 Observations & Minor Suggestions

1. Parameter Naming Consistency

The parameter name action differs from ifPresent which uses consumer. While both are semantically correct:

• Current: action matches java.util.Optional.ifPresentOrElse
• Alternative: consumer would match the existing ifPresent method

Recommendation: Keep action as it prioritizes consistency with the Java standard library over internal consistency, which seems to be the right trade-off.

2. Missing @param Tag

The Javadoc includes parameter descriptions in the summary but could benefit from explicit @param tags for IDE integration, though I note this matches the style of the existing ifPresent method (lines 65-74), so this is consistent with the codebase style.

3. Test Coverage Considerations

The tests verify the type checker behavior but don't include runtime assertion tests. Consider whether runtime behavior tests would be valuable (e.g., verifying that exactly one branch executes), though this may be outside the scope of this repository's test philosophy.

Test suggestion (optional):

void m6(@Nullable String o) {
    int[] count = {0};
    Opt.ifPresentOrElse(o, s -> count[0]++, () -> count[0]--);
    // Could add assertion that count[0] is either 1 or -1
}

4. Edge Case: Null emptyAction

The implementation doesn't explicitly guard against a null emptyAction parameter. While this would be a caller error, java.util.Optional also throws NullPointerException in this case, so the current behavior is consistent.

🔒 Security & Performance

• Security: No concerns. The method performs simple conditional execution with no sensitive operations.
• Performance: Optimal. The implementation is as efficient as possible with minimal overhead.

📋 Code Quality Summary

• Code Style: ✅ Follows project conventions
• Implementation: ✅ Correct and efficient
• Testing: ✅ Adequate for type checking validation
• Documentation: ✅ Clear and complete

Final Verdict

Approved ✅

This is a well-implemented feature that successfully resolves the TODO comment. The code is clean, properly tested for the nullness checker, and maintains consistency with both the existing Opt API and the Java standard library's Optional class.

The implementation is ready to merge as-is. The minor observations above are suggestions for consideration but not blockers.