Use @Present annotation on the method that is assuredly returning a non-empty optional

[shubhamugare]

Use @Present annotation on the return of the method that is assuredly returning a non-empty optional. Following test should pass.

  @Test
  public void OptionalEmptinessPresentTest() {
    compilationHelper
        .setArgs(
            Arrays.asList(
                "-d",
                temporaryFolder.getRoot().getAbsolutePath(),
                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
                "-XepOpt:NullAway:CheckOptionalEmptiness=true"))
        .addSourceLines(
            "TestNegative.java",
            "package com.uber;",
            "import com.google.common.base.Optional;",
            "rg.checkerframework.checker.optional.qual.Present",
            "import com.google.common.base.Function;",
            "public class TestNegative {",
            "  class ABC { ",
            "       @present Optional<Object> getTheOptional(int x){",
            "           if(x != 0){",
            "             return Optional.of(new Object());",
            "           }",
            "       return Optional.of(new Object());",
            "       }",
            "  }",
            "  void foo() {",
            "      ABC abc = new ABC();",
            "      // no error since @present annotation on the invoked method",
            "       abc.getTheOptional(5).get().toString();",
            "      }",
            "    }",
            "}")
        .doTest();
  }

[msridhar]

Did you see this used in some code? Seems reasonable to support, just wondering about motivation.

[shubhamugare]

Yeah, when I turned on the flag on Uber's code, there were few some occurrences. The methods are not annotated. Though it will be much better if we could support even when the get call is not direct. Like,

   ABC abc = new ABC();",
   Optional<Object> z = abc.getTheOptional(5);
  
// no error since @present annotation on the invoked method
  z.get().toString();

This general case seems harder to support compared to one in the issue. I am not sure how many of the examples I found in the Uber code were of this type or the other type, but in the second case it is easier to add redundant isPresent check.

[msridhar]

Yeah for the general case we'd need to implement full optional type system like Checker Framework's system. Not worth it for now. As an alternative to a redundant isPresent, we could add a castToPresent call that we handle specially, just like castToNonNull.

[reitzmichnicht]

Isn't it bad api design if it returns an optional that is always present?

[lazaroclapp]

@reitzmichnicht Off the top of my head, I can think of a case where this would be unavoidable: an overriding method that wants to return T but implements an interface that returns Optional<T>. The overriding method could mark itself as returning @Present Optional<T> to indicate it returns T, while remaining compatible with the interface when used as an object of the type of said interface (in which case the presence information is lost).

Also, API evolution, where the method used to have cases in which it returned an empty optional, but those cases don't exist anymore, and the Optional<T> return is part of the public API and thus can't immediately be changed (ideally, you'd then have @Deprecated @Present Optional<T> foo(...) and T newFoo(...) or something, but that's not always trivial).

Finally, sometimes bad API design exists outside your control and tooling has to work around it 😉

That said, I feel this feature is meaningful but fairly low priority overall. castToPresent(...) is a reasonable workaround for "I know that under this conditions, this Optional will never be empty".