Repository: google/closure-compiler
Pull request #790 (2015-01-22 19:22:36)

Include a space before single quote and bang when pretty-printing.
Fixes issue 782.


alexeagle: Thanks, I looked through the unary operators but wasn't thinking about how they can appear prefix or suffix, and that makes a difference. Added the missing test cases and will find a fix.
alexeagle: Ping!
dimvar: OK, looks good. I just had a minor comment. Can you resolve the conflicts and squash the commits? And then I'll merge it.
alexeagle: All set.

Patch:
diff --git a/src/com/google/javascript/jscomp/CodeConsumer.java b/src/com/google/javascript/jscomp/CodeConsumer.java
index 772ed482e..08d892e1b 100644
--- a/src/com/google/javascript/jscomp/CodeConsumer.java
+++ b/src/com/google/javascript/jscomp/CodeConsumer.java
@@ -199,7 +199,7 @@ abstract class CodeConsumer {
       // is valid and should print like
       // / // / /
       append(" ");
-    } else if (c == '"' && isWordChar(getLastChar())) {
+    } else if ((c == '"' || c == '\'') && isWordChar(getLastChar())) {
       maybeInsertSpace();
     }
 
diff --git a/src/com/google/javascript/jscomp/CodeGenerator.java b/src/com/google/javascript/jscomp/CodeGenerator.java
index 37f175156..70b087421 100644
--- a/src/com/google/javascript/jscomp/CodeGenerator.java
+++ b/src/com/google/javascript/jscomp/CodeGenerator.java
@@ -188,6 +188,7 @@ class CodeGenerator {
       case Token.THROW:
         Preconditions.checkState(childCount == 1);
         add("throw");
+        cc.maybeInsertSpace();
         add(first);
 
         // Must have a ';' after a throw statement, otherwise safari can't
@@ -198,6 +199,7 @@ class CodeGenerator {
       case Token.RETURN:
         add("return");
         if (childCount == 1) {
+          cc.maybeInsertSpace();
           add(first);
         } else {
           Preconditions.checkState(childCount == 0);
@@ -813,6 +815,7 @@ class CodeGenerator {
       case Token.YIELD:
         Preconditions.checkState(childCount == 1);
         add("yield");
+        cc.maybeInsertSpace();
         if (n.isYieldFor()) {
           add("*");
         }
diff --git a/src/com/google/javascript/jscomp/CodePrinter.java b/src/com/google/javascript/jscomp/CodePrinter.java
index dd2a6379c..fe969b8f0 100644
--- a/src/com/google/javascript/jscomp/CodePrinter.java
+++ b/src/com/google/javascript/jscomp/CodePrinter.java
@@ -333,14 +333,12 @@ public final class CodePrinter {
 
     @Override
     void appendOp(String op, boolean binOp) {
+      if (getLastChar() != ' ' && binOp && op.charAt(0) != ',') {
+        append(" ");
+      }
+      append(op);
       if (binOp) {
-        if (getLastChar() != ' ' && op.charAt(0) != ',') {
-          append(" ");
-        }
-        append(op);
         append(" ");
-      } else {
-        append(op);
       }
     }
 


Test patch:
diff --git a/test/com/google/javascript/jscomp/CodePrinterTest.java b/test/com/google/javascript/jscomp/CodePrinterTest.java
index 6df1b9cb5..0c4e858f2 100644
--- a/test/com/google/javascript/jscomp/CodePrinterTest.java
+++ b/test/com/google/javascript/jscomp/CodePrinterTest.java
@@ -104,58 +104,19 @@ public class CodePrinterTest extends TestCase {
     return new CodePrinter.Builder(parse(js)).setCompilerOptions(options).build();
   }
 
-  CompilerOptions newCompilerOptions(boolean prettyprint, int lineThreshold) {
+  private abstract class CompilerOptionBuilder {
+    abstract void setOptions(CompilerOptions options);
+  }
+
+  CompilerOptions newCompilerOptions(CompilerOptionBuilder builder) {
     CompilerOptions options = new CompilerOptions();
     options.setTrustedStrings(trustedStrings);
     options.preserveTypeAnnotations = preserveTypeAnnotations;
     options.setLanguageOut(languageMode);
-    options.setPrettyPrint(prettyprint);
-    options.setLineLengthThreshold(lineThreshold);
-    return options;
-  }
-
-  CompilerOptions newCompilerOptions(boolean prettyprint, int lineThreshold, boolean lineBreak) {
-    CompilerOptions options = newCompilerOptions(prettyprint, lineThreshold);
-    options.setLineBreak(lineBreak);
+    builder.setOptions(options);
     return options;
   }
 
-  String parsePrint(String js, boolean prettyprint, int lineThreshold) {
-    return parsePrint(js, newCompilerOptions(prettyprint, lineThreshold));
-  }
-
-  String parsePrint(String js, boolean prettyprint, boolean lineBreak, int lineThreshold) {
-    return parsePrint(js, newCompilerOptions(prettyprint, lineThreshold, lineBreak));
-  }
-
-  String parsePrint(String js, boolean prettyprint, boolean lineBreak,
-      boolean preferLineBreakAtEof, int lineThreshold) {
-    CompilerOptions options = newCompilerOptions(prettyprint, lineThreshold, lineBreak);
-    options.setPreferLineBreakAtEndOfFile(preferLineBreakAtEof);
-    return parsePrint(js, options);
-  }
-
-  String parsePrint(String js, boolean prettyprint, boolean lineBreak, int lineThreshold,
-      boolean outputTypes) {
-    return new CodePrinter.Builder(parse(js, true))
-        .setCompilerOptions(newCompilerOptions(prettyprint, lineThreshold, lineBreak))
-        .setOutputTypes(outputTypes)
-        .setTypeRegistry(lastCompiler.getTypeRegistry())
-        .build();
-  }
-
-  String parsePrint(String js, boolean prettyprint, boolean lineBreak,
-                    int lineThreshold, boolean outputTypes,
-                    boolean tagAsStrict) {
-    return new CodePrinter.Builder(parse(js, true))
-        .setCompilerOptions(newCompilerOptions(prettyprint, lineThreshold, lineBreak))
-        .setOutputTypes(outputTypes)
-        .setTypeRegistry(lastCompiler.getTypeRegistry())
-        .setTagAsStrict(tagAsStrict)
-        .build();
-  }
-
-
   String printNode(Node n) {
     CompilerOptions options = new CompilerOptions();
     options.setLineLengthThreshold(CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD);
@@ -641,7 +602,12 @@ public class CodePrinterTest extends TestCase {
   private void assertPrint(String js, String expected) {
     parse(expected); // validate the expected string is valid JS
     assertEquals(expected,
-        parsePrint(js, false, CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD));
+        parsePrint(js, newCompilerOptions(new CompilerOptionBuilder() {
+          @Override void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(false);
+            options.setLineLengthThreshold(CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD);
+          }
+        })));
   }
 
   private void assertPrintSame(String js) {
@@ -717,8 +683,14 @@ public class CodePrinterTest extends TestCase {
 
   private void assertLineBreak(String js, String expected) {
     assertEquals(expected,
-        parsePrint(js, false, true,
-            CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD));
+        parsePrint(js, newCompilerOptions(new CompilerOptionBuilder() {
+          @Override
+          void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(false);
+            options.setLineBreak(true);
+            options.setLineLengthThreshold(CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD);
+          }
+        })));
   }
 
   public void testPreferLineBreakAtEndOfFile() {
@@ -754,9 +726,25 @@ public class CodePrinterTest extends TestCase {
   private void assertLineBreakAtEndOfFile(String js,
       String expectedWithoutBreakAtEnd, String expectedWithBreakAtEnd) {
     assertEquals(expectedWithoutBreakAtEnd,
-        parsePrint(js, false, false, false, 30));
+        parsePrint(js, newCompilerOptions(new CompilerOptionBuilder() {
+          @Override
+          void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(false);
+            options.setLineBreak(false);
+            options.setLineLengthThreshold(30);
+            options.setPreferLineBreakAtEndOfFile(false);
+          }
+        })));
     assertEquals(expectedWithBreakAtEnd,
-        parsePrint(js, false, false, true, 30));
+        parsePrint(js, newCompilerOptions(new CompilerOptionBuilder() {
+          @Override
+          void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(false);
+            options.setLineBreak(false);
+            options.setLineLengthThreshold(30);
+            options.setPreferLineBreakAtEndOfFile(true);
+          }
+        })));
   }
 
   public void testPrettyPrinter() {
@@ -852,6 +840,7 @@ public class CodePrinterTest extends TestCase {
 
     assertPrettyPrint("var a;", "var a;\n");
     assertPrettyPrint("i--", "i--;\n");
+    assertPrettyPrint("i++", "i++;\n");
 
     // There must be a space before and after binary operators.
     assertPrettyPrint("var foo = 3+5;",
@@ -978,6 +967,50 @@ public class CodePrinterTest extends TestCase {
     assertPrettyPrint("(a=1)=>123;", "(a = 1) => 123;\n");
   }
 
+  // For https://github.com/google/closure-compiler/issues/782
+  public void testPrettyPrinter_spaceBeforeSingleQuote() throws Exception {
+    assertPrettyPrint("var f = function() { return 'hello'; };",
+        "var f = function() {\n" +
+            "  return 'hello';\n" +
+            "};\n",
+        new CompilerOptionBuilder() {
+          @Override
+          void setOptions(CompilerOptions options) {
+            options.setPreferSingleQuotes(true);
+          }
+        });
+  }
+
+  // For https://github.com/google/closure-compiler/issues/782
+  public void testPrettyPrinter_spaceBeforeUnaryOperators() throws Exception {
+    languageMode = LanguageMode.ECMASCRIPT6;
+
+    assertPrettyPrint("var f = function() { return !b; };",
+        "var f = function() {\n" +
+            "  return !b;\n" +
+            "};\n");
+    assertPrettyPrint("var f = function*(){yield -b}",
+        "var f = function*() {\n" +
+            "  yield -b;\n" +
+            "};\n");
+    assertPrettyPrint("var f = function() { return +b; };",
+        "var f = function() {\n" +
+            "  return +b;\n" +
+            "};\n");
+    assertPrettyPrint("var f = function() { throw ~b; };",
+        "var f = function() {\n" +
+            "  throw ~b;\n" +
+            "};\n");
+    assertPrettyPrint("var f = function() { return ++b; };",
+        "var f = function() {\n" +
+            "  return ++b;\n" +
+            "};\n");
+    assertPrettyPrint("var f = function() { return --b; };",
+        "var f = function() {\n" +
+            "  return --b;\n" +
+            "};\n");
+  }
+
   public void testTypeAnnotations() {
     assertTypeAnnotations(
         "/** @constructor */ function Foo(){}",
@@ -1180,15 +1213,39 @@ public class CodePrinterTest extends TestCase {
   }
 
   private void assertPrettyPrint(String js, String expected) {
+    assertPrettyPrint(js, expected, new CompilerOptionBuilder() {
+      @Override void setOptions(CompilerOptions options) { }
+    });
+  }
+
+  private void assertPrettyPrint(String js, String expected,
+                                 final CompilerOptionBuilder optionBuilder) {
     assertEquals(expected,
-        parsePrint(js, true, false,
-            CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD));
+        parsePrint(js, newCompilerOptions(new CompilerOptionBuilder() {
+          @Override
+          void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(true);
+            options.setLineBreak(false);
+            options.setLineLengthThreshold(CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD);
+            optionBuilder.setOptions(options);
+          }
+        })));
   }
 
   private void assertTypeAnnotations(String js, String expected) {
     assertEquals(expected,
-        parsePrint(js, true, false,
-            CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD, true));
+        new CodePrinter.Builder(parse(js, true))
+            .setCompilerOptions(newCompilerOptions(new CompilerOptionBuilder() {
+              @Override
+              void setOptions(CompilerOptions options) {
+                options.setPrettyPrint(true);
+                options.setLineBreak(false);
+                options.setLineLengthThreshold(CodePrinter.DEFAULT_LINE_LENGTH_THRESHOLD);
+              }
+            }))
+            .setOutputTypes(true)
+            .setTypeRegistry(lastCompiler.getTypeRegistry())
+            .build());
   }
 
   public void testSubtraction() {
@@ -1257,7 +1314,14 @@ public class CodePrinterTest extends TestCase {
 
   private void assertLineLength(String js, String expected) {
     assertEquals(expected,
-        parsePrint(js, false, true, 10));
+        parsePrint(js, newCompilerOptions(new CompilerOptionBuilder() {
+          @Override
+          void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(false);
+            options.setLineBreak(true);
+            options.setLineLengthThreshold(10);
+          }
+        })));
   }
 
   public void testParsePrintParse() {
@@ -1611,7 +1675,19 @@ public class CodePrinterTest extends TestCase {
   }
 
   public void testStrict() {
-    String result = parsePrint("var x", false, false, 0, false, true);
+    String result = new CodePrinter.Builder(parse("var x", true))
+        .setCompilerOptions(newCompilerOptions(new CompilerOptionBuilder() {
+          @Override
+          void setOptions(CompilerOptions options) {
+            options.setPrettyPrint(false);
+            options.setLineBreak(false);
+            options.setLineLengthThreshold(0);
+          }
+        }))
+        .setOutputTypes(false)
+        .setTypeRegistry(lastCompiler.getTypeRegistry())
+        .setTagAsStrict(true)
+        .build();
     assertEquals("'use strict';var x", result);
   }
 
diff --git a/test/com/google/javascript/jscomp/parsing/TypeSyntaxTest.java b/test/com/google/javascript/jscomp/parsing/TypeSyntaxTest.java
index b0c8160f3..ff175fc27 100644
--- a/test/com/google/javascript/jscomp/parsing/TypeSyntaxTest.java
+++ b/test/com/google/javascript/jscomp/parsing/TypeSyntaxTest.java
@@ -116,7 +116,7 @@ public class TypeSyntaxTest extends TestCase {
   }
 
   public void testFunctionReturn() {
-    Node fn = parse("function foo(): string {\n  return'hello';\n}").getFirstChild();
+    Node fn = parse("function foo(): string {\n  return 'hello';\n}").getFirstChild();
     Node fnType = fn.getDeclaredTypeExpression();
     assertTreeEquals("string type", TypeDeclarationsIRFactory.stringType(), fnType);
   }
@@ -135,7 +135,7 @@ public class TypeSyntaxTest extends TestCase {
 
   public void testFunctionReturn_typeInJsdocOnly() throws Exception {
     parse("function /** string */ foo() { return 'hello'; }",
-        "function/** string */foo() {\n  return'hello';\n}");
+        "function/** string */foo() {\n  return 'hello';\n}");
   }
 
   public void testCompositeType() {