-
Notifications
You must be signed in to change notification settings - Fork 284
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
Fix new Error Prone 2.4.0 detected issues. #403
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
|
@@ -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; | ||||||
|
@@ -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, | ||||||
|
@@ -279,8 +278,7 @@ 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(); | ||||||
for (JarEntry jarEntry : inputJar.stream().collect(ImmutableList.toImmutableList())) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here and below: the list collection (= copy) can be avoided e.g. as follows:
Suggested change
This could be relevant when dealing with very large JAR files. (Due to generics this trick doesn't seem to quite work for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah very cool! I didn't know you could convert streams to iterators (and iterables) so easily! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Just so you know: aware of this note and planning to do a follow up PR, but I haven't had a chance :) Unless someone else wants to go for it, I'll try to follow up early next week) |
||||||
InputStream is = inputJar.getInputStream(jarEntry); | ||||||
copyAndAnnotateJarEntry( | ||||||
jarEntry, | ||||||
|
@@ -305,7 +303,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, | ||||||
|
@@ -317,9 +314,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")) { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,6 @@ | |
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; | ||
|
@@ -40,20 +39,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()); | ||
} | ||
jar1.stream().forEach(entry -> jar1Entries.add(entry.getName())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I would do something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
JarFile jar2 = new JarFile(jarFile2); | ||
Set<String> jar2Entries = new HashSet<>(); | ||
for (Enumeration<JarEntry> entries = jar2.entries(); entries.hasMoreElements(); ) { | ||
jar2Entries.add(entries.nextElement().getName()); | ||
} | ||
jar2.stream().forEach(entry -> jar2Entries.add(entry.getName())); | ||
return jar1Entries.equals(jar2Entries); | ||
} | ||
|
||
|
@@ -67,20 +61,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()); | ||
} | ||
zip1.stream().forEach(entry -> zip1Entries.add(entry.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()); | ||
} | ||
zip2.stream().forEach(entry -> zip2Entries.add(entry.getName())); | ||
if (!zip1Entries.equals(zip2Entries)) { | ||
return false; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In these cases can we do a
forEach()
on the stream rather than collecting in a list and looping over that? That seems a bit cleaner and avoids copying to another list.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we can't. The code bellow can throw
IOException
. If that happens within a lambda, we need to catch it there, if it happens within an iteration we can propagate it up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a code comment to that effect? Or not if you think it's obvious, up to you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also done 😄 (before I saw your comment, but not sure if before you made it...)