Skip to content

Commit

Permalink
Serialization of Type Change Suggestions for Type Violations. (#517)
Browse files Browse the repository at this point in the history
This PR enables ```NullAway``` to serialize information on code locations to resolve the reporting errors if they can be resolved via a type change. This information can be used by other tools such as [AutoFixer](https://github.com/nimakarimipour/NullAwayAutoFixer) to have an understanding of the source code, code points where violations occurred, and possible solutions. In the example below, ```NullAway``` will report the error: 
```assigning @nullable null to @nonnull bar```. 

```java
class Foo {
     Object bar = new Object();
     
     void baz(){
           this.bar = null;
     }
}
```
This PR enables ```NullAway``` to suggest ```bar``` type to be ```@Nullable``` to resolve the error and creates two output files. ```errors.tsv``` where all reporting errors are stored and ```fixes.tsv``` where all the type change suggestions are stored.

1. ```errors.tsv```: Every row represents a reporting error with the format: ```MESSAGE_TYPE, MESSAGE, ENCLOSING CLASS, ENCLOSING METHOD```
For instance: ```ASSIGN_FIELD_NULLABLE, assigning nullable null to field bar, Foo, baz```
2. ```fixes.tsv```: Every row represents a type change suggestion with the format: ```LOCATION, MESSAGE_TYPE, ANNOT```
For instance: ```Field bar of Foo, ASSIGN_FIELD_NULLABLE, @Nullable```

To activate the feature above the following flags must be passed to ```NullAway``` via ```ErrorProneFlags```:

```
"-XepOpt:NullAway:SerializeFixMetadata=true"
"-XepOpt:NullAway:FixSerializationConfigPath=configpath"
```

When ```-XepOpt:NullAway:SerializeFixMetadata=true``` is observed in the flags, ```NullAway``` will look for a file at ```FixSerializationConfigPath``` to activate/deactivate serialization features.

Below is the ```FixMetadataConfigPath``` format:

```xml
<serialization>
    <suggest activation="true" enclosing="true"/>
    <output>
           "outputdir"
    </output>
    <annotation>
        <nullable>javax.annotation.Nullable</nullable>
        <nonnull>javax.annotation.Nonnull</nonnull>
    </annotation>
</serialization>
```

If ```-XepOpt:NullAway:SerializeFixMetadata=true``` is observed, NullAway will serialize all reporting errors to ```errors.tsv``` regardless of flag values in ```FixSerializationConfig```.

Finding enclosing method/class for suggested ```fixes``` is costly and not always required, they can be controlled using ```enclosing ``` attribute in the config file above. If any of the keys in the template config above is not found, the default value will be considered.

Please note, at the places where explicit ```@Nonnull``` is observed, ```NullAway``` will acknowledge that annotation and will not suggest any type change.
  • Loading branch information
nimakarimipour authored Feb 9, 2022
1 parent 5b5ad0d commit 9dbdd65
Show file tree
Hide file tree
Showing 29 changed files with 2,304 additions and 44 deletions.
23 changes: 23 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@

import com.google.auto.value.AutoValue;
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.errorprone.util.ASTHelpers;
import com.sun.tools.javac.code.Symbol;
import com.uber.nullaway.fixserialization.FixSerializationConfig;
import java.util.LinkedHashSet;
import java.util.Set;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -115,6 +117,27 @@ public abstract class AbstractConfig implements Config {

protected Set<String> customNullableAnnotations;

/**
* If active, NullAway will write all reporting errors in output directory. The output directory
* along with the activation status of other serialization features are stored in {@link
* FixSerializationConfig}.
*/
protected boolean serializationActivationFlag;

protected FixSerializationConfig fixSerializationConfig;

@Override
public boolean serializationIsActive() {
return serializationActivationFlag;
}

@Override
public FixSerializationConfig getSerializationConfig() {
Preconditions.checkArgument(
serializationActivationFlag, "Fix Serialization is not active, cannot access it's config.");
return fixSerializationConfig;
}

protected static Pattern getPackagePattern(Set<String> packagePrefixes) {
// noinspection ConstantConditions
String choiceRegexp =
Expand Down
18 changes: 18 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/Config.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,30 @@

import com.google.common.collect.ImmutableSet;
import com.sun.tools.javac.code.Symbol;
import com.uber.nullaway.fixserialization.FixSerializationConfig;
import java.util.Set;
import javax.annotation.Nullable;

/** Provides configuration parameters for the nullability checker. */
public interface Config {

/**
* Checks if Serialization feature is active.
*
* @return true, if Fix Serialization feature is active.
*/
boolean serializationIsActive();

/**
* Getter method for {@link FixSerializationConfig}.
*
* <p>Fix Serialization feature must be activated, otherwise calling this method will fail the
* execution.
*
* @return {@link FixSerializationConfig} instance in Config.
*/
FixSerializationConfig getSerializationConfig();

/**
* Checks if a symbol comes from an annotated package.
*
Expand Down
11 changes: 11 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import com.google.common.collect.ImmutableSet;
import com.sun.tools.javac.code.Symbol;
import com.uber.nullaway.fixserialization.FixSerializationConfig;
import java.util.Set;
import javax.annotation.Nullable;

Expand All @@ -53,6 +54,16 @@ public class DummyOptionsConfig implements Config {

public DummyOptionsConfig() {}

@Override
public boolean serializationIsActive() {
throw new IllegalStateException(ERROR_MESSAGE);
}

@Override
public FixSerializationConfig getSerializationConfig() {
throw new IllegalStateException(ERROR_MESSAGE);
}

@Override
public boolean fromAnnotatedPackage(Symbol.ClassSymbol symbol) {
throw new IllegalStateException(ERROR_MESSAGE);
Expand Down
69 changes: 58 additions & 11 deletions nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import static com.uber.nullaway.NullAway.getTreesInstance;

import com.google.common.base.Joiner;
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 All @@ -51,6 +52,7 @@
import com.sun.tools.javac.tree.JCTree.JCCompilationUnit;
import com.sun.tools.javac.util.DiagnosticSource;
import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition;
import com.uber.nullaway.fixserialization.SerializationService;
import java.util.Iterator;
import java.util.List;
import java.util.Set;
Expand Down Expand Up @@ -82,12 +84,18 @@ public class ErrorBuilder {
* @param errorMessage the error message object.
* @param descriptionBuilder the description builder for the error.
* @param state the visitor state (used for e.g. suppression finding).
* @param nonNullTarget if non-null, this error involved a pseudo-assignment of a @Nullable
* expression into a @NonNull target, and this parameter is the Symbol for that target.
* @return the error description
*/
Description createErrorDescription(
ErrorMessage errorMessage, Description.Builder descriptionBuilder, VisitorState state) {
ErrorMessage errorMessage,
Description.Builder descriptionBuilder,
VisitorState state,
@Nullable Symbol nonNullTarget) {
Tree enclosingSuppressTree = suppressibleNode(state.getPath());
return createErrorDescription(errorMessage, enclosingSuppressTree, descriptionBuilder, state);
return createErrorDescription(
errorMessage, enclosingSuppressTree, descriptionBuilder, state, nonNullTarget);
}

/**
Expand All @@ -97,13 +105,16 @@ Description createErrorDescription(
* @param suggestTree the location at which a fix suggestion should be made
* @param descriptionBuilder the description builder for the error.
* @param state the visitor state (used for e.g. suppression finding).
* @param nonNullTarget if non-null, this error involved a pseudo-assignment of a @Nullable
* expression into a @NonNull target, and this parameter is the Symbol for that target.
* @return the error description
*/
public Description createErrorDescription(
ErrorMessage errorMessage,
@Nullable Tree suggestTree,
Description.Builder descriptionBuilder,
VisitorState state) {
VisitorState state,
@Nullable Symbol nonNullTarget) {
Description.Builder builder = descriptionBuilder.setMessage(errorMessage.message);
String checkName = CORE_CHECK_NAME;
if (errorMessage.messageType.equals(GET_ON_EMPTY_OPTIONAL)) {
Expand All @@ -123,6 +134,14 @@ public Description createErrorDescription(
if (config.suggestSuppressions() && suggestTree != null) {
builder = addSuggestedSuppression(errorMessage, suggestTree, builder);
}

if (config.serializationIsActive()) {
if (nonNullTarget != null) {
SerializationService.serializeFixSuggestion(config, state, nonNullTarget, errorMessage);
}
SerializationService.serializeReportingError(config, state, errorMessage);
}

// #letbuildersbuild
return builder.build();
}
Expand Down Expand Up @@ -201,19 +220,26 @@ private Description.Builder addSuggestedSuppression(
* @param descriptionBuilder the description builder for the error.
* @param state the visitor state for the location which triggered the error (i.e. for suppression
* finding)
* @param nonNullTarget if non-null, this error involved a pseudo-assignment of a @Nullable
* expression into a @NonNull target, and this parameter is the Symbol for that target.
* @return the error description.
*/
Description createErrorDescriptionForNullAssignment(
ErrorMessage errorMessage,
@Nullable Tree suggestTreeIfCastToNonNull,
Description.Builder descriptionBuilder,
VisitorState state) {
VisitorState state,
Symbol nonNullTarget) {
if (config.getCastToNonNullMethod() != null) {
return createErrorDescription(
errorMessage, suggestTreeIfCastToNonNull, descriptionBuilder, state);
errorMessage, suggestTreeIfCastToNonNull, descriptionBuilder, state, nonNullTarget);
} else {
return createErrorDescription(
errorMessage, suppressibleNode(state.getPath()), descriptionBuilder, state);
errorMessage,
suppressibleNode(state.getPath()),
descriptionBuilder,
state,
nonNullTarget);
}
}

Expand Down Expand Up @@ -311,21 +337,40 @@ private Description.Builder removeCastToNonNullFix(
return builder.addFix(fix);
}

/**
* Reports initialization errors where a constructor fails to guarantee non-fields are initialized
* along all paths at exit points.
*
* @param methodSymbol Constructor symbol.
* @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) {
Description.Builder descriptionBuilder,
ImmutableList<Symbol> nonNullFields) {
// Check needed here, despite check in hasPathSuppression because initialization
// checking happens at the class-level (meaning state.getPath() might not include the
// method itself).
if (symbolHasSuppressWarningsAnnotation(methodSymbol, INITIALIZATION_CHECK_NAME)) {
return;
}
Tree methodTree = getTreesInstance(state).getTree(methodSymbol);
ErrorMessage errorMessage = new ErrorMessage(METHOD_NO_INIT, message);
state.reportMatch(
createErrorDescription(
new ErrorMessage(METHOD_NO_INIT, message), methodTree, descriptionBuilder, state));
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 Expand Up @@ -429,14 +474,16 @@ void reportInitErrorOnField(Symbol symbol, VisitorState state, Description.Build
FIELD_NO_INIT, "@NonNull static field " + fieldName + " not initialized"),
tree,
builder,
state));
state,
symbol));
} else {
state.reportMatch(
createErrorDescription(
new ErrorMessage(FIELD_NO_INIT, "@NonNull field " + fieldName + " not initialized"),
tree,
builder,
state));
state,
symbol));
}
}
}
8 changes: 8 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,12 @@ public enum MessageTypes {
WRONG_OVERRIDE_POSTCONDITION,
WRONG_OVERRIDE_PRECONDITION,
}

public String getMessage() {
return message;
}

public MessageTypes getMessageType() {
return messageType;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

import com.google.common.collect.ImmutableSet;
import com.google.errorprone.ErrorProneFlags;
import com.uber.nullaway.fixserialization.FixSerializationConfig;
import java.util.Collections;
import java.util.LinkedHashSet;
import java.util.Optional;
Expand Down Expand Up @@ -74,6 +75,11 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
static final String FL_JI_REGEX_MODEL_PATH = EP_FL_NAMESPACE + ":JarInferRegexStripModelJar";
static final String FL_JI_REGEX_CODE_PATH = EP_FL_NAMESPACE + ":JarInferRegexStripCodeJar";
static final String FL_ERROR_URL = EP_FL_NAMESPACE + ":ErrorURL";
/** --- Serialization configs --- */
static final String FL_FIX_SERIALIZATION = EP_FL_NAMESPACE + ":SerializeFixMetadata";

static final String FL_FIX_SERIALIZATION_CONFIG_PATH =
EP_FL_NAMESPACE + ":FixSerializationConfigPath";

private static final String DELIMITER = ",";

Expand Down Expand Up @@ -200,6 +206,35 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
+ FL_ACKNOWLEDGE_RESTRICTIVE
+ " is also set");
}
serializationActivationFlag = flags.getBoolean(FL_FIX_SERIALIZATION).orElse(false);
Optional<String> fixSerializationConfigPath = flags.get(FL_FIX_SERIALIZATION_CONFIG_PATH);
if (serializationActivationFlag && !fixSerializationConfigPath.isPresent()) {
throw new IllegalStateException(
"DO NOT report an issue to Error Prone for this crash! NullAway Fix Serialization configuration is "
+ "incorrect. "
+ "Must specify AutoFixer Output Directory, using the "
+ "-XepOpt:"
+ FL_FIX_SERIALIZATION_CONFIG_PATH
+ " flag. If you feel you have gotten this message in error report an issue"
+ " at https://github.com/uber/NullAway/issues.");
}
/*
* if fixSerializationActivationFlag is false, the default constructor is invoked for
* creating FixSerializationConfig which all features are deactivated. This lets the
* field be @Nonnull, allowing us to avoid null checks in various places.
*/
fixSerializationConfig =
serializationActivationFlag
? new FixSerializationConfig(fixSerializationConfigPath.get())
: new FixSerializationConfig();
if (serializationActivationFlag && isSuggestSuppressions) {
throw new IllegalStateException(
"In order to activate Fix Serialization mode ("
+ FL_FIX_SERIALIZATION
+ "), Suggest Suppressions mode must be deactivated ("
+ FL_SUGGEST_SUPPRESSIONS
+ ")");
}
}

private static ImmutableSet<String> getFlagStringSet(ErrorProneFlags flags, String flagName) {
Expand Down
Loading

0 comments on commit 9dbdd65

Please sign in to comment.