Possible bug in Optional.empty() check with exception handling

[noobgramming]

Hello! I'm trying out CheckOptionalEmptiness and I think I found a bug.

It reports Optional after exception handling as possibly empty, even if all code paths in try{} throw or return. And it does so even when you pre-initialize to Optional.empty()

This code example isn't right, see updated one a few comments below

    Optional<Object> optObject = Optional.empty(); //<-- thought initializing to empty would fix lint, it doesn't
    try {
      optObject = doAThingThatMightThrow(optObject); // <-- the object this returns isn't nullable either
    } catch (Exception e) {
      if (configuration.debugMode()) {
        throw e; // <-- this path always throws
      } else {
        log.error(
            "Not debug mode, logging error and continuing on",
            e);
        return new ResponseObject(); <-- and the other path here always returns
      }
    }
    if (optCommand.isEmpty()) {
       // do stuff
    } else {
      final var object = optObject.get(); // <-- lint reports error here [NullAway] Invoking get() on possibly empty Optional optObject

[msridhar]

Hi @noobgramming, for this line:

if (optCommand.isEmpty()) {

Should that be optObject rather than optCommand? I don't see optCommand declared anywhere.

[lazaroclapp]

Hey @msridhar, waiting for confirmation from @noobgramming , but I think the if-check is indeed on an unrelated variable, and the logic for why optObject is non-empty can be simplified to:

Optional<Object> optObject = Optional.empty();
try {
  optObject = doAThingThatMightThrow(optObject);
} catch(Exception e) {
  return; // Unconditional return
}
final var object = optObject.get();

In that case, I do see a discrepancy between our handling of null and our handling of empty().

See the following test cases:

  @Test
  public void testTrySanity() {
    makeTestHelperWithArgs(
            Arrays.asList(
                    "-d",
                    temporaryFolder.getRoot().getAbsolutePath(),
                    "-XepOpt:NullAway:AnnotatedPackages=com.uber",
                    "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
                    "-XepOpt:NullAway:CheckOptionalEmptiness=true"))
            .addSourceLines(
                    "Test.java",
                    "package com.uber;",
                    "import java.util.Optional;",
                    "class Test {",
                    "   Object test(){",
                    "     Optional<Object> o = Optional.empty();",
                    "     try {",
                    "       o = Optional.of(\"something\");",
                    "     } catch (Exception e) {",
                    "       return \"error\";",
                    "     }",
                    // Unexpected error here: [NullAway] Invoking get() on possibly empty Optional o
                    "     return o.get();",
                    "   }",
                    "}")
            .doTest();
  }

  // Passes, as expected.
  @Test
  public void testTrySanity2() {
    defaultCompilationHelper
            .addSourceLines(
                    "Test.java",
                    "package com.uber;",
                    "import javax.annotation.Nullable;",
                    "class Test {",
                    "   Object test(){",
                    "     Object o = null;",
                    "     try {",
                    "       o = \"something\";",
                    "     } catch (Exception e) {",
                    "       return \"error\";",
                    "     }",
                    "     return o;",
                    "   }",
                    "}")
            .doTest();
  }

No idea what causes the difference.

[msridhar]

@lazaroclapp that's really interesting. I have no idea why there is an error reported for the Optional version of your test. It will take some digging into the dataflow analysis to figure it out. This is a nice catch!

[noobgramming]

Hi @noobgramming, for this line:

if (optCommand.isEmpty()) {

Should that be optObject rather than optCommand? I don't see optCommand declared anywhere.

Yes sorry. This was extracted from much larger, more confusing code :) I tried to simplify

Updated version below still triggers the behavior. I extracted the code again, verifying it works this time

  public void nullCheck(Configuration configuration) {
    Optional<Object> optObject;
    try {
      optObject = doThingThatMightThrow(); // always returns Optional.empty()
    } catch (Exception e) {
      if (configuration.debugMode()) {
        throw e;
      } else {
        log.error(
            "error",
            e);
        return;
      }
    }
    if (optObject.isEmpty()) {
      log.info("info ");
      return;
    } else {
      final var obj = optObject.get(); // <-- linter warning here [NullAway] Invoking get() on possibly empty Optional optObject
    }
  }

However, @lazaroclapp your simplification may still be correct because I'm unsure which part (the exception handling or empty check) is causing NullAway to act weird. I included the exception handling because I found it unlikely that such a straightforward empty check was causing this

EDIT: Also, this is on Java 17 in case that matters

[msridhar]

Thanks, @noobgramming. This definitely looks like a bug. We will look as soon as we get some time

[msridhar]

I finally got some time to dig a bit into this. I found that the same kind of false positive occurs with this test:

  @Test
  public void returnFromCatch() {
    defaultCompilationHelper
        .addSourceLines(
            "Test.java",
            "package com.uber;",
            "import javax.annotation.Nullable;",
            "class Test {",
            "  @Nullable Object f;",
            "  String foo() {",
            "    Test t = new Test();",
            "    try {",
            "      t.f = new Object();",
            "    } catch (Exception e) {",
            "      return \"error\";",
            "    }",
                 // false positive warning for this line
            "    return t.f.toString();",
            "  }",
            "}")
        .doTest();
  }

The issue turns out to have nothing to do with exceptions. The issue is that we will not track an access path for a field if the root is not this:

NullAway/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java

Lines 531 to 540 in f3c1ebf

	// we don't allow arbitrary access paths to be tracked from assignments
	// here we still require an access of a field of this, or a static field
	FieldAccessNode fieldAccessNode = (FieldAccessNode) target;
	Node receiver = fieldAccessNode.getReceiver();
	setNonnullIfAnalyzeable(updates, receiver);
	if ((receiver instanceof ThisNode || fieldAccessNode.isStatic())
	&& fieldAccessNode.getElement().getKind().equals(ElementKind.FIELD)
	&& !castToNonNull(ASTHelpers.getType(target.getTree())).isPrimitive()) {
	updates.set(fieldAccessNode, value);
	}

I presume we did this for performance reasons way back when (this code has not changed since the initial open-source commit of NullAway). This impacts handling of the optional case as well, since we simulate the contents of an Optional object as a pseudo-field.

The downside of this optimization is confusing tool behavior, like in the case originally reported here. I'd be open to trying to track more access paths as part of analysis, as long as we measure the performance impact, both on our micro-benchmarks and also on internal code at Uber. E.g., we could track an access path after an assignment to a field of any local variable, not just this. I would not expect too much of a blowup from that.

@lazaroclapp any thoughts here? I may hold off on this change until after the next release, since at that point we expect to add NullAway itself as another performance benchmark.

[msridhar]

🤦‍♂️ Ignore my previous comment. I figured out something much simpler is going on here; we don't have any support for the Optional.isEmpty() method! It was added in Java 11 but our handler doesn't know about it. This should be much easiesr to fix 🙂