error-prone-support/error-prone-contrib/README.md

Picnic's Error Prone Contrib

This project provides a plugin containing a collection of Error Prone checks.

How to contribute

Contributions are more than welcome! Below we list tasks that are on our TODO list. If you have others ideas/plans, feel free to file an issue or open a pull request.

Building

This is a Maven project, so running mvn clean install performs a full clean build. Some relevant flags:

• -Dverification.warn makes the warnings and errors emitted by various plugins and the Java compiler non-fatal, where possible.
• -Dverification.skip disables various non-essential plugins and compiles the code with minimal checks (i.e. without linting, Error Prone checks, etc.)
• -Dversion.error-prone=some-version runs the build using the specified version of Error Prone. This is useful e.g. when testing a locally built Error Prone SNAPSHOT.
• -Perror-prone-fork run the build using Picnic's Error Prone fork, hosted on Jitpack. This fork generally contains a few changes on top of the latest Error Prone release.

Two other goals that one may find relevant:

• mvn fmt:format formats the code using google-java-format.
• mvn pitest:mutationCoverage runs mutation tests using PIT. The results can be reviewed by opening the respective target/pit-reports/index.html files. For more information check the PIT Maven plugin.

When loading the project in IntelliJ IDEA (and perhaps other IDEs) errors about the inaccessibility of com.sun.tools.javac.* classes may be reported. If this happens, configure your IDE to enable the add-exports profile.

Contribution guidelines

To the extend possible, the pull request process guards our coding guidelines. Some pointers:

• Checks should we topical: Ideally they address a single concern.
• Where possible checks should provide fixes, and ideally these are completely behavior preserving. In order for a check to be adopted by users it must not "get in the way". So for a check which addresses a relatively trivial stylistic concern it is doubly important that the violations it detects can be auto-patched.
• Make sure you have read Error Prone's criteria for new checks. Most guidelines described there apply to this project as well, except that this project does focus quite heavy on style enforcement. But that just makes the previous point doubly important.
• Make sure that a check's (mutation) coverage is or remains about as high as it can be. Not only does this lead to better tests, it also points out opportunities to simplify the code.
• Please restrict the scope of a pull request to a single feature or fix. Don't sneak in unrelated changes.
• When in doubt about whether a pull request will be accepted, please first file an issue to discuss it.

Our wishlist

We expect the following tasks to help improve the quality of this open source project:

• Publish the artifact to Maven Central, then document the coordinates in this README.md.
• Document how to enable the checks.
• Document how to apply patches.
• Document each of the checks.
• Add Travis CI, SonarQube and Codecov integrations.
• Investigate whether it makes sense to include license headers in each file. If so, set that up and enforce it.
• Add non-Java file formatting support, like we have internally at Picnic. (I.e., somehow open-source that stuff.)
• Add relevant "badges" at the top of this README.md.
• Auto-generate a website listing each of the checks, just like the Error Prone bug patterns page. The Error Prone repository contains code for this.
• Set up a script/procedure for testing the plugin against other open source projects. By running the checks against a variety of code bases we are more likely to catch errors due to unexpected code constructs. False positives and regressions can be caught this way as well. For inspiration, review how Checkstyle does this.
• Have the repository be analyzed by Better Code Hub and potentially publish the results.
• Consider integrating with FOSSA.
• Review all places in the code where a Description is currently created, and see whether a custom error message (Description.Builder#setMessage) is warranted.
• Improve an existing check (see XXX-marked comments in the code) or write a new one (see the list of suggestions below).

Ideas for new checks

The following is a list of checks we'd like to see implemented:

• A check with functionality equivalent to the "Policeman's Forbidden API Checker" Maven plugin, with a focus on disallowing usage of the methods and fields listed by the "JDK System Out" and "JDK Unsafe" signature groups. Using Error Prone's method matchers forbidden method calls can easily be identified. But Error Prone can go one step further by auto-patching violations. For each violation two fixes can be proposed: a purely behavior-preserving fix which makes the platform-dependent behavior explicit, and another which replaces the platform-dependent behavior with the preferred alternative. (Such as using UTF-8 instead of the system default charset.)
• The Modernizer Maven Plugin can similarly be replaced with an equivalent Error Prone check. The latter would allow automatic replacement of outdated APIs through Error Prone's patch functionality.
• A subset of the refactor operations provided by the Eclipse-specific AutoRefactor plugin.
• A check which replaces fully qualified types with simple types in contexts where this does not introduce ambiguity. Should consider both actual Java code and Javadoc @link references.
• A check which simplifies array expressions. It would replace empty array expressions of the form new int[] {} with new int[0]. Statements of the form byte[] arr = new byte[] {'c'}; would be shortened to byte[] arr = {'c'};.
• A check which replaces expressions of the form String.format("some prefix %s", arg) with "some prefix " + arg, and similar for simple suffixes. Can perhaps be generalized further, though it's unclear how far. (Well, a String.format call without arguments can certainly be simplified, too.)
• A check which replaces single-character strings with chars where possible. For example as argument to StringBuilder.append and in string concatenations.
• A check which adds or removes the first Locale argument to String.format and similar calls as necessary. (For example, a format string containing only %s placeholders is locale-insensitive unless any of the arguments is a Formattable, while %f placeholders are locale-sensitive.)
• A check which replaces String.replaceAll with String.replace if the first argument is certainly not a regular expression. And if both arguments are single-character strings then the (char, char) overload can be invoked instead.
• A check which flags (and ideally, replaces) try-finally constructs with equivalent try-with-resources constructs.
• A check which drops exceptions declared in throws clauses if they are (a) not actually thrown and (b) the associated method cannot be overridden.
• A check which tries to statically import certain methods whenever used, if possible. The set of targeted methods should be configurable, but may default to e.g. java.util.Function.identity(), the static methods exposed by java.util.stream.Collectors and the various Guava collector factory methods.
• A check which replaces new Random().someMethod() calls with ThreadLocalRandom.current().someMethod() calls, so as to avoid unnecessary synchronization.
• A check which drops this. from this.someMethod() calls and which optionally does the same for fields, if no ambiguity arises.
• A check which replaces Integer.valueOf calls with Integer.parseInt or vice versa in order to prevent auto (un)boxing, and likewise for other number types.
• A check which flags nullable collections.
• A check which flags AutoCloseable resources not managed by a try-with-resources construct. Certain subtypes, such as jOOQ's DSLContext should be excluded.
• A check which flags java.time methods which implicitly consult the system clock, suggesting that a passed-in Clock is used instead.
• A check which flags public methods on public classes which reference non-public types. This can cause IllegalAccessErrors and BootstrapMethodErrors at runtime.
• A check which swaps the LHS and RHS in expressions of the form nonConstant.equals(someNonNullConstant).
• A check which annotates methods which only throw an exception with @Deprecated or @DoNotCall.
• A check which flags imports from other test classes.
• A Guava-specific check which replaces Joiner.join calls with String.join calls in those cases where the latter is a proper substitute for the former.
• A Guava-specific check which flags {Immutable,}Multimap type usages where {Immutable,}{List,Set}Multimap would be more appropriate.
• A Guava-specific check which rewrites if (conditional) { throw new IllegalArgumentException(); } and variants to an equivalent checkArgument statement. Idem for other exception types.
• A Guava-specific check which replaces simple anonymous CacheLoader subclass declarations with CacheLoader.from(someLambda).
• A Spring-specific check which enforces that methods with the @Scheduled annotation are also annotated with New Relic's @Trace annotation. Such methods should ideally not also represent Spring MVC endpoints.
• A Spring-specific check which enforces that @RequestMapping annotations, when applied to a method, explicitly specify one or more target HTTP methods.
• A Spring-specific check which looks for classes in which all @RequestMapping annotations (and the various aliases) specify the same path/value property and then moves that path to the class level.
• A Spring-specific check which flags @Value("some.property") annotations, as these almost certainly should be @Value("${some.property}").
• A Spring-specific check which drops the required attribute from @RequestParam annotations when the defaultValue attribute is also specified.
• A Spring-specific check which rewrites a class which uses field injection to one which uses constructor injection. This check wouldn't be strictly behavior preserving, but could be used for a one-off code base migration.
• A Spring-specific check which disallows field injection, except in AbstractTestNGSpringContextTests subclasses. (One known edge case: self-injections so that a bean can call itself through an implicit proxy.)
• A Spring-specific check which verifies that public methods on all classes whose name matches a certain pattern, e.g. .*Service, are annotated @Secured.
• A Spring-specific check which verifies that annotations such as @RequestParam are only present in @RestController classes.
• A Spring-specific check which disallows @ResponseStatus on MVC endpoint methods, as this prevents communication of error status codes.
• A Hibernate Validator-specific check which looks for @UnwrapValidatedValue usages and migrates the associated constraint annotations to the generic type argument to which they (are presumed to) apply.
• A TestNG-specific check which drops method-level @Test annotations if a matching/more specific annotation is already present at the class level.
• A TestNG-specific check which enforces that all tests are in a group.
• A TestNG-specific check which flags field assignments in @BeforeMethod-annotated methods unless the class is annotated @Test(singleThreaded = true).
• A TestNG-specific check which flags usages of the expectedExceptions attribute of the @Test annotation, pointing to assertThrows.
• A Jongo-specific check which disallows the creation of sparse indices, in favour of partial indices.
• An Immutables-specific check which replaces checkState/IllegalStateException usages inside a @Value.Check-annotated method with checkArgument/IllegalArgument, since the method is invoked when a caller attempts to create an immutable instance.
• An Immutables-specific check which disallows references to collection types other than the Guava immutable collections, including inside generic type arguments.
• An SLF4J-specific check which drops or adds a trailing dot from log messages, as applicable.
• A Mockito-specific check which identifies sequences of statements which mock a significant number of methods on a single object with "default data"; such constructions can often benefit from a different type of default answer, such as Answers.RETURNS_MOCKS.
• An RxJava-specific check which flags .toCompletable() calls on expressions of type Single<Completable> etc., as most likely .flatMapCompletable(Functions.identity()) was meant instead. Idem for other variations.
• An RxJava-specific check which flags expr.firstOrError() calls and suggests expr.switchIfEmpty(Single.error(...)), so that an application-specific exception is thrown instead of NoSuchElementException.
• An RxJava-specific check which flags use of #assertValueSet without #assertValueCount, as the former method doesn't do what one may intuitively expect it to do. See ReactiveX/RxJava#6151.

Refaster extension ideas

XXX: This section should live elsewhere.

It's much easier to implement a Refaster rule than an Error Prone bug checker, but on the flip side Refaster is much less expressive. While this gap can never be fully closed, there are some ways in which Refaster's scope of utility could be extended. The following is a non-exhaustive list of ideas on how to extend Refaster's expressiveness:

• Allow more control over which methods are statically imported by @UseImportPolicy. Sometimes the @AfterTemplate contains more than one static method invocation, and only a subset should be statically imported.
• Provide a way to express that a lambda expression should also match an equivalent method reference and/or vice versa.
• Provide a way to express that certain method invocations should also be replaced when expressed as a method reference, or vice versa. (The latter may be simpler: perhaps the rule T::m1 -> T::m2 can optionally be interpreted to also cover T.m1(..) -> T.m2(...).)
• Some Refaster refactorings (e.g. when dealing with lazy evaluation) are valid only when some free parameter is a constant, variable reference or some other pure expression. Introduce a way to express such a constraint. For example, rewriting optional1.map(Optional::of).orElse(optional2) to optional1.or(() -> optional2) is not behavior preserving if evaluation of optional2 has side-effects.
• Similarly, certain refactoring operations are only valid if one of the matches expressions is not @Nullable. It'd be nice to be able to express this.
• Generalize @Placeholder support such that rules can reference e.g. "any concrete unary method". This would allow refactorings such as Mono.just(constant).flatmap(this::someFun) -> Mono.defer(() -> someFun(constant)).
• Sometimes a Refaster refactoring can cause the resulting code not to compile due to a lack of generic type information. Identify and resolve such occurrences. For example, an @AfterTemplate may require the insertion of a statically imported method, which can cause required generic type information to be lost. In such a case don't statically import the method, so that the generic type information can be retained. (There may be cases where generic type information should even be added. Find an example.)
• Upon application of a template Refaster can throw a No binding for Key{identifier=someAfterTemplateParam} exception. When this happens the template is invalid. Instead perform this check at compile time, such that such malformed templates cannot be defined in the first place.
• Provide a way to express "match if (not) annotated (with X)". See #1 for a motivating example.
• Provide a way to place match constraints on compile time constants. For example, "match if this integer is less than 2" or "match if this string matches the regular expression X".
• Provide a way to express transformations of compile-time constants. This would allow one to e.g. rewrite single-character strings to chars or vice versa, thereby accommodating a target API. Another example would be to replace SLF4J's {} place holders with %s or vice versa. Yet another example would be to rewrite BigDecimal.valueOf("<some-long-value>") to BigDecimal.valueOf(theParsedLongValue).
• More generally, investigate ways to plug in in fully dynamic behavior, e.g. by providing hooks using which arbitrary predicates/transformations can be plugged in. The result would be a Refaster/BugChecker hybrid. A feature such as this could form the basis for many other features listed here. (As a concrete example, consider the ability to reference com.google.errorprone.matchers.Matcher implementations.)
• Provide a way to match lambda expressions and method references which match a specified functional interface. This would allow rewrites such as Mono.fromCallable(this::doesNotThrowCheckException) -> Mono.fromSupplier(this::doesNotThrowCheckException).
• Provide an extension API using which methods or expressions can be defined based on functional properties. A motivating example is the Java Collections framework, which allows many ways to define (im)mutable (un)ordered collections with(out) duplicates. One could then express things like "match any method call with collects its inputs into an immutable ordered list". An enum analogous to java.util.stream.Collector.Characteristics could be used. Out of the box JDK and Guava collection factory methods could be classified, with the user having the option to extend the classification.
• Refaster currently unconditionally ignores expressions containing comments. Provide two additional modes: (a) match and drop the comments or (b) transport the comments to before/after the replaced expression.
• Extend Refaster to drop imports that come become unnecessary as a result of a refactoring. This e.g. allows one to replace a statically import TestNG fail(...) invocation with a statically imported equivalent AssertJ fail(...) invocation. (Observe that without an impor cleanup this replacement would cause a compilation error.)
• Extend the @Repeated match semantics such that it also covers non-varargs methods. For a motivating example see google/error-prone#568.
• When matching explicit type references, also match super types. For a motivating example, see the two subtly difference loop definitions in CollectionRemoveAllFromCollectionBlock.
• Figure out why Refaster sometimes doesn't match the correct generic overload. See the AssertThatIterableHasOneComparableElementEqualTo template for an example.