Skip to content

Commit

Permalink
Remove fixes.tsv serialization from NullAway serialization service (#…
Browse files Browse the repository at this point in the history 
…1063)

This PR removes the now-redundant serialization of fixes.tsv following
updates to errors.tsv (#643). Previously, `fixes.tsv` recorded the
locations of all non-null variables that participated in
pseudo-assignments from nullable to non-null types, triggering NullAway
errors. However, there was no direct link between these non-null
locations and the corresponding errors. #643 addressed this by enhancing
error serialization to include the relevant non-null location with each
reported error. As a result, `fixes.tsv` is no longer needed, and
[NullAwayAnnotator](https://github.com/ucr-riple/NullAwayAnnotator) now
relies solely on `errors.tsv`.

This PR removes all serialization related to `fixes.tsv` and updates
unit test expected output accordingly. Each unit test was adjusted to
verify serialized errors, preserving their value and functionality.

---------

Co-authored-by: Manu Sridharan <[email protected]>
  • Loading branch information
@nimakarimipour @msridhar
nimakarimipour and msridhar authored Nov 5, 2024
1 parent 17fc1ba commit 1e9c754
Show file tree
Hide file tree
Showing 9 changed files with 227 additions and 639 deletions.
16 changes: 1 addition & 15 deletions nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@

import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
Expand Down Expand Up @@ -141,9 +140,6 @@ public Description createErrorDescription(
}

if (config.serializationIsActive()) {
if (nonNullTarget != null) {
SerializationService.serializeFixSuggestion(config, state, nonNullTarget, errorMessage);
}
// For the case of initializer errors, the leaf of state.getPath() may not be the field /
// method on which the error is being reported (since we do a class-wide analysis to find such
// errors). In such cases, the suggestTree is the appropriate field / method tree, so use
Expand Down Expand Up @@ -403,15 +399,12 @@ private Description.Builder removeCastToNonNullFix(
* @param message Error message.
* @param state The VisitorState object.
* @param descriptionBuilder the description builder for the error.
* @param nonNullFields list of @Nonnull fields that are not guaranteed to be initialized along
* all paths at exit points of the constructor.
*/
void reportInitializerError(
Symbol.MethodSymbol methodSymbol,
String message,
VisitorState state,
Description.Builder descriptionBuilder,
ImmutableList<Symbol> nonNullFields) {
Description.Builder descriptionBuilder) {
// Check needed here, despite check in hasPathSuppression because initialization
// checking happens at the class-level (meaning state.getPath() might not include the
// method itself).
Expand All @@ -424,13 +417,6 @@ void reportInitializerError(
ErrorMessage errorMessage = new ErrorMessage(METHOD_NO_INIT, message);
state.reportMatch(
createErrorDescription(errorMessage, methodTree, descriptionBuilder, state, null));
if (config.serializationIsActive()) {
// For now, we serialize each fix suggestion separately and measure their effectiveness
// separately
nonNullFields.forEach(
symbol ->
SerializationService.serializeFixSuggestion(config, state, symbol, errorMessage));
}
}

boolean symbolHasSuppressWarningsAnnotation(Symbol symbol, String suppression) {
Expand Down
8 changes: 1 addition & 7 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2062,17 +2062,11 @@ private void checkFieldInitialization(ClassTree tree, VisitorState state) {
}
}
for (Element constructorElement : errorFieldsForInitializer.keySet()) {
ImmutableList<Symbol> fieldSymbols =
errorFieldsForInitializer.get(constructorElement).stream()
.map(element -> ASTHelpers.getSymbol(getTreesInstance(state).getTree(element)))
.collect(ImmutableList.toImmutableList());

errorBuilder.reportInitializerError(
(Symbol.MethodSymbol) constructorElement,
errMsgForInitializer(errorFieldsForInitializer.get(constructorElement), state),
state,
buildDescription(getTreesInstance(state).getTree(constructorElement)),
fieldSymbols);
buildDescription(getTreesInstance(state).getTree(constructorElement)));
}
// For static fields
Set<Symbol> notInitializedStaticFields = notInitializedStatic(entities, state);
Expand Down
51 changes: 2 additions & 49 deletions nullaway/src/main/java/com/uber/nullaway/fixserialization/FixSerializationConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@

import com.google.common.base.Preconditions;
import com.uber.nullaway.fixserialization.adapters.SerializationAdapter;
import com.uber.nullaway.fixserialization.out.SuggestedNullableFixInfo;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
Expand All @@ -38,23 +37,6 @@
/** Config class for Fix Serialization package. */
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 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;

/**
* If enabled, serialized information of a fix suggest will also include the enclosing method and
* class of the element involved in error. Finding enclosing elements is costly and will only be
* computed at request.
*/
public final boolean suggestEnclosing;

/**
* If enabled, NullAway will serialize information about methods that initialize a field and leave
* it {@code @NonNull} at exit point.
Expand All @@ -68,20 +50,12 @@ public class FixSerializationConfig {

/** Default Constructor, all features are disabled with this config. */
public FixSerializationConfig() {
suggestEnabled = false;
suggestEnclosing = false;
fieldInitInfoEnabled = false;
outputDirectory = null;
serializer = null;
}

public FixSerializationConfig(
boolean suggestEnabled,
boolean suggestEnclosing,
boolean fieldInitInfoEnabled,
@Nullable String outputDirectory) {
this.suggestEnabled = suggestEnabled;
this.suggestEnclosing = suggestEnclosing;
public FixSerializationConfig(boolean fieldInitInfoEnabled, @Nullable String outputDirectory) {
this.fieldInitInfoEnabled = fieldInitInfoEnabled;
this.outputDirectory = outputDirectory;
serializer =
Expand Down Expand Up @@ -111,17 +85,6 @@ public FixSerializationConfig(String configFilePath, int serializationVersion) {
XMLUtil.getValueFromTag(document, "/serialization/path", String.class).orElse(null);
Preconditions.checkNotNull(
this.outputDirectory, "Error in FixSerialization Config: Output path cannot be null");
suggestEnabled =
XMLUtil.getValueFromAttribute(document, "/serialization/suggest", "active", Boolean.class)
.orElse(false);
suggestEnclosing =
XMLUtil.getValueFromAttribute(
document, "/serialization/suggest", "enclosing", Boolean.class)
.orElse(false);
if (suggestEnclosing && !suggestEnabled) {
throw new IllegalStateException(
"Error in the fix serialization configuration, suggest flag must be enabled to activate enclosing method and class serialization.");
}
fieldInitInfoEnabled =
XMLUtil.getValueFromAttribute(
document, "/serialization/fieldInitInfo", "active", Boolean.class)
Expand All @@ -138,23 +101,13 @@ public FixSerializationConfig(String configFilePath, int serializationVersion) {
/** Builder class for Serialization Config */
public static class Builder {

private boolean suggestEnabled;
private boolean suggestEnclosing;
private boolean fieldInitInfo;
private @Nullable String outputDir;

public Builder() {
suggestEnabled = false;
suggestEnclosing = false;
fieldInitInfo = false;
}

public Builder setSuggest(boolean value, boolean withEnclosing) {
this.suggestEnabled = value;
this.suggestEnclosing = withEnclosing && suggestEnabled;
return this;
}

public Builder setFieldInitInfo(boolean enabled) {
this.fieldInitInfo = enabled;
return this;
Expand All @@ -179,7 +132,7 @@ public FixSerializationConfig build() {
if (outputDir == null) {
throw new IllegalStateException("did not set mandatory output directory");
}
return new FixSerializationConfig(suggestEnabled, suggestEnclosing, fieldInitInfo, outputDir);
return new FixSerializationConfig(fieldInitInfo, outputDir);
}
}
}
65 changes: 0 additions & 65 deletions nullaway/src/main/java/com/uber/nullaway/fixserialization/SerializationService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,10 @@
import com.google.common.collect.ImmutableMap;
import com.google.errorprone.VisitorState;
import com.sun.source.tree.Tree;
import com.sun.source.util.TreePath;
import com.sun.source.util.Trees;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.processing.JavacProcessingEnvironment;
import com.uber.nullaway.Config;
import com.uber.nullaway.ErrorMessage;
import com.uber.nullaway.Nullness;
import com.uber.nullaway.fixserialization.location.SymbolLocation;
import com.uber.nullaway.fixserialization.out.ErrorInfo;
import com.uber.nullaway.fixserialization.out.SuggestedNullableFixInfo;
import java.util.Map;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -77,42 +71,6 @@ public static String escapeSpecialCharacters(String str) {
return str;
}

/**
* Serializes the suggested type change of an element in the source code that can resolve the
* error. We do not want suggested fix changes to override explicit annotations in the code,
* therefore, if the target element has an explicit {@code @Nonnull} annotation, no type change is
* suggested.
*
* @param config NullAway config.
* @param state Visitor state.
* @param target Target element to alternate it's type.
* @param errorMessage Error caused by the target.
*/
public static void serializeFixSuggestion(
Config config, VisitorState state, Symbol target, ErrorMessage errorMessage) {
FixSerializationConfig serializationConfig = config.getSerializationConfig();
if (!serializationConfig.suggestEnabled) {
return;
}
// Skip if the element has an explicit @Nonnull annotation.
if (Nullness.hasNonNullAnnotation(target, config)) {
return;
}
Trees trees = Trees.instance(JavacProcessingEnvironment.instance(state.context));
// Skip if the element is received as bytecode.
if (trees.getPath(target) == null) {
return;
}
SymbolLocation location = SymbolLocation.createLocationFromSymbol(target);
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(
suggestedNullableFixInfo, serializationConfig.suggestEnclosing);
}

/**
* Serializes the reporting error.
*
Expand All @@ -132,27 +90,4 @@ public static void serializeReportingError(
serializer, "Serializer shouldn't be null at this point, error in configuration setting!");
serializer.serializeErrorInfo(new ErrorInfo(state.getPath(), errorTree, errorMessage, target));
}

/**
* Builds the {@link SuggestedNullableFixInfo} instance based on the {@link ErrorMessage} type.
*/
private static SuggestedNullableFixInfo buildFixMetadata(
TreePath path, ErrorMessage errorMessage, SymbolLocation location) {
SuggestedNullableFixInfo suggestedNullableFixInfo;
switch (errorMessage.getMessageType()) {
case RETURN_NULLABLE:
case WRONG_OVERRIDE_RETURN:
case WRONG_OVERRIDE_PARAM:
case PASS_NULLABLE:
case FIELD_NO_INIT:
case ASSIGN_FIELD_NULLABLE:
case METHOD_NO_INIT:
suggestedNullableFixInfo = new SuggestedNullableFixInfo(path, location, errorMessage);
break;
default:
throw new IllegalStateException(
"Cannot suggest a type to resolve error of type: " + errorMessage.getMessageType());
}
return suggestedNullableFixInfo;
}
}
24 changes: 0 additions & 24 deletions nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import com.uber.nullaway.fixserialization.adapters.SerializationAdapter;
import com.uber.nullaway.fixserialization.out.ErrorInfo;
import com.uber.nullaway.fixserialization.out.FieldInitializationInfo;
import com.uber.nullaway.fixserialization.out.SuggestedNullableFixInfo;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStream;
Expand All @@ -47,9 +46,6 @@ public class Serializer {
/** Path to write errors. */
private final Path errorOutputPath;

/** Path to write suggested fix metadata. */
private final Path suggestedFixesOutputPath;

/** Path to write suggested fix metadata. */
private final Path fieldInitializationOutputPath;

Expand All @@ -63,29 +59,12 @@ public class Serializer {
public Serializer(FixSerializationConfig config, SerializationAdapter serializationAdapter) {
String outputDirectory = config.outputDirectory;
this.errorOutputPath = Paths.get(outputDirectory, "errors.tsv");
this.suggestedFixesOutputPath = Paths.get(outputDirectory, "fixes.tsv");
this.fieldInitializationOutputPath = Paths.get(outputDirectory, "field_init.tsv");
this.serializationAdapter = serializationAdapter;
serializeVersion(outputDirectory);
initializeOutputFiles(config);
}

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

/**
* Appends the string representation of the {@link ErrorMessage}.
*
Expand Down Expand Up @@ -145,9 +124,6 @@ private void serializeVersion(@Nullable String outputDirectory) {
private void initializeOutputFiles(FixSerializationConfig config) {
try {
Files.createDirectories(Paths.get(config.outputDirectory));
if (config.suggestEnabled) {
initializeFile(suggestedFixesOutputPath, SuggestedNullableFixInfo.header());
}
if (config.fieldInitInfoEnabled) {
initializeFile(fieldInitializationOutputPath, FieldInitializationInfo.header());
}
Expand Down
6 changes: 0 additions & 6 deletions nullaway/src/main/java/com/uber/nullaway/fixserialization/XMLUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,6 @@ public static void writeInXMLFormat(FixSerializationConfig config, String path)
Element rootElement = doc.createElement("serialization");
doc.appendChild(rootElement);

// Suggest
Element suggestElement = doc.createElement("suggest");
suggestElement.setAttribute("active", String.valueOf(config.suggestEnabled));
suggestElement.setAttribute("enclosing", String.valueOf(config.suggestEnclosing));
rootElement.appendChild(suggestElement);

// Field Initialization
Element fieldInitInfoEnabled = doc.createElement("fieldInitInfo");
fieldInitInfoEnabled.setAttribute("active", String.valueOf(config.fieldInitInfoEnabled));
Expand Down
Loading

0 comments on commit 1e9c754

Please sign in to comment.