Add support for error prone as native compiler option #1047

davido · 2016-12-12T21:48:06Z

Fixes #994.

Summary:
http://errorprone.info is a static analysis tool for Java that catches
common programming mistakes at compile-time. With this change it can
be permanenty or instantly activated as default compiler. Moreover,
java_library rule is extended as well.

The configuration option: java.error_prone_javac is considered to be
experimental and is not documented.

Test Plan:

Permanently activate error prone: add this line to .buckconfig:

   [java]
     error_prone_javac = true

Instantly activate error prone:

  $ buck build --config java.error_prone_javac=true <rule>

Activate error prone per rule base:

java_library(
    name = 'foo',
    error_prone_javac = True,
    [...]

facebook-github-bot · 2016-12-12T21:48:12Z

By analyzing the blame information on this pull request, we identified @yiding, @bolinfest and @marcinkosiba to be potential reviewers.

facebook-github-bot · 2016-12-13T06:53:06Z

@davido updated the pull request - view changes

facebook-github-bot · 2016-12-13T07:19:00Z

@davido updated the pull request - view changes

davido · 2016-12-13T11:00:51Z

DEFS

+    kwargs[ext] = []
+  extra_args = kwargs[ext]
+  # Immutables has an issue with OptionalEquality, that is flagged by error prone
+  extra_args.extend(['-Xep:OptionalEquality:OFF'])


Why not to decrease the severity to WARN instead

extra_args.extend(['-Xep:OptionalEquality:WARN'])

Oh, I see, that the same issue was raised by @tbroyer on error prone issue tracker: [1], and the solution was to skip warnings in generated code by -XepDisableWarningsInGeneratedCode.

[1] Support @SuppressWarnings("all") google/error-prone#329

davido · 2016-12-13T20:25:15Z

src/com/facebook/buck/artifact_cache/AbstractCacheResult.java

@@ -71,6 +71,7 @@
  @JsonProperty("artifactSizeBytes") protected abstract Optional<Long> artifactSizeBytes();

  public String getCacheSource() {
+      new Exception();


facebook-github-bot · 2016-12-13T20:38:33Z

@davido updated the pull request - view changes

kageiit · 2016-12-14T00:04:12Z

scripts/travisci_run.sh

@@ -17,6 +17,9 @@ export BUCK_NUM_THREADS=3
 # Make sure that everything builds in case a library is not covered by a test.
 ./bin/buck build --num-threads=$BUCK_NUM_THREADS src/... test/...

+# Run error prone checks
+./bin/buck build --config sanitizers.error_prone=1 --num-threads=$BUCK_NUM_THREADS src/... test/...


This will run the build twice. Can we just do this by default?

The reason I did it was because there were errors in error prone checks for test/... . But I agree, let us do that and fix the breakage before submitting this.

facebook-github-bot · 2016-12-14T05:53:58Z

@davido updated the pull request - view changes

kageiit · 2016-12-14T05:56:24Z

Since we are already checking in these files, can we just provide an option to set a .buckconfig option to make the default javac the error prone compiler for end consumers of buck? This way they dont need to obtain an error prone javac jar themselves

davido · 2016-12-14T06:47:27Z

Since we are already checking in these files, can we just provide an option to set a .buckconfig option to make the default javac the error prone compiler for end consumers of buck?

That's what #994 is all about. That's the plan and is the next change in this series.

facebook-github-bot · 2016-12-14T22:44:23Z

@davido updated the pull request - view changes

facebook-github-bot · 2016-12-14T22:49:27Z

@davido updated the pull request - view changes

davido · 2016-12-14T22:52:24Z

@kageiit

Since we are already checking in these files, can we just provide an option to set a .buckconfig option to make the default javac the error prone compiler for end consumers of buck?

Done. The option is: java.error_prone_javac. Add it to your .buckconfig or pass instantly with buck build --config java.error_prone_javac=true foo and enjoy error prone checks.

kageiit · 2016-12-14T23:08:07Z

Done. The option is: java.error_prone_javac. Add it to your .buckconfig or pass instantly with buck build --config java.error_prone_javac=true foo and enjoy error prone checks.

Can we make this option be configurable per target as well similar to javac_jar for java and android rules? Also needs docs

davido · 2016-12-16T14:41:19Z

Can we make this option be configurable per target as well similar to javac_jar for java and android rules?

Working on it right now. Should it be the same as in .buckconfig:

  java_library(
    name = 'foo',
    error_prone_javac = True,
    [...]

facebook-github-bot · 2016-12-19T07:36:14Z

@davido updated the pull request - view changes

rmapes · 2016-12-19T13:36:14Z

This will need some kind of automated tests (integration test or unit test), and an update to the soy docs before landing

davido · 2016-12-19T13:57:42Z

@rmapes I agree, but I would like to hear general opinion on the approach, before investing more time in polishing the CL.

facebook-github-bot · 2017-03-08T22:05:00Z

@davido updated the pull request - view changes

kageiit · 2017-03-08T23:13:18Z

src/com/facebook/buck/jvm/java/ErrorProneJavac.java

+import javax.tools.JavaCompiler;
+
+public class ErrorProneJavac extends Jsr199Javac {
+  private final static String ERROR_PRONE_VERSION = "2.15";


change to 2.0.18

facebook-github-bot · 2017-03-08T23:42:37Z

@davido updated the pull request - view changes

Fixes facebook#994. Summary: http://errorprone.info is a static analysis tool for Java that catches common programming mistakes at compile-time. With this change it can be permanenty or instantly activated as default compiler. The configuration option: java.error_prone_javac is considered to be experimental and is not documented. Test Plan: 1. Permanently activate error prone: add this line to .buckconfig: [java] error_prone_javac = true 2. Instantly activate error prone: $ buck build --config java.error_prone_javac=true <rule> 3. Activate error prone per rule base: java_library( name = 'foo', error_prone_javac = True, [...]

Coneko · 2017-03-09T17:17:54Z

I've imported the fixes to the issues detected with error prone, and I've verified it works correctly on our CI.

I'm not convinced we want to bundle error prone with Buck for consumers.

Taking a step back can we fix whatever issue you hit when trying to specify the error prone compiler to be a remote_file from maven?

Orthogonal to that having Buck itself be built using error prone is nice, so if you want you can change this PR to do that instead. I've had to edit the project to make it work in IntelliJ though, and I've deleted the 2.0.17 jars you've left in.

Also even when the Buck build is configured to use error prone the ant build isn't, would be better to have that one reflect the change as well so they don't disagree.

kageiit · 2017-03-09T18:41:45Z

I'm not convinced we want to bundle error prone with Buck for consumers.

Can you explain why you feel so? As a consumer we have been using the native integration in buck for several months to catch many errors at build time and it did not require us to setup any complicated error prone setup other than turning a flag on. Bazel uses error prone as the default compiler for all java and android code. This PR is merely adding it as an opt in.

Many well used opensource libraries like dagger ship with error prone checks in their artifacts now and can prevent many expensive errors at compile time. I feel like this is a great feature to have.

To add to that, we are also working on open-sourcing replacement for the eradicate checker from infer using error prone which blows eradicate away in terms of raw performance and lets issues with null pointers be detected early on at compile time and prevents playing constant catchup with commits like these 6d8af3a . If we feel like error prone ensures better quality code with buck, why not let the consumers get the same benefit?

I feel not adding in error prone as an option will prevent taking buck to the next level in terms of what it can do for its consumers.

Also even when the Buck build is configured to use error prone the ant build isn't, would be better to have that one reflect the change as well so they don't disagree.

We should definitely build with error prone on both the buck and ant paths.

remote_file from maven?

I think it is reasonable to be a remote file from maven.

Summary: For Java 9 compatibility. Extracted from #1047 . Test Plan: Imported from GitHub, without a Test Plan: line. Reviewed By: marcinkosiba, Coneko fbshipit-source-id: 4edbf4f

bolinfest · 2017-03-09T21:37:23Z

@davido I believe that the folks who work on Infer achieve this by overriding java_library.javac_jar: https://buckbuild.com/rule/java_library.html#javac_jar. Could you do that instead? I'm not excited about making error_prone_javac part of java_library()'s API.

Incidentally, you might want to check out Infer if you haven't already: http://fbinfer.com/docs/analyzing-apps-or-projects.html.

kageiit · 2017-03-10T06:35:56Z

@bolinfest See my comment above #1047 (comment)

Error prone might be a better option than eradicate in the long term in terms of performance. Eradicate is one of the most time consuming parts of our current build process with the way it currently integrates into buck.

I think its fine to remove it as part of the java_library api. But I think it might be still extremely useful to have a global config to turn error prone on in buckconfig as build systems like bazel ship by default with error prone and it would be great to have that as an option in buck as well.

Summary: Miscellaneous issues detected with error prone. Extracted from #1047 . Test Plan: CI Reviewed By: illicitonion, Coneko fbshipit-source-id: c55ad90

mkosiba · 2017-03-10T16:46:57Z

I'm not convinced we want to bundle error prone with Buck for consumers.

I'll elaborate as Coneko is passing on a point I originally made. We don't want to bundle this by default as it's 9MB extra to our binary size, which also affects startup time. I'm not saying "it should be hard to use", just "if you don't use it you shouldn't need pay for it". It seems like we're all agreeing that having it be a remote_file is a good way out.

kageiit · 2017-03-17T23:13:15Z

@davido 2.0.19 is out

https://groups.google.com/forum/#!msg/error-prone-announce/sXXuFyDDY1c/ESUs9e_uAAAJ

Can we move ep to a remote_file so we can move this change forward?

kageiit · 2017-03-22T21:00:22Z

@davido any update on the remote_file part?

kageiit · 2017-03-22T23:19:04Z

@marcinkosiba @Coneko how would one go about adding a remote_file to third-party? There are no existing examples. How would that work with ant build?

v-jizhang · 2019-12-27T18:51:59Z

#1020

facebook-github-bot added GH Review: review-needed CLA Signed labels Dec 12, 2016

davido force-pushed the optionally_activate_error_prone branch from 007c297 to 0d1e5de Compare December 13, 2016 06:53

davido force-pushed the optionally_activate_error_prone branch from 0d1e5de to 13877cb Compare December 13, 2016 07:18

davido commented Dec 13, 2016

View reviewed changes

davido force-pushed the optionally_activate_error_prone branch from 13877cb to f5dc780 Compare December 13, 2016 20:36

kageiit reviewed Dec 14, 2016

View reviewed changes

davido force-pushed the optionally_activate_error_prone branch from f5dc780 to 4dafffe Compare December 14, 2016 05:53

davido force-pushed the optionally_activate_error_prone branch from 4dafffe to df430e5 Compare December 14, 2016 22:44

davido force-pushed the optionally_activate_error_prone branch from df430e5 to 5a4983a Compare December 14, 2016 22:48

davido changed the title ~~Optionally activate error prone checks~~ Add support for error prone as native compiler option Dec 14, 2016

davido changed the title ~~Add support for error prone as native compiler option~~ WIP: Add support for error prone as native compiler option Dec 16, 2016

davido force-pushed the optionally_activate_error_prone branch from 5a4983a to 8779a13 Compare December 19, 2016 07:36

davido changed the title ~~WIP: Add support for error prone as native compiler option~~ Add support for error prone as native compiler option Dec 19, 2016

SmartDexingStep: Rewrite lambda as function

6fe3462

davido force-pushed the optionally_activate_error_prone branch from 60f95f6 to cad81f5 Compare March 8, 2017 22:04

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Mar 8, 2017

kageiit suggested changes Mar 8, 2017

View reviewed changes

davido force-pushed the optionally_activate_error_prone branch from cad81f5 to 33ed2be Compare March 8, 2017 23:42

davido added 2 commits March 9, 2017 00:44

Run error prone checks in Travis CI

33ed2be

kageiit approved these changes Mar 8, 2017

View reviewed changes

Coneko added GH Review: accepted and removed GH Review: review-needed labels Mar 9, 2017

facebook-github-bot added the Import Started label Mar 9, 2017

Coneko self-assigned this Mar 9, 2017

Coneko added GH Review: needs-revision and removed GH Review: accepted labels Mar 9, 2017

facebook-github-bot pushed a commit that referenced this pull request Mar 10, 2017

Fix error prone issues.

cccc595

Summary: Miscellaneous issues detected with error prone. Extracted from #1047 . Test Plan: CI Reviewed By: illicitonion, Coneko fbshipit-source-id: c55ad90

styurin unassigned Coneko Oct 31, 2017

v-jizhang closed this Dec 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for error prone as native compiler option #1047

Add support for error prone as native compiler option #1047

davido commented Dec 12, 2016 •

edited

Loading

facebook-github-bot commented Dec 12, 2016

facebook-github-bot commented Dec 13, 2016

facebook-github-bot commented Dec 13, 2016

davido Dec 13, 2016 •

edited

Loading

davido Dec 13, 2016 •

edited

Loading

davido Dec 13, 2016

facebook-github-bot commented Dec 13, 2016

kageiit Dec 14, 2016

davido Dec 14, 2016

davido Dec 14, 2016

facebook-github-bot commented Dec 14, 2016

kageiit commented Dec 14, 2016

davido commented Dec 14, 2016 •

edited

Loading

facebook-github-bot commented Dec 14, 2016

facebook-github-bot commented Dec 14, 2016

davido commented Dec 14, 2016 •

edited

Loading

kageiit commented Dec 14, 2016

davido commented Dec 16, 2016 •

edited

Loading

facebook-github-bot commented Dec 19, 2016

rmapes commented Dec 19, 2016

davido commented Dec 19, 2016

facebook-github-bot commented Mar 8, 2017

kageiit Mar 8, 2017

davido Mar 8, 2017

facebook-github-bot commented Mar 8, 2017

Coneko commented Mar 9, 2017

kageiit commented Mar 9, 2017

bolinfest commented Mar 9, 2017

kageiit commented Mar 10, 2017

mkosiba commented Mar 10, 2017 •

edited

Loading

kageiit commented Mar 17, 2017

kageiit commented Mar 22, 2017

kageiit commented Mar 22, 2017

v-jizhang commented Dec 27, 2019

Add support for error prone as native compiler option #1047

Add support for error prone as native compiler option #1047

Conversation

davido commented Dec 12, 2016 • edited Loading

facebook-github-bot commented Dec 12, 2016

facebook-github-bot commented Dec 13, 2016

facebook-github-bot commented Dec 13, 2016

davido Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

davido Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

davido Dec 13, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Dec 13, 2016

kageiit Dec 14, 2016

Choose a reason for hiding this comment

davido Dec 14, 2016

Choose a reason for hiding this comment

davido Dec 14, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Dec 14, 2016

kageiit commented Dec 14, 2016

davido commented Dec 14, 2016 • edited Loading

facebook-github-bot commented Dec 14, 2016

facebook-github-bot commented Dec 14, 2016

davido commented Dec 14, 2016 • edited Loading

kageiit commented Dec 14, 2016

davido commented Dec 16, 2016 • edited Loading

facebook-github-bot commented Dec 19, 2016

rmapes commented Dec 19, 2016

davido commented Dec 19, 2016

facebook-github-bot commented Mar 8, 2017

kageiit Mar 8, 2017

Choose a reason for hiding this comment

davido Mar 8, 2017

Choose a reason for hiding this comment

facebook-github-bot commented Mar 8, 2017

Coneko commented Mar 9, 2017

kageiit commented Mar 9, 2017

bolinfest commented Mar 9, 2017

kageiit commented Mar 10, 2017

mkosiba commented Mar 10, 2017 • edited Loading

kageiit commented Mar 17, 2017

kageiit commented Mar 22, 2017

kageiit commented Mar 22, 2017

v-jizhang commented Dec 27, 2019

davido commented Dec 12, 2016 •

edited

Loading

davido Dec 13, 2016 •

edited

Loading

davido Dec 13, 2016 •

edited

Loading

davido commented Dec 14, 2016 •

edited

Loading

davido commented Dec 14, 2016 •

edited

Loading

davido commented Dec 16, 2016 •

edited

Loading

mkosiba commented Mar 10, 2017 •

edited

Loading