Skip to content

Conversation

jlaur
Copy link

@jlaur jlaur commented Aug 12, 2025

This adds a check to generate a warning when Optional is used as a field type.

Optional is primarily meant as a return type, not a field type. See for example Brian Goetz, Java’s language architect, shedding light on Oracle’s intention with the Optional type.

Example code which did not generate any warnings, but hides a potential NullPointerException in a concurrency scenario:

private Optional<String> foo = Optional.empty();

private void test()
{
    if (foo.isPresent()) {
        String bar = foo.get();
    }
}

Whereas this will generate a warning:

private @Nullable String foo;

private void test()
{
    if (foo != null) {
        String bar = foo;
    }
}

and this will not:

private @Nullable String foo;

private void test()
{
    String foo = this.foo;
    if (foo != null) {
        String bar = foo;
    }
}

This is a follow-up to the suggestion made here: openhab/openhab-addons#19061 (comment)

A few @openhab/add-ons-maintainers already favors the idea, but it would be good to hear from @openhab/core-maintainers as well in case anyone thinks this approach is too drastic or are directly against it.

It should be mentioned that some add-ons already use Optional as a field type and thus will start having these warnings. I intend to create at least a few pull requests to address some of those.

Personal test notes:

$env:JAVA_HOME="C:\Program Files\Java\jdk-11.0.13"
mvn clean install

Then to use the check when building an add-on:

mvn clean install -D sat.version=0.18.0-SNAPSHOT

@jlaur jlaur requested a review from a team as a code owner August 12, 2025 19:39
@wborn
Copy link
Member

wborn commented Aug 12, 2025

It's also documented in the Optional JavaDoc:

Optional is primarily intended for use as a method return type where there is a clear need to represent "no result," and where using null is likely to cause errors. A variable whose type is Optional should never itself be null; it should always point to an Optional instance.

In IntelliJ IDEA there are also checks that show warnings when using Optional not as intended. E.g. when using it in a method parameter or in a serializable class, see:

https://www.jetbrains.com/help/inspectopedia/OptionalUsedAsFieldOrParameterType.html

@wborn wborn requested a review from Copilot August 13, 2025 07:34
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a custom Checkstyle check to warn when Optional is used as a field type, addressing Oracle's intention that Optional should primarily be used as a return type. The check enforces best practices by detecting Optional fields through various import patterns and generates warnings to encourage the use of @Nullable annotations instead.

Key changes:

  • New OptionalFieldCheck that detects Optional usage in fields across different import styles
  • Comprehensive test coverage for various Optional usage scenarios
  • Integration of the check into the Checkstyle ruleset configuration

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

File Description
rules.xml Adds the new OptionalFieldCheck module to the Checkstyle configuration
OptionalFieldCheck.java Core implementation that detects Optional field usage through AST parsing
OptionalFieldCheckTest.java Unit tests covering different Optional import and usage patterns
Test resource files Various test cases for different Optional usage scenarios

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@jlaur
Copy link
Author

jlaur commented Sep 21, 2025

@openhab/sat-maintainers - is this just waiting for review capacity, or are you waiting for me to do something? 🙂

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