uber / uber/NullAway

CheckOptionalEmptiness disregards initial value, fallback value

Open
#889 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

help wanted
Dominant language
Java
Stars
4.1k
Forks
370
Avg merge
1d 13h
Merged PRs (30d)
68

Description

From the history of CheckOptionalEmptiness I'm not entirely certain which exact issues it is intended to protect against but I ran into a surprise with j.u.Optional::or that led me to map out a number of false positive warnings. These seem to revolve around

  1. the initial value passed to an Optional factory is ignored
  2. fallback providers are not recognized

Both cases fail with [NullAway] Invoking get() on possibly empty Optional o

Some of these look like #557 but I don't see other related issues.

All of my cases for posterity:

Object value;
Optional<Object> o;
                                                              
o = Optional.empty();
                                                              
if (o.isPresent()) {
    value = o.get(); // true negative
} else {
    value = o.get(); // true positive
}
                                                              
if (o.isEmpty()) {
    value = o.get(); // true positive
} else {
    value = o.get(); // true negative
}
                                                              
o = Optional.of(new Object());
value = o.get(); // false positive
                                                              
o = Optional.ofNullable(new Object()); // huh, no ErrorProne lint!
value = o.get(); // false positive
                                                              
try {
    o = Optional.of(new Object());
} catch (Exception e) {
    throw new RuntimeException();
}
value = o.get(); // false positive
                                                              
try {
    o = Optional.of(new Object());
    if (o.isEmpty()) {
        throw new RuntimeException();
    }
    value = o.get(); // true negative
} catch (Exception e) { }
                                                              
o = o.isPresent() ? o : Optional.of(new Object());
value = o.get(); // false positive
                                                              
o = o.isEmpty() ? Optional.of(new Object()) : o;
value = o.get(); // false positive
                                                              
o = o.or(() -> Optional.of(new Object()));
value = o.get(); // false positive
                                                              
value = o.orElse(new Object()); // true negative
                                                              
value = o.orElseGet(() -> new Object()); // true negative
value = o.orElseGet(Object::new); // true negative
                                                              
value = o.orElseThrow(); // true negative
value = o.orElseThrow(RuntimeException::new); // true negative

using JDK 17 and

      <build>
        <plugins>
          <plugin>
            <artifactId>maven-compiler-plugin</artifactId>
            <configuration>
              <fork>true</fork>
              <annotationProcessorPaths>
                <path>
                  <groupId>com.google.errorprone</groupId>
                  <artifactId>error_prone_core</artifactId>
                  <version>2.24.1</version>
                </path>
                <path>
                  <groupId>com.uber.nullaway</groupId>
                  <artifactId>nullaway</artifactId>
                  <version>0.10.19</version>
                </path>
              </annotationProcessorPaths>
              <compilerArgs>
                <arg>-XDcompilePolicy=simple</arg>
                <arg>
                  -Xplugin:ErrorProne \
                  -Xep:NullAway:ERROR \
                  -XepOpt:NullAway:AnnotatedPackages=foo \
                  -XepOpt:NullAway:CheckOptionalEmptiness=true \
                </arg>
                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED</arg>
                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</arg>
                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED</arg>
                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>
              </compilerArgs>
            </configuration>
          </plugin>
        </plugins>
      </build>

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by locating the CheckOptionalEmptiness implementation and its existing Optional analysis tests. Reproduce the Java 17 cases for Optional.of, conditional assignments, and Optional.or, then add coverage for the reported false positives while preserving the existing true-positive checks. Done means valid non-empty flows no longer produce warnings and genuinely unsafe get() calls still do.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.