NullAway icon indicating copy to clipboard operation
NullAway copied to clipboard

CheckOptionalEmptiness disregards initial value, fallback value

Open commonquail opened this issue 2 years ago • 1 comments

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>

commonquail avatar Jan 04 '24 18:01 commonquail

@commonquail thanks for the detailed report! My guess is for many of these cases we need to model more Optional APIs. (The or() case looks trickier.) We would welcome a PR for better support here.

msridhar avatar Jan 10 '24 05:01 msridhar