Format Java source files automatically

CONTRIBUTING.md

@@ -155,19 +155,68 @@ For Eclipse, go to `Preferences->Java->Installed JREs` and add `-ea` to
 
 ### Java Language Formatting Guidelines
 
+Java files in the Elasticsearch codebase are formatted with the Eclipse JDT
+formatter, using the [Spotless
+Gradle](https://github.com/diffplug/spotless/tree/master/plugin-gradle)
+plugin. This plugin is configured on a project-by-project basis, via
+`build.gradle` in the root of the repository. So long as at least one
+project is configured, the formatting check can be run explicitly with:
+
+    ./gradlew spotlessJavaCheck
+
+The code can be formatted with:
+
+    ./gradlew spotlessApply
+
+These tasks can also be run for specific subprojects, e.g.
+
+    ./gradlew server:spotlessJavaCheck
+
 Please follow these formatting guidelines:
 
 * Java indent is 4 spaces
 * Line width is 140 characters
-* Lines of code surrounded by `// tag` and `// end` comments are included in the
-documentation and should only be 76 characters wide not counting
-leading indentation
-* The rest is left to Java coding standards
-* Disable “auto-format on save” to prevent unnecessary format changes. This makes reviews much harder as it generates unnecessary formatting changes. If your IDE supports formatting only modified chunks that is fine to do.
-* Wildcard imports (`import foo.bar.baz.*`) are forbidden and will cause the build to fail. This can be done automatically by your IDE:
- * Eclipse: `Preferences->Java->Code Style->Organize Imports`. There are two boxes labeled "`Number of (static )? imports needed for .*`". Set their values to 99999 or some other absurdly high value.
- * IntelliJ: `Preferences/Settings->Editor->Code Style->Java->Imports`. There are two configuration options: `Class count to use import with '*'` and `Names count to use static import with '*'`. Set their values to 99999 or some other absurdly high value.
-* Don't worry too much about import order. Try not to change it but don't worry about fighting your IDE to stop it from doing so.
+* Lines of code surrounded by `// tag` and `// end` comments are included
+  in the documentation and should only be 76 characters wide not counting
+  leading indentation
+* Wildcard imports (`import foo.bar.baz.*`) are forbidden and will cause
+  the build to fail. This can be done automatically by your IDE:
+   * Eclipse: `Preferences->Java->Code Style->Organize Imports`. There are
+     two boxes labeled "`Number of (static )? imports needed for .*`". Set
+     their values to 99999 or some other absurdly high value.
+   * IntelliJ: `Preferences/Settings->Editor->Code Style->Java->Imports`.
+     There are two configuration options: `Class count to use import with
+     '*'` and `Names count to use static import with '*'`. Set their values
+     to 99999 or some other absurdly high value.
+
+#### Editor / IDE Support
+
+Eclipse IDEs can import the file [elasticsearch.eclipseformat.xml]
+directly.
+
+IntelliJ IDEs can
+[import](https://blog.jetbrains.com/idea/2014/01/intellij-idea-13-importing-code-formatter-settings-from-eclipse/)
+the same settings file, and / or use the [Eclipse Code
+Formatter](https://plugins.jetbrains.com/plugin/6546-eclipse-code-formatter)
+plugin.
+
+You can also tell Spotless to [format a specific
+file](https://github.com/diffplug/spotless/tree/master/plugin-gradle#can-i-apply-spotless-to-specific-files)
+from the command line.
+
+#### Formatting failures
+
+Sometimes Spotless will report a "misbehaving rule which can't make up its
+mind" and will recommend enabling the `paddedCell()` setting. If you
+enabled this settings and run the format check again,
+Spotless will write files to
+`$PROJECT/build/spotless-diagnose-java/` to aid diagnosis. It writes
+different copies of the formatted files, so that you can see how they
+differ and infer what is the problem.
+
+The `paddedCell() option is disabled for normal operation in order to
+detect any misbehaviour. You can enabled the option from the command line
+by running Gradle with `-Dspotless.paddedcell`.
 
 ### License Headers
 

build.gradle

@@ -35,6 +35,7 @@ plugins {
     id 'com.gradle.build-scan' version '2.4'
     id 'lifecycle-base'
     id 'elasticsearch.global-build-info'
+    id "com.diffplug.gradle.spotless" version "3.24.2" apply false
 }
 
 apply plugin: 'nebula.info-scm'
@@ -98,6 +99,34 @@ subprojects {
   plugins.withType(BuildPlugin).whenPluginAdded {
     project.licenseFile = project.rootProject.file('licenses/APACHE-LICENSE-2.0.txt')
     project.noticeFile = project.rootProject.file('NOTICE.txt')
+
+    // Projects that should be formatted and checked with Spotless are
+    // listed here, by project path. Once the number of formatted projects
+    // is greater than the number of unformatted projects, this can be
+    // switched to an exclude list, and eventualy removed completely.
+    def projectPathsToFormat = [
+      // ':build-tools'
+    ]
+
+    if (projectPathsToFormat.contains(project.path)) {
+      project.apply plugin: "com.diffplug.gradle.spotless"
+
+      spotless {
+        java {
+
+          removeUnusedImports()
+          eclipse().configFile rootProject.file('.eclipseformat.xml')
+          trimTrailingWhitespace()
+
+          // See CONTRIBUTING.md for details of when to enabled this.
+          if (System.getProperty('spotless.paddedcell') != null) {
+            paddedCell()
+          }
+        }
+      }
+
+      precommit.dependsOn 'spotlessJavaCheck'
+    }
   }
 }
 

buildSrc/build.gradle

@@ -112,7 +112,7 @@ dependencies {
   compile 'com.netflix.nebula:gradle-extra-configurations-plugin:3.0.3'
   compile 'com.netflix.nebula:nebula-publishing-plugin:4.4.4'
   compile 'com.netflix.nebula:gradle-info-plugin:3.0.3'
-  compile 'org.eclipse.jgit:org.eclipse.jgit:3.2.0.201312181205-r'
+  compile 'org.eclipse.jgit:org.eclipse.jgit:5.5.0.201909110433-r'
   compile 'com.perforce:p4java:2012.3.551082' // THIS IS SUPPOSED TO BE OPTIONAL IN THE FUTURE....
   compile 'org.apache.rat:apache-rat:0.11'
   compile "org.elasticsearch:jna:4.5.1"

modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java

@@ -227,7 +227,8 @@ Constructor<?> compile(Loader loader, Set<String> extractedVariables, String nam
             }
 
             return clazz.getConstructors()[0];
-        } catch (Exception exception) { // Catch everything to let the user know this is something caused internally.
+        } catch (Exception exception) {
+            // Catch everything to let the user know this is something caused internally.
             throw new IllegalStateException("An internal error occurred attempting to define the script [" + name + "].", exception);
         }
     }

modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java

@@ -60,32 +60,35 @@
  * {@link java.lang.invoke.LambdaMetafactory} since the Painless casting model
  * cannot be fully supported through this class.
  *
- * For each lambda function/method reference used within a Painless script
+ * <p>For each lambda function/method reference used within a Painless script
  * a class will be generated at link-time using the
  * {@link LambdaBootstrap#lambdaBootstrap} method that contains the following:
- * 1. member fields for any captured variables
- * 2. a constructor that will take in captured variables and assign them to
+ *
+ * <ol>
+ * <li>member fields for any captured variables
+ * <li>a constructor that will take in captured variables and assign them to
  * their respective member fields
- * 3. a static ctor delegation method, if the lambda function is a ctor.
- * 4. a method that will load the member fields representing captured variables
+ * <li>a static ctor delegation method, if the lambda function is a ctor.
+ * <li>a method that will load the member fields representing captured variables
  * and take in any other necessary values based on the arguments passed into the
  * lambda function/reference method; it will then make a delegated call to the
- * actual lambda function/reference method
+ * actual lambda function/reference method.
+ * </ol>
  *
- * Take for example the following Painless script:
+ * <p>Take for example the following Painless script:
  *
- * {@code
+ * <pre>{@code
  * List list1 = new ArrayList(); "
  * list1.add(2); "
  * List list2 = new ArrayList(); "
  * list1.forEach(x -> list2.add(x));"
  * return list[0]"
- * }
+ * }</pre>
  *
- * The script contains a lambda function with a captured variable.
+ * <p>The script contains a lambda function with a captured variable.
  * The following Lambda class would be generated:
  *
- * {@code
+ * <pre>{@code
  *     public static final class $$Lambda0 implements Consumer {
  *         private List arg$0;
  *
@@ -109,9 +112,9 @@
  *         }
  *         ...
  *     }
- * }
+ * }</pre>
  *
- * Also the accept method actually uses an invokedynamic
+ * <p>Also the accept method actually uses an invokedynamic
  * instruction to call the lambda$0 method so that
  * {@link MethodHandle#asType} can be used to do the necessary
  * conversions between argument types without having to hard
@@ -120,7 +123,7 @@
  * calls the constructor. This method is used by the
  * invokedynamic call to initialize the instance.
  *
- * When the {@link CallSite} is linked the linked method depends
+ * <p>When the {@link CallSite} is linked the linked method depends
  * on whether or not there are captures.  If there are no captures
  * the same instance of the generated lambda class will be
  * returned each time by the factory method as there are no

modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java

@@ -316,7 +316,8 @@ private <T> T generateFactory(Loader loader, ScriptContext<T> context, Set<Strin
 
         try {
             return context.factoryClazz.cast(factory.getConstructor().newInstance());
-        } catch (Exception exception) { // Catch everything to let the user know this is something caused internally.
+        } catch (Exception exception) {
+            // Catch everything to let the user know this is something caused internally.
             throw new IllegalStateException(
                 "An internal error occurred attempting to define the factory class [" + className + "].", exception);
         }

modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EAssignment.java

@@ -291,7 +291,8 @@ void write(MethodWriter writer, Globals globals) {
                                                                                                        // from dup the value onto the stack
             }
 
-            lhs.store(writer, globals); // store the lhs's value from the stack in its respective variable/field/array
+            // store the lhs's value from the stack in its respective variable/field/array
+            lhs.store(writer, globals);
         } else if (operation != null) {
             // Handle the case where we are doing a compound assignment that
             // does not represent a String concatenation.
@@ -309,9 +310,9 @@ void write(MethodWriter writer, Globals globals) {
                                         // to the promotion type between the lhs and rhs types
             rhs.write(writer, globals); // write the bytecode for the rhs
 
-        // XXX: fix these types, but first we need def compound assignment tests.
-        // its tricky here as there are possibly explicit casts, too.
-        // write the operation instruction for compound assignment
+            // XXX: fix these types, but first we need def compound assignment tests.
+            // its tricky here as there are possibly explicit casts, too.
+            // write the operation instruction for compound assignment
             if (promote == def.class) {
                 writer.writeDynamicBinaryInstruction(
                     location, promote, def.class, def.class, operation, DefBootstrap.OPERATOR_COMPOUND_ASSIGNMENT);
@@ -322,23 +323,24 @@ void write(MethodWriter writer, Globals globals) {
             writer.writeCast(back); // if necessary cast the promotion type value back to the lhs's type
 
             if (lhs.read && !post) {
-                writer.writeDup(MethodWriter.getType(lhs.actual).getSize(), lhs.accessElementCount()); // dup the value if the lhs is also
-                                                                                                       // read from and is not a post
-                                                                                                       // increment
+                // dup the value if the lhs is also read from and is not a post increment
+                writer.writeDup(MethodWriter.getType(lhs.actual).getSize(), lhs.accessElementCount());
             }
 
-            lhs.store(writer, globals); // store the lhs's value from the stack in its respective variable/field/array
+            // store the lhs's value from the stack in its respective variable/field/array
+            lhs.store(writer, globals);
         } else {
             // Handle the case for a simple write.
 
             rhs.write(writer, globals); // write the bytecode for the rhs rhs
 
             if (lhs.read) {
-                writer.writeDup(MethodWriter.getType(lhs.actual).getSize(), lhs.accessElementCount()); // dup the value if the lhs
-                                                                                                       // is also read from
+                // dup the value if the lhs is also read from
+                writer.writeDup(MethodWriter.getType(lhs.actual).getSize(), lhs.accessElementCount());
             }
 
-            lhs.store(writer, globals); // store the lhs's value from the stack in its respective variable/field/array
+            // store the lhs's value from the stack in its respective variable/field/array
+            lhs.store(writer, globals);
         }
     }
 

modules/lang-painless/src/main/java/org/elasticsearch/painless/node/package-info.java

@@ -109,7 +109,7 @@
  * <p>
  * Generally, expression nodes have member data that evaluate static and def types.  The typical order for an expression node
  * during the analysis phase looks like the following:
- * {@code
+ * <pre>{@code
  * For known expected types:
  *
  * expression.child.expected = expectedType      // set the known expected type
@@ -132,7 +132,7 @@
  * expression.child = expression.child.cast(...) // add an implicit cast node if the child node's
  *                                               // actual type is not the expected type and set the
  *                                               // expression's child to the implicit cast node
- * }
+ * }</pre>
  * Expression nodes just call each child during the writing phase.
  * <p>
  * Postfix nodes represent postfixes in a variable/method chain including braces, calls, or fields.

modules/lang-painless/src/test/java/org/elasticsearch/painless/ArrayLikeObjectTestCase.java

@@ -87,7 +87,8 @@ private void expectOutOfBounds(int index, String script, Object val) {
              */
             assertThat(e.getMessage(), outOfBoundsExceptionMessageMatcher(index, 5));
         } catch (AssertionError ae) {
-            ae.addSuppressed(e);   // Mark the exception we are testing as suppressed so we get its stack trace.
+            // Mark the exception we are testing as suppressed so we get its stack trace.
+            ae.addSuppressed(e);
             throw ae;
         }
     }