Skip to content

Commit

Permalink
Merge 7821282 into 16923e5
Browse files Browse the repository at this point in the history
  • Loading branch information
@lazaroclapp
lazaroclapp committed Feb 27, 2023
2 parents 16923e5 + 7821282 commit 88fe6a7
Show file tree
Hide file tree
Showing 8 changed files with 209 additions and 6 deletions.
7 changes: 7 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ public abstract class AbstractConfig implements Config {

protected String autofixSuppressionComment;

protected ImmutableSet<String> skippedLibraryModels;

/** --- JarInfer configs --- */
protected boolean jarInferEnabled;

Expand Down Expand Up @@ -292,6 +294,11 @@ public boolean isContractAnnotation(String annotationName) {
return contractAnnotations.contains(annotationName);
}

@Override
public boolean isSkippedLibraryModel(String classDotMethod) {
return skippedLibraryModels.contains(classDotMethod);
}

@AutoValue
abstract static class MethodClassAndName {

Expand Down
14 changes: 14 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/Config.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,20 @@ public interface Config {
*/
String getAutofixSuppressionComment();

/**
* Checks if the given library model should be skipped/ignored.
*
* <p>For ease of configuration in the command line, this works at the level of the (class, method
* name) pair, meaning it applies for all methods with the same name in the same class, even if
* they have different signatures, and to all library models applicable to that method (i.e. on
* the method's return, arguments, etc).
*
* @param classDotMethod The method from the model, in [fully_qualified_class_name].[method_name]
* format (no args)
* @return True if the library model should be skipped.
*/
boolean isSkippedLibraryModel(String classDotMethod);

// --- JarInfer configs ---

/**
Expand Down
5 changes: 5 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,11 @@ public String getAutofixSuppressionComment() {
throw new IllegalStateException(ERROR_MESSAGE);
}

@Override
public boolean isSkippedLibraryModel(String classDotMethod) {
throw new IllegalStateException(ERROR_MESSAGE);
}

@Override
public boolean isJarInferEnabled() {
throw new IllegalStateException(ERROR_MESSAGE);
Expand Down
4 changes: 4 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
static final String FL_OPTIONAL_CLASS_PATHS =
EP_FL_NAMESPACE + ":CheckOptionalEmptinessCustomClasses";
static final String FL_SUPPRESS_COMMENT = EP_FL_NAMESPACE + ":AutoFixSuppressionComment";

static final String FL_SKIP_LIBRARY_MODELS = EP_FL_NAMESPACE + ":IgnoreLibraryModelsFor";

/** --- JarInfer configs --- */
static final String FL_JI_ENABLED = EP_FL_NAMESPACE + ":JarInferEnabled";

Expand Down Expand Up @@ -209,6 +212,7 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
throw new IllegalStateException(
"Invalid -XepOpt:" + FL_SUPPRESS_COMMENT + " value. Comment must be single line.");
}
skippedLibraryModels = getFlagStringSet(flags, FL_SKIP_LIBRARY_MODELS);
/** --- JarInfer configs --- */
jarInferEnabled = flags.getBoolean(FL_JI_ENABLED).orElse(false);
jarInferUseReturnAnnotations = flags.getBoolean(FL_JI_USE_RETURN).orElse(false);
Expand Down
42 changes: 38 additions & 4 deletions nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public class LibraryModelsHandler extends BaseNoOpHandler {
public LibraryModelsHandler(Config config) {
super();
this.config = config;
libraryModels = loadLibraryModels();
libraryModels = loadLibraryModels(config);
}

@Override
Expand Down Expand Up @@ -283,12 +283,12 @@ private void setUnconditionalArgumentNullness(
}
}

private static LibraryModels loadLibraryModels() {
private static LibraryModels loadLibraryModels(Config config) {
Iterable<LibraryModels> externalLibraryModels =
ServiceLoader.load(LibraryModels.class, LibraryModels.class.getClassLoader());
ImmutableSet.Builder<LibraryModels> libModelsBuilder = new ImmutableSet.Builder<>();
libModelsBuilder.add(new DefaultLibraryModels()).addAll(externalLibraryModels);
return new CombinedLibraryModels(libModelsBuilder.build());
return new CombinedLibraryModels(libModelsBuilder.build(), config);
}

private static class DefaultLibraryModels implements LibraryModels {
Expand Down Expand Up @@ -751,6 +751,8 @@ public ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods() {

private static class CombinedLibraryModels implements LibraryModels {

private final Config config;

private final ImmutableSetMultimap<MethodRef, Integer> failIfNullParameters;

private final ImmutableSetMultimap<MethodRef, Integer> explicitlyNullableParameters;
Expand All @@ -769,7 +771,8 @@ private static class CombinedLibraryModels implements LibraryModels {

private final ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods;

public CombinedLibraryModels(Iterable<LibraryModels> models) {
public CombinedLibraryModels(Iterable<LibraryModels> models, Config config) {
this.config = config;
ImmutableSetMultimap.Builder<MethodRef, Integer> failIfNullParametersBuilder =
new ImmutableSetMultimap.Builder<>();
ImmutableSetMultimap.Builder<MethodRef, Integer> explicitlyNullableParametersBuilder =
Expand All @@ -788,34 +791,61 @@ public CombinedLibraryModels(Iterable<LibraryModels> models) {
new ImmutableSetMultimap.Builder<>();
for (LibraryModels libraryModels : models) {
for (Map.Entry<MethodRef, Integer> entry : libraryModels.failIfNullParameters().entries()) {
if (shouldSkipModel(entry.getKey())) {
continue;
}
failIfNullParametersBuilder.put(entry);
}
for (Map.Entry<MethodRef, Integer> entry :
libraryModels.explicitlyNullableParameters().entries()) {
if (shouldSkipModel(entry.getKey())) {
continue;
}
explicitlyNullableParametersBuilder.put(entry);
}
for (Map.Entry<MethodRef, Integer> entry : libraryModels.nonNullParameters().entries()) {
if (shouldSkipModel(entry.getKey())) {
continue;
}
nonNullParametersBuilder.put(entry);
}
for (Map.Entry<MethodRef, Integer> entry :
libraryModels.nullImpliesTrueParameters().entries()) {
if (shouldSkipModel(entry.getKey())) {
continue;
}
nullImpliesTrueParametersBuilder.put(entry);
}
for (Map.Entry<MethodRef, Integer> entry :
libraryModels.nullImpliesFalseParameters().entries()) {
if (shouldSkipModel(entry.getKey())) {
continue;
}
nullImpliesFalseParametersBuilder.put(entry);
}
for (Map.Entry<MethodRef, Integer> entry :
libraryModels.nullImpliesNullParameters().entries()) {
if (shouldSkipModel(entry.getKey())) {
continue;
}
nullImpliesNullParametersBuilder.put(entry);
}
for (MethodRef name : libraryModels.nullableReturns()) {
if (shouldSkipModel(name)) {
continue;
}
nullableReturnsBuilder.add(name);
}
for (MethodRef name : libraryModels.nonNullReturns()) {
if (shouldSkipModel(name)) {
continue;
}
nonNullReturnsBuilder.add(name);
}
for (Map.Entry<MethodRef, Integer> entry : libraryModels.castToNonNullMethods().entries()) {
if (shouldSkipModel(entry.getKey())) {
continue;
}
castToNonNullMethodsBuilder.put(entry);
}
}
Expand All @@ -830,6 +860,10 @@ public CombinedLibraryModels(Iterable<LibraryModels> models) {
castToNonNullMethods = castToNonNullMethodsBuilder.build();
}

private boolean shouldSkipModel(MethodRef key) {
return config.isSkippedLibraryModel(key.enclosingClass + "." + key.methodName);
}

@Override
public ImmutableSetMultimap<MethodRef, Integer> failIfNullParameters() {
return failIfNullParameters;
Expand Down
118 changes: 118 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,4 +107,122 @@ public void libraryModelsOverrideRestrictiveAnnotations() {
"}")
.doTest();
}

@Test
public void libraryModelsAndSelectiveSkippingViaCommandLineOptions() {
// First test with the library models in effect
makeLibraryModelsTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated"))
.addSourceLines(
"CallMethods.java",
"package com.uber;",
"import com.uber.lib.unannotated.UnannotatedWithModels;",
"import javax.annotation.Nullable;",
"public class CallMethods {",
" Object testWithoutCheck(UnannotatedWithModels u) {",
" // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type",
" return u.returnsNullUnannotated();",
" }",
" Object testWithCheck(@Nullable Object o) {",
" if (UnannotatedWithModels.isNonNull(o)) {",
" return o;",
" }",
" return new Object();",
" }",
"}")
.addSourceLines(
"OverrideCheck.java",
"package com.uber;",
"import com.uber.lib.unannotated.UnannotatedWithModels;",
"import javax.annotation.Nullable;",
"public class OverrideCheck extends UnannotatedWithModels {",
" @Nullable",
" public Object returnsNullUnannotated() {",
" return null;",
" }",
"}")
.doTest();
// Now test disabling the library model
makeLibraryModelsTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
"-XepOpt:NullAway:IgnoreLibraryModelsFor=com.uber.lib.unannotated.UnannotatedWithModels.returnsNullUnannotated,com.uber.lib.unannotated.UnannotatedWithModels.isNonNull"))
.addSourceLines(
"CallMethods.java",
"package com.uber;",
"import com.uber.lib.unannotated.UnannotatedWithModels;",
"import javax.annotation.Nullable;",
"public class CallMethods {",
" Object testWithoutCheck(UnannotatedWithModels u) {",
" // Ok. Library model ignored",
" return u.returnsNullUnannotated();",
" }",
" Object testWithoutCheckNonIgnoredMethod(UnannotatedWithModels u) {",
" // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type",
" return u.returnsNullUnannotated2();",
" }",
" Object testWithCheck(@Nullable Object o) {",
" if (UnannotatedWithModels.isNonNull(o)) {",
" // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type",
" return o;",
" }",
" return new Object();",
" }",
"}")
.addSourceLines(
"OverrideCheck.java",
"package com.uber;",
"import com.uber.lib.unannotated.UnannotatedWithModels;",
"import javax.annotation.Nullable;",
"public class OverrideCheck extends UnannotatedWithModels {",
" @Nullable", // Still safe, because the method is not @NonNull, it's unannotated
// without model!
" public Object returnsNullUnannotated() {",
" return null;",
" }",
"}")
.doTest();
}

@Test
public void libraryModelsAndSelectiveSkippingViaCommandLineOptions2() {
makeLibraryModelsTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
"-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true",
"-XepOpt:NullAway:IgnoreLibraryModelsFor=com.uber.lib.unannotated.RestrictivelyAnnotatedFIWithModelOverride.apply"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.uber.lib.unannotated.RestrictivelyAnnotatedFIWithModelOverride;",
"import javax.annotation.Nullable;",
"public class Test {",
" void bar(RestrictivelyAnnotatedFIWithModelOverride f) {",
" // Param is @NullableDecl in bytecode, and library model making it non-null is skipped",
" f.apply(null);",
" }",
" void foo() {",
" RestrictivelyAnnotatedFIWithModelOverride func = (x) -> {",
" // Param is @NullableDecl in bytecode, and overriding library model is ignored, thus unsafe",
" // BUG: Diagnostic contains: dereferenced expression x is @Nullable",
" return x.toString();",
" };",
" }",
" void baz() {",
" // BUG: Diagnostic contains: unbound instance method reference cannot be used",
" bar(Object::toString);",
" }",
"}")
.doTest();
}
}
16 changes: 16 additions & 0 deletions test-java-lib/src/main/java/com/uber/lib/unannotated/UnannotatedWithModels.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.uber.lib.unannotated;

public class UnannotatedWithModels {

public Object returnsNullUnannotated() {
return null;
}

public Object returnsNullUnannotated2() {
return null;
}

public static boolean isNonNull(Object o) {
return o != null;
}
}
9 changes: 7 additions & 2 deletions test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ public ImmutableSetMultimap<MethodRef, Integer> nullImpliesTrueParameters() {

@Override
public ImmutableSetMultimap<MethodRef, Integer> nullImpliesFalseParameters() {
return ImmutableSetMultimap.of();
return ImmutableSetMultimap.of(
methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "isNonNull(java.lang.Object)"),
0);
}

@Override
Expand All @@ -63,7 +65,10 @@ public ImmutableSetMultimap<MethodRef, Integer> nullImpliesNullParameters() {

@Override
public ImmutableSet<MethodRef> nullableReturns() {
return ImmutableSet.of(methodRef("com.uber.Foo", "bar()"));
return ImmutableSet.of(
methodRef("com.uber.Foo", "bar()"),
methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "returnsNullUnannotated()"),
methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "returnsNullUnannotated2()"));
}

@Override
Expand Down

0 comments on commit 88fe6a7

Please sign in to comment.