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

CheckOptionalEmptiness disregards initial value, fallback value #889

Open
commonquail opened this issue Jan 4, 2024 · 1 comment
Open

Comments

@commonquail
Copy link

commonquail commented Jan 4, 2024

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>
@msridhar
Copy link
Collaborator

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants