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

Proposal: Understand Optional isPresent/isEmpty/isAbsent #274

Closed
ZacSweers opened this issue Dec 17, 2018 · 3 comments
Closed

Proposal: Understand Optional isPresent/isEmpty/isAbsent #274

ZacSweers opened this issue Dec 17, 2018 · 3 comments
Assignees

Comments

@ZacSweers
Copy link
Contributor

Right now NullAway can understand cases like AutoValue where

if (a.foo() != null) {
  a.foo().bar();
}

I propose adding support for an Optional analogue:

if (fooOptional.isPresent()) {
  fooOptional.get().bar(); // Implicitly understood to be safe
}

I think this is a good fit for NullAway since Optional is Java 8/Guava's alternative to null in common cases, and has precedent for this kind of data flow awareness of "was null-checked already". This would just be a different flavor.

@msridhar
Copy link
Collaborator

I like this idea, and agree it's not far from what NullAway already does. Basically we'd want to model the presence state of Optionals and report errors on bad get() calls. But we could pretty much model presence as nullability of the "contents" of the Optional.

@lazaroclapp what do you think? I'm not sure if this would work best in a Handler or just as a core part of NullAway.

Of course implementation would take some work so we'd need to figure out how that would get done 😄

@lazaroclapp lazaroclapp self-assigned this Dec 18, 2018
@lazaroclapp
Copy link
Collaborator

lazaroclapp commented Dec 18, 2018

@hzsweers @msridhar Definitely fits with the model, not sure how it will fit with the code, thought. My impression is that the change would be a bit more involved than what what can currently be done with a Handler, but I am not sure. Basically, a duplicate of the entire Nullness dataflow propagation, but for a new Emptyness type system?

Tentatively assigning to self so I can take a look when I am back in Jan, but feel free to take a look at how hard this would be if you are interested in it and have enough time :)

@msridhar
Copy link
Collaborator

msridhar commented Dec 18, 2018

At least in JDK 8 the java.util.Optional type has a value field, so we could implement with a handler that updates the nullability of value (similar to the existing Apache Thrift handler). The main difference is that a get() call should be treated like a "de-reference" of value, so an error may be reported at such call sites. I think we don't let Handlers report new errors, but otherwise it may not be too far from what we can already do? Haven't thought through it fully.

lazaroclapp pushed a commit that referenced this issue Mar 6, 2019
Added Optional Emptiness handler which does couple of things
1. on `optionalFoo.get()` hints that it can be Nullable. 
2. on `optionalFoo.isPresent()` sets the `optionalFoo.get()` access path to Non-null.  

This allows 
```
if (fooOptional.isPresent()) {
  fooOptional.get().bar(); // Implicitly understood to be safe
}
```
and returns a warning if not implied safe. 

Fixes #274 
Added unit tests
Handler is gated by a configuration flag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants