Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove nullable annotation configuration in fix serialization. #621

Merged
merged 4 commits into from Jul 14, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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 Down Expand Up @@ -74,8 +74,6 @@ 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. */
Expand All @@ -85,7 +83,6 @@ public FixSerializationConfig() {
fieldInitInfoEnabled = false;
methodParamProtectionTestEnabled = false;
paramTestIndex = Integer.MAX_VALUE;
annotationConfig = new AnnotationConfig();
outputDirectory = null;
serializer = null;
}
Expand All @@ -96,15 +93,13 @@ public FixSerializationConfig(
boolean fieldInitInfoEnabled,
boolean methodParamProtectionTestEnabled,
int paramTestIndex,
AnnotationConfig annotationConfig,
String outputDirectory) {
this.suggestEnabled = suggestEnabled;
this.suggestEnclosing = suggestEnclosing;
this.fieldInitInfoEnabled = fieldInitInfoEnabled;
this.methodParamProtectionTestEnabled = methodParamProtectionTestEnabled;
this.paramTestIndex = paramTestIndex;
this.outputDirectory = outputDirectory;
this.annotationConfig = annotationConfig;
serializer = new Serializer(this);
}

Expand Down Expand Up @@ -148,13 +143,6 @@ public FixSerializationConfig(String configFilePath) {
paramTestIndex =
XMLUtil.getValueFromAttribute(document, "/serialization/paramTest", "index", Integer.class)
.orElse(Integer.MAX_VALUE);
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 @@ -171,16 +159,12 @@ public static class Builder {
private boolean fieldInitInfo;
private boolean methodParamProtectionTestEnabled;
private int paramIndex;
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 @@ -189,12 +173,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 Down Expand Up @@ -231,7 +209,6 @@ public FixSerializationConfig build() {
fieldInitInfo,
methodParamProtectionTestEnabled,
paramIndex,
new AnnotationConfig(nullable, nonnull),
outputDir);
}
}
Expand Down
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;
}
}
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
Expand Up @@ -128,16 +128,6 @@ public static void writeInXMLFormat(FixSerializationConfig config, String path)
paramTestElement.setAttribute("index", String.valueOf(config.paramTestIndex));
rootElement.appendChild(paramTestElement);

// 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
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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the rename here? It makes sense for now, but I wonder if we will have to flip it right back once we start adding other types of fixes? Or maybe the idea will be to add a common supertype for fixes and retain this one as a subclass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the renaming since instantiating this class only means we are suggesting Nullable and the value Nullable is hardcoded in this class. I though for better understanding the class purpose, I should refine the name.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good for now. We might revisit this if we are adding other kinds of fixes, but right not it isn't clear if that will happen within NullAway or within the annotator or where, so this makes sense to me.


/** 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. */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this comment as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This file was deleted.