-
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 all commits
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,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())) { | ||||||
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 +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, | ||||||
|
@@ -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")) { | ||||||
|
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...)