From 535772a5075c99aaa794aa847a19884d0f319041 Mon Sep 17 00:00:00 2001 From: Nima Karimipour Date: Fri, 13 Jan 2023 21:32:26 -0800 Subject: [PATCH] [FixSerialization] Update path serialization for reported errors and fixes. (#714) This PR updates serialization services to serialize the actual `Path` instead of `URI` for all reporting errors and fixes. --- .../nullaway/fixserialization/Serializer.java | 31 +++++++++++++ .../adapters/SerializationV2Adapter.java | 6 +-- .../location/AbstractSymbolLocation.java | 14 ++++-- .../location/FieldLocation.java | 2 +- .../location/MethodLocation.java | 2 +- .../location/MethodParameterLocation.java | 2 +- .../fixserialization/out/ErrorInfo.java | 19 ++++---- .../nullaway/NullAwaySerializationTest.java | 43 +++++++++++------- .../com/uber/nullaway/tools/ErrorDisplay.java | 32 ++++++++----- .../uber/nullaway/tools/FieldInitDisplay.java | 14 +++--- .../com/uber/nullaway/tools/FixDisplay.java | 14 +++--- .../tools/SerializationTestHelper.java | 45 +++++++++++++++++++ .../tools/version1/ErrorDisplayV1.java | 14 +++--- 13 files changed, 171 insertions(+), 67 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java index 20ddf3d185..bff571ee01 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java @@ -31,6 +31,7 @@ import java.io.IOException; import java.io.OutputStream; import java.io.Writer; +import java.net.URI; import java.nio.charset.Charset; import java.nio.file.Files; import java.nio.file.Path; @@ -164,4 +165,34 @@ private void appendToFile(String row, Path path) { throw new RuntimeException("Error happened for writing at file: " + path, e); } } + + /** + * Converts the given uri to the real path. Note, in NullAway CI tests, source files exists in + * memory and there is no real path leading to those files. Instead, we just serialize the path + * from uri as the full paths are not checked in tests. + * + * @param uri Given uri. + * @return Real path for the give uri. + */ + @Nullable + public static Path pathToSourceFileFromURI(@Nullable URI uri) { + if (uri == null) { + return null; + } + if ("jimfs".equals(uri.getScheme())) { + // In NullAway unit tests, files are stored in memory and have this scheme. + return Paths.get(uri); + } + if (!"file".equals(uri.getScheme())) { + return null; + } + Path path = Paths.get(uri); + try { + return path.toRealPath(); + } catch (IOException e) { + // In this case, we still would like to continue the serialization instead of returning null + // and not serializing anything. + return path; + } + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV2Adapter.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV2Adapter.java index 278fe068cf..aa12587080 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV2Adapter.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV2Adapter.java @@ -51,13 +51,13 @@ public String getErrorsOutputFileHeader() { "enc_class", "enc_member", "offset", - "uri", + "path", "target_kind", "target_class", "target_method", "target_param", "target_index", - "target_uri"); + "target_path"); } @Override @@ -71,7 +71,7 @@ public String serializeError(ErrorInfo errorInfo) { : "null"), (errorInfo.getRegionMember() != null ? errorInfo.getRegionMember().toString() : "null"), String.valueOf(errorInfo.getOffset()), - errorInfo.getUri().getPath(), + errorInfo.getPath() != null ? errorInfo.getPath().toString() : "null", (errorInfo.getNonnullTarget() != null ? SymbolLocation.createLocationFromSymbol(errorInfo.getNonnullTarget()) .tabSeparatedToString() diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/AbstractSymbolLocation.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/AbstractSymbolLocation.java index 5c1e87bf31..da31bf7666 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/AbstractSymbolLocation.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/AbstractSymbolLocation.java @@ -27,7 +27,9 @@ import com.google.common.base.Preconditions; import com.google.errorprone.util.ASTHelpers; import com.sun.tools.javac.code.Symbol; +import com.uber.nullaway.fixserialization.Serializer; import java.net.URI; +import java.nio.file.Path; import javax.annotation.Nullable; import javax.lang.model.element.ElementKind; @@ -36,8 +38,8 @@ public abstract class AbstractSymbolLocation implements SymbolLocation { /** Element kind of the targeted symbol */ protected final ElementKind type; - /** URI of the file containing the symbol, if available. */ - @Nullable protected final URI uri; + /** Path of the file containing the symbol, if available. */ + @Nullable protected final Path path; /** Enclosing class of the symbol. */ protected final Symbol.ClassSymbol enclosingClass; @@ -51,9 +53,15 @@ public AbstractSymbolLocation(ElementKind type, Symbol target) { + "."); this.type = type; this.enclosingClass = castToNonNull(ASTHelpers.enclosingClass(target)); - this.uri = + // We currently serialize the URI for the classfile if the URI for the sourcefile is not + // available, but only if said URI corresponds to a "file:" or "jimfs:" scheme (i.e. not + // "jar:"). It's likely that this is no longer needed and should be removed as a follow up: + // https://github.com/uber/NullAway/issues/716 Leaving this workaround up temporarily for the + // sake of experiments with version `1.3.6-alpha-N` of the auto-annotator. + URI pathInURI = enclosingClass.sourcefile != null ? enclosingClass.sourcefile.toUri() : (enclosingClass.classfile != null ? enclosingClass.classfile.toUri() : null); + this.path = Serializer.pathToSourceFileFromURI(pathInURI); } } diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/FieldLocation.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/FieldLocation.java index 571f3ef510..533b921492 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/FieldLocation.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/FieldLocation.java @@ -45,6 +45,6 @@ public String tabSeparatedToString() { "null", variableSymbol.toString(), "null", - uri != null ? uri.toASCIIString() : "null"); + path != null ? path.toString() : "null"); } } diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodLocation.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodLocation.java index c511a4e52e..1b23fc2d58 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodLocation.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodLocation.java @@ -45,6 +45,6 @@ public String tabSeparatedToString() { enclosingMethod.toString(), "null", "null", - uri != null ? uri.toASCIIString() : "null"); + path != null ? path.toString() : "null"); } } diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodParameterLocation.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodParameterLocation.java index 381479019b..95dea975ff 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodParameterLocation.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodParameterLocation.java @@ -66,6 +66,6 @@ public String tabSeparatedToString() { enclosingMethod.toString(), paramSymbol.toString(), String.valueOf(index), - uri != null ? uri.toASCIIString() : "null"); + path != null ? path.toString() : "null"); } } diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java index a0ca6ee703..e6311c2d94 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java @@ -30,7 +30,8 @@ import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.util.JCDiagnostic; import com.uber.nullaway.ErrorMessage; -import java.net.URI; +import com.uber.nullaway.fixserialization.Serializer; +import java.nio.file.Path; import javax.annotation.Nullable; /** Stores information regarding an error which will be reported by NullAway. */ @@ -53,8 +54,8 @@ public class ErrorInfo { /** Offset of program point where this error is reported. */ private final int offset; - /** Uri to the containing source file where this error is reported. */ - private final URI uri; + /** Path to the containing source file where this error is reported. */ + @Nullable private final Path path; public ErrorInfo( TreePath path, Tree errorTree, ErrorMessage errorMessage, @Nullable Symbol nonnullTarget) { @@ -67,7 +68,8 @@ public ErrorInfo( this.nonnullTarget = nonnullTarget; JCDiagnostic.DiagnosticPosition treePosition = (JCDiagnostic.DiagnosticPosition) errorTree; this.offset = treePosition.getStartPosition(); - this.uri = path.getCompilationUnit().getSourceFile().toUri(); + this.path = + Serializer.pathToSourceFileFromURI(path.getCompilationUnit().getSourceFile().toUri()); } /** @@ -121,12 +123,13 @@ public int getOffset() { } /** - * Returns URI to the containing source file where this error is reported. + * Returns Path to the containing source file where this error is reported. * - * @return URI to the containing source file where this error is reported. + * @return Path to the containing source file where this error is reported. */ - public URI getUri() { - return uri; + @Nullable + public Path getPath() { + return path; } /** Finds the class and member of program point where the error is reported. */ diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java index 6631b22a75..45d838863a 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java @@ -79,12 +79,13 @@ public NullAwaySerializationTest() { "Needs exactly 10 values to create FixDisplay object but found: " + values.length); // Fixes are written in Temp Directory and is not known at compile time, therefore, // relative paths are getting compared. - FixDisplay display = - new FixDisplay(values[7], values[2], values[3], values[0], values[1], values[5]); - if (display.uri.contains("com/uber/")) { - display.uri = display.uri.substring(display.uri.indexOf("com/uber/")); - } - return display; + return new FixDisplay( + values[7], + values[2], + values[3], + values[0], + values[1], + SerializationTestHelper.getRelativePathFromUnitTestTempDirectory(values[5])); }; this.errorDisplayFactory = values -> { @@ -97,13 +98,13 @@ public NullAwaySerializationTest() { values[2], values[3], Integer.parseInt(values[4]), - values[5], + SerializationTestHelper.getRelativePathFromUnitTestTempDirectory(values[5]), values[6], values[7], values[8], values[9], values[10], - values[11]); + SerializationTestHelper.getRelativePathFromUnitTestTempDirectory(values[11])); }; this.fieldInitDisplayFactory = values -> { @@ -111,13 +112,13 @@ public NullAwaySerializationTest() { values.length == 7, "Needs exactly 7 values to create FieldInitDisplay object but found: " + values.length); - FieldInitDisplay display = - new FieldInitDisplay( - values[6], values[2], values[3], values[0], values[1], values[5]); - if (display.uri.contains("com/uber/")) { - display.uri = display.uri.substring(display.uri.indexOf("com/uber/")); - } - return display; + return new FieldInitDisplay( + values[6], + values[2], + values[3], + values[0], + values[1], + SerializationTestHelper.getRelativePathFromUnitTestTempDirectory(values[5])); }; } @@ -1865,8 +1866,16 @@ public void errorSerializationVersion1() { "Needs exactly 10 values to create ErrorDisplay for version 1 object but found: " + values.length); return new ErrorDisplayV1( - values[0], values[1], values[2], values[3], values[4], values[5], values[6], - values[7], values[8], values[9]); + values[0], + values[1], + values[2], + values[3], + values[4], + values[5], + values[6], + values[7], + values[8], + SerializationTestHelper.getRelativePathFromUnitTestTempDirectory(values[9])); }; SerializationTestHelper tester = new SerializationTestHelper<>(root); tester diff --git a/nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java b/nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java index 23f8fe1230..0c6d2b7363 100644 --- a/nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java +++ b/nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java @@ -39,7 +39,7 @@ public class ErrorDisplay implements Display { public final String method; public final String variable; public final String index; - public final String uri; + public final String nonElementPath; public ErrorDisplay( String type, @@ -53,21 +53,19 @@ public ErrorDisplay( String method, String variable, String index, - String uri) { + String nonElementPath) { this.type = type; this.message = message; this.encMember = encMember; this.encClass = encClass; this.offset = offset; - // relative paths are getting compared. - this.path = path.contains("com/uber/") ? path.substring(path.indexOf("com/uber/")) : path; + this.path = path; this.kind = kind; this.clazz = clazz; this.method = method; this.variable = variable; this.index = index; - // relative paths are getting compared. - this.uri = uri.contains("com/uber/") ? uri.substring(uri.indexOf("com/uber/")) : uri; + this.nonElementPath = nonElementPath; } public ErrorDisplay( @@ -94,19 +92,29 @@ public boolean equals(Object o) { && clazz.equals(that.clazz) && encClass.equals(that.encClass) && offset == that.offset - && path.equals(that.path) + && SerializationTestHelper.pathsAreEqual(path, that.path) && kind.equals(that.kind) && method.equals(that.method) && variable.equals(that.variable) && index.equals(that.index) - && uri.equals(that.uri); + && SerializationTestHelper.pathsAreEqual(nonElementPath, that.nonElementPath); } @Override public int hashCode() { return Objects.hash( - type, message, encMember, encClass, offset, path, kind, clazz, method, variable, index, - uri); + type, + message, + encMember, + encClass, + offset, + path, + kind, + clazz, + method, + variable, + index, + nonElementPath); } @Override @@ -145,8 +153,8 @@ public String toString() { + "\n\tindex='" + index + '\'' - + "\n\turi='" - + uri + + "\n\tnonElementPath='" + + nonElementPath + '\'' + '}'; } diff --git a/nullaway/src/test/java/com/uber/nullaway/tools/FieldInitDisplay.java b/nullaway/src/test/java/com/uber/nullaway/tools/FieldInitDisplay.java index 594e9ee915..1f31260709 100644 --- a/nullaway/src/test/java/com/uber/nullaway/tools/FieldInitDisplay.java +++ b/nullaway/src/test/java/com/uber/nullaway/tools/FieldInitDisplay.java @@ -34,16 +34,16 @@ public class FieldInitDisplay implements Display { public final String location; public final String className; public final String field; - public String uri; + public final String path; public FieldInitDisplay( - String field, String method, String param, String location, String className, String uri) { + String field, String method, String param, String location, String className, String path) { this.field = field; this.method = method; this.param = param; this.location = location; this.className = className; - this.uri = uri; + this.path = path; } @Override @@ -60,12 +60,12 @@ public boolean equals(Object o) { && Objects.equals(location, that.location) && Objects.equals(className, that.className) && Objects.equals(field, that.field) - && Objects.equals(uri, that.uri); + && SerializationTestHelper.pathsAreEqual(path, that.path); } @Override public int hashCode() { - return Objects.hash(method, param, location, className, field, uri); + return Objects.hash(method, param, location, className, field, path); } @Override @@ -86,8 +86,8 @@ public String toString() { + ", \n\tclassName='" + className + '\'' - + ", \n\turi='" - + uri + + ", \n\tpath='" + + path + '\'' + "\n }\n"; } diff --git a/nullaway/src/test/java/com/uber/nullaway/tools/FixDisplay.java b/nullaway/src/test/java/com/uber/nullaway/tools/FixDisplay.java index 005acea98e..213d8dff30 100644 --- a/nullaway/src/test/java/com/uber/nullaway/tools/FixDisplay.java +++ b/nullaway/src/test/java/com/uber/nullaway/tools/FixDisplay.java @@ -33,7 +33,7 @@ public class FixDisplay implements Display { public final String param; public final String location; public final String className; - public String uri; + public final String path; public FixDisplay( String annotation, @@ -41,13 +41,13 @@ public FixDisplay( String param, String location, String className, - String uri) { + String path) { this.annotation = annotation; this.method = method; this.param = param; this.location = location; this.className = className; - this.uri = uri; + this.path = path; } @Override @@ -68,8 +68,8 @@ public String toString() { + ", \n\tclassName='" + className + '\'' - + ", \n\turi='" - + uri + + ", \n\tpath='" + + path + '\'' + "\n }\n"; } @@ -88,11 +88,11 @@ public boolean equals(Object o) { && Objects.equals(param, fix.param) && Objects.equals(location, fix.location) && Objects.equals(className, fix.className) - && Objects.equals(uri, fix.uri); + && SerializationTestHelper.pathsAreEqual(path, fix.path); } @Override public int hashCode() { - return Objects.hash(annotation, method, param, location, className, uri); + return Objects.hash(annotation, method, param, location, className, path); } } diff --git a/nullaway/src/test/java/com/uber/nullaway/tools/SerializationTestHelper.java b/nullaway/src/test/java/com/uber/nullaway/tools/SerializationTestHelper.java index 0afa80bc6e..6a01999882 100644 --- a/nullaway/src/test/java/com/uber/nullaway/tools/SerializationTestHelper.java +++ b/nullaway/src/test/java/com/uber/nullaway/tools/SerializationTestHelper.java @@ -33,8 +33,10 @@ import java.nio.charset.Charset; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; public class SerializationTestHelper { @@ -157,4 +159,47 @@ private List readActualOutputs(Path outputPath) { } return outputs; } + + /** + * Checks if given paths are equal. Under different OS environments, identical paths might have a + * different string representation. In windows all forward slashes are replaced with backslashes. + * + * @param expected Expected serialized path. + * @param found Serialized path. + * @return true, if paths are identical. + */ + public static boolean pathsAreEqual(String expected, String found) { + if (found.equals(expected)) { + return true; + } + return found.replaceAll("\\\\", "/").equals(expected); + } + + /** + * Extracts relative path from the serialized full path. + * + * @param pathInString Full serialized path. + * @return Relative path to "com" from the given path including starting from "com" directory. + */ + public static String getRelativePathFromUnitTestTempDirectory(String pathInString) { + if (pathInString.equals("null")) { + return "null"; + } + // using atomic refs to use them inside inner class below. This is not due to any concurrent + // modifications. + AtomicReference relativePath = new AtomicReference<>(Paths.get("com")); + AtomicReference relativePathStarted = new AtomicReference<>(false); + Path path = Paths.get(pathInString); + path.iterator() + .forEachRemaining( + remaining -> { + if (relativePathStarted.get()) { + relativePath.set(relativePath.get().resolve(remaining)); + } + if (remaining.toString().startsWith("com")) { + relativePathStarted.set(true); + } + }); + return relativePath.get().toString(); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/tools/version1/ErrorDisplayV1.java b/nullaway/src/test/java/com/uber/nullaway/tools/version1/ErrorDisplayV1.java index 2cd5a86084..523efc6cc2 100644 --- a/nullaway/src/test/java/com/uber/nullaway/tools/version1/ErrorDisplayV1.java +++ b/nullaway/src/test/java/com/uber/nullaway/tools/version1/ErrorDisplayV1.java @@ -22,6 +22,7 @@ package com.uber.nullaway.tools.version1; import com.uber.nullaway.tools.Display; +import com.uber.nullaway.tools.SerializationTestHelper; import java.util.Objects; /** @@ -39,7 +40,7 @@ public class ErrorDisplayV1 implements Display { public final String method; public final String variable; public final String index; - public final String uri; + public final String path; public ErrorDisplayV1( String type, @@ -51,7 +52,7 @@ public ErrorDisplayV1( String method, String variable, String index, - String uri) { + String path) { this.type = type; this.message = message; this.encMember = encMember; @@ -61,8 +62,7 @@ public ErrorDisplayV1( this.method = method; this.variable = variable; this.index = index; - // relative paths are getting compared. - this.uri = uri.contains("com/uber/") ? uri.substring(uri.indexOf("com/uber/")) : uri; + this.path = path; } public ErrorDisplayV1(String type, String message, String encClass, String encMember) { @@ -89,13 +89,13 @@ public boolean equals(Object o) { && method.equals(that.method) && variable.equals(that.variable) && index.equals(that.index) - && uri.equals(that.uri); + && SerializationTestHelper.pathsAreEqual(path, that.path); } @Override public int hashCode() { return Objects.hash( - type, message, encMember, encClass, kind, clazz, method, variable, index, uri); + type, message, encMember, encClass, kind, clazz, method, variable, index, path); } @Override @@ -129,7 +129,7 @@ public String toString() { + index + '\'' + "\n\turi='" - + uri + + path + '\'' + '}'; }