Skip to content

Commit

Permalink
minor changes in a test case
Browse files Browse the repository at this point in the history
Serialization version 3: update method serialization to exclude type use annotations and type arguments (#735)

This PR updates serialization to version `3` and burns version `2`.

Changes in serialization version 3:

- Type arguments and Type use annotations are excluded from the serialized method signatures.

---
Additional context:
This PR updates method serialization to ensure only relevant information in a method signature is serialized. Previous to this PR, method signatures serialization was relying on calling  `.toString()` method, but if a parameter contains an annotation of type `@Target({TYPE_USE})` the annotation will exist in the `.toSting()` output which is not part of a method signature. This PR updates serialization to ensure exact method signature as expected is serialized.

Suggested change

suggested changes

ternary operator as a method argument

Docs: `-XepExcludedPaths` was added in 2.1.3, not 2.13 (#744)

Add command line option to skip specific library models. (#741)

This PR adds the CLI option `-XepOpt:NullAway:IgnoreLibraryModelsFor` which takes a list of methods, given as fully qualified class name + method simple name (i.e. independent of argument types).

Methods matching this list will be skipped when loading library models (from any implementation of the `LibraryModels`'s `@AutoService` as well as our included models). This can be used in a per-compilation target basis to disable NullAway's library models for ease of upgrading to new versions with stricter modeling of common JDK or third-party APIs.

We considered a few alternative approaches here, but decided against:

- Simply using another instance of `LibraryModels` to "invert" the models given by NullAway's default library models: This would have required no code changes, but doesn't work in all cases. If NullAway's default models make the return of method `foo()` `@Nullable`, for example, then a new model that makes the return `@NonNull` will break `@Nullable` overrides of `foo()`. In general, we want to go back to "optimistic assumptions" rather than just replace the library model.
- We could have a list of methods in the `LibraryModels` for which to ignore previous models, and have those override any models on those methods coming from a different `LibraryModels` implementation. But, from the point of view of the user configuring NullAway, this is complex: they need to have an instance of custom library models in their build, and changing java plugin classpath deps on a per-target basis is more complex than changing CLI arguments (e.g. due to JVM re-use by the build system).
- We could provide more specific disabling of library models (e.g. a specific method signature or removing only one particular kind of model from a method, such as keeping the model on the return value, but removing it from an argument, or removing a null-implies-false model or similar). We could revisit this in the future, but supporting this would make the syntax of the CLI values a lot more complex. For now, we believe just turning off all models for a given method is a sufficient degree of granularity.
- Per-package/per-class/regex based ignore specs: See above. Avoiding complexity until we need it.

Note: If and when this lands, it needs a Wiki documentation update!

---------
Co-authored-by: Manu Sridharan <[email protected]>
  • Loading branch information
NikitaAware and msridhar committed Mar 8, 2023
1 parent 4820854 commit dc232c9
Show file tree
Hide file tree
Showing 16 changed files with 509 additions and 51 deletions.
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ A complete Android `build.gradle` example is [here](https://gist.github.com/msri

#### Annotation Processors / Generated Code

Some annotation processors like [Dagger](https://google.github.io/dagger/) and [AutoValue](https://github.com/google/auto/tree/master/value) generate code into the same package namespace as your own code. This can cause problems when setting NullAway to the `ERROR` level as suggested above, since errors in this generated code will block the build. Currently the best solution to this problem is to completely disable Error Prone on generated code, using the `-XepExcludedPaths` option added in Error Prone 2.13 (documented [here](http://errorprone.info/docs/flags), use `options.errorprone.excludedPaths=` in Gradle). To use, figure out which directory contains the generated code, and add that directory to the excluded path regex.
Some annotation processors like [Dagger](https://google.github.io/dagger/) and [AutoValue](https://github.com/google/auto/tree/master/value) generate code into the same package namespace as your own code. This can cause problems when setting NullAway to the `ERROR` level as suggested above, since errors in this generated code will block the build. Currently the best solution to this problem is to completely disable Error Prone on generated code, using the `-XepExcludedPaths` option added in Error Prone 2.1.3 (documented [here](http://errorprone.info/docs/flags), use `options.errorprone.excludedPaths=` in Gradle). To use, figure out which directory contains the generated code, and add that directory to the excluded path regex.

**Note for Dagger users**: Dagger versions older than 2.12 can have bad interactions with NullAway; see [here](https://github.com/uber/NullAway/issues/48#issuecomment-340018409). Please update to Dagger 2.12 to fix the problem.

Expand Down
7 changes: 7 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java
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
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
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
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
6 changes: 2 additions & 4 deletions nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java
Original file line number Diff line number Diff line change
Expand Up @@ -403,11 +403,9 @@ public void checkTypeParameterNullnessForConditionalExpression(ConditionalExpres
return;
}

Tree truePartTree;
Tree falsePartTree;
Tree truePartTree = tree.getTrueExpression();
Tree falsePartTree = tree.getFalseExpression();

truePartTree = tree.getTrueExpression();
falsePartTree = tree.getFalseExpression();
Type condExprType = getTreeType(tree);
Type truePartType = getTreeType(truePartTree);
Type falsePartType = getTreeType(falsePartTree);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import com.google.common.base.Preconditions;
import com.uber.nullaway.fixserialization.adapters.SerializationAdapter;
import com.uber.nullaway.fixserialization.adapters.SerializationV1Adapter;
import com.uber.nullaway.fixserialization.adapters.SerializationV2Adapter;
import com.uber.nullaway.fixserialization.adapters.SerializationV3Adapter;
import com.uber.nullaway.fixserialization.out.SuggestedNullableFixInfo;
import java.io.IOException;
import java.nio.file.Files;
Expand Down Expand Up @@ -137,7 +137,10 @@ private SerializationAdapter initializeAdapter(int version) {
case 1:
return new SerializationV1Adapter();
case 2:
return new SerializationV2Adapter();
throw new RuntimeException(
"Serialization version v2 is skipped and was used for an alpha version of the auto-annotator tool. Please use version 3 instead.");
case 3:
return new SerializationV3Adapter();
default:
throw new RuntimeException(
"Unrecognized NullAway serialization version: "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public interface SerializationAdapter {
* Latest version number. If version is not defined by the user, NullAway will use the
* corresponding adapter to this version in its serialization.
*/
int LATEST_VERSION = 2;
int LATEST_VERSION = 3;

/**
* Returns header of "errors.tsv" which contains all serialized {@link ErrorInfo} reported by
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,18 @@
package com.uber.nullaway.fixserialization.adapters;

import static com.uber.nullaway.fixserialization.out.ErrorInfo.EMPTY_NONNULL_TARGET_LOCATION_STRING;
import static java.util.stream.Collectors.joining;

import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.util.Name;
import com.uber.nullaway.fixserialization.SerializationService;
import com.uber.nullaway.fixserialization.Serializer;
import com.uber.nullaway.fixserialization.location.SymbolLocation;
import com.uber.nullaway.fixserialization.out.ErrorInfo;

/**
* Adapter for serialization version 2.
* Adapter for serialization version 3.
*
* <p>Updates to previous version (version 1):
*
Expand All @@ -40,9 +43,10 @@
* the error is reported.
* <li>Serialized errors contain an extra column indicating the path to the containing source file
* where the error is reported
* <li>Type arguments and Type use annotations are excluded from the serialized method signatures.
* </ul>
*/
public class SerializationV2Adapter implements SerializationAdapter {
public class SerializationV3Adapter implements SerializationAdapter {

@Override
public String getErrorsOutputFileHeader() {
Expand Down Expand Up @@ -80,11 +84,42 @@ public String serializeError(ErrorInfo errorInfo) {

@Override
public int getSerializationVersion() {
return 2;
return 3;
}

@Override
public String serializeMethodSignature(Symbol.MethodSymbol methodSymbol) {
return methodSymbol.toString();
StringBuilder sb = new StringBuilder();
if (methodSymbol.isConstructor()) {
// For constructors, method's simple name is <init> and not the enclosing class, need to
// locate the enclosing class.
Symbol.ClassSymbol encClass = methodSymbol.owner.enclClass();
Name name = encClass.getSimpleName();
if (name.isEmpty()) {
// An anonymous class cannot declare its own constructor. Based on this assumption and our
// use case, we should not serialize the method signature.
throw new RuntimeException(
"Did not expect method serialization for anonymous class constructor: "
+ methodSymbol
+ ", in anonymous class: "
+ encClass);
}
sb.append(name);
} else {
// For methods, we use the name of the method.
sb.append(methodSymbol.getSimpleName());
}
sb.append(
methodSymbol.getParameters().stream()
.map(
parameter ->
// check if array
(parameter.type instanceof Type.ArrayType)
// if is array, get the element type and append "[]"
? ((Type.ArrayType) parameter.type).elemtype.tsym + "[]"
// else, just get the type
: parameter.type.tsym.toString())
.collect(joining(",", "(", ")")));
return sb.toString();
}
}
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
Loading

0 comments on commit dc232c9

Please sign in to comment.