Improve string reflowing (#35)

* Better displayName

* StringWrapper formats & wraps once more, getting rid of hack from Level

* fix MainTest

* Add generated changelog entries

* better

Author: dansanduleac

changelog/@unreleased/pr-35.v2.yml

@@ -0,0 +1,7 @@
+type: fix
+fix:
+  description: Reflow long strings more thoroughly, taking into account that once
+    a string is broken, its first chunk might fit on the starting line and thus it
+    needs to be reflowed again.
+  links:
+  - https://github.com/palantir/palantir-java-format/pull/35

palantir-java-format/src/main/java/com/palantir/javaformat/doc/Level.java

@@ -195,7 +195,7 @@ public State computeBreaks(CommentsHelper commentsHelper, int maxWidth, State st
 
             // Allow long strings to stay on the same line, expecting that StringWrapper will
             // reflow them later.
-            if (prefixFits || isSingleString()) {
+            if (prefixFits) {
                 State newState = state.withNoIndent();
                 if (breakBehaviour == BreakBehaviour.BREAK_ONLY_IF_INNER_LEVELS_THEN_FIT_ON_ONE_LINE) {
                     newState = newState.withIndentIncrementedBy(plusIndent);
@@ -207,18 +207,6 @@ public State computeBreaks(CommentsHelper commentsHelper, int maxWidth, State st
         return state.updateAfterLevel(broken);
     }
 
-    /**
-     * Trick to cooperate with StringWrapper. If the value is a single element (e.g. a String), then allow it to stay on
-     * this line, and rely on the StringWrapper to break it accordingly.
-     *
-     * <p>This is to prevent StringWrapperIntegrationTest#idemponent from breaking.
-     */
-    private boolean isSingleString() {
-        return !this.docs.stream().anyMatch(doc -> doc instanceof Level)
-                && this.docs.stream().filter(doc -> doc instanceof Break).count() == 1
-                && getFlat().startsWith(" \"");
-    }
-
     private Optional<State> tryBreakLastLevel(
             CommentsHelper commentsHelper, int maxWidth, State state, boolean recursive) {
         if (docs.isEmpty() || !(getLast(docs) instanceof Level)) {

palantir-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java

@@ -22,6 +22,7 @@
 import com.google.common.base.Strings;
 import com.google.common.base.Verify;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Range;
 import com.google.common.collect.TreeRangeMap;
@@ -35,6 +36,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.stream.Stream;
 import org.openjdk.javax.tools.Diagnostic;
@@ -81,6 +83,14 @@ static String wrap(final int columnLimit, String input, Formatter formatter) thr
 
         String result = applyReplacements(input, replacements);
 
+        // Format again, because broken strings might now fit on the first line in case of assignments
+        String secondPass = formatter.formatSource(result, rangesAfterAppliedReplacements(replacements));
+
+        if (!secondPass.equals(result)) {
+            replacements = getReflowReplacements(columnLimit, secondPass);
+            result = applyReplacements(secondPass, replacements);
+        }
+
         {
             // We really don't want bugs in this pass to change the behaviour of programs we're
             // formatting, so check that the pretty-printed AST is the same before and after reformatting.
@@ -98,6 +108,26 @@ static String wrap(final int columnLimit, String input, Formatter formatter) thr
         return result;
     }
 
+    @SuppressWarnings("ResultOfMethodCallIgnored")
+    private static ImmutableSet<Range<Integer>> rangesAfterAppliedReplacements(
+            TreeRangeMap<Integer, String> replacements) {
+        ImmutableSet.Builder<Range<Integer>> outputRanges = ImmutableSet.builder();
+        int offset = 0;
+        for (Entry<Range<Integer>, String> entry : replacements.asMapOfRanges().entrySet()) {
+            Range<Integer> range = entry.getKey();
+            String replacement = entry.getValue();
+
+            int lower = offset + range.lowerEndpoint();
+            int upper = lower + replacement.length();
+            outputRanges.add(Range.closedOpen(lower, upper));
+
+            int originalLength = range.upperEndpoint() - range.lowerEndpoint();
+            int newLength = upper - lower;
+            offset += newLength - originalLength;
+        }
+        return outputRanges.build();
+    }
+
     private static TreeRangeMap<Integer, String> getReflowReplacements(int columnLimit, final String input)
             throws FormatterException {
         JCTree.JCCompilationUnit unit = parse(input, /* allowStringFolding= */ false);
@@ -412,8 +442,7 @@ public CharSequence getCharContent(boolean ignoreEncodingErrors) {
     }
 
     /** Applies replacements to the given string. */
-    private static String applyReplacements(String javaInput, TreeRangeMap<Integer, String> replacementMap)
-            throws FormatterException {
+    private static String applyReplacements(String javaInput, TreeRangeMap<Integer, String> replacementMap) {
         // process in descending order so the replacement ranges aren't perturbed if any replacements
         // differ in size from the input
         Map<Range<Integer>, String> ranges = replacementMap.asDescendingMapOfRanges();

palantir-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java

@@ -478,8 +478,8 @@ public void reflowLongStrings() throws Exception {
         };
         String[] expected = {
             "class T {",
-            "  String s = \"one long incredibly unbroken sentence moving from topic to topic so that no" + " one had\"",
-            "      + \" a chance to interrupt\";",
+            "  String s = \"one long incredibly unbroken sentence moving from topic to topic so that no one had a\"",
+            "      + \" chance to interrupt\";",
             "}",
             "",
         };
@@ -503,7 +503,8 @@ public void noReflowLongStrings() throws Exception {
         };
         String[] expected = {
             "class T {",
-            "  String s = \"one long incredibly unbroken sentence moving from topic to topic so that no"
+            "  String s =",
+            "      \"one long incredibly unbroken sentence moving from topic to topic so that no"
                     + " one had a chance to interrupt\";",
             "}",
             "",

palantir-java-format/src/test/java/com/palantir/javaformat/java/StringWrapperIntegrationTest.java

@@ -32,7 +32,7 @@ public class StringWrapperIntegrationTest {
 
     private static FileBasedTests tests = new FileBasedTests(StringWrapperIntegrationTest.class);
 
-    @ParameterizedClass.Parameters(name = "{index}: {0}")
+    @ParameterizedClass.Parameters(name = "{0}")
     public static List<Object[]> parameters() throws IOException {
         return tests.paramsAsNameInputOutput();
     }

palantir-java-format/src/test/resources/com/palantir/javaformat/java/StringWrapperIntegrationTest/9.output

@@ -1,3 +1,4 @@
 class T {
-  String s = "onelongincrediblyunbrokensentencemovingfromtopictotopicsothatnoonehadachancetointerrupt_____________________)_";
+  String s =
+      "onelongincrediblyunbrokensentencemovingfromtopictotopicsothatnoonehadachancetointerrupt_____________________)_";
 }