Improve error handling

Bail out of formatting if any invariants are violated (using a new
unchecked exception) instead of trying to continue and accumulating a
list of errors.  Trying to continue rarely produces additional useful
information, and may obscure the root cause of the problem.

MOE_MIGRATED_REVID=130001891

Author: cushon

core/src/main/java/com/google/googlejavaformat/FormatterDiagnostic.java

@@ -14,30 +14,44 @@
 
 package com.google.googlejavaformat;
 
-import com.google.common.base.Preconditions;
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
 
 /** An error that prevented formatting from succeeding. */
 public class FormatterDiagnostic {
   private final int lineNumber;
   private final String message;
   private final int column;
 
-  public FormatterDiagnostic(int lineNumber, int column, String message) {
-    Preconditions.checkArgument(lineNumber >= 0);
-    Preconditions.checkArgument(column >= 0);
-    Preconditions.checkNotNull(message);
+  public static FormatterDiagnostic create(String message) {
+    return new FormatterDiagnostic(-1, -1, message);
+  }
+
+  public static FormatterDiagnostic create(int lineNumber, int column, String message) {
+    checkArgument(lineNumber >= 0);
+    checkArgument(column >= 0);
+    checkNotNull(message);
+    return new FormatterDiagnostic(lineNumber, column, message);
+  }
 
+  private FormatterDiagnostic(int lineNumber, int column, String message) {
     this.lineNumber = lineNumber;
     this.column = column;
     this.message = message;
   }
 
-  /** Returns the line number on which the error occurred. */
+  /**
+   * Returns the line number on which the error occurred, or {@code -1} if the error does not have a
+   * line number.
+   */
   public int line() {
     return lineNumber;
   }
 
-  /** Returns the 0-indexed column number on which the error occurred. */
+  /**
+   * Returns the 0-indexed column number on which the error occurred, or {@code -1} if the error
+   * does not have a column.
+   */
   public int column() {
     return column;
   }
@@ -47,13 +61,18 @@ public String message() {
     return message;
   }
 
-  @Override
   public String toString() {
     StringBuilder sb = new StringBuilder();
-    sb.append(lineNumber).append(':');
-    // internal column numbers are 0-based, but diagnostics use 1-based indexing by convention
-    sb.append(column + 1).append(':');
-    sb.append(' ');
+    if (lineNumber >= 0) {
+      sb.append(lineNumber).append(':');
+    }
+    if (column >= 0) {
+      // internal column numbers are 0-based, but diagnostics use 1-based indexing by convention
+      sb.append(column + 1).append(':');
+    }
+    if (lineNumber >= 0 || column >= 0) {
+      sb.append(' ');
+    }
     sb.append("error: ").append(message);
     return sb.toString();
   }

core/src/main/java/com/google/googlejavaformat/FormattingError.java

@@ -0,0 +1,29 @@
+/*
+ * Copyright 2016 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+ * in compliance with the License. You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package com.google.googlejavaformat;
+
+/** An unchecked formatting error. */
+public class FormattingError extends Error {
+
+  private final FormatterDiagnostic diagnostic;
+
+  public FormattingError(FormatterDiagnostic diagnostic) {
+    this.diagnostic = diagnostic;
+  }
+
+  public FormatterDiagnostic diagnostic() {
+    return diagnostic;
+  }
+}

core/src/main/java/com/google/googlejavaformat/Input.java

@@ -124,7 +124,7 @@ public String toString() {
    * character offsets to numbers.
    * */
   public FormatterDiagnostic createDiagnostic(int inputPosition, String message) {
-    return new FormatterDiagnostic(
+    return FormatterDiagnostic.create(
         getLineNumber(inputPosition), getColumnNumber(inputPosition), message);
   }
 }

core/src/main/java/com/google/googlejavaformat/OpsBuilder.java

@@ -144,22 +144,20 @@ public BlankLineWanted merge(BlankLineWanted other) {
   private final Input input;
   private final List<Op> ops = new ArrayList<>();
   private final Output output;
-  private final List<FormatterDiagnostic> errors;
   private static final Indent.Const ZERO = Indent.Const.ZERO;
 
   private int tokenI = 0;
   private int inputPosition = Integer.MIN_VALUE;
 
   /**
    * The {@code OpsBuilder} constructor.
+   *
    * @param input the {@link Input}, used for retrieve information from the AST
    * @param output the {@link Output}, used here only to record blank-line information
-   * @param errors mutable list to receive errors
    */
-  public OpsBuilder(Input input, Output output, List<FormatterDiagnostic> errors) {
+  public OpsBuilder(Input input, Output output) {
     this.input = input;
     this.output = output;
-    this.errors = errors; // Assignment of mutable collection.
   }
 
   /** Get the {@code OpsBuilder}'s {@link Input}. */
@@ -255,7 +253,7 @@ public final void token(
        * (for example) we're guessing at an optional token.
        */
       if (realOrImaginary.isReal()) {
-        errors.add(
+        throw new FormattingError(
             input.createDiagnostic(
                 inputPosition, String.format("generated extra token \"%s\"", token)));
       }
@@ -550,7 +548,6 @@ public final String toString() {
     return MoreObjects.toStringHelper(this)
         .add("input", input)
         .add("ops", ops)
-        .add("errors", errors)
         .add("output", output)
         .add("tokenI", tokenI)
         .add("inputPosition", inputPosition)

core/src/main/java/com/google/googlejavaformat/java/Formatter.java

@@ -25,6 +25,7 @@
 import com.google.googlejavaformat.Doc;
 import com.google.googlejavaformat.DocBuilder;
 import com.google.googlejavaformat.FormatterDiagnostic;
+import com.google.googlejavaformat.FormattingError;
 import com.google.googlejavaformat.Op;
 import com.google.googlejavaformat.OpsBuilder;
 import java.io.IOException;
@@ -89,18 +90,15 @@ public Formatter(JavaFormatterOptions options) {
   }
 
   /**
-   * Construct a {@code Formatter} given a Java compilation unit. Parses the code; builds a
-   * {@link JavaInput} and the corresponding {@link JavaOutput}.
+   * Construct a {@code Formatter} given a Java compilation unit. Parses the code; builds a {@link
+   * JavaInput} and the corresponding {@link JavaOutput}.
+   *
    * @param javaInput the input, a Java compilation unit
    * @param javaOutput the {@link JavaOutput}
    * @param options the {@link JavaFormatterOptions}
-   * @param errors mutable list to receive errors
    */
-  static void format(
-      JavaInput javaInput,
-      JavaOutput javaOutput,
-      JavaFormatterOptions options,
-      List<FormatterDiagnostic> errors) {
+  static void format(JavaInput javaInput, JavaOutput javaOutput, JavaFormatterOptions options)
+      throws FormatterException {
     ASTParser parser = ASTParser.newParser(AST.JLS8);
     parser.setSource(javaInput.getText().toCharArray());
     @SuppressWarnings("unchecked") // safe by specification
@@ -110,12 +108,13 @@ static void format(
     CompilationUnit unit = (CompilationUnit) parser.createAST(null);
     javaInput.setCompilationUnit(unit);
     if (unit.getMessages().length > 0) {
+      List<FormatterDiagnostic> errors = new ArrayList<>();
       for (Message message : unit.getMessages()) {
         errors.add(javaInput.createDiagnostic(message.getStartPosition(), message.getMessage()));
       }
-      return;
+      throw new FormatterException(errors);
     }
-    OpsBuilder builder = new OpsBuilder(javaInput, javaOutput, errors);
+    OpsBuilder builder = new OpsBuilder(javaInput, javaOutput);
     // Output the compilation unit.
     new JavaInputAstVisitor(builder, options.indentationMultiplier()).visit(unit);
     builder.sync(javaInput.getText().length());
@@ -182,10 +181,10 @@ public ImmutableList<Replacement> getFormatReplacements(
 
     JavaInput javaInput = new JavaInput(input);
     JavaOutput javaOutput = new JavaOutput(javaInput, new JavaCommentsHelper(options));
-    List<FormatterDiagnostic> errors = new ArrayList<>();
-    format(javaInput, javaOutput, options, errors);
-    if (!errors.isEmpty()) {
-      throw new FormatterException(errors);
+    try {
+      format(javaInput, javaOutput, options);
+    } catch (FormattingError e) {
+      throw new FormatterException(e.diagnostic());
     }
     RangeSet<Integer> tokenRangeSet = javaInput.characterRangesToTokenRanges(characterRanges);
     return javaOutput.getFormatReplacements(tokenRangeSet);

core/src/main/java/com/google/googlejavaformat/java/FormatterException.java

@@ -14,7 +14,6 @@
 
 package com.google.googlejavaformat.java;
 
-import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.googlejavaformat.FormatterDiagnostic;
 import java.util.List;
@@ -24,15 +23,18 @@ public final class FormatterException extends Exception {
 
   private ImmutableList<FormatterDiagnostic> diagnostics;
 
-  FormatterException(String message) {
-    super(message);
+  public FormatterException(String message) {
+    this(FormatterDiagnostic.create(message));
   }
 
-  public FormatterException(List<FormatterDiagnostic> diagnostics) {
-    super(Joiner.on('\n').join(diagnostics));
-    this.diagnostics = ImmutableList.copyOf(diagnostics);
+  public FormatterException(FormatterDiagnostic diagnostic) {
+    this(ImmutableList.of(diagnostic));
   }
 
+  public FormatterException(Iterable<FormatterDiagnostic> diagnostics) {
+    super(diagnostics.iterator().next().toString());
+    this.diagnostics = ImmutableList.copyOf(diagnostics);
+  }
   public List<FormatterDiagnostic> diagnostics() {
     return diagnostics;
   }

core/src/main/java/com/google/googlejavaformat/java/Main.java

@@ -17,6 +17,7 @@
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.io.ByteStreams;
+import com.google.googlejavaformat.FormatterDiagnostic;
 import com.google.googlejavaformat.java.JavaFormatterOptions.Style;
 import java.io.IOError;
 import java.io.IOException;
@@ -141,7 +142,9 @@ private int formatFiles(CommandLineOptions parameters, JavaFormatterOptions opti
         continue;
       } catch (ExecutionException e) {
         if (e.getCause() instanceof FormatterException) {
-          errWriter.println(result.getKey() + ":" + e.getCause().getMessage());
+          for (FormatterDiagnostic diagnostic : ((FormatterException) e.getCause()).diagnostics()) {
+            errWriter.println(result.getKey() + ":" + diagnostic.toString());
+          }
         } else {
           errWriter.println(result.getKey() + ": error: " + e.getCause().getMessage());
           e.getCause().printStackTrace(errWriter);
@@ -179,7 +182,9 @@ private int formatStdin(CommandLineOptions parameters, JavaFormatterOptions opti
       outWriter.write(output);
       return 0;
     } catch (FormatterException e) {
-      errWriter.println(STDIN_FILENAME + ":" + e.getMessage());
+      for (FormatterDiagnostic diagnostic : e.diagnostics()) {
+        errWriter.println(STDIN_FILENAME + ":" + diagnostic.toString());
+      }
       return 1;
       // TODO(cpovirk): Catch other types of exception (as we do in the formatFiles case).
     }

core/src/test/java/com/google/googlejavaformat/java/ImportOrdererTest.java

@@ -386,7 +386,8 @@ public void reorder() throws FormatterException {
         throw e;
       }
       assertThat(reordered).endsWith("\n");
-      assertThat(e.getMessage()).isEqualTo(reordered.substring(2, reordered.length() - 1));
+      assertThat(e.getMessage())
+          .isEqualTo("error: " + reordered.substring(2, reordered.length() - 1));
     }
   }
 }