Skip to content

Commit

Permalink
Remove nullable annotation configuration in fix serialization. (#621)
Browse files Browse the repository at this point in the history
Following GH-517 If `serializeFixMetadata` is active, `NullAway` will serialize suggested type changes (currently just adding `@Nullable`). In the `FixSerializationConfig` file we can set a custom annotation that `NullAway` will use in the output serializations. 

However, this process can be simplified by just asking `NullAway` to serialize type change locations with a default annotation name, any tool processing this output can use their own preferred annotation.

After this PR the following entity will be removed from the config `.xml` file:
```xml
<annotation>
       <nonnull> ... </nonnull>
       <nullable> ... </nullable>
</annotation
```
And a sample out below:
```
Field bar of Foo, ASSIGN_FIELD_NULLABLE, @nullable
```

Will be changed to simply:
```
Field bar of Foo, ASSIGN_FIELD_NULLABLE, nullable
```
  • Loading branch information
nimakarimipour authored Jul 14, 2022
1 parent 58029f8 commit 2198833
Show file tree
Hide file tree
Showing 7 changed files with 58 additions and 273 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
package com.uber.nullaway.fixserialization;

import com.google.common.base.Preconditions;
import com.uber.nullaway.fixserialization.qual.AnnotationConfig;
import com.uber.nullaway.fixserialization.out.SuggestedNullableFixInfo;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
Expand All @@ -41,9 +41,9 @@ public class FixSerializationConfig {
* If enabled, the corresponding output file will be cleared and for all reported errors, NullAway
* will serialize information and suggest type changes to resolve them, in case these errors could
* be fixed by adding a {@code @Nullable} annotation. These type change suggestions are in form of
* {@link com.uber.nullaway.fixserialization.out.SuggestedFixInfo} instances and will be
* serialized at output directory. If deactivated, no {@code SuggestedFixInfo} will be created and
* the output file will remain untouched.
* {@link SuggestedNullableFixInfo} instances and will be serialized at output directory. If
* deactivated, no {@code SuggestedFixInfo} will be created and the output file will remain
* untouched.
*/
public final boolean suggestEnabled;
/**
Expand All @@ -62,16 +62,13 @@ public class FixSerializationConfig {
/** The directory where all files generated/read by Fix Serialization package resides. */
@Nullable public final String outputDirectory;

public final AnnotationConfig annotationConfig;

@Nullable private final Serializer serializer;

/** Default Constructor, all features are disabled with this config. */
public FixSerializationConfig() {
suggestEnabled = false;
suggestEnclosing = false;
fieldInitInfoEnabled = false;
annotationConfig = new AnnotationConfig();
outputDirectory = null;
serializer = null;
}
Expand All @@ -80,13 +77,11 @@ public FixSerializationConfig(
boolean suggestEnabled,
boolean suggestEnclosing,
boolean fieldInitInfoEnabled,
AnnotationConfig annotationConfig,
String outputDirectory) {
@Nullable String outputDirectory) {
this.suggestEnabled = suggestEnabled;
this.suggestEnclosing = suggestEnclosing;
this.fieldInitInfoEnabled = fieldInitInfoEnabled;
this.outputDirectory = outputDirectory;
this.annotationConfig = annotationConfig;
serializer = new Serializer(this);
}

Expand Down Expand Up @@ -124,13 +119,6 @@ public FixSerializationConfig(String configFilePath) {
XMLUtil.getValueFromAttribute(
document, "/serialization/fieldInitInfo", "active", Boolean.class)
.orElse(false);
String nullableAnnot =
XMLUtil.getValueFromTag(document, "/serialization/annotation/nullable", String.class)
.orElse("javax.annotation.Nullable");
String nonnullAnnot =
XMLUtil.getValueFromTag(document, "/serialization/annotation/nonnull", String.class)
.orElse("javax.annotation.Nonnull");
this.annotationConfig = new AnnotationConfig(nullableAnnot, nonnullAnnot);
serializer = new Serializer(this);
}

Expand All @@ -145,16 +133,12 @@ public static class Builder {
private boolean suggestEnabled;
private boolean suggestEnclosing;
private boolean fieldInitInfo;
private String nullable;
private String nonnull;
@Nullable private String outputDir;

public Builder() {
suggestEnabled = false;
suggestEnclosing = false;
fieldInitInfo = false;
nullable = "javax.annotation.Nullable";
nonnull = "javax.annotation.Nonnull";
}

public Builder setSuggest(boolean value, boolean withEnclosing) {
Expand All @@ -163,12 +147,6 @@ public Builder setSuggest(boolean value, boolean withEnclosing) {
return this;
}

public Builder setAnnotations(String nullable, String nonnull) {
this.nullable = nullable;
this.nonnull = nonnull;
return this;
}

public Builder setFieldInitInfo(boolean enabled) {
this.fieldInitInfo = enabled;
return this;
Expand All @@ -193,12 +171,7 @@ public FixSerializationConfig build() {
if (outputDir == null) {
throw new IllegalStateException("did not set mandatory output directory");
}
return new FixSerializationConfig(
suggestEnabled,
suggestEnclosing,
fieldInitInfo,
new AnnotationConfig(nullable, nonnull),
outputDir);
return new FixSerializationConfig(suggestEnabled, suggestEnclosing, fieldInitInfo, outputDir);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
import com.uber.nullaway.Nullness;
import com.uber.nullaway.fixserialization.location.FixLocation;
import com.uber.nullaway.fixserialization.out.ErrorInfo;
import com.uber.nullaway.fixserialization.out.SuggestedFixInfo;
import com.uber.nullaway.fixserialization.out.SuggestedNullableFixInfo;

/** A facade class to interact with fix serialization package. */
public class SerializationService {
Expand Down Expand Up @@ -65,12 +65,13 @@ public static void serializeFixSuggestion(
return;
}
FixLocation location = FixLocation.createFixLocationFromSymbol(target);
SuggestedFixInfo suggestedFixInfo =
buildFixMetadata(config, state.getPath(), errorMessage, location);
SuggestedNullableFixInfo suggestedNullableFixInfo =
buildFixMetadata(state.getPath(), errorMessage, location);
Serializer serializer = serializationConfig.getSerializer();
Preconditions.checkNotNull(
serializer, "Serializer shouldn't be null at this point, error in configuration setting!");
serializer.serializeSuggestedFixInfo(suggestedFixInfo, serializationConfig.suggestEnclosing);
serializer.serializeSuggestedFixInfo(
suggestedNullableFixInfo, serializationConfig.suggestEnclosing);
}

/**
Expand All @@ -88,10 +89,12 @@ public static void serializeReportingError(
serializer.serializeErrorInfo(new ErrorInfo(state.getPath(), errorMessage));
}

/** Builds the {@link SuggestedFixInfo} instance based on the {@link ErrorMessage} type. */
private static SuggestedFixInfo buildFixMetadata(
Config config, TreePath path, ErrorMessage errorMessage, FixLocation location) {
SuggestedFixInfo suggestedFixInfo;
/**
* Builds the {@link SuggestedNullableFixInfo} instance based on the {@link ErrorMessage} type.
*/
private static SuggestedNullableFixInfo buildFixMetadata(
TreePath path, ErrorMessage errorMessage, FixLocation location) {
SuggestedNullableFixInfo suggestedNullableFixInfo;
switch (errorMessage.getMessageType()) {
case RETURN_NULLABLE:
case WRONG_OVERRIDE_RETURN:
Expand All @@ -100,17 +103,12 @@ private static SuggestedFixInfo buildFixMetadata(
case FIELD_NO_INIT:
case ASSIGN_FIELD_NULLABLE:
case METHOD_NO_INIT:
suggestedFixInfo =
new SuggestedFixInfo(
path,
location,
errorMessage,
config.getSerializationConfig().annotationConfig.getNullable());
suggestedNullableFixInfo = new SuggestedNullableFixInfo(path, location, errorMessage);
break;
default:
throw new IllegalStateException(
"Cannot suggest a type to resolve error of type: " + errorMessage.getMessageType());
}
return suggestedFixInfo;
return suggestedNullableFixInfo;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import com.uber.nullaway.ErrorMessage;
import com.uber.nullaway.fixserialization.out.ErrorInfo;
import com.uber.nullaway.fixserialization.out.FieldInitializationInfo;
import com.uber.nullaway.fixserialization.out.SuggestedFixInfo;
import com.uber.nullaway.fixserialization.out.SuggestedNullableFixInfo;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStream;
Expand Down Expand Up @@ -55,16 +55,17 @@ public Serializer(FixSerializationConfig config) {
}

/**
* Appends the string representation of the {@link SuggestedFixInfo}.
* Appends the string representation of the {@link SuggestedNullableFixInfo}.
*
* @param suggestedFixInfo SuggestedFixInfo object.
* @param suggestedNullableFixInfo SuggestedFixInfo object.
* @param enclosing Flag to control if enclosing method and class should be included.
*/
public void serializeSuggestedFixInfo(SuggestedFixInfo suggestedFixInfo, boolean enclosing) {
public void serializeSuggestedFixInfo(
SuggestedNullableFixInfo suggestedNullableFixInfo, boolean enclosing) {
if (enclosing) {
suggestedFixInfo.initEnclosing();
suggestedNullableFixInfo.initEnclosing();
}
appendToFile(suggestedFixInfo.tabSeparatedToString(), suggestedFixesOutputPath);
appendToFile(suggestedNullableFixInfo.tabSeparatedToString(), suggestedFixesOutputPath);
}

/**
Expand Down Expand Up @@ -102,7 +103,7 @@ private void initializeOutputFiles(FixSerializationConfig config) {
try {
Files.createDirectories(Paths.get(config.outputDirectory));
if (config.suggestEnabled) {
initializeFile(suggestedFixesOutputPath, SuggestedFixInfo.header());
initializeFile(suggestedFixesOutputPath, SuggestedNullableFixInfo.header());
}
if (config.fieldInitInfoEnabled) {
initializeFile(fieldInitializationOutputPath, FieldInitializationInfo.header());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,16 +121,6 @@ public static void writeInXMLFormat(FixSerializationConfig config, String path)
fieldInitInfoEnabled.setAttribute("active", String.valueOf(config.fieldInitInfoEnabled));
rootElement.appendChild(fieldInitInfoEnabled);

// Annotations
Element annots = doc.createElement("annotation");
Element nonnull = doc.createElement("nonnull");
nonnull.setTextContent(config.annotationConfig.getNonNull().getFullName());
Element nullable = doc.createElement("nullable");
nullable.setTextContent(config.annotationConfig.getNullable().getFullName());
annots.appendChild(nullable);
annots.appendChild(nonnull);
rootElement.appendChild(annots);

// Output dir
Element outputDir = doc.createElement("path");
outputDir.setTextContent(config.outputDirectory);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,64 +26,56 @@
import com.sun.source.util.TreePath;
import com.uber.nullaway.ErrorMessage;
import com.uber.nullaway.fixserialization.location.FixLocation;
import com.uber.nullaway.fixserialization.qual.AnnotationConfig;
import java.util.Objects;

/** Stores information suggesting a type change of an element in source code. */
public class SuggestedFixInfo {
/** Stores information suggesting adding @Nullable on an element in source code. */
public class SuggestedNullableFixInfo {

/** FixLocation of the target element in source code. */
private final FixLocation fixLocation;
/** Error which will be resolved by this type change. */
private final ErrorMessage errorMessage;
/** Suggested annotation. */
private final AnnotationConfig.Annotation annotation;

private final ClassAndMethodInfo classAndMethodInfo;

public SuggestedFixInfo(
TreePath path,
FixLocation fixLocation,
ErrorMessage errorMessage,
AnnotationConfig.Annotation annotation) {
public SuggestedNullableFixInfo(
TreePath path, FixLocation fixLocation, ErrorMessage errorMessage) {
this.classAndMethodInfo = new ClassAndMethodInfo(path);
this.fixLocation = fixLocation;
this.errorMessage = errorMessage;
this.annotation = annotation;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof SuggestedFixInfo)) {
if (!(o instanceof SuggestedNullableFixInfo)) {
return false;
}
SuggestedFixInfo suggestedFixInfo = (SuggestedFixInfo) o;
return Objects.equals(fixLocation, suggestedFixInfo.fixLocation)
&& Objects.equals(annotation, suggestedFixInfo.annotation)
SuggestedNullableFixInfo suggestedNullableFixInfo = (SuggestedNullableFixInfo) o;
return Objects.equals(fixLocation, suggestedNullableFixInfo.fixLocation)
&& Objects.equals(
errorMessage.getMessageType().toString(),
suggestedFixInfo.errorMessage.getMessageType().toString());
suggestedNullableFixInfo.errorMessage.getMessageType().toString());
}

@Override
public int hashCode() {
return Objects.hash(fixLocation, annotation, errorMessage.getMessageType().toString());
return Objects.hash(fixLocation, errorMessage.getMessageType().toString());
}

/**
* returns string representation of content of an object.
*
* @return string representation of contents of an object in a line seperated by tabs.
* @return string representation of contents of an object in a line separated by tabs.
*/
public String tabSeparatedToString() {
return fixLocation.tabSeparatedToString()
+ '\t'
+ errorMessage.getMessageType().toString()
+ '\t'
+ annotation
+ "nullable"
+ '\t'
+ (classAndMethodInfo.getClazz() == null
? "null"
Expand All @@ -100,8 +92,8 @@ public void initEnclosing() {
}

/**
* Creates header of an output file containing all {@link SuggestedFixInfo} written in string
* which values are separated by tabs.
* Creates header of an output file containing all {@link SuggestedNullableFixInfo} written in
* string which values are separated by tabs.
*
* @return string representation of the header separated by tabs.
*/
Expand Down
Loading

0 comments on commit 2198833

Please sign in to comment.