Update to Error Prone 2.3.2

[msridhar]

Only a few fixes required.

I had to disable analysis of generated code since AutoValue-generated code doesn't pass the UnnecessaryParentheses check. I tried updating AutoValue to fix this, and decided to keep the update.

[lazaroclapp]

LGTM

[lazaroclapp]

Actually, I don't get why the tests are failing. It seems to be something about IllegalAccessError in this line: compilationHelper = CompilationTestHelper.newInstance(NullAway.class, getClass());

But, even on the current error-prone master, that method is public.

[msridhar]

Yeah, trying to figure it out

…

On Thu, Oct 11, 2018 at 1:17 PM Lázaro Clapp ***@***.***> wrote: Actually, I don't get why the tests are failing. It seems to be something about IllegalAccessError in this line: compilationHelper = CompilationTestHelper.newInstance(NullAway.class, getClass()); But, even on the current error-prone master, that method is public. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#242 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALyUfZFWTTzaeqA8G5Gujum2RrzCc9rks5uj6dRgaJpZM4XYRY8> .

[msridhar]

@lazaroclapp @subarnob what is going with passing -cp here?

NullAway/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java

Lines 1078 to 1079 in 237a6ef

	"-cp",
	"../jar-infer/test-java-lib-jarinfer/build/libs/test-java-lib-jarinfer.jar"))

I wonder if it's breaking these tests with this update by overriding the test classpath.

[lazaroclapp]

@lazaroclapp @subarnob what is going with passing -cp here?

NullAway/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java

Lines 1078 to 1079 in 237a6ef

"-cp",
"../jar-infer/test-java-lib-jarinfer/build/libs/test-java-lib-jarinfer.jar"))
I wonder if it's breaking these tests with this update by overriding the test classpath.

Not entirely sure.

This is already a test dependency for nullaway: https://github.com/uber/NullAway/blob/master/nullaway/build.gradle#L57 But removing the -cp argument causes the tests to fail with missing errors.

I believe the code that scans for jars with models here: https://github.com/uber/NullAway/blob/master/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java#L110 looks at the -cp (or similar) option of the javac command, and can't see the way in which CompilationTestHelper makes the outer classpath available, hence the requirement to explicitly add that jar on the javac command.

[msridhar]

I believe the code that scans for jars with models here: https://github.com/uber/NullAway/blob/master/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java#L110 looks at the -cp (or similar) option of the javac command, and can't see the way in which CompilationTestHelper makes the outer classpath available, hence the requirement to explicitly add that jar on the javac command.

Can we do something like this trick to avoid looking at -cp?

[lazaroclapp]

I believe the code that scans for jars with models here: https://github.com/uber/NullAway/blob/master/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java#L110 looks at the -cp (or similar) option of the javac command, and can't see the way in which CompilationTestHelper makes the outer classpath available, hence the requirement to explicitly add that jar on the javac command.

Can we do something like this trick to avoid looking at -cp?

👍 Oh, nice find!

It seems to work, at least as far as NullAway's own tests are concerned. See #243 . I'd still like to test this a bit internally before merging that PR, though :)

[msridhar]

Ok, cool, once #243 is merged I'll rebase on that and hopefully we'll be good to go with this one

[tbroyer]

Any reason for not using -XepDisableWarningsInGeneratedCode instead?

Also, does that mean that AutoValue-generated sources end up into build/classes? You may want to set options.annotationProcessorGeneratedSourcesDirectory on JavaCompile tasks to put them somewhere else so they're not packaged into the JAR. Fwiw, net.ltgt.apt will do that for you 😉
(I checked, and indeed nulaway-0.6.0.jar contains AUtoValue-generated sources)

[msridhar]

We need this since we use -Werror, and -XepDisableWarningsInGeneratedCode only disables warnings.

Nice tip on sources ending up in the final artifact! I opened #245 on fixing.

[tbroyer]

-XepDisableWarningsInGeneratedCode happens before warnings are emitted, so -Werror won't change the outcome (vs excluded paths), and it helps catch Error Prone errors.

[Stephan202]

@msridhar we have a similar setup and combine those two flags with -XepAllErrorsAsWarnings. That way there are no warnings (nor errors) in generated code, while both warnings and errors in the rest of the code base fail the build.

[msridhar]

Thanks for the tips! I've fixed here to use -XepDisableWarningsInGeneratedCode. And @Stephan202 that's a neat trick! Will keep it in mind for future projects.

[tbroyer]

Fwiw, you could also mark the class as final.

[msridhar]

I like the instanceof check better anyway, but I'll also make the class final.