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

Update Library Model Loader to mark fields Nullable #220

Merged
merged 15 commits into from
Dec 28, 2023
Merged
2 changes: 1 addition & 1 deletion annotator-core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ googleJavaFormat {
}

// Should be the latest supporting version of NullAway.
def NULLAWAY_TEST = "0.10.10"
def NULLAWAY_TEST = "0.10.19-SNAPSHOT"

tasks.test.dependsOn(':annotator-scanner:publishToMavenLocal')

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,9 @@ public class Config {
/** Sets of context path information for all downstream dependencies. */
public final ImmutableSet<ModuleConfiguration> downstreamConfigurations;
/**
* Path to NullAway library model loader, which enables the communication between annotator and
* NullAway when processing downstream dependencies.
* Path to NullAway library model loader resource directory, which enables the communication
* between annotator and NullAway when processing downstream dependencies. Annotator will write
* annotation on files in this directory and the using Library Model Loader reads them.
*/
public final Path nullawayLibraryModelLoaderPath;
/** Command to build the all downstream dependencies at once. */
Expand Down Expand Up @@ -299,7 +300,7 @@ public Config(String[] args) {
"nlmlp",
"nullaway-library-model-loader-path",
true,
"NullAway Library Model loader path");
"NullAway Library Model loader resource directory path");
nullawayLibraryModelLoaderPathOption.setRequired(false);
options.addOption(nullawayLibraryModelLoaderPathOption);
// Down stream analysis: Analysis mode.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@

package edu.ucr.cs.riple.core.injectors;

import com.google.common.base.Function;
import com.google.common.base.Preconditions;
import edu.ucr.cs.riple.core.Config;
import edu.ucr.cs.riple.core.Context;
import edu.ucr.cs.riple.core.util.Utility;
import edu.ucr.cs.riple.injector.changes.AddAnnotation;
import edu.ucr.cs.riple.injector.changes.RemoveAnnotation;
import java.io.BufferedOutputStream;
Expand All @@ -36,6 +38,7 @@
import java.nio.file.Path;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

/**
* Wrapper tool used to inject annotations virtually to the source code. This injector serializes
Expand All @@ -51,6 +54,10 @@ public class VirtualInjector extends AnnotationInjector {
* retrieve the path to library model loader.
*/
private final Config config;
/** Name of the resource file in library model loader which contains list of nullable methods. */
public static final String NULLABLE_METHOD_LIST_FILE_NAME = "nullable-methods.tsv";
/** Name of the resource file in library model loader which contains list of nullable fields. */
public static final String NULLABLE_FIELD_LIST_FILE_NAME = "nullable-fields.tsv";

public VirtualInjector(Context context) {
super(context);
Expand All @@ -75,33 +82,57 @@ public void injectAnnotations(Set<AddAnnotation> changes) {
throw new IllegalStateException(
"Downstream dependencies analysis not activated, cannot inject annotations virtually!");
}
try (BufferedOutputStream os =
new BufferedOutputStream(new FileOutputStream(libraryModelPath.toFile()))) {
Set<String> rows =
changes.stream()
.filter(addAnnotation -> addAnnotation.getLocation().isOnMethod())
.map(
annot ->
annot.getLocation().clazz
+ "\t"
+ annot.getLocation().toMethod().method
+ "\n")
.collect(Collectors.toSet());
// write methods
writeAnnotationsToFile(
changes.stream().filter(addAnnotation -> addAnnotation.getLocation().isOnMethod()),
libraryModelPath.resolve(NULLABLE_METHOD_LIST_FILE_NAME),
annot ->
Stream.of(
annot.getLocation().clazz + "\t" + annot.getLocation().toMethod().method + "\n"));
// write fields
writeAnnotationsToFile(
changes.stream().filter(addAnnotation -> addAnnotation.getLocation().isOnField()),
libraryModelPath.resolve(NULLABLE_FIELD_LIST_FILE_NAME),
annot ->
// An annotation on a single statement with multiple declaration will be considered for
// all declared variables. Hence, we have to mark all variables as nullable.
// E.g. for
// class Foo {
// @Nullable Object a, b;
// }
// we have to consider both a and b be nullable and write each one
// ("Foo\ta" and "Foo\tb")
// on a separate line.
annot.getLocation().toField().variables.stream()
.map(variable -> annot.getLocation().clazz + "\t" + variable + "\n"));
}

/**
* Writes the passed annotation to the passed file. It uses the passed mapper to map the
* annotation to a string. And writes each string to a separate line.
nimakarimipour marked this conversation as resolved.
Show resolved Hide resolved
*
* @param annotations Annotations to be written.
* @param path Path to the file to be written.
* @param mapper Mapper to map the annotation to a string.
nimakarimipour marked this conversation as resolved.
Show resolved Hide resolved
*/
private static void writeAnnotationsToFile(
Stream<AddAnnotation> annotations,
Path path,
Function<AddAnnotation, Stream<String>> mapper) {
try (BufferedOutputStream os = new BufferedOutputStream(new FileOutputStream(path.toFile()))) {
Set<String> rows = annotations.flatMap(mapper).collect(Collectors.toSet());
for (String row : rows) {
os.write(row.getBytes(Charset.defaultCharset()), 0, row.length());
}
os.flush();
Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal, but I don't think this call is needed, since the stream will immediately be closed afterward

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, fixed in f775354

Copy link
Member

Choose a reason for hiding this comment

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

I still see the call to os.flush()? Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the call to os.flush() redundant (or guaranteed to be executed after the block) in try-with-resources block ? IntelliJ only grays out call to os.close() but not for os.flush().

Copy link
Member

Choose a reason for hiding this comment

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

try-with-resources guarantees the stream will be closed, and I believe it will be flushed automatically before it is closed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok thanks for that. Then we can remove it. Fixed in 24f0a24

} catch (IOException e) {
throw new RuntimeException("Error happened for writing at file: " + libraryModelPath, e);
throw new RuntimeException("Error happened for writing at file: " + path, e);
}
}

/** Removes any existing entry from library models. */
private void clear() {
try {
new FileOutputStream(libraryModelPath.toFile()).close();
} catch (IOException e) {
throw new RuntimeException("Could not clear library model loader content", e);
}
Utility.clearFileContentsAtPath(libraryModelPath.resolve(NULLABLE_FIELD_LIST_FILE_NAME));
Utility.clearFileContentsAtPath(libraryModelPath.resolve(NULLABLE_METHOD_LIST_FILE_NAME));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardOpenOption;
import java.time.Duration;
import java.time.temporal.ChronoUnit;
import java.util.Collections;
Expand Down Expand Up @@ -82,6 +83,23 @@ public static void executeCommand(Config config, String command) {
}
}

/**
* Clears the content of the file at the given path if the file exists.
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you just delete the file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, that sounds better. Fixed in 6dd203e

*
* @param path Path to the file.
*/
public static void clearFileContentsAtPath(Path path) {
if (Files.exists(path)) {
try (BufferedWriter writer =
Files.newBufferedWriter(
path, Charset.defaultCharset(), StandardOpenOption.TRUNCATE_EXISTING)) {
writer.flush();
} catch (IOException e) {
throw new RuntimeException("Could not clear content of file: " + path, e);
}
}
}

/**
* Writes reports content in json format in reports.json file in the output directory.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -415,15 +415,7 @@ public void makeAnnotatorConfigFile(Path configPath) {
Utility.getPathToLibraryModel(outDirPath)
.resolve(
Paths.get(
"src",
"main",
"resources",
"edu",
"ucr",
"cs",
"riple",
"librarymodel",
"nullable-methods.tsv"));
"src", "main", "resources", "edu", "ucr", "cs", "riple", "librarymodel"));
} else {
builder.buildCommand = projectBuilder.computeTargetBuildCommand(this.outDirPath);
}
Expand Down
2 changes: 1 addition & 1 deletion gradle/dependencies.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def versions = [
commonsio : "2.11.0",
progressbar : "0.9.2",
junit : "5.7.2",
nullaway : "0.9.8",
nullaway : "0.10.19-SNAPSHOT",
mockito : "5.2.0",
]

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,15 @@

package edu.ucr.cs.riple.librarymodel;

import static com.uber.nullaway.LibraryModels.FieldRef.fieldRef;
import static com.uber.nullaway.LibraryModels.MethodRef.methodRef;

import com.google.auto.service.AutoService;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSetMultimap;
import com.uber.nullaway.LibraryModels;
import com.uber.nullaway.handlers.stream.StreamTypeRecord;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
Expand All @@ -38,21 +41,28 @@
public class LibraryModelLoader implements LibraryModels {

public final String NULLABLE_METHOD_LIST_FILE_NAME = "nullable-methods.tsv";
public final String NULLABLE_FIELD_LIST_FILE_NAME = "nullable-fields.tsv";
public final ImmutableSet<MethodRef> nullableMethods;
public final ImmutableSet<FieldRef> nullableFields;

// Assuming this constructor will be called when picked by service loader
public LibraryModelLoader() {
this.nullableMethods = parseTSVFileFromResourcesToMethodRef(NULLABLE_METHOD_LIST_FILE_NAME);
this.nullableMethods =
parseTSVFileFromResourcesToMemberRef(
NULLABLE_METHOD_LIST_FILE_NAME, values -> methodRef(values[0], values[1]));
this.nullableFields =
parseTSVFileFromResourcesToMemberRef(
NULLABLE_FIELD_LIST_FILE_NAME, values -> fieldRef(values[0], values[1]));
}

/**
* Loads a file from resources and parses the content into set of {@link
* com.uber.nullaway.LibraryModels.MethodRef}.
* Loads a file from resources and creates an instance of type T from each line of the file.
*
* @param name File name in resources.
* @return ImmutableSet of content in the passed file. Returns empty if the file does not exist.
* @return ImmutableSet of contents in the file. Returns empty if the file does not exist.
*/
private ImmutableSet<MethodRef> parseTSVFileFromResourcesToMethodRef(String name) {
private <T> ImmutableSet<T> parseTSVFileFromResourcesToMemberRef(
msridhar marked this conversation as resolved.
Show resolved Hide resolved
String name, Factory<T> factory) {
// Check if resource exists
if (getClass().getResource(name) == null) {
return ImmutableSet.of();
Expand All @@ -61,13 +71,13 @@ private ImmutableSet<MethodRef> parseTSVFileFromResourcesToMethodRef(String name
if (is == null) {
return ImmutableSet.of();
}
ImmutableSet.Builder<MethodRef> contents = ImmutableSet.builder();
ImmutableSet.Builder<T> contents = ImmutableSet.builder();
BufferedReader reader =
new BufferedReader(new InputStreamReader(is, Charset.defaultCharset()));
String line = reader.readLine();
while (line != null) {
String[] values = line.split("\\t");
contents.add(methodRef(values[0], values[1]));
contents.add(factory.create(values));
line = reader.readLine();
}
return contents.build();
Expand Down Expand Up @@ -120,4 +130,29 @@ public ImmutableSet<MethodRef> nonNullReturns() {
public ImmutableSetMultimap<MethodRef, Integer> castToNonNullMethods() {
return ImmutableSetMultimap.of();
}

@Override
public ImmutableSet<FieldRef> nullableFields() {
return nullableFields;
}

@Override
public ImmutableList<StreamTypeRecord> customStreamNullabilitySpecs() {
return LibraryModels.super.customStreamNullabilitySpecs();
}
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this override unnecessary? It just calls the super method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this, I really don't know what I was thinking 🤦🏻‍♂️ de61967


/**
* Factory interface for creating an instance of type T from a string array.
*
* @param <T> Type of the instance to create.
*/
interface Factory<T> {
msridhar marked this conversation as resolved.
Show resolved Hide resolved
/**
* Creates an instance of type T from a string array.
*
* @param values String array to create the instance from.
* @return An instance of type T.
*/
T create(String[] values);
}
}
Loading