Skip to content

Commit

Permalink
Custom path to Optional class for Optional emptiness handler (#288)
Browse files Browse the repository at this point in the history 
Allows a passing custom Optional classes to the emptiness handler, using the
 `-XepOpt:NullAway:CheckOptionalEmptinessCustomClasses =` flag.

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`
  • Loading branch information
@shubhamugare @lazaroclapp
shubhamugare authored and lazaroclapp committed Mar 22, 2019
1 parent 0b9cdef commit 31a1842
Show file tree
Hide file tree
Showing 8 changed files with 217 additions and 23 deletions.
7 changes: 7 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java
View file
Expand Up @@ -70,6 +70,8 @@ public abstract class AbstractConfig implements Config {

protected boolean checkOptionalEmptiness;

protected Set<String> optionalClassPaths;

protected boolean assertsEnabled;

/**
Expand Down Expand Up @@ -189,6 +191,11 @@ public boolean checkOptionalEmptiness() {
return checkOptionalEmptiness;
}

@Override
public Set<String> getOptionalClassPaths() {
return optionalClassPaths;
}

@Override
public boolean assertsEnabled() {
return assertsEnabled;
Expand Down
7 changes: 7 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/Config.java
View file
Expand Up @@ -24,6 +24,7 @@

import com.google.common.collect.ImmutableSet;
import com.sun.tools.javac.code.Symbol;
import java.util.Set;
import javax.annotation.Nullable;

/** Provides configuration parameters for the nullability checker. */
Expand Down Expand Up @@ -115,6 +116,12 @@ public interface Config {
*/
boolean checkOptionalEmptiness();

/**
* @return the paths for Optional class. The list always contains the path of {@link
* java.util.Optional}.
*/
Set<String> getOptionalClassPaths();

/**
* @return the fully qualified name of a method which will take a @Nullable version of a value and
* return an @NonNull copy (likely through an unsafe downcast, but performing runtime checking
Expand Down
6 changes: 6 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java
View file
Expand Up @@ -27,6 +27,7 @@

import com.google.common.collect.ImmutableSet;
import com.sun.tools.javac.code.Symbol;
import java.util.Set;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -117,6 +118,11 @@ public boolean checkOptionalEmptiness() {
throw new IllegalStateException(error_msg);
}

@Override
public Set<String> getOptionalClassPaths() {
throw new IllegalStateException(error_msg);
}

@Override
@Nullable
public String getCastToNonNullMethod() {
Expand Down
7 changes: 7 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java
View file
Expand Up @@ -56,6 +56,8 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
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_PATHS =
EP_FL_NAMESPACE + ":CheckOptionalEmptinessCustomClasses";
static final String FL_SUPPRESS_COMMENT = EP_FL_NAMESPACE + ":AutoFixSuppressionComment";
/** --- JarInfer configs --- */
static final String FL_JI_ENABLED = EP_FL_NAMESPACE + ":JarInferEnabled";
Expand Down Expand Up @@ -141,6 +143,11 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
getFlagStringSet(flags, FL_EXCLUDED_FIELD_ANNOT, DEFAULT_EXCLUDED_FIELD_ANNOT));
castToNonNullMethod = flags.get(FL_CTNN_METHOD).orElse(null);
autofixSuppressionComment = flags.get(FL_SUPPRESS_COMMENT).orElse("");
optionalClassPaths =
new ImmutableSet.Builder<String>()
.addAll(getFlagStringSet(flags, FL_OPTIONAL_CLASS_PATHS))
.add("java.util.Optional")
.build();
if (autofixSuppressionComment.contains("\n")) {
throw new IllegalStateException(
"Invalid -XepOpt" + FL_SUPPRESS_COMMENT + " value. Comment must be single line.");
Expand Down
2 changes: 1 addition & 1 deletion nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java
View file
Expand Up @@ -51,7 +51,7 @@ public static Handler buildDefault(Config config) {
handlerListBuilder.add(new ContractHandler());
handlerListBuilder.add(new ApacheThriftIsSetHandler());
if (config.checkOptionalEmptiness()) {
handlerListBuilder.add(new OptionalEmptinessHandler());
handlerListBuilder.add(new OptionalEmptinessHandler(config));
}
return new CompositeHandler(handlerListBuilder.build());
}
Expand Down
49 changes: 28 additions & 21 deletions nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java
View file
Expand Up @@ -21,7 +21,7 @@
*/
package com.uber.nullaway.handlers;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import com.google.errorprone.VisitorState;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ClassTree;
Expand All @@ -31,12 +31,13 @@
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Types;
import com.sun.tools.javac.util.Context;
import com.uber.nullaway.Config;
import com.uber.nullaway.ErrorMessage;
import com.uber.nullaway.NullAway;
import com.uber.nullaway.Nullness;
import com.uber.nullaway.dataflow.AccessPath;
import com.uber.nullaway.dataflow.AccessPathNullnessPropagation;
import java.util.Optional;
import java.util.Objects;
import javax.annotation.Nullable;
import javax.lang.model.element.Element;
import javax.lang.model.element.ElementKind;
Expand All @@ -49,9 +50,12 @@
*/
public class OptionalEmptinessHandler extends BaseNoOpHandler {

private static String OPTIONAL_PATH = "java.util.Optional";
@Nullable private ImmutableSet<Type> optionalTypes;
private final Config config;

@Nullable private Optional<Type> optionalType;
OptionalEmptinessHandler(Config config) {
this.config = config;
}

@Override
public boolean onOverrideMayBeNullExpr(
Expand All @@ -66,11 +70,14 @@ && optionalIsGetCall((Symbol.MethodSymbol) ASTHelpers.getSymbol(expr), state.get
@Override
public void onMatchTopLevelClass(
NullAway analysis, ClassTree tree, VisitorState state, Symbol.ClassSymbol classSymbol) {
if (optionalType == null) {
optionalType =
Optional.ofNullable(state.getTypeFromString(OPTIONAL_PATH))
.map(state.getTypes()::erasure);
}
optionalTypes =
config
.getOptionalClassPaths()
.stream()
.map(state::getTypeFromString)
.filter(Objects::nonNull)
.map(state.getTypes()::erasure)
.collect(ImmutableSet.toImmutableSet());
}

@Override
Expand Down Expand Up @@ -143,20 +150,20 @@ private void updateNonNullAPsForElement(
}

private boolean optionalIsPresentCall(Symbol.MethodSymbol symbol, Types types) {
Preconditions.checkNotNull(optionalType);
// noinspection ConstantConditions
return optionalType.isPresent()
&& symbol.getSimpleName().toString().equals("isPresent")
&& symbol.getParameters().length() == 0
&& types.isSubtype(symbol.owner.type, optionalType.get());
for (Type optionalType : optionalTypes) {
if (symbol.getSimpleName().toString().equals("isPresent")
&& symbol.getParameters().length() == 0
&& types.isSubtype(symbol.owner.type, optionalType)) return true;
}
return false;
}

private boolean optionalIsGetCall(Symbol.MethodSymbol symbol, Types types) {
Preconditions.checkNotNull(optionalType);
// noinspection ConstantConditions
return optionalType.isPresent()
&& symbol.getSimpleName().toString().equals("get")
&& symbol.getParameters().length() == 0
&& types.isSubtype(symbol.owner.type, optionalType.get());
for (Type optionalType : optionalTypes) {
if (symbol.getSimpleName().toString().equals("get")
&& symbol.getParameters().length() == 0
&& types.isSubtype(symbol.owner.type, optionalType)) return true;
}
return false;
}
}
2 changes: 1 addition & 1 deletion nullaway/src/main/java/com/uber/nullaway/handlers/RxNullabilityPropagator.java
View file
Expand Up @@ -204,7 +204,7 @@ class RxNullabilityPropagator extends BaseNoOpHandler {
private final Map<ReturnTree, Tree> returnToEnclosingMethodOrLambda =
new LinkedHashMap<ReturnTree, Tree>();

// Similar to above, but mapping espression-bodies to their enclosing lambdas
// Similar to above, but mapping expression-bodies to their enclosing lambdas
private final Map<ExpressionTree, LambdaExpressionTree> expressionBodyToFilterLambda =
new LinkedHashMap<ExpressionTree, LambdaExpressionTree>();

Expand Down
160 changes: 160 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java
View file
Expand Up @@ -1232,6 +1232,166 @@ public void OptionalEmptinessHandlerTest() {
.doTest();
}

@Test
public void OptionalEmptinessHandlerWithSingleCustomPathTest() {
compilationHelper
.setArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
"-XepOpt:NullAway:CheckOptionalEmptiness=true",
"-XepOpt:NullAway:CheckOptionalEmptinessCustomClasses=com.google.common.base.Optional"))
.addSourceLines(
"TestNegative.java",
"package com.uber;",
"import com.google.common.base.Optional;",
"import javax.annotation.Nullable;",
"import com.google.common.base.Function;",
"public class TestNegative {",
" void foo() {",
" Optional<Object> a = Optional.absent();",
" // no error since a.isPresent() is called",
" if(a.isPresent()){",
" a.get().toString();",
" }",
" }",
" public void lambdaConsumer(Function a){",
" return;",
" }",
" void bar() {",
" Optional<Object> b = Optional.absent();",
" if(b.isPresent()){",
" lambdaConsumer(v -> b.get().toString());",
" }",
" }",
"}")
.addSourceLines(
"TestPositive.java",
"package com.uber;",
"import java.util.Optional;",
"import javax.annotation.Nullable;",
"import com.google.common.base.Function;",
"public class TestPositive {",
" void foo() {",
" Optional<Object> a = Optional.empty();",
" // BUG: Diagnostic contains: Optional a can be empty",
" a.get().toString();",
" }",
" public void lambdaConsumer(Function a){",
" return;",
" }",
" void bar() {",
" Optional<Object> b = Optional.empty();",
" // BUG: Diagnostic contains: Optional b can be empty",
" lambdaConsumer(v -> b.get().toString());",
" }",
"}")
.addSourceLines(
"TestPositive2.java",
"package com.uber;",
"import com.google.common.base.Optional;",
"import javax.annotation.Nullable;",
"import com.google.common.base.Function;",
"public class TestPositive2 {",
" void foo() {",
" Optional<Object> a = Optional.absent();",
" // BUG: Diagnostic contains: Optional a can be empty",
" a.get().toString();",
" }",
" public void lambdaConsumer(Function a){",
" return;",
" }",
" void bar() {",
" Optional<Object> b = Optional.absent();",
" // BUG: Diagnostic contains: Optional b can be empty",
" lambdaConsumer(v -> b.get().toString());",
" }",
"}")
.doTest();
}

@Test
public void OptionalEmptinessHandlerWithTwoCustomPathsTest() {
compilationHelper
.setArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
"-XepOpt:NullAway:CheckOptionalEmptiness=true",
"-XepOpt:NullAway:CheckOptionalEmptinessCustomClasses=does.not.matter.Optional,com.google.common.base.Optional"))
.addSourceLines(
"TestNegative.java",
"package com.uber;",
"import com.google.common.base.Optional;",
"import javax.annotation.Nullable;",
"import com.google.common.base.Function;",
"public class TestNegative {",
" void foo() {",
" Optional<Object> a = Optional.absent();",
" // no error since a.isPresent() is called",
" if(a.isPresent()){",
" a.get().toString();",
" }",
" }",
" public void lambdaConsumer(Function a){",
" return;",
" }",
" void bar() {",
" Optional<Object> b = Optional.absent();",
" if(b.isPresent()){",
" lambdaConsumer(v -> b.get().toString());",
" }",
" }",
"}")
.addSourceLines(
"TestPositive.java",
"package com.uber;",
"import java.util.Optional;",
"import javax.annotation.Nullable;",
"import com.google.common.base.Function;",
"public class TestPositive {",
" void foo() {",
" Optional<Object> a = Optional.empty();",
" // BUG: Diagnostic contains: Optional a can be empty",
" a.get().toString();",
" }",
" public void lambdaConsumer(Function a){",
" return;",
" }",
" void bar() {",
" Optional<Object> b = Optional.empty();",
" // BUG: Diagnostic contains: Optional b can be empty",
" lambdaConsumer(v -> b.get().toString());",
" }",
"}")
.addSourceLines(
"TestPositive2.java",
"package com.uber;",
"import com.google.common.base.Optional;",
"import javax.annotation.Nullable;",
"import com.google.common.base.Function;",
"public class TestPositive2 {",
" void foo() {",
" Optional<Object> a = Optional.absent();",
" // BUG: Diagnostic contains: Optional a can be empty",
" a.get().toString();",
" }",
" public void lambdaConsumer(Function a){",
" return;",
" }",
" void bar() {",
" Optional<Object> b = Optional.absent();",
" // BUG: Diagnostic contains: Optional b can be empty",
" lambdaConsumer(v -> b.get().toString());",
" }",
"}")
.doTest();
}

@Test
public void OptionalEmptinessUncheckedTest() {
compilationHelper
Expand Down

0 comments on commit 31a1842

Please sign in to comment.