Skip to content

Commit

Permalink
Fix new Error Prone 2.4.0 detected issues. (#403)
Browse files Browse the repository at this point in the history 
This removes the suppressions introduced in #400 and makes more permanent fixes.

Fixes #402.
  • Loading branch information
@lazaroclapp
lazaroclapp committed Jun 24, 2020
1 parent bf3de0f commit 7f9aacb
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 47 deletions.
17 changes: 9 additions & 8 deletions jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/BytecodeAnnotator.java
View file
Expand Up @@ -15,6 +15,7 @@
*/
package com.uber.nullaway.jarinfer;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import java.io.BufferedReader;
Expand All @@ -23,7 +24,6 @@
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.OutputStream;
import java.util.Enumeration;
import java.util.List;
import java.util.Set;
import java.util.jar.JarEntry;
Expand Down Expand Up @@ -264,7 +264,6 @@ private static void copyAndAnnotateJarEntry(
* @param debug flag to output debug logs.
* @throws IOException if an error happens when reading or writing to jar or class streams.
*/
@SuppressWarnings("JdkObsolete")
public static void annotateBytecodeInJar(
JarFile inputJar,
JarOutputStream jarOS,
Expand All @@ -279,8 +278,9 @@ public static void annotateBytecodeInJar(
// Do not use JarInputStream in place of JarFile/JarEntry. JarInputStream misses MANIFEST.MF
// while iterating over the entries in the stream.
// Reference: https://bugs.openjdk.java.net/browse/JDK-8215788
for (Enumeration<JarEntry> entries = inputJar.entries(); entries.hasMoreElements(); ) {
JarEntry jarEntry = entries.nextElement();
// Note: we can't just put the code below inside stream().forach(), because it can throw
// IOException.
for (JarEntry jarEntry : inputJar.stream().collect(ImmutableList.toImmutableList())) {
InputStream is = inputJar.getInputStream(jarEntry);
copyAndAnnotateJarEntry(
jarEntry,
Expand All @@ -305,7 +305,6 @@ public static void annotateBytecodeInJar(
* @param debug flag to output debug logs.
* @throws IOException if an error happens when reading or writing to AAR/JAR/class streams.
*/
@SuppressWarnings("JdkObsolete")
public static void annotateBytecodeInAar(
ZipFile inputZip,
ZipOutputStream zipOS,
Expand All @@ -317,9 +316,11 @@ public static void annotateBytecodeInAar(
BytecodeAnnotator.debug = debug;
LOG(debug, "DEBUG", "nullableReturns: " + nullableReturns);
LOG(debug, "DEBUG", "nonnullParams: " + nonnullParams);
for (Enumeration<? extends ZipEntry> entries = inputZip.entries();
entries.hasMoreElements(); ) {
ZipEntry zipEntry = entries.nextElement();
// Error Prone doesn't like usages of the old Java Enumerator APIs. ZipFile does not implement
// Iterable, and likely
// never will (see https://bugs.openjdk.java.net/browse/JDK-6581715). So this seems like the
// best remaining way:
for (ZipEntry zipEntry : inputZip.stream().collect(ImmutableList.toImmutableList())) {
InputStream is = inputZip.getInputStream(zipEntry);
zipOS.putNextEntry(new ZipEntry(zipEntry.getName()));
if (zipEntry.getName().equals("classes.jar")) {
Expand Down
12 changes: 3 additions & 9 deletions jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/AnnotationChecker.java
View file
Expand Up @@ -16,9 +16,9 @@
package com.uber.nullaway.jarinfer;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import java.io.IOException;
import java.io.InputStream;
import java.util.Enumeration;
import java.util.List;
import java.util.Map;
import java.util.jar.JarEntry;
Expand Down Expand Up @@ -50,14 +50,11 @@ public class AnnotationChecker {
* 'Expect*' annotations are present.
* @throws IOException if an error happens when reading the AAR file.
*/
@SuppressWarnings("JdkObsolete")
public static boolean checkMethodAnnotationsInAar(
String aarFile, Map<String, String> expectedToActualAnnotations) throws IOException {
Preconditions.checkArgument(aarFile.endsWith(".aar"), "invalid aar file: " + aarFile);
ZipFile zip = new ZipFile(aarFile);
Enumeration<? extends ZipEntry> zipEntries = zip.entries();
while (zipEntries.hasMoreElements()) {
ZipEntry zipEntry = zipEntries.nextElement();
for (ZipEntry zipEntry : zip.stream().collect(ImmutableList.toImmutableList())) {
if (zipEntry.getName().equals("classes.jar")) {
JarInputStream jarIS = new JarInputStream(zip.getInputStream(zipEntry));
JarEntry jarEntry = jarIS.getNextJarEntry();
Expand Down Expand Up @@ -86,14 +83,11 @@ public static boolean checkMethodAnnotationsInAar(
* 'Expect*' annotations are present.
* @throws IOException if an error happens when reading the jar file.
*/
@SuppressWarnings("JdkObsolete")
public static boolean checkMethodAnnotationsInJar(
String jarFile, Map<String, String> expectedToActualAnnotations) throws IOException {
Preconditions.checkArgument(jarFile.endsWith(".jar"), "invalid jar file: " + jarFile);
JarFile jar = new JarFile(jarFile);
Enumeration<JarEntry> entries = jar.entries();
while (entries.hasMoreElements()) {
JarEntry entry = entries.nextElement();
for (JarEntry entry : jar.stream().collect(ImmutableList.toImmutableList())) {
if (entry.getName().endsWith(".class")
&& !checkMethodAnnotationsInClass(
jar.getInputStream(entry), expectedToActualAnnotations)) {
Expand Down
28 changes: 9 additions & 19 deletions jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/EntriesComparator.java
View file
Expand Up @@ -16,10 +16,10 @@
package com.uber.nullaway.jarinfer;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.Enumeration;
import java.util.HashSet;
import java.util.Set;
import java.util.jar.JarEntry;
Expand All @@ -40,20 +40,15 @@ public class EntriesComparator {
* @return True iff the entries present in the two jar files are the same.
* @throws IOException if an error happens when reading jar files.
*/
@SuppressWarnings("JdkObsolete")
public static boolean compareEntriesInJars(String jarFile1, String jarFile2) throws IOException {
Preconditions.checkArgument(jarFile1.endsWith(".jar"), "invalid jar file: " + jarFile1);
Preconditions.checkArgument(jarFile2.endsWith(".jar"), "invalid jar file: " + jarFile2);
JarFile jar1 = new JarFile(jarFile1);
Set<String> jar1Entries = new HashSet<>();
for (Enumeration<JarEntry> entries = jar1.entries(); entries.hasMoreElements(); ) {
jar1Entries.add(entries.nextElement().getName());
}
JarFile jar2 = new JarFile(jarFile2);
Set<String> jar2Entries = new HashSet<>();
for (Enumeration<JarEntry> entries = jar2.entries(); entries.hasMoreElements(); ) {
jar2Entries.add(entries.nextElement().getName());
}
Set<String> jar1Entries =
jar1.stream().map(ZipEntry::getName).collect(ImmutableSet.toImmutableSet());
Set<String> jar2Entries =
jar2.stream().map(ZipEntry::getName).collect(ImmutableSet.toImmutableSet());
return jar1Entries.equals(jar2Entries);
}

Expand All @@ -67,20 +62,15 @@ public static boolean compareEntriesInJars(String jarFile1, String jarFile2) thr
* "classes.jar" in the two aar files are the same.
* @throws IOException if an error happens when reading aar files.
*/
@SuppressWarnings("JdkObsolete")
public static boolean compareEntriesInAars(String aarFile1, String aarFile2) throws IOException {
Preconditions.checkArgument(aarFile1.endsWith(".aar"), "invalid aar file: " + aarFile1);
Preconditions.checkArgument(aarFile2.endsWith(".aar"), "invalid aar file: " + aarFile2);
ZipFile zip1 = new ZipFile(aarFile1);
Set<String> zip1Entries = new HashSet<>();
for (Enumeration<? extends ZipEntry> entries = zip1.entries(); entries.hasMoreElements(); ) {
zip1Entries.add(entries.nextElement().getName());
}
ZipFile zip2 = new ZipFile(aarFile2);
Set<String> zip2Entries = new HashSet<>();
for (Enumeration<? extends ZipEntry> entries = zip2.entries(); entries.hasMoreElements(); ) {
zip2Entries.add(entries.nextElement().getName());
}
Set<String> zip1Entries =
zip1.stream().map(ZipEntry::getName).collect(ImmutableSet.toImmutableSet());
Set<String> zip2Entries =
zip2.stream().map(ZipEntry::getName).collect(ImmutableSet.toImmutableSet());
if (!zip1Entries.equals(zip2Entries)) {
return false;
}
Expand Down
22 changes: 11 additions & 11 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Expand Up @@ -174,9 +174,7 @@ public class NullAway extends BugChecker
static final String INITIALIZATION_CHECK_NAME = "NullAway.Init";
static final String OPTIONAL_CHECK_NAME = "NullAway.Optional";

@SuppressWarnings("UnnecessaryLambda")
private static final Matcher<ExpressionTree> THIS_MATCHER =
(expressionTree, state) -> isThisIdentifier(expressionTree);
private static final Matcher<ExpressionTree> THIS_MATCHER = NullAway::isThisIdentifierMatcher;

private final Predicate<MethodInvocationNode> nonAnnotatedMethod;

Expand Down Expand Up @@ -233,15 +231,15 @@ public class NullAway extends BugChecker
public NullAway() {
config = new DummyOptionsConfig();
handler = Handlers.buildEmpty();
nonAnnotatedMethod = nonAnnotatedMethodCheck();
nonAnnotatedMethod = this::isMethodUnannotated;
customSuppressionAnnotations = ImmutableSet.of();
errorBuilder = new ErrorBuilder(config, "", ImmutableSet.of());
}

public NullAway(ErrorProneFlags flags) {
config = new ErrorProneCLIFlagsConfig(flags);
handler = Handlers.buildDefault(config);
nonAnnotatedMethod = nonAnnotatedMethodCheck();
nonAnnotatedMethod = this::isMethodUnannotated;
customSuppressionAnnotations = initCustomSuppressions();
errorBuilder = new ErrorBuilder(config, canonicalName(), allNames());
// workaround for Checker Framework static state bug;
Expand All @@ -263,12 +261,9 @@ private ImmutableSet<Class<? extends Annotation>> initCustomSuppressions() {
return builder.build();
}

@SuppressWarnings("UnnecessaryLambda")
private Predicate<MethodInvocationNode> nonAnnotatedMethodCheck() {
return invocationNode ->
invocationNode == null
|| NullabilityUtil.isUnannotated(
ASTHelpers.getSymbol(invocationNode.getTree()), config);
private boolean isMethodUnannotated(MethodInvocationNode invocationNode) {
return invocationNode == null
|| NullabilityUtil.isUnannotated(ASTHelpers.getSymbol(invocationNode.getTree()), config);
}

@Override
Expand Down Expand Up @@ -2102,6 +2097,11 @@ private static boolean isThisIdentifier(ExpressionTree expressionTree) {
&& ((IdentifierTree) expressionTree).getName().toString().equals("this");
}

private static boolean isThisIdentifierMatcher(
ExpressionTree expressionTree, VisitorState state) {
return isThisIdentifier(expressionTree);
}

public ErrorBuilder getErrorBuilder() {
return errorBuilder;
}
Expand Down

0 comments on commit 7f9aacb

Please sign in to comment.