Custom path to Optional class for Optional emptiness handler #288

shubhamugare · 2019-03-14T08:04:41Z

Allowing custom path to the Optional class with -XepOpt:NullAway: CheckOptionalEmptinessCustomClasses = flag.
This custom path is used by optional emptiness handler.

No option at all passed in the command line => set is (java.utils.Optional)
Explicit option passed => the classes in that set + java.utils.Optional

Added unit test

lazaroclapp

See comments. Mostly: missing support for multiple optional types, and Config should be passed to hanlders at construction, not on every interposition point.

lazaroclapp · 2019-03-14T18:11:50Z

nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java

@@ -70,6 +70,8 @@

  protected boolean checkOptionalEmptiness;

+  protected String optionalClassPath;


Should probably be an ImmutableSet<String>, like with unannotatedClasses. It should also be @Nullable (for the case where checkOptionalEmptiness is false). We want to support the case of: a) No Optional classes passed, b) One Optional class passed (or taken from the default), c) N Optional classes passed. This field would only support (b)

lazaroclapp · 2019-03-14T18:15:21Z

nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java

+      ClassTree tree,
+      VisitorState state,
+      Symbol.ClassSymbol classSymbol,
+      Config config) {


Don't pass this here, pass it to the handler's constructor in Handlers.buildDefault(...), that's the convention used by existing handlers.

Before I go ahead and do a full pass, it looks to me like this remains to be addressed :)

Done now 😅

lazaroclapp · 2019-03-14T18:16:31Z

nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java

    if (optionalType == null) {
      optionalType =
-          Optional.ofNullable(state.getTypeFromString(OPTIONAL_PATH))
+          Optional.ofNullable(state.getTypeFromString(config.getOptionalClassPath()))


Must be generalized to deal with multiple Optional classes.

lazaroclapp · 2019-03-14T18:18:58Z

nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java

+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
+                "-XepOpt:NullAway:CheckOptionalEmptiness=true",
+                "-XepOpt:NullAway:OptionalClassPath=com.google.common.base.Optional"))


👍 Would suggest also:

Testing for "-XepOpt:NullAway:OptionalClassPath=\"\"" (I'd say this should just raise a config error if XepOpt:NullAway:CheckOptionalEmptiness is set)

Testing for "-XepOpt:NullAway:OptionalClassPath=java.utils.Optional,com.google.common.base.Optional"

Should we not have java.utils.Optional by default in the set?

The way I would do it, if possible:

No option at all passed in the command line => set is (java.utils.Optional)

Explicit option passed, empty or unparsable => error

Explicit option passed, parsable non-empty set => the classes in that set, and nothing else

This allows people using this handler for their own custom Optional without enabling it for java.utils.Optional.

Then again, not sure if it's more consistent with how we do other stuff to just have java.utils.Optional always on (when CheckOptionalEmptiness is set), then rename this option to something like CustomOptionalClasses. Maybe on second thought, I am liking this later way of doing it better.

@msridhar , thoughts ?

I think we should always check java.util.Optional and this option should add additional aliases for it.

Okay then. So first two cases as Lazaro described and and in case 3. Explicit option passed, parsable non-empty set => the classes in that set + java.utils.Optional.
That works @lazaroclapp?

I am not sure if we really need the unparsable check. Since we do not have it for other configs. Also I'm not sure how to define it exactly. So let me know if you think it should be necessary.
@msridhar @lazaroclapp

I think given what Manu is saying, then there is just two cases:

No option at all passed in the command line => set is (java.utils.Optional)

CustomOptionalClasses=a.b.c.A1;a.b.c.A2 => set is (java.utils.Optional,a.b.c.A1,a.b.c.A2)

Still worth to check that it works with zero (no CustomOptionalClasses option), one and two classes being passed.

Added test for zero, one and two custom paths case.

lazaroclapp · 2019-03-21T19:56:25Z

nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java

@@ -56,7 +56,7 @@
  static final String FL_ACKNOWLEDGE_RESTRICTIVE =
      EP_FL_NAMESPACE + ":AcknowledgeRestrictiveAnnotations";
  static final String FL_CHECK_OPTIONAL_EMPTINESS = EP_FL_NAMESPACE + ":CheckOptionalEmptiness";
-  static final String FL_OPTIONAL_CLASS_PACKAGE = EP_FL_NAMESPACE + ":OptionalClassPath";
+  static final String FL_OPTIONAL_CLASS_PATHS = EP_FL_NAMESPACE + ":OptionalClassPaths";


I still think this should be CustomOptionalClasses, otherwise I keep parsing it on my head as "Optional (class paths)" rather than "(Optional class) paths"

Not that my alternative is perfect, but... mmm... maybe CheckOptionalEmptinessCustomClasses? A bit verbose, but at least ties the two options together.

Changed it to CheckOptionalEmptinessCustomClasses, sounds better

lazaroclapp

Looking much better to me now, still one more test case I'd like to see and I am not sure why we are using Set<Optional<Type>> rather than ImmutableSet<Type>

lazaroclapp · 2019-03-21T21:21:21Z

nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java

@@ -49,9 +51,12 @@
 */
 public class OptionalEmptinessHandler extends BaseNoOpHandler {

-  private static String OPTIONAL_PATH = "java.util.Optional";
+  @Nullable private Set<Optional<Type>> optionalTypes;


Why is this a Set<Optional<Type>> rather than a Set<Type>? We could just filter out any type paths for which state.getTypeFromString(type) returns null.

Also, it probably should be an ImmutableSet, since it won't change after the call to onMatchTopLevelClass

lazaroclapp · 2019-03-21T21:24:14Z

nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java

+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
+                "-XepOpt:NullAway:CheckOptionalEmptiness=true",
+                "-XepOpt:NullAway:CheckOptionalEmptinessCustomClasses=com.google.common.base.Optional"))


Please add one more (smaller) test, that checks that java.util.Optional is still recognized when this argument is set, as per our discussion. Tests are a good way to document that sort of design edge-case, after all.

Done already 👍

(True, I missed this one, my bad 😅 )

lazaroclapp · 2019-03-21T22:16:54Z

nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java

+                .map(state::getTypeFromString)
+                .filter(Objects::nonNull)
+                .map(state.getTypes()::erasure)
+                .collect(Collectors.toSet()));


Guava 22 has collectors: https://google.github.io/guava/releases/22.0/api/docs/com/google/common/collect/ImmutableSet.html#toImmutableSet--

So no need to build a Java set and call copyOf(...)

Thanks! Done 👍

lazaroclapp

Extremely minor nit, but overall it LGTM :)

Custom path to Optional class for Optional emptiness handler

c366ede

lazaroclapp requested changes Mar 14, 2019

View reviewed changes

Addressed comments

ba417ed

lazaroclapp reviewed Mar 21, 2019

View reviewed changes

Addressed comments

89a416b

lazaroclapp requested changes Mar 21, 2019

View reviewed changes

Addressed comments

ebb6870

lazaroclapp reviewed Mar 21, 2019

View reviewed changes

lazaroclapp approved these changes Mar 21, 2019

View reviewed changes

minor nit

eddf6ae

lazaroclapp merged commit 31a1842 into uber:master Mar 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom path to Optional class for Optional emptiness handler #288

Custom path to Optional class for Optional emptiness handler #288

shubhamugare commented Mar 14, 2019 •

edited

lazaroclapp left a comment

lazaroclapp Mar 14, 2019

shubhamugare Mar 21, 2019

lazaroclapp Mar 14, 2019

shubhamugare Mar 18, 2019

lazaroclapp Mar 21, 2019

shubhamugare Mar 21, 2019

lazaroclapp Mar 14, 2019

shubhamugare Mar 18, 2019

lazaroclapp Mar 14, 2019 •

edited

shubhamugare Mar 14, 2019

lazaroclapp Mar 14, 2019 •

edited

msridhar Mar 14, 2019

shubhamugare Mar 15, 2019 •

edited

shubhamugare Mar 18, 2019

lazaroclapp Mar 21, 2019

shubhamugare Mar 21, 2019

lazaroclapp Mar 21, 2019

lazaroclapp Mar 21, 2019

shubhamugare Mar 21, 2019

lazaroclapp left a comment

lazaroclapp Mar 21, 2019

shubhamugare Mar 21, 2019

lazaroclapp Mar 21, 2019

shubhamugare Mar 21, 2019

lazaroclapp Mar 21, 2019

lazaroclapp Mar 21, 2019

lazaroclapp Mar 21, 2019

shubhamugare Mar 22, 2019

lazaroclapp left a comment

		@@ -70,6 +70,8 @@

		protected boolean checkOptionalEmptiness;

		protected String optionalClassPath;

Custom path to Optional class for Optional emptiness handler #288

Custom path to Optional class for Optional emptiness handler #288

Conversation

shubhamugare commented Mar 14, 2019 • edited

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp Mar 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp Mar 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubhamugare Mar 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

shubhamugare commented Mar 14, 2019 •

edited

lazaroclapp Mar 14, 2019 •

edited

lazaroclapp Mar 14, 2019 •

edited

shubhamugare Mar 15, 2019 •

edited